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: <CAMj1kXEoYcS6YPU0mBdvijDRK6ZVB7mPYZsCVpz7sYotabrxtQ@mail.gmail.com>
Date: Fri, 2 May 2025 18:41:33 +0200
From: Ard Biesheuvel <ardb@...nel.org>
To: Yeoreum Yun <yeoreum.yun@....com>
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

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