[PATCH v2 0/2] x86/sev-es: Resolve early #VC handler UB

[PATCH v2 0/2] x86/sev-es: Resolve early #VC handler UB

[Adam Dunlap]

In v0 of this patch, I mistakenly claimed that
boot_cpu_data.x86_virt_bits was not getting set to 0 in some
configurations. However, it is in fact reliably being set to 0 because
it is specified to be in the .data..read_mostly section, and .data is
initialized at this point. The boot failures that we were seeing were
actually due to the gcc 12.3 compiler exploiting the undefined
behavior to return false from __is_canonical_address when
boot_cpu_data.x86_virt_bits is 0.

This patch set applies the 2 changes posted by Dave Hansen: remove the
UB by explicily checking for 0, and remove the period of time where
x86_virt_bits may be temporarily set to the wrong value where the #VC
handler could observe it.


Adam Dunlap (2):
  x86/sev-es: Allow copy_from_kernel_nofault in earlier boot
  x86/sev-es: Only set x86_virt_bits to correct value

 arch/x86/kernel/cpu/common.c | 37 +++++++++++++++++++++---------------
 arch/x86/mm/maccess.c        | 19 +++++++++++++-----
 2 files changed, 36 insertions(+), 20 deletions(-)

-- 
2.42.0.283.g2d96d420d3-goog

[PATCH v2 1/2] x86/sev-es: Allow copy_from_kernel_nofault in earlier boot

[Adam Dunlap]

Previously, if copy_from_kernel_nofault was called before
boot_cpu_data.x86_virt_bits was set up, then it would trigger undefined
behavior due to a shift by 64. This ended up causing boot failures in
the latest version of ubuntu2204 in the gcp project when using SEV-SNP.
Specifically, this function is called during an early #VC handler which
is triggered by a cpuid to check if nx is implemented.

Fixes: 1aa9aa8ee517 ("x86/sev-es: Setup GHCB-based boot #VC handler")
Suggested-by: Dave Hansen <dave.hansen@linux.intel.com>
Signed-off-by: Adam Dunlap <acdunlap@google.com>
---
 arch/x86/mm/maccess.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/arch/x86/mm/maccess.c b/arch/x86/mm/maccess.c
index 5a53c2cc169c..6993f026adec 100644
--- a/arch/x86/mm/maccess.c
+++ b/arch/x86/mm/maccess.c
@@ -9,12 +9,21 @@ bool copy_from_kernel_nofault_allowed(const void *unsafe_src, size_t size)
 	unsigned long vaddr = (unsigned long)unsafe_src;
 
 	/*
-	 * Range covering the highest possible canonical userspace address
-	 * as well as non-canonical address range. For the canonical range
-	 * we also need to include the userspace guard page.
+	 * Do not allow userspace addresses.  This disallows
+	 * normal userspace and the userspace guard page:
 	 */
-	return vaddr >= TASK_SIZE_MAX + PAGE_SIZE &&
-	       __is_canonical_address(vaddr, boot_cpu_data.x86_virt_bits);
+	if (vaddr < TASK_SIZE_MAX + PAGE_SIZE)
+		return false;
+
+	/*
+	 * Allow everything during early boot before 'x86_virt_bits'
+	 * is initialized.  Needed for instruction decoding in early
+	 * exception handlers.
+	 */
+	if (!boot_cpu_data.x86_virt_bits)
+		return true;
+
+	return __is_canonical_address(vaddr, boot_cpu_data.x86_virt_bits);
 }
 #else
 bool copy_from_kernel_nofault_allowed(const void *unsafe_src, size_t size)
-- 
2.42.0.283.g2d96d420d3-goog

[PATCH v2 2/2] x86/sev-es: Only set x86_virt_bits to correct value

[Adam Dunlap]

Instead of setting x86_virt_bits to a possibly-correct value and then
correcting it later, do all the necessary checks before setting it.

At this point, the #VC handler references boot_cpu_data.x86_virt_bits,
and in the previous version, it would be triggered by the cpuids between
the point at which it is set to 48 and when it is set to the correct
value.

Suggested-by: Dave Hansen <dave.hansen@linux.intel.com>
Signed-off-by: Adam Dunlap <acdunlap@google.com>
---
 arch/x86/kernel/cpu/common.c | 37 +++++++++++++++++++++---------------
 1 file changed, 22 insertions(+), 15 deletions(-)

diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 52683fddafaf..23888d3da16f 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1099,17 +1099,32 @@ void get_cpu_cap(struct cpuinfo_x86 *c)
 void get_cpu_address_sizes(struct cpuinfo_x86 *c)
 {
 	u32 eax, ebx, ecx, edx;
+	bool vp_bits_from_cpuid = true;
 
-	if (c->extended_cpuid_level >= 0x80000008) {
+	if (!cpu_has(c, X86_FEATURE_CPUID) ||
+	    (c->extended_cpuid_level < 0x80000008))
+		vp_bits_from_cpuid = false;
+
+	if (vp_bits_from_cpuid) {
 		cpuid(0x80000008, &eax, &ebx, &ecx, &edx);
 
 		c->x86_virt_bits = (eax >> 8) & 0xff;
 		c->x86_phys_bits = eax & 0xff;
+	} else {
+		if (IS_ENABLED(CONFIG_X86_64)) {
+			c->x86_clflush_size = 64;
+			c->x86_phys_bits = 36;
+			c->x86_virt_bits = 48;
+		} else {
+			c->x86_clflush_size = 32;
+			c->x86_virt_bits = 32;
+			c->x86_phys_bits = 32;
+
+			if (cpu_has(c, X86_FEATURE_PAE) ||
+			    cpu_has(c, X86_FEATURE_PSE36))
+				c->x86_phys_bits = 36;
+		}
 	}
-#ifdef CONFIG_X86_32
-	else if (cpu_has(c, X86_FEATURE_PAE) || cpu_has(c, X86_FEATURE_PSE36))
-		c->x86_phys_bits = 36;
-#endif
 	c->x86_cache_bits = c->x86_phys_bits;
 }
 
@@ -1539,15 +1554,6 @@ static void __init cpu_parse_early_param(void)
  */
 static void __init early_identify_cpu(struct cpuinfo_x86 *c)
 {
-#ifdef CONFIG_X86_64
-	c->x86_clflush_size = 64;
-	c->x86_phys_bits = 36;
-	c->x86_virt_bits = 48;
-#else
-	c->x86_clflush_size = 32;
-	c->x86_phys_bits = 32;
-	c->x86_virt_bits = 32;
-#endif
 	c->x86_cache_alignment = c->x86_clflush_size;
 
 	memset(&c->x86_capability, 0, sizeof(c->x86_capability));
@@ -1561,7 +1567,6 @@ static void __init early_identify_cpu(struct cpuinfo_x86 *c)
 		cpu_detect(c);
 		get_cpu_vendor(c);
 		get_cpu_cap(c);
-		get_cpu_address_sizes(c);
 		setup_force_cpu_cap(X86_FEATURE_CPUID);
 		cpu_parse_early_param();
 
@@ -1577,6 +1582,8 @@ static void __init early_identify_cpu(struct cpuinfo_x86 *c)
 		setup_clear_cpu_cap(X86_FEATURE_CPUID);
 	}
 
+	get_cpu_address_sizes(c);
+
 	setup_force_cpu_cap(X86_FEATURE_ALWAYS);
 
 	cpu_set_bug_bits(c);
-- 
2.42.0.283.g2d96d420d3-goog

Re: [PATCH v2 1/2] x86/sev-es: Allow copy_from_kernel_nofault in earlier boot

[Sean Christopherson]

On Mon, Sep 11, 2023, Adam Dunlap wrote:
> Previously, if copy_from_kernel_nofault was called before
> boot_cpu_data.x86_virt_bits was set up, then it would trigger undefined
> behavior due to a shift by 64. This ended up causing boot failures in
> the latest version of ubuntu2204 in the gcp project when using SEV-SNP.
> Specifically, this function is called during an early #VC handler which
> is triggered by a cpuid to check if nx is implemented.

Why not stuff boot_cpu_data.x86_virt_bits to a "default" value that is guaranteed
to be accurate (or at least safe) for the purposes of the early boot code.  I.e.
set it to 48 for 64-bit kernels.

That'd avoid the extra conditional, and would avoid laying whack-a-mole with
anything else that consumes x86_virt_bits.

> Fixes: 1aa9aa8ee517 ("x86/sev-es: Setup GHCB-based boot #VC handler")
> Suggested-by: Dave Hansen <dave.hansen@linux.intel.com>
> Signed-off-by: Adam Dunlap <acdunlap@google.com>
> ---
>  arch/x86/mm/maccess.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/mm/maccess.c b/arch/x86/mm/maccess.c
> index 5a53c2cc169c..6993f026adec 100644
> --- a/arch/x86/mm/maccess.c
> +++ b/arch/x86/mm/maccess.c
> @@ -9,12 +9,21 @@ bool copy_from_kernel_nofault_allowed(const void *unsafe_src, size_t size)
>  	unsigned long vaddr = (unsigned long)unsafe_src;
>  
>  	/*
> -	 * Range covering the highest possible canonical userspace address
> -	 * as well as non-canonical address range. For the canonical range
> -	 * we also need to include the userspace guard page.
> +	 * Do not allow userspace addresses.  This disallows
> +	 * normal userspace and the userspace guard page:
>  	 */
> -	return vaddr >= TASK_SIZE_MAX + PAGE_SIZE &&
> -	       __is_canonical_address(vaddr, boot_cpu_data.x86_virt_bits);
> +	if (vaddr < TASK_SIZE_MAX + PAGE_SIZE)
> +		return false;
> +
> +	/*
> +	 * Allow everything during early boot before 'x86_virt_bits'
> +	 * is initialized.  Needed for instruction decoding in early
> +	 * exception handlers.
> +	 */
> +	if (!boot_cpu_data.x86_virt_bits)
> +		return true;
> +
> +	return __is_canonical_address(vaddr, boot_cpu_data.x86_virt_bits);
>  }
>  #else
>  bool copy_from_kernel_nofault_allowed(const void *unsafe_src, size_t size)
> -- 
> 2.42.0.283.g2d96d420d3-goog
> 

Re: [PATCH v2 1/2] x86/sev-es: Allow copy_from_kernel_nofault in earlier boot

[Dave Hansen]

On 9/20/23 13:37, Sean Christopherson wrote:
> On Mon, Sep 11, 2023, Adam Dunlap wrote:
>> Previously, if copy_from_kernel_nofault was called before
>> boot_cpu_data.x86_virt_bits was set up, then it would trigger undefined
>> behavior due to a shift by 64. This ended up causing boot failures in
>> the latest version of ubuntu2204 in the gcp project when using SEV-SNP.
>> Specifically, this function is called during an early #VC handler which
>> is triggered by a cpuid to check if nx is implemented.
> Why not stuff boot_cpu_data.x86_virt_bits to a "default" value that is guaranteed
> to be accurate (or at least safe) for the purposes of the early boot code.  I.e.
> set it to 48 for 64-bit kernels.
> 
> That'd avoid the extra conditional, and would avoid laying whack-a-mole with
> anything else that consumes x86_virt_bits.

I'd be worried that could break things even more subtly.

If we're truly worried about whack-a-mole, we should stick
'x86_virt_bits' in a wrapper, whine if it's accessed inadvertently, and
*then* return some mostly sane data.

That way we can actually go look at the caller and see what the heck
it's doing.

I did poke around and managed to convince myself that this site _is_ the
only one at the moment.

Re: [PATCH v2 0/2] x86/sev-es: Resolve early #VC handler UB

[Ingo Molnar]

* Adam Dunlap <acdunlap@google.com> wrote:

> In v0 of this patch, I mistakenly claimed that
> boot_cpu_data.x86_virt_bits was not getting set to 0 in some
> configurations. However, it is in fact reliably being set to 0 because
> it is specified to be in the .data..read_mostly section, and .data is
> initialized at this point. The boot failures that we were seeing were
> actually due to the gcc 12.3 compiler exploiting the undefined
> behavior to return false from __is_canonical_address when
> boot_cpu_data.x86_virt_bits is 0.
> 
> This patch set applies the 2 changes posted by Dave Hansen: remove the
> UB by explicily checking for 0, and remove the period of time where
> x86_virt_bits may be temporarily set to the wrong value where the #VC
> handler could observe it.
> 
> 
> Adam Dunlap (2):
>   x86/sev-es: Allow copy_from_kernel_nofault in earlier boot
>   x86/sev-es: Only set x86_virt_bits to correct value

I've applied these fixes to tip:x86/mm, thanks!

Note that I switched around the order of patches: we first want
to enhance x86_virt_bits to be reliable, only then do we want to
change copy_from_kernel_nofault_allowed() to be able to rely on it,
to fix early #VC accesses, right?

I also improved the changelogs & titles a bit.

Thanks,

	Ingo

[tip: x86/mm] x86/sev-es: Set x86_virt_bits to the correct value straight away, instead of a two-phase approach

[tip-bot2 for Adam Dunlap]

The following commit has been merged into the x86/mm branch of tip:

Commit-ID:     fbf6449f84bf5e4ad09f2c09ee70ed7d629b5ff6
Gitweb:        https://git.kernel.org/tip/fbf6449f84bf5e4ad09f2c09ee70ed7d629b5ff6
Author:        Adam Dunlap <acdunlap@google.com>
AuthorDate:    Mon, 11 Sep 2023 17:27:03 -07:00
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Thu, 28 Sep 2023 22:49:35 +02:00

x86/sev-es: Set x86_virt_bits to the correct value straight away, instead of a two-phase approach

Instead of setting x86_virt_bits to a possibly-correct value and then
correcting it later, do all the necessary checks before setting it.

At this point, the #VC handler references boot_cpu_data.x86_virt_bits,
and in the previous version, it would be triggered by the CPUIDs between
the point at which it is set to 48 and when it is set to the correct
value.

Suggested-by: Dave Hansen <dave.hansen@linux.intel.com>
Signed-off-by: Adam Dunlap <acdunlap@google.com>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Tested-by: Jacob Xu <jacobhxu@google.com>
Link: https://lore.kernel.org/r/20230912002703.3924521-3-acdunlap@google.com
---
 arch/x86/kernel/cpu/common.c | 37 ++++++++++++++++++++---------------
 1 file changed, 22 insertions(+), 15 deletions(-)

diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 382d4e6..8d7063e 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1114,17 +1114,32 @@ void get_cpu_cap(struct cpuinfo_x86 *c)
 void get_cpu_address_sizes(struct cpuinfo_x86 *c)
 {
 	u32 eax, ebx, ecx, edx;
+	bool vp_bits_from_cpuid = true;
 
-	if (c->extended_cpuid_level >= 0x80000008) {
+	if (!cpu_has(c, X86_FEATURE_CPUID) ||
+	    (c->extended_cpuid_level < 0x80000008))
+		vp_bits_from_cpuid = false;
+
+	if (vp_bits_from_cpuid) {
 		cpuid(0x80000008, &eax, &ebx, &ecx, &edx);
 
 		c->x86_virt_bits = (eax >> 8) & 0xff;
 		c->x86_phys_bits = eax & 0xff;
+	} else {
+		if (IS_ENABLED(CONFIG_X86_64)) {
+			c->x86_clflush_size = 64;
+			c->x86_phys_bits = 36;
+			c->x86_virt_bits = 48;
+		} else {
+			c->x86_clflush_size = 32;
+			c->x86_virt_bits = 32;
+			c->x86_phys_bits = 32;
+
+			if (cpu_has(c, X86_FEATURE_PAE) ||
+			    cpu_has(c, X86_FEATURE_PSE36))
+				c->x86_phys_bits = 36;
+		}
 	}
-#ifdef CONFIG_X86_32
-	else if (cpu_has(c, X86_FEATURE_PAE) || cpu_has(c, X86_FEATURE_PSE36))
-		c->x86_phys_bits = 36;
-#endif
 	c->x86_cache_bits = c->x86_phys_bits;
 }
 
@@ -1579,15 +1594,6 @@ static void __init cpu_parse_early_param(void)
  */
 static void __init early_identify_cpu(struct cpuinfo_x86 *c)
 {
-#ifdef CONFIG_X86_64
-	c->x86_clflush_size = 64;
-	c->x86_phys_bits = 36;
-	c->x86_virt_bits = 48;
-#else
-	c->x86_clflush_size = 32;
-	c->x86_phys_bits = 32;
-	c->x86_virt_bits = 32;
-#endif
 	c->x86_cache_alignment = c->x86_clflush_size;
 
 	memset(&c->x86_capability, 0, sizeof(c->x86_capability));
@@ -1601,7 +1607,6 @@ static void __init early_identify_cpu(struct cpuinfo_x86 *c)
 		cpu_detect(c);
 		get_cpu_vendor(c);
 		get_cpu_cap(c);
-		get_cpu_address_sizes(c);
 		setup_force_cpu_cap(X86_FEATURE_CPUID);
 		cpu_parse_early_param();
 
@@ -1617,6 +1622,8 @@ static void __init early_identify_cpu(struct cpuinfo_x86 *c)
 		setup_clear_cpu_cap(X86_FEATURE_CPUID);
 	}
 
+	get_cpu_address_sizes(c);
+
 	setup_force_cpu_cap(X86_FEATURE_ALWAYS);
 
 	cpu_set_bug_bits(c);

[tip: x86/mm] x86/sev-es: Allow copy_from_kernel_nofault() in earlier boot

[tip-bot2 for Adam Dunlap]

The following commit has been merged into the x86/mm branch of tip:

Commit-ID:     f79936545fb122856bd78b189d3c7ee59928c751
Gitweb:        https://git.kernel.org/tip/f79936545fb122856bd78b189d3c7ee59928c751
Author:        Adam Dunlap <acdunlap@google.com>
AuthorDate:    Mon, 11 Sep 2023 17:27:02 -07:00
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Thu, 28 Sep 2023 22:49:35 +02:00

x86/sev-es: Allow copy_from_kernel_nofault() in earlier boot

Previously, if copy_from_kernel_nofault() was called before
boot_cpu_data.x86_virt_bits was set up, then it would trigger undefined
behavior due to a shift by 64.

This ended up causing boot failures in the latest version of ubuntu2204
in the gcp project when using SEV-SNP.

Specifically, this function is called during an early #VC handler which
is triggered by a CPUID to check if NX is implemented.

Fixes: 1aa9aa8ee517 ("x86/sev-es: Setup GHCB-based boot #VC handler")
Suggested-by: Dave Hansen <dave.hansen@linux.intel.com>
Signed-off-by: Adam Dunlap <acdunlap@google.com>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Tested-by: Jacob Xu <jacobhxu@google.com>
Link: https://lore.kernel.org/r/20230912002703.3924521-2-acdunlap@google.com
---
 arch/x86/mm/maccess.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/arch/x86/mm/maccess.c b/arch/x86/mm/maccess.c
index 5a53c2c..6993f02 100644
--- a/arch/x86/mm/maccess.c
+++ b/arch/x86/mm/maccess.c
@@ -9,12 +9,21 @@ bool copy_from_kernel_nofault_allowed(const void *unsafe_src, size_t size)
 	unsigned long vaddr = (unsigned long)unsafe_src;
 
 	/*
-	 * Range covering the highest possible canonical userspace address
-	 * as well as non-canonical address range. For the canonical range
-	 * we also need to include the userspace guard page.
+	 * Do not allow userspace addresses.  This disallows
+	 * normal userspace and the userspace guard page:
 	 */
-	return vaddr >= TASK_SIZE_MAX + PAGE_SIZE &&
-	       __is_canonical_address(vaddr, boot_cpu_data.x86_virt_bits);
+	if (vaddr < TASK_SIZE_MAX + PAGE_SIZE)
+		return false;
+
+	/*
+	 * Allow everything during early boot before 'x86_virt_bits'
+	 * is initialized.  Needed for instruction decoding in early
+	 * exception handlers.
+	 */
+	if (!boot_cpu_data.x86_virt_bits)
+		return true;
+
+	return __is_canonical_address(vaddr, boot_cpu_data.x86_virt_bits);
 }
 #else
 bool copy_from_kernel_nofault_allowed(const void *unsafe_src, size_t size)

Re: [PATCH v2 2/2] x86/sev-es: Only set x86_virt_bits to correct value

[Nathan Chancellor]

Hi Adam,

On Mon, Sep 11, 2023 at 05:27:03PM -0700, Adam Dunlap wrote:
> Instead of setting x86_virt_bits to a possibly-correct value and then
> correcting it later, do all the necessary checks before setting it.
> 
> At this point, the #VC handler references boot_cpu_data.x86_virt_bits,
> and in the previous version, it would be triggered by the cpuids between
> the point at which it is set to 48 and when it is set to the correct
> value.
> 
> Suggested-by: Dave Hansen <dave.hansen@linux.intel.com>
> Signed-off-by: Adam Dunlap <acdunlap@google.com>

Our continuous integration started seeing panics when booting ARCH=i386
without KVM after this change landed in -tip as commit fbf6449f84bf
("x86/sev-es: Set x86_virt_bits to the correct value straight away,
instead of a two-phase approach"):

https://github.com/ClangBuiltLinux/continuous-integration2/actions/runs/6374441341/job/17299441736

I can reproduce this with GCC 13.2.0 from kernel.org and QEMU 8.1.1 (in
case those versions matter):

https://mirrors.edge.kernel.org/pub/tools/crosstool/

$ make -skj"$(nproc)" ARCH=i386 CROSS_COMPILE=i386-linux- defconfig bzImage

$ qemu-system-i386 \
    -display none \
    -nodefaults \
    -append 'console=ttyS0 earlycon=uart8250,io,0x3f8' \
    -kernel arch/x86/boot/bzImage \
    -initrd rootfs.cpio \
    -m 512m \
    -serial mon:stdio
[    0.000000] Linux version 6.6.0-rc1-00008-gfbf6449f84bf (nathan@dev-arch.thelio-3990X) (i386-linux-gcc (GCC) 13.2.0, GNU ld (GNU Binutils) 2.41) #1 SMP PREEMPT_DYNAMIC Mon Oct  2 12:55:06 MST 2023
...
[    0.061831] BUG: kernel NULL pointer dereference, address: 00000020
[    0.062135] #PF: supervisor write access in kernel mode
[    0.062279] #PF: error_code(0x0002) - not-present page
[    0.062430] *pde = 00000000
[    0.062602] Oops: 0002 [#1] PREEMPT SMP
[    0.062839] CPU: 0 PID: 0 Comm: swapper Not tainted 6.6.0-rc1-00008-gfbf6449f84bf #1
[    0.063086] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.2-0-gea1b7a073390-prebuilt.qemu.org 04/01/2014
[    0.063515] EIP: __ring_buffer_alloc+0x3a/0x1a4
[    0.064135] Code: 8b 15 74 da c2 ce 8d 42 6f f7 da 21 d0 ba c0 0d 00 00 e8 2d d0 07 00 85 c0 0f 84 15 01 00 00 8b 4d f0 89 c3 81 c6 f3 0f 00 00 <c7> 40 10 00 00 00 00 8d 40 4c ba 2d a7 8b ce 89 78 b4 c7 40 ec 60
[    0.064795] EAX: 00000010 EBX: 00000010 ECX: ced82e8d EDX: 00000dc0
[    0.064970] ESI: 00001ff3 EDI: 00000001 EBP: cea3df54 ESP: cea3df44
[    0.065151] DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068 EFLAGS: 00200006
[    0.065347] CR0: 80050033 CR2: 00000020 CR3: 0ed2d000 CR4: 00000090
[    0.065596] Call Trace:
[    0.066142]  ? show_regs+0x4d/0x54
[    0.066293]  ? __die+0x21/0x68
[    0.066383]  ? page_fault_oops+0x18c/0x368
[    0.066502]  ? kernelmode_fixup_or_oops.constprop.0+0x84/0xdc
[    0.066663]  ? __bad_area_nosemaphore.constprop.0+0x125/0x1dc
[    0.066811]  ? __alloc_pages+0x156/0xe34
[    0.066920]  ? bad_area_nosemaphore+0x12/0x18
[    0.067036]  ? do_user_addr_fault+0x133/0x3e8
[    0.067156]  ? exc_page_fault+0x51/0x13c
[    0.067269]  ? pvclock_clocksource_read_nowd+0x110/0x110
[    0.067421]  ? handle_exception+0x133/0x133
[    0.067553]  ? pvclock_clocksource_read_nowd+0x110/0x110
[    0.067724]  ? __ring_buffer_alloc+0x3a/0x1a4
[    0.067846]  ? pvclock_clocksource_read_nowd+0x110/0x110
[    0.067983]  ? __ring_buffer_alloc+0x3a/0x1a4
[    0.068113]  early_trace_init+0xab/0x390
[    0.068296]  ? ring_buffer_reset_online_cpus+0xfc/0xfc
[    0.068443]  start_kernel+0x217/0x650
[    0.068542]  ? set_init_arg+0x70/0x70
[    0.068643]  i386_start_kernel+0x43/0x44
[    0.068754]  startup_32_smp+0x156/0x158
[    0.068960] Modules linked in:
[    0.069166] CR2: 0000000000000020
[    0.069495] ---[ end trace 0000000000000000 ]---
[    0.069653] EIP: __ring_buffer_alloc+0x3a/0x1a4
[    0.069781] Code: 8b 15 74 da c2 ce 8d 42 6f f7 da 21 d0 ba c0 0d 00 00 e8 2d d0 07 00 85 c0 0f 84 15 01 00 00 8b 4d f0 89 c3 81 c6 f3 0f 00 00 <c7> 40 10 00 00 00 00 8d 40 4c ba 2d a7 8b ce 89 78 b4 c7 40 ec 60
[    0.070249] EAX: 00000010 EBX: 00000010 ECX: ced82e8d EDX: 00000dc0
[    0.070412] ESI: 00001ff3 EDI: 00000001 EBP: cea3df54 ESP: cea3df44
[    0.070577] DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068 EFLAGS: 00200006
[    0.070761] CR0: 80050033 CR2: 00000020 CR3: 0ed2d000 CR4: 00000090
[    0.071023] Kernel panic - not syncing: Attempted to kill the idle task!
[    0.071643] ---[ end Kernel panic - not syncing: Attempted to kill the idle task! ]---
...

In case it is necessary, our rootfs image is available at
https://github.com/ClangBuiltLinux/boot-utils/releases.

As mentioned before, this interestingly does not reproduce with
'-enable-kvm', so it could be potentially related to QEMU's TCG. If
there is any additional information I can provide or patches I can test,
I am more than happy to do so.

Cheers,
Nathan

# bad: [18a1f37f57ff28fa544124151ef0171ffdafd162] Merge branch into tip/master: 'x86/tdx'
# good: [9f3ebbef746f89f860a90ced99a359202ea86fde] Merge tag '6.6-rc3-ksmbd-server-fixes' of git://git.samba.org/ksmbd
git bisect start '18a1f37f57ff28fa544124151ef0171ffdafd162' '9f3ebbef746f89f860a90ced99a359202ea86fde'
# good: [6395aa368403d595c8aab4ee1ac451b84fb37640] Merge branch into tip/master: 'sched/core'
git bisect good 6395aa368403d595c8aab4ee1ac451b84fb37640
# good: [7eddb9db1104079c5f5ac6b17a46f7a7860cf444] Merge branch into tip/master: 'x86/boot'
git bisect good 7eddb9db1104079c5f5ac6b17a46f7a7860cf444
# good: [3fb58599f5a9efaa422c9c9e78e3e799a58d4f63] Merge branch into tip/master: 'x86/entry'
git bisect good 3fb58599f5a9efaa422c9c9e78e3e799a58d4f63
# bad: [062ce2831f0526dec8930a35f1624c0ae50dd667] Merge branch into tip/master: 'x86/platform'
git bisect bad 062ce2831f0526dec8930a35f1624c0ae50dd667
# good: [f4c5ca9850124fb5715eff06cffb1beed837500c] x86_64: Show CR4.PSE on auxiliaries like on BSP
git bisect good f4c5ca9850124fb5715eff06cffb1beed837500c
# good: [24775700eaa93ff83b2a0f1e005879cdf186cdd9] x86/amd_nb: Add AMD Family MI300 PCI IDs
git bisect good 24775700eaa93ff83b2a0f1e005879cdf186cdd9
# bad: [fbf6449f84bf5e4ad09f2c09ee70ed7d629b5ff6] x86/sev-es: Set x86_virt_bits to the correct value straight away, instead of a two-phase approach
git bisect bad fbf6449f84bf5e4ad09f2c09ee70ed7d629b5ff6
# good: [f79936545fb122856bd78b189d3c7ee59928c751] x86/sev-es: Allow copy_from_kernel_nofault() in earlier boot
git bisect good f79936545fb122856bd78b189d3c7ee59928c751
# first bad commit: [fbf6449f84bf5e4ad09f2c09ee70ed7d629b5ff6] x86/sev-es: Set x86_virt_bits to the correct value straight away, instead of a two-phase approach

Re: [PATCH v2 2/2] x86/sev-es: Only set x86_virt_bits to correct value

[Dave Hansen]

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

On 10/2/23 13:04, Nathan Chancellor wrote:
> On Mon, Sep 11, 2023 at 05:27:03PM -0700, Adam Dunlap wrote:
>> Instead of setting x86_virt_bits to a possibly-correct value and then
>> correcting it later, do all the necessary checks before setting it.
>>
>> At this point, the #VC handler references boot_cpu_data.x86_virt_bits,
>> and in the previous version, it would be triggered by the cpuids between
>> the point at which it is set to 48 and when it is set to the correct
>> value.
>>
>> Suggested-by: Dave Hansen <dave.hansen@linux.intel.com>
>> Signed-off-by: Adam Dunlap <acdunlap@google.com>
> Our continuous integration started seeing panics when booting ARCH=i386
> without KVM after this change landed in -tip as commit fbf6449f84bf
> ("x86/sev-es: Set x86_virt_bits to the correct value straight away,
> instead of a two-phase approach"):

I can't reproduce this, but I'm running a gcc-built kernel and I haven't
tried very hard to replicate your qemu setup.

I did notice, though, that the patch in question forgot to move one
assignment.  Could you see if the attached patch fixes things for you?

[-- Attachment #2: cache-bits-debug.patch --]
[-- Type: text/x-patch, Size: 750 bytes --]

diff -puN arch/x86/kernel/cpu/common.c~cache-bits-debug arch/x86/kernel/cpu/common.c
--- a/arch/x86/kernel/cpu/common.c~cache-bits-debug	2023-10-02 14:20:48.273415423 -0700
+++ b/arch/x86/kernel/cpu/common.c	2023-10-02 14:24:52.710129051 -0700
@@ -1142,6 +1142,7 @@ void get_cpu_address_sizes(struct cpuinf
 		}
 	}
 	c->x86_cache_bits = c->x86_phys_bits;
+	c->x86_cache_alignment = c->x86_clflush_size;
 }
 
 static void identify_cpu_without_cpuid(struct cpuinfo_x86 *c)
@@ -1595,8 +1596,6 @@ static void __init cpu_parse_early_param
  */
 static void __init early_identify_cpu(struct cpuinfo_x86 *c)
 {
-	c->x86_cache_alignment = c->x86_clflush_size;
-
 	memset(&c->x86_capability, 0, sizeof(c->x86_capability));
 	c->extended_cpuid_level = 0;
 
_

Re: [PATCH v2 2/2] x86/sev-es: Only set x86_virt_bits to correct value

[Adam Dunlap]

On Mon, Oct 2, 2023 at 2:41 PM Dave Hansen <dave.hansen@intel.com> wrote:
>
> On 10/2/23 13:04, Nathan Chancellor wrote:
> > On Mon, Sep 11, 2023 at 05:27:03PM -0700, Adam Dunlap wrote:
> >> Instead of setting x86_virt_bits to a possibly-correct value and then
> >> correcting it later, do all the necessary checks before setting it.
> >>
> >> At this point, the #VC handler references boot_cpu_data.x86_virt_bits,
> >> and in the previous version, it would be triggered by the cpuids between
> >> the point at which it is set to 48 and when it is set to the correct
> >> value.
> >>
> >> Suggested-by: Dave Hansen <dave.hansen@linux.intel.com>
> >> Signed-off-by: Adam Dunlap <acdunlap@google.com>
> > Our continuous integration started seeing panics when booting ARCH=i386
> > without KVM after this change landed in -tip as commit fbf6449f84bf
> > ("x86/sev-es: Set x86_virt_bits to the correct value straight away,
> > instead of a two-phase approach"):
>
> I can't reproduce this, but I'm running a gcc-built kernel and I haven't
> tried very hard to replicate your qemu setup.
>
> I did notice, though, that the patch in question forgot to move one
> assignment.  Could you see if the attached patch fixes things for you?

I reproduced the issue as Nathan described and your attached patch fixes it
for me.

[PATCH] x86/boot: Move x86_cache_alignment initialization to correct spot

[Dave Hansen]

c->x86_cache_alignment is initialized from c->x86_clflush_size.
However, commit fbf6449f84bf moved c->x86_clflush_size initialization
to later in boot without moving the c->x86_cache_alignment assignment.

This presumably left c->x86_cache_alignment set to zero for longer
than it should be.

The result was an oops on 32-bit kernels while accessing a pointer
at 0x20.  The 0x20 came from accessing a structure member at offset
0x10 (buffer->cpumask) from a ZERO_SIZE_PTR=0x10.  kmalloc() can
evidently return ZERO_SIZE_PTR when it's given 0 as its alignment
requirement.

Move the c->x86_cache_alignment initialization to be after
c->x86_clflush_size has an actual value.

Fixes: fbf6449f84bf ("x86/sev-es: Set x86_virt_bits to the correct value straight away, instead of a two-phase approach")
Cc: Adam Dunlap <acdunlap@google.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jacob Xu <jacobhxu@google.com>
Link: https://lore.kernel.org/all/20231002200426.GA4127272@dev-arch.thelio-3990X/
---
 arch/x86/kernel/cpu/common.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 8d7063e4f63c9..9c51ad5bbf319 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1141,6 +1141,7 @@ void get_cpu_address_sizes(struct cpuinfo_x86 *c)
 		}
 	}
 	c->x86_cache_bits = c->x86_phys_bits;
+	c->x86_cache_alignment = c->x86_clflush_size;
 }
 
 static void identify_cpu_without_cpuid(struct cpuinfo_x86 *c)
@@ -1594,8 +1595,6 @@ static void __init cpu_parse_early_param(void)
  */
 static void __init early_identify_cpu(struct cpuinfo_x86 *c)
 {
-	c->x86_cache_alignment = c->x86_clflush_size;
-
 	memset(&c->x86_capability, 0, sizeof(c->x86_capability));
 	c->extended_cpuid_level = 0;
 
-- 
2.34.1

Re: [PATCH] x86/boot: Move x86_cache_alignment initialization to correct spot

[Nathan Chancellor]

On Mon, Oct 02, 2023 at 03:00:45PM -0700, Dave Hansen wrote:
> c->x86_cache_alignment is initialized from c->x86_clflush_size.
> However, commit fbf6449f84bf moved c->x86_clflush_size initialization
> to later in boot without moving the c->x86_cache_alignment assignment.
> 
> This presumably left c->x86_cache_alignment set to zero for longer
> than it should be.
> 
> The result was an oops on 32-bit kernels while accessing a pointer
> at 0x20.  The 0x20 came from accessing a structure member at offset
> 0x10 (buffer->cpumask) from a ZERO_SIZE_PTR=0x10.  kmalloc() can
> evidently return ZERO_SIZE_PTR when it's given 0 as its alignment
> requirement.
> 
> Move the c->x86_cache_alignment initialization to be after
> c->x86_clflush_size has an actual value.
> 
> Fixes: fbf6449f84bf ("x86/sev-es: Set x86_virt_bits to the correct value straight away, instead of a two-phase approach")
> Cc: Adam Dunlap <acdunlap@google.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Jacob Xu <jacobhxu@google.com>
> Link: https://lore.kernel.org/all/20231002200426.GA4127272@dev-arch.thelio-3990X/

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

Thanks for the quick fix!

> ---
>  arch/x86/kernel/cpu/common.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index 8d7063e4f63c9..9c51ad5bbf319 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -1141,6 +1141,7 @@ void get_cpu_address_sizes(struct cpuinfo_x86 *c)
>  		}
>  	}
>  	c->x86_cache_bits = c->x86_phys_bits;
> +	c->x86_cache_alignment = c->x86_clflush_size;
>  }
>  
>  static void identify_cpu_without_cpuid(struct cpuinfo_x86 *c)
> @@ -1594,8 +1595,6 @@ static void __init cpu_parse_early_param(void)
>   */
>  static void __init early_identify_cpu(struct cpuinfo_x86 *c)
>  {
> -	c->x86_cache_alignment = c->x86_clflush_size;
> -
>  	memset(&c->x86_capability, 0, sizeof(c->x86_capability));
>  	c->extended_cpuid_level = 0;
>  
> -- 
> 2.34.1
> 

Re: [PATCH] x86/boot: Move x86_cache_alignment initialization to correct spot

[Ingo Molnar]

* Nathan Chancellor <nathan@kernel.org> wrote:

> On Mon, Oct 02, 2023 at 03:00:45PM -0700, Dave Hansen wrote:
> > c->x86_cache_alignment is initialized from c->x86_clflush_size.
> > However, commit fbf6449f84bf moved c->x86_clflush_size initialization
> > to later in boot without moving the c->x86_cache_alignment assignment.
> > 
> > This presumably left c->x86_cache_alignment set to zero for longer
> > than it should be.
> > 
> > The result was an oops on 32-bit kernels while accessing a pointer
> > at 0x20.  The 0x20 came from accessing a structure member at offset
> > 0x10 (buffer->cpumask) from a ZERO_SIZE_PTR=0x10.  kmalloc() can
> > evidently return ZERO_SIZE_PTR when it's given 0 as its alignment
> > requirement.
> > 
> > Move the c->x86_cache_alignment initialization to be after
> > c->x86_clflush_size has an actual value.
> > 
> > Fixes: fbf6449f84bf ("x86/sev-es: Set x86_virt_bits to the correct value straight away, instead of a two-phase approach")
> > Cc: Adam Dunlap <acdunlap@google.com>
> > Cc: Ingo Molnar <mingo@kernel.org>
> > Cc: Jacob Xu <jacobhxu@google.com>
> > Link: https://lore.kernel.org/all/20231002200426.GA4127272@dev-arch.thelio-3990X/
> 
> Tested-by: Nathan Chancellor <nathan@kernel.org>
> 
> Thanks for the quick fix!

Thanks for the quick testing - I've applied this fix on top
of fbf6449f84bf in tip:x86/mm.

Dave, I've added your SOB - let me know if that's not OK:

  Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>

Thanks,

	Ingo

[tip: x86/mm] x86/boot: Move x86_cache_alignment initialization to correct spot

[tip-bot2 for Dave Hansen]

The following commit has been merged into the x86/mm branch of tip:

Commit-ID:     3e32552652917f10c0aa8ac75cdc8f0b8d257dec
Gitweb:        https://git.kernel.org/tip/3e32552652917f10c0aa8ac75cdc8f0b8d257dec
Author:        Dave Hansen <dave.hansen@linux.intel.com>
AuthorDate:    Mon, 02 Oct 2023 15:00:45 -07:00
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Tue, 03 Oct 2023 09:27:12 +02:00

x86/boot: Move x86_cache_alignment initialization to correct spot

c->x86_cache_alignment is initialized from c->x86_clflush_size.
However, commit fbf6449f84bf moved c->x86_clflush_size initialization
to later in boot without moving the c->x86_cache_alignment assignment:

  fbf6449f84bf ("x86/sev-es: Set x86_virt_bits to the correct value straight away, instead of a two-phase approach")

This presumably left c->x86_cache_alignment set to zero for longer
than it should be.

The result was an oops on 32-bit kernels while accessing a pointer
at 0x20.  The 0x20 came from accessing a structure member at offset
0x10 (buffer->cpumask) from a ZERO_SIZE_PTR=0x10.  kmalloc() can
evidently return ZERO_SIZE_PTR when it's given 0 as its alignment
requirement.

Move the c->x86_cache_alignment initialization to be after
c->x86_clflush_size has an actual value.

Fixes: fbf6449f84bf ("x86/sev-es: Set x86_virt_bits to the correct value straight away, instead of a two-phase approach")
Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Tested-by: Nathan Chancellor <nathan@kernel.org>
Link: https://lore.kernel.org/r/20231002220045.1014760-1-dave.hansen@linux.intel.com
---
 arch/x86/kernel/cpu/common.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 8d7063e..9c51ad5 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1141,6 +1141,7 @@ void get_cpu_address_sizes(struct cpuinfo_x86 *c)
 		}
 	}
 	c->x86_cache_bits = c->x86_phys_bits;
+	c->x86_cache_alignment = c->x86_clflush_size;
 }
 
 static void identify_cpu_without_cpuid(struct cpuinfo_x86 *c)
@@ -1594,8 +1595,6 @@ static void __init cpu_parse_early_param(void)
  */
 static void __init early_identify_cpu(struct cpuinfo_x86 *c)
 {
-	c->x86_cache_alignment = c->x86_clflush_size;
-
 	memset(&c->x86_capability, 0, sizeof(c->x86_capability));
 	c->extended_cpuid_level = 0;