[PATCH] linker_list: Fix ll_entry_get alignment

[Simon Glass]

Hi Sean,

On Sun, 1 Oct 2023 at 19:43, Sean Anderson <seanga2 at gmail.com> wrote:
>
> On 10/1/23 21:16, Simon Glass wrote:
> > 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?
>
> Most of the time, yes.
>
> > 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.
>
> Yeah, this would be the best way to find errors in the current system.
>
> But maybe ll_entry_get should look like
>
> #define ll_entry_get(_type, _name, _list)                               \
>         ({                                                              \
>                 ll_entry_declare(_type, _name, _list);                  \
>                 _type *_ll_result =                                     \
>                         &_u_boot_list_2_##_list##_2_##_name;            \
>                 _ll_result;                                             \
>         })
>
> (untested)
>
> Regardless, I think a link-time check would be a good sanity check.

OK, well if you take a crack at it, you have a failing case to test with!

Regards,
Simon