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

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

[Yeoreum Yun]

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)

Note (1) is load the arm64_use_ng_mappings value in w8.
and (2) is set the text or data prot with the w8 value to set PTE_NG bit.
If .bss section doesn't initialized, x8 can include garbage value
-- In case of some platform, x8 loaded with 0xcf -- it could generate
wrong mapping. (i.e) text_prot is expected with
PAGE_KERNEL_ROX(0x0040000000000F83) but
with garbage x8 -- 0xcf, it sets with (0x0040000000067F83)
and This makes boot failure with translation fault.

This error cannot happen according to code generated by compiler.
here is the case of gcc:

   ffff80008260a940 <__pi_create_init_idmap>:
   ffff80008260a940: d100c3ff      sub     sp, sp, #0x30
   ffff80008260a944: aa0003ed      mov     x13, x0
   ffff80008260a948: 91400400      add     x0, x0, #0x1, lsl #12 // =0x1000
   ffff80008260a94c: a9017bfd      stp     x29, x30, [sp, #0x10]
   ffff80008260a950: 910043fd      add     x29, sp, #0x10
   ffff80008260a954: f90017e0      str     x0, [sp, #0x28]
   ffff80008260a958: d00048c0      adrp    x0, 0xffff800082f24000 <reset_devices>
   ffff80008260a95c: 394aa000      ldrb    w0, [x0, #0x2a8]
   ffff80008260a960: 37000640      tbnz    w0, #0x0, 0xffff80008260aa28 <__pi_create_init_idmap+0xe8> ---(3)
   ffff80008260a964: d280f060      mov     x0, #0x783      // =1923
   ffff80008260a968: d280e062      mov     x2, #0x703      // =1795
   ffff80008260a96c: f2e00800      movk    x0, #0x40, lsl #48
   ffff80008260a970: f2e00d02      movk    x2, #0x68, lsl #48
   ffff80008260a974: aa2103e4      mvn     x4, x1
   ffff80008260a978: 8a210049      bic     x9, x2, x1
   ...
   ffff80008260aa28: d281f060      mov     x0, #0xf83      // =3971
   ffff80008260aa2c: d281e062      mov     x2, #0xf03      // =3843
   ffff80008260aa30: f2e00800      movk    x0, #0x40, lsl #48

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.

To resolve this, annotate arm64_use_ng_mappings as ro_after_init.

Fixes: 84b04d3e6bdb ("arm64: kernel: Create initial ID map from C code")
Cc: <stable@vger.kernel.org> # 6.9.x
Signed-off-by: Yeoreum Yun <yeoreum.yun@arm.com>
---
There is another way to solve this problem by setting
test/data_prot with _PAGE_DEFAULT which doesn't include PTE_MAYBE_NG
with constanst check in create_init_idmap() to be free from
arm64_use_ng_mappings. but i think it would be better to change
arm64_use_ng_mappings as ro_after_init because it doesn't change after
init phase and solve this problem too.
---
 arch/arm64/kernel/cpufeature.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index d2104a1e7843..967ffb1cbd52 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -114,7 +114,7 @@ static struct arm64_cpu_capabilities const __ro_after_init *cpucap_ptrs[ARM64_NC

 DECLARE_BITMAP(boot_cpucaps, ARM64_NCAPS);

-bool arm64_use_ng_mappings = false;
+bool arm64_use_ng_mappings __ro_after_init = false;
 EXPORT_SYMBOL(arm64_use_ng_mappings);

 DEFINE_PER_CPU_READ_MOSTLY(const char *, this_cpu_vector) = vectors;
--
LEVI:{C3F47F37-75D8-414A-A8BA-3980EC8A46D7}

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

[Nathan Chancellor]

Hi Yeoreum,

On Fri, May 02, 2025 at 03:57:55PM +0100, Yeoreum Yun 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)
> 
> Note (1) is load the arm64_use_ng_mappings value in w8.
> and (2) is set the text or data prot with the w8 value to set PTE_NG bit.
> If .bss section doesn't initialized, x8 can include garbage value
> -- In case of some platform, x8 loaded with 0xcf -- it could generate
> wrong mapping. (i.e) text_prot is expected with
> PAGE_KERNEL_ROX(0x0040000000000F83) but
> with garbage x8 -- 0xcf, it sets with (0x0040000000067F83)
> and This makes boot failure with translation fault.
> 
> This error cannot happen according to code generated by compiler.
> here is the case of gcc:
> 
>    ffff80008260a940 <__pi_create_init_idmap>:
>    ffff80008260a940: d100c3ff      sub     sp, sp, #0x30
>    ffff80008260a944: aa0003ed      mov     x13, x0
>    ffff80008260a948: 91400400      add     x0, x0, #0x1, lsl #12 // =0x1000
>    ffff80008260a94c: a9017bfd      stp     x29, x30, [sp, #0x10]
>    ffff80008260a950: 910043fd      add     x29, sp, #0x10
>    ffff80008260a954: f90017e0      str     x0, [sp, #0x28]
>    ffff80008260a958: d00048c0      adrp    x0, 0xffff800082f24000 <reset_devices>
>    ffff80008260a95c: 394aa000      ldrb    w0, [x0, #0x2a8]
>    ffff80008260a960: 37000640      tbnz    w0, #0x0, 0xffff80008260aa28 <__pi_create_init_idmap+0xe8> ---(3)
>    ffff80008260a964: d280f060      mov     x0, #0x783      // =1923
>    ffff80008260a968: d280e062      mov     x2, #0x703      // =1795
>    ffff80008260a96c: f2e00800      movk    x0, #0x40, lsl #48
>    ffff80008260a970: f2e00d02      movk    x2, #0x68, lsl #48
>    ffff80008260a974: aa2103e4      mvn     x4, x1
>    ffff80008260a978: 8a210049      bic     x9, x2, x1
>    ...
>    ffff80008260aa28: d281f060      mov     x0, #0xf83      // =3971
>    ffff80008260aa2c: d281e062      mov     x2, #0xf03      // =3843
>    ffff80008260aa30: f2e00800      movk    x0, #0x40, lsl #48
> 
> 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.
> 
> To resolve this, annotate arm64_use_ng_mappings as ro_after_init.
> 
> Fixes: 84b04d3e6bdb ("arm64: kernel: Create initial ID map from C code")
> Cc: <stable@vger.kernel.org> # 6.9.x
> Signed-off-by: Yeoreum Yun <yeoreum.yun@arm.com>
> ---

This appears to resolve the issue that I reported to LLVM upstream:

https://github.com/llvm/llvm-project/issues/138019

Tested-by: Nathan Chancellor <nathan@kernel.org>

It does not look like there is anything for the compiler to fix in this
case, correct?

> ---
>  arch/arm64/kernel/cpufeature.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index d2104a1e7843..967ffb1cbd52 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -114,7 +114,7 @@ static struct arm64_cpu_capabilities const __ro_after_init *cpucap_ptrs[ARM64_NC
> 
>  DECLARE_BITMAP(boot_cpucaps, ARM64_NCAPS);
> 
> -bool arm64_use_ng_mappings = false;
> +bool arm64_use_ng_mappings __ro_after_init = false;
>  EXPORT_SYMBOL(arm64_use_ng_mappings);
> 
>  DEFINE_PER_CPU_READ_MOSTLY(const char *, this_cpu_vector) = vectors;
> --
> LEVI:{C3F47F37-75D8-414A-A8BA-3980EC8A46D7}
> 
> 

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

[Ard Biesheuvel]

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

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

[Yeoreum Yun]

Hi Nathan,

> Hi Yeoreum,
>
> On Fri, May 02, 2025 at 03:57:55PM +0100, Yeoreum Yun 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)
> >
> > Note (1) is load the arm64_use_ng_mappings value in w8.
> > and (2) is set the text or data prot with the w8 value to set PTE_NG bit.
> > If .bss section doesn't initialized, x8 can include garbage value
> > -- In case of some platform, x8 loaded with 0xcf -- it could generate
> > wrong mapping. (i.e) text_prot is expected with
> > PAGE_KERNEL_ROX(0x0040000000000F83) but
> > with garbage x8 -- 0xcf, it sets with (0x0040000000067F83)
> > and This makes boot failure with translation fault.
> >
> > This error cannot happen according to code generated by compiler.
> > here is the case of gcc:
> >
> >    ffff80008260a940 <__pi_create_init_idmap>:
> >    ffff80008260a940: d100c3ff      sub     sp, sp, #0x30
> >    ffff80008260a944: aa0003ed      mov     x13, x0
> >    ffff80008260a948: 91400400      add     x0, x0, #0x1, lsl #12 // =0x1000
> >    ffff80008260a94c: a9017bfd      stp     x29, x30, [sp, #0x10]
> >    ffff80008260a950: 910043fd      add     x29, sp, #0x10
> >    ffff80008260a954: f90017e0      str     x0, [sp, #0x28]
> >    ffff80008260a958: d00048c0      adrp    x0, 0xffff800082f24000 <reset_devices>
> >    ffff80008260a95c: 394aa000      ldrb    w0, [x0, #0x2a8]
> >    ffff80008260a960: 37000640      tbnz    w0, #0x0, 0xffff80008260aa28 <__pi_create_init_idmap+0xe8> ---(3)
> >    ffff80008260a964: d280f060      mov     x0, #0x783      // =1923
> >    ffff80008260a968: d280e062      mov     x2, #0x703      // =1795
> >    ffff80008260a96c: f2e00800      movk    x0, #0x40, lsl #48
> >    ffff80008260a970: f2e00d02      movk    x2, #0x68, lsl #48
> >    ffff80008260a974: aa2103e4      mvn     x4, x1
> >    ffff80008260a978: 8a210049      bic     x9, x2, x1
> >    ...
> >    ffff80008260aa28: d281f060      mov     x0, #0xf83      // =3971
> >    ffff80008260aa2c: d281e062      mov     x2, #0xf03      // =3843
> >    ffff80008260aa30: f2e00800      movk    x0, #0x40, lsl #48
> >
> > 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.
> >
> > To resolve this, annotate arm64_use_ng_mappings as ro_after_init.
> >
> > Fixes: 84b04d3e6bdb ("arm64: kernel: Create initial ID map from C code")
> > Cc: <stable@vger.kernel.org> # 6.9.x
> > Signed-off-by: Yeoreum Yun <yeoreum.yun@arm.com>
> > ---
>
> This appears to resolve the issue that I reported to LLVM upstream:
>
> https://github.com/llvm/llvm-project/issues/138019
>
> Tested-by: Nathan Chancellor <nathan@kernel.org>
>
> It does not look like there is anything for the compiler to fix in this
> case, correct?

No. There's no need any change in compiler.

Thanks!
>
> > ---
> >  arch/arm64/kernel/cpufeature.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> > index d2104a1e7843..967ffb1cbd52 100644
> > --- a/arch/arm64/kernel/cpufeature.c
> > +++ b/arch/arm64/kernel/cpufeature.c
> > @@ -114,7 +114,7 @@ static struct arm64_cpu_capabilities const __ro_after_init *cpucap_ptrs[ARM64_NC
> >
> >  DECLARE_BITMAP(boot_cpucaps, ARM64_NCAPS);
> >
> > -bool arm64_use_ng_mappings = false;
> > +bool arm64_use_ng_mappings __ro_after_init = false;
> >  EXPORT_SYMBOL(arm64_use_ng_mappings);
> >
> >  DEFINE_PER_CPU_READ_MOSTLY(const char *, this_cpu_vector) = vectors;
> > --
> > LEVI:{C3F47F37-75D8-414A-A8BA-3980EC8A46D7}
> >
> >

--
Sincerely,
Yeoreum Yun

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

[Yeoreum Yun]

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

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

[Catalin Marinas]

On Fri, May 02, 2025 at 06:41:33PM +0200, Ard Biesheuvel wrote:
> 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.

Maybe something like the section reference checker we use for __init -
verify that the early C code does not refer anything in the BSS section.

-- 
Catalin

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

[Yeoreum Yun]

> On Fri, May 02, 2025 at 06:41:33PM +0200, Ard Biesheuvel wrote:
> > 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.
>
> Maybe something like the section reference checker we use for __init -
> verify that the early C code does not refer anything in the BSS section.

Maybe but it would be better to be checked at compile time (I don't
know it's possible) otherwise, early C code writer should check
mandatroy by calling is_kernel_bss_data() (not exist) for data it refers.

> --
> Catalin

--
Sincerely,
Yeoreum Yun

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

[Catalin Marinas]

On Fri, May 02, 2025 at 07:14:12PM +0100, Yeoreum Yun wrote:
> > On Fri, May 02, 2025 at 06:41:33PM +0200, Ard Biesheuvel wrote:
> > > 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.
> >
> > Maybe something like the section reference checker we use for __init -
> > verify that the early C code does not refer anything in the BSS section.
> 
> Maybe but it would be better to be checked at compile time (I don't
> know it's possible) otherwise, early C code writer should check
> mandatroy by calling is_kernel_bss_data() (not exist) for data it refers.

This would be compile time (or rather final link time). See
scripts/mod/modpost.c (the sectioncheck[] array) on how we check if, for
example, a .text section references a .init one. We could move the whole
pi code to its own section (e.g. .init.nommu.*) and add modpost checks
for references to the bss or other sections.

-- 
Catalin

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

[Ard Biesheuvel]

On Sat, 3 May 2025 at 12:06, Catalin Marinas <catalin.marinas@arm.com> wrote:
>
> On Fri, May 02, 2025 at 07:14:12PM +0100, Yeoreum Yun wrote:
> > > On Fri, May 02, 2025 at 06:41:33PM +0200, Ard Biesheuvel wrote:
> > > > 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.
> > >
> > > Maybe something like the section reference checker we use for __init -
> > > verify that the early C code does not refer anything in the BSS section.
> >
> > Maybe but it would be better to be checked at compile time (I don't
> > know it's possible) otherwise, early C code writer should check
> > mandatroy by calling is_kernel_bss_data() (not exist) for data it refers.
>
> This would be compile time (or rather final link time). See
> scripts/mod/modpost.c (the sectioncheck[] array) on how we check if, for
> example, a .text section references a .init one. We could move the whole
> pi code to its own section (e.g. .init.nommu.*) and add modpost checks
> for references to the bss or other sections.
>

We can take care of this locally in image-vars.h, where each variable
used by the startup code is made available to it explicitly.

Sending out a separate series shortly.

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

[Yeoreum Yun]

On Sat, May 03, 2025 at 11:06:03AM +0100, Catalin Marinas wrote:
> On Fri, May 02, 2025 at 07:14:12PM +0100, Yeoreum Yun wrote:
> > > On Fri, May 02, 2025 at 06:41:33PM +0200, Ard Biesheuvel wrote:
> > > > 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.
> > >
> > > Maybe something like the section reference checker we use for __init -
> > > verify that the early C code does not refer anything in the BSS section.
> >
> > Maybe but it would be better to be checked at compile time (I don't
> > know it's possible) otherwise, early C code writer should check
> > mandatroy by calling is_kernel_bss_data() (not exist) for data it refers.
>
> This would be compile time (or rather final link time). See
> scripts/mod/modpost.c (the sectioncheck[] array) on how we check if, for
> example, a .text section references a .init one. We could move the whole
> pi code to its own section (e.g. .init.nommu.*) and add modpost checks
> for references to the bss or other sections.

Oh, only thought about some compiler option.
and Thanks to let me know!

> --
> Catalin

--
Sincerely,
Yeoreum Yun