[PATCH] ARM: decompressor: fix BSS size calculation for LLVM ld.lld

[Fangrui Song]

On 2021-02-05, Ard Biesheuvel wrote:
>On Fri, 5 Feb 2021 at 19:00, Nick Desaulniers <ndesaulniers at google.com> wrote:
>>
>> On Fri, Feb 5, 2021 at 12:52 AM Ard Biesheuvel <ardb at kernel.org> wrote:
>> >
>> > The LLVM ld.lld linker uses a different symbol type for __bss_start,
>> > resulting in the calculation of KBSS_SZ to be thrown off. Up until now,
>> > this has gone unnoticed as it only affects the appended DTB case, but
>> > pending changes for ARM in the way the decompressed kernel is cleaned
>> > from the caches has uncovered this problem.
>> >
>> > On a ld.lld build:
>> >
>> >   $ nm vmlinux |grep bss_
>> >   c1c22034 D __bss_start
>> >   c1c86e98 B __bss_stop
>> >
>> > resulting in
>> >
>>
>> $ readelf -s arch/arm/boot/compressed/vmlinux | grep bss_size
>>
>> >   433: c1c86e98     0 NOTYPE  GLOBAL DEFAULT  ABS _kernel_bss_size
>> >
>> > which is obviously incorrect, and may cause the cache clean to access
>> > unmapped memory, or cause the size calculation to wrap, resulting in no
>> > cache clean to be performed at all.
>> >
>> > Fix this by updating the sed regex to take D type symbols into account.
>> >
>> > Cc: Nick Desaulniers <ndesaulniers at google.com>
>> > Cc: Nathan Chancellor <nathan at kernel.org>
>> > Cc: Guillaume Tucker <guillaume.tucker at collabora.com>
>> > Link: https://lore.kernel.org/linux-arm-kernel/6c65bcef-d4e7-25fa-43cf-2c435bb61bb9@collabora.com/
>> > Signed-off-by: Ard Biesheuvel <ardb at kernel.org>
>>
>>
>> Reviewed-by: Nick Desaulniers <ndesaulniers at google.com>
>> Tested-by: Nick Desaulniers <ndesaulniers at google.com>
>>
>> Thanks for debugging+fixing this, and Guillaume for the report.  It's
>> nice to see a fix so early; thinking back to last year before KernelCI
>> integration, we probably would have only noticed when CrOS went to
>> upgrade their rk3288 platform devices.
>>
>> Some other tags that might be nice to apply:
>>
>> Cc: stable at kernel.org
>> Fixes: 429f7a062e3b ("ARM: decompressor: fix BSS size calculation")
>> Reported-by: Guillaume Tucker <guillaume.tucker at collabora.com>
>> Reported-by: "kernelci.org bot" <bot at kernelci.org>
>>
>
>Thanks. I'll add these tags and drop this patch into the patch system
>
>> Tests run:
>>
>...
>>
>> + Fangrui,
>> Fangrui, __bss_start looks like it's linker script defined by the
>> BSS_SECTION macro from include/asm-generic/vmlinux.lds.h:1160 being
>> used in arch/arm/kernel/vmlinux.lds.S:149.  Should these symbols be
>> placed in .bss? Might save a few bytes in the image, unless there's an
>> initial value that's encoded with them?
>>
>
>Not sure what you are asking here. These symbols just delineate .bss,
>they don't take up any space themselves.
>
>What seems to be happening is that the placement of __bss_start
>outside of the .sbss/.bss section declarations causes it to be
>annotated as residing in .data.

In my build (make -sk -j 50 ARCH=arm CROSS_COMPILE=arm-linux-gnueabi- LLVM=1 O=/tmp/out/arm -j 50 defconfig vmlinux), vmlinux has:

   [34] .data..percpu     PROGBITS        c195f000 151f000 00c20c 00  WA  0   0 64
   [35] .data             PROGBITS        c1a00000 1530000 217914 00  WA  0   0 4096
   [36] __bug_table       PROGBITS        c1c17918 1747918 00a5cc 00  WA  0   0  4
   [37] .sbss             PROGBITS        c1c21ee4 1751ee4 000000 00  WA  0   0  1
   [38] .bss              NOBITS          c1c21f00 1751f00 063f98 00  WA  0   0 64

__bss_start is defined relative to __bug_table.
The issue is that __bss_start is defined outside of the output section description of .bss,
so its st_shndx (section) is not guaranteed.

I noticed that many linker script fragments used by the kernel have similar problems.
The delineating symbols are defined outside of output sections. With ld --orphan-handling=,
we can somewhat guarantee there are no undesired interleaving sections, but it is hard to
ensure their st_shndx values.

>
>> > ---
>> >  arch/arm/boot/compressed/Makefile | 4 ++--
>> >  1 file changed, 2 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/arch/arm/boot/compressed/Makefile b/arch/arm/boot/compressed/Makefile
>> > index fb521efcc6c2..54307db7854d 100644
>> > --- a/arch/arm/boot/compressed/Makefile
>> > +++ b/arch/arm/boot/compressed/Makefile
>> > @@ -115,8 +115,8 @@ asflags-y := -DZIMAGE
>> >
>> >  # Supply kernel BSS size to the decompressor via a linker symbol.
>> >  KBSS_SZ = $(shell echo $$(($$($(NM) $(obj)/../../../../vmlinux | \
>> > -               sed -n -e 's/^\([^ ]*\) [AB] __bss_start$$/-0x\1/p' \
>> > -                      -e 's/^\([^ ]*\) [AB] __bss_stop$$/+0x\1/p') )) )
>> > +               sed -n -e 's/^\([^ ]*\) [ABD] __bss_start$$/-0x\1/p' \
>> > +                      -e 's/^\([^ ]*\) [ABD] __bss_stop$$/+0x\1/p') )) )
>>
>> I wasn't sure whether we still needed `A`, but
>> commit 6cea14f55474 ("ARM: replace unnecessary perl with sed and the
>> shell $(( )) operator")
>> references that depending on the version of binutils you might observe
>> that.  There's no more info on which version or under what conditions.
>> Lest we reintroduce this same problem for that version, it's fine to
>> leave it.
>>

In my observation a symbol may be SHN_ABS (nm key 'A') if it is

* defined by --defsym
* a symbol assignment outside of output sections in -no-pie link mode

>Agreed.
>
>> >  LDFLAGS_vmlinux = --defsym _kernel_bss_size=$(KBSS_SZ)
>> >  # Supply ZRELADDR to the decompressor via a linker symbol.
>> >  ifneq ($(CONFIG_AUTO_ZRELADDR),y)
>> > --
>> > 2.30.0
>> >
>>
>>
>> --
>> Thanks,
>> ~Nick Desaulniers