Re: [PATCH] arm64/cpufeature: annotate arm64_use_ng_mappings with ro_after_init to prevent wrong idmap generation

[Yeoreum Yun <yeoreum.yun@arm.com>]

Hi Ard,

> On Fri, 2 May 2025 at 16:58, Yeoreum Yun <yeoreum.yun@arm.com> wrote:
> >
> > create_init_idmap() could be called before .bss section initialization
> > which is done in early_map_kernel() since data/test_prot could be set
> > wrong for PTE_MAYBE_NG macro.
> >
> > PTE_MAYBE_NG macro is set according to value of "arm64_use_ng_mappings".
> > and this variable is located in .bss section.
> >
> >    # llvm-objdump-21 --syms vmlinux-gcc | grep arm64_use_ng_mappings
> >      ffff800082f242a8 g O .bss    0000000000000001 arm64_use_ng_mappings
> >
> > If .bss section doesn't initialized, "arm64_use_ng_mappings" would be set
> > with garbage value ant then the text_prot or data_prot could be set
> > with garbgae value.
> >
> > Here is what i saw with kernel compiled via llvm-21
> >
> >   // create_init_idmap()
> >   ffff80008255c058: d10103ff            sub     sp, sp, #0x40
> >   ffff80008255c05c: a9017bfd            stp     x29, x30, [sp, #0x10]
> >   ffff80008255c060: a90257f6            stp     x22, x21, [sp, #0x20]
> >   ffff80008255c064: a9034ff4            stp     x20, x19, [sp, #0x30]
> >   ffff80008255c068: 910043fd            add     x29, sp, #0x10
> >   ffff80008255c06c: 90003fc8            adrp    x8, 0xffff800082d54000
> >   ffff80008255c070: d280e06a            mov     x10, #0x703     // =1795
> >   ffff80008255c074: 91400409            add     x9, x0, #0x1, lsl #12 // =0x1000
> >   ffff80008255c078: 394a4108            ldrb    w8, [x8, #0x290] ------------- (1)
> >   ffff80008255c07c: f2e00d0a            movk    x10, #0x68, lsl #48
> >   ffff80008255c080: f90007e9            str     x9, [sp, #0x8]
> >   ffff80008255c084: aa0103f3            mov     x19, x1
> >   ffff80008255c088: aa0003f4            mov     x20, x0
> >   ffff80008255c08c: 14000000            b       0xffff80008255c08c <__pi_create_init_idmap+0x34>
> >   ffff80008255c090: aa082d56            orr     x22, x10, x8, lsl #11 -------- (2)
> >
>
> Interesting. So Clang just shifts the value of "arm64_use_ng_mappings"
> into bit #11, on the basis that 'bool' is a u8 that can only hold
> values 0 or 1.
>
> It is actually kind of nice that this happened, or we would likely
> have never found out - setting nG inadvertently on the initial ID map
> is not something one would ever notice in practice.
> ...

Yeap. it's a quite nice and funny :D

> >
> > In case of gcc, according to value of arm64_use_ng_mappings (annoated as(3)),
> > it branches to each prot settup code.
> > However this is also problem since it branches according to garbage
> > value too -- idmapping with wrong pgprot.
> >
>
> I think the only way to deal with this in a robust manner is to never
> call C code before clearing BSS. But this would mean clearing BSS
> before setting up the ID map, which means it will run with the caches
> disabled, making it slower and also making it necessary to perform
> cache invalidation afterwards.
>
> Making arm64_use_ng_mappings __ro_after_init seems like a useful
> change by itself, so I am not objecting to that. But we don't solve it
> more fundamentally, please at least add a big fat comment why it is
> important that the variable remains there.

Agree. I'll add the comment on arm64_use_ng_mapping.

Thanks!

--
Sincerely,
Yeoreum Yun