linux-kernel - Re: [PATCH] arm64/cpufeature: annotate arm64_use_ng_mappings with ro_after

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aBT/i/1DmFsU0GUk@e129823.arm.com>
Date: Fri, 2 May 2025 18:23:23 +0100
From: Yeoreum Yun <yeoreum.yun@....com>
To: Ard Biesheuvel <ardb@...nel.org>
Cc: catalin.marinas@....com, will@...nel.org, nathan@...nel.org,
	nick.desaulniers+lkml@...il.com, morbo@...gle.com,
	justinstitt@...gle.com, broonie@...nel.org, maz@...nel.org,
	oliver.upton@...ux.dev, frederic@...nel.org, joey.gouly@....com,
	james.morse@....com, hardevsinh.palaniya@...iconsignals.io,
	shameerali.kolothum.thodi@...wei.com, ryan.roberts@....com,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	llvm@...ts.linux.dev, stable@...r.kernel.org
Subject: Re: [PATCH] arm64/cpufeature: annotate arm64_use_ng_mappings with
 ro_after_init to prevent wrong idmap generation

Hi Ard,

> On Fri, 2 May 2025 at 16:58, Yeoreum Yun <yeoreum.yun@....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