[PATCH] linker_list: Fix ll_entry_get alignment

[Simon Glass]

Hi Sean,

On Sat, 30 Sept 2023 at 09:23, Sean Anderson <seanga2 at gmail.com> wrote:
>
> On 9/30/23 10:36, Sean Anderson wrote:
> > When ll_entry_get is used on a list entry ll_entry_declare'd in the same
> > file, the lack of alignment on the access will override the
> > ll_entry_declare alignment. This causes GCC to use the default section
> > alignment of 32 bytes. As list entries are not necessarily 32-byte aligned,
> > this will cause a gap in the linker list, corrupting further entries.
> >
> > As a specific example, get_fs_loader uses DM_DRIVER_GET(fs_loader) in the
> > same file where U_BOOT_DRIVER(fs_loader) is present. This causes a crash
> > when walking the driver list.
> >
> > Fix this by adding appropriate alignment to all accesses.
> >
> > Fixes: 42ebaae3a33 ("common: Implement support for linker-generated arrays")
> > Signed-off-by: Sean Anderson <seanga2 at gmail.com>
> > ---
> >
> >   include/linker_lists.h | 5 +++--
> >   1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/linker_lists.h b/include/linker_lists.h
> > index f9a2ee0c762..e0c8a01b9ba 100644
> > --- a/include/linker_lists.h
> > +++ b/include/linker_lists.h
> > @@ -209,7 +209,8 @@
> >    */
> >   #define ll_entry_get(_type, _name, _list)                           \
> >       ({                                                              \
> > -             extern _type _u_boot_list_2_##_list##_2_##_name;        \
> > +             extern _type __aligned(4)                               \
> > +                     _u_boot_list_2_##_list##_2_##_name;             \
> >               _type *_ll_result =                                     \
> >                       &_u_boot_list_2_##_list##_2_##_name;            \
> >               _ll_result;                                             \
> > @@ -229,7 +230,7 @@
> >    * @_list: name of the list
> >    */
> >   #define ll_entry_ref(_type, _name, _list)                           \
> > -     ((_type *)&_u_boot_list_2_##_list##_2_##_name)
> > +     ((_type __aligned(4) *)&_u_boot_list_2_##_list##_2_##_name)
>
> OK, so this causes an error in clang. And it isn't really necessary
> because the entry is already declared at this point.
>
> So I guess the right fix is to replace DM_DRIVER_GET with DM_DRIVER_REF in
> get_fs_loader. But this seems like a really big footgun. You can use the
> wrong one and there are no errors except at runtime. I wonder if we can add
> a warning of some kind?

I can imagine having a runtime check, something like:

ll_check(sizeof(struct something))

which checks that the linker list (end - start) is a multiple of the
struct size. Do you think that would find the problem?

If so, then it could be perhaps be turned into a link-time check. This
produces a list of the linker lists along with their individual
members:

or ll in $(nm /tmp/b/coreboot/u-boot |grep u_boot_list_2 |sed
's/.*_u_boot_list_2_\(.*\)_2_.*/\1/' |uniq); do echo; echo "linker
list: %ll"; nm /tmp/b/coreboot/u-boot |grep $ll; done

...
linker list: ut_str_test
011a9a20 D _u_boot_list_2_ut_str_test_2_str_dectoul
011a9a30 D _u_boot_list_2_ut_str_test_2_str_hextoul
011a9a40 D _u_boot_list_2_ut_str_test_2_str_itoa
011a9a50 D _u_boot_list_2_ut_str_test_2_str_simple_strtoul
011a9a60 D _u_boot_list_2_ut_str_test_2_str_simple_strtoull
011a9a70 D _u_boot_list_2_ut_str_test_2_str_trailing
011a9a80 D _u_boot_list_2_ut_str_test_2_str_upper
011a9a90 D _u_boot_list_2_ut_str_test_2_str_xtoa
011a9aa0 D _u_boot_list_2_ut_str_test_2_test_str_to_list
...

Then you can check that the address of each one increments by the same amount.

Maybe.

Regards,
Simon