Re: [PATCH 2/2] kallsyms: Optimise symbol name search

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 4587 bytes --]

> kallsyms: Optimise symbol name search

TL;DR:
- kallsyms 24-bit table struct may be padded to 4 bytes on ARM OABI, giving
  wrong offsets and a possible out-of-bounds read
- weak table symbols may not read as NULL on relocatable x86-32, so the wrong
  table may be chosen

> diff --git a/kernel/kallsyms_internal.h b/kernel/kallsyms_internal.h
> index 81a867dbe57d..be503f3f993f 100644
> --- a/kernel/kallsyms_internal.h
> +++ b/kernel/kallsyms_internal.h
> @@ -13,6 +13,8 @@ extern const char kallsyms_token_table[];
>  extern const u16 kallsyms_token_index[];
>
>  extern const unsigned int kallsyms_markers[];
> -extern const u8 kallsyms_seqs_of_names[];
> +
> +extern struct { unsigned int v:24 __attribute__((packed)); } kallsyms_off24_of_names[] __attribute__((weak));

Is this struct guaranteed to be 3 bytes on every architecture?

The packed attribute is on the bitfield member only, not on the struct.  That
reduces the member alignment, but it does not stop an ABI from padding the
struct size.  On ARM OABI kernels structures are padded to a multiple of 32
bits.  Documentation/driver-api/ioctl.rst says "On ARM OABI user space,
structures are padded to multiples of 32-bit", and GCC applies
STRUCTURE_SIZE_BOUNDARY unless the record itself is packed.

OABI kernels can still be built.  arch/arm/Kconfig offers AEABI as optional
for CPU_SA110 || CPU_SA1100, and the assabet, footbridge, rpc, neponset and
netwinder defconfigs all have '# CONFIG_AEABI is not set'.  For those builds
arch/arm/Makefile compiles with -mabi=apcs-gnu.

In that configuration sizeof(kallsyms_off24_of_names[0]) would be 4, but
write_src() in scripts/kallsyms.c still writes 3 bytes per entry:

    printf("\t.byte 0x%02x, 0x%02x, 0x%02x\t/* %s */\n", ...

get_symbol_name(index) would then read at 4 * index instead of 3 * index for
every index > 0:

    if (kallsyms_off24_of_names)
            return kallsyms_off24_of_names[index].v;

That returns garbage offsets, and for the top quarter of the indexes it reads
past the end of the table.  kallsyms_lookup_names() and
kallsyms_on_each_match_symbol() would then compare against arbitrary
positions in kallsyms_names, which can be up to 16 MiB past its start, so
lookups fail or return wrong addresses, and there is possibly an
out-of-bounds read.

The old u8 kallsyms_seqs_of_names[] with manual byte assembly had no layout
dependency.  Would it be better to put __packed on the struct itself, as
struct __una_u32 { u32 x; } __packed; does in
include/linux/unaligned/packed_struct.h?  Alternatively, a u8 array read
with get_unaligned_le24()/get_unaligned_be24() would avoid the problem.

> +extern u32 kallsyms_off32_of_names[] __attribute__((weak));

Can these two weak references be relied on to read back as NULL when the
symbol is left undefined?

Commit 951bcae6c5a0 ("kallsyms: Avoid weak references for kallsyms symbols")
removed weak references, noting that they need a GOT in position-independent
code and are "not usually supported by RIP or PC relative symbol
references".  Commit c442db3f49f2 ("kbuild: remove PROVIDE() for kallsyms
symbols") then added the empty step-0 kallsyms object so that no weak or
PROVIDE() fallback is needed.

The previous weak references were always resolved in the final link.  Here
one of the two symbols is always left unresolved in the final vmlinux, and
get_symbol_name() relies on its address being NULL.

That does not seem to hold on every relocatable kernel.  On x86-32 with
X86_NEED_RELOCS (RELOCATABLE or RANDOMIZE_BASE), the R_386_32 relocation for
'$kallsyms_off24_of_names' goes through do_reloc32() in
arch/x86/tools/relocs.c.  Unlike do_reloc64(), which has:

    if (sym->st_shndx == SHN_UNDEF)
            return 0;

do_reloc32() does not skip undefined symbols, so the relocation is added to
relocs32.  When the decompressor relocates the kernel by a non-zero delta,
the unresolved weak address becomes delta instead of 0.

If the names table needs the 32-bit layout, kallsyms_off24_of_names is the
undefined one, so 'if (kallsyms_off24_of_names)' in get_symbol_name() is
then true and it reads 3-byte entries from a bogus low address.

Could the generator always emit both labels, with one of them empty, and
could the kernel pick the table with a non-weak test, for example by
comparing the label addresses or by using a size or flag word?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/36795037147