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

On 2021-02-05, Ard Biesheuvel wrote:

On Fri, 5 Feb 2021 at 19:00, Nick Desaulniers [off-list ref] wrote:

quoted

On Fri, Feb 5, 2021 at 12:52 AM Ard Biesheuvel [off-list ref] wrote:

quoted

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

quoted

  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@google.com>
Cc: Nathan Chancellor <nathan@kernel.org>
Cc: Guillaume Tucker <redacted>
Link: https://lore.kernel.org/linux-arm-kernel/6c65bcef-d4e7-25fa-43cf-2c435bb61bb9@collabora.com/ (local)
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>

Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
Tested-by: Nick Desaulniers <ndesaulniers@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@kernel.org
Fixes: 429f7a062e3b ("ARM: decompressor: fix BSS size calculation")
Reported-by: Guillaume Tucker <redacted>
Reported-by: "kernelci.org bot" <redacted>

Thanks. I'll add these tags and drop this patch into the patch system

quoted

Tests run:

...

quoted

+ 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.

quoted

quoted

---
 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.

quoted

quoted

 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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel