[PATCH 1/3] xen, pvh: fix unbootable VMs (PVH + KASAN)

[PATCH 1/3] xen, pvh: fix unbootable VMs (PVH + KASAN)

[Alexey Dobriyan]

Uninstrument arch/x86/platform/pvh/enlighten.c: KASAN is not setup
_this_ early in the boot process.

Steps to reproduce:

	make allnoconfig
	make sure CONFIG_AMD_MEM_ENCRYPT is disabled
		AMD_MEM_ENCRYPT independently uninstruments lib/string.o
		so PVH boot code calls into uninstrumented memset() and
		memcmp() which can make the bug disappear depending on
		the compiler.
	enable CONFIG_PVH
	enable CONFIG_KASAN
	enable serial console
		this is fun exercise if you never done it from nothing :^)

	make

	qemu-system-x86_64	\
		-enable-kvm	\
		-cpu host	\
		-smp cpus=1	\
		-m 4096		\
		-serial stdio	\
		-kernel vmlinux \
		-append 'console=ttyS0 ignore_loglevel'

Messages on serial console will easily tell OK kernel from unbootable
kernel. In bad case qemu hangs in an infinite loop stroboscoping
"SeaBIOS" message.

Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---

 arch/x86/platform/pvh/Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/platform/pvh/Makefile b/arch/x86/platform/pvh/Makefile
index 5dec5067c9fb..c43fb7964dc4 100644
--- a/arch/x86/platform/pvh/Makefile
+++ b/arch/x86/platform/pvh/Makefile
@@ -1,5 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0
 OBJECT_FILES_NON_STANDARD_head.o := y
+KASAN_SANITIZE := n
 
 obj-$(CONFIG_PVH) += enlighten.o
 obj-$(CONFIG_PVH) += head.o

Re: [PATCH 1/3] xen, pvh: fix unbootable VMs (PVH + KASAN)

[Jürgen Groß]

On 01.08.24 21:16, Alexey Dobriyan wrote:
> Uninstrument arch/x86/platform/pvh/enlighten.c: KASAN is not setup
> _this_ early in the boot process.
> 
> Steps to reproduce:
> 
> 	make allnoconfig
> 	make sure CONFIG_AMD_MEM_ENCRYPT is disabled
> 		AMD_MEM_ENCRYPT independently uninstruments lib/string.o
> 		so PVH boot code calls into uninstrumented memset() and
> 		memcmp() which can make the bug disappear depending on
> 		the compiler.
> 	enable CONFIG_PVH
> 	enable CONFIG_KASAN
> 	enable serial console
> 		this is fun exercise if you never done it from nothing :^)
> 
> 	make
> 
> 	qemu-system-x86_64	\
> 		-enable-kvm	\
> 		-cpu host	\
> 		-smp cpus=1	\
> 		-m 4096		\
> 		-serial stdio	\
> 		-kernel vmlinux \
> 		-append 'console=ttyS0 ignore_loglevel'
> 
> Messages on serial console will easily tell OK kernel from unbootable
> kernel. In bad case qemu hangs in an infinite loop stroboscoping
> "SeaBIOS" message.
> 
> Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>

Acked-by: Juergen Gross <jgross@suse.com>


Juergen

[PATCH 2/3] x86/cpu: fix unbootable VMs by inlining memcmp in hypervisor_cpuid_base

[Alexey Dobriyan]

If this memcmp() is not inlined then PVH early boot code can call
into KASAN-instrumented memcmp() which results in unbootable VMs:

	pvh_start_xen
	xen_prepare_pvh
	xen_cpuid_base
	hypervisor_cpuid_base
	memcmp

Ubuntu's gcc version 11.4.0 (Ubuntu 11.4.0-1ubuntu1~22.04) inlines
memcmp with patch and the bug is partially fixed.

Leave FIXME just in case someone cares enough to compare 3 pairs of
integers like 3 pairs of integers.

Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---

 arch/x86/include/asm/cpuid.h | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/cpuid.h b/arch/x86/include/asm/cpuid.h
index 6b122a31da06..3eca7824430e 100644
--- a/arch/x86/include/asm/cpuid.h
+++ b/arch/x86/include/asm/cpuid.h
@@ -196,7 +196,20 @@ static inline uint32_t hypervisor_cpuid_base(const char *sig, uint32_t leaves)
 	for_each_possible_hypervisor_cpuid_base(base) {
 		cpuid(base, &eax, &signature[0], &signature[1], &signature[2]);
 
-		if (!memcmp(sig, signature, 12) &&
+		/*
+		 * FIXME rewrite cpuid comparators to accept uint32_t[3].
+		 *
+		 * This memcmp()
+		 * a) is called from PVH early boot code
+		 *    before instrumentation is set up,
+		 * b) may be compiled to "call memcmp" (not inlined),
+		 * c) memcmp() itself may be instrumented.
+		 *
+		 * Any combination of 2 is fine, but all 3 aren't.
+		 *
+		 * Force inline this function call.
+		 */
+		if (!__builtin_memcmp(sig, signature, 12) &&
 		    (leaves == 0 || ((eax - base) >= leaves)))
 			return base;
 	}

[PATCH 3/3] xen, pvh: fix unbootable VMs by inlining memset in xen_prepare_pvh

[Alexey Dobriyan]

If this memset() is not inlined than PVH early boot code can call
into KASAN-instrumented memset() which results in unbootable VMs.

Ubuntu's 22.04.4 LTS gcc version 11.4.0 (Ubuntu 11.4.0-1ubuntu1~22.04)
doesn't inline this memset but inlines __builtin_memset.

Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---

 arch/x86/platform/pvh/enlighten.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/platform/pvh/enlighten.c b/arch/x86/platform/pvh/enlighten.c
index 8c2d4b8de25d..c2f6e202217c 100644
--- a/arch/x86/platform/pvh/enlighten.c
+++ b/arch/x86/platform/pvh/enlighten.c
@@ -133,7 +133,8 @@ void __init xen_prepare_pvh(void)
 		BUG();
 	}
 
-	memset(&pvh_bootparams, 0, sizeof(pvh_bootparams));
+	/* This can compile to "call memset" and memset() can be instrumented. */
+	__builtin_memset(&pvh_bootparams, 0, sizeof(pvh_bootparams));
 
 	hypervisor_specific_init(xen_guest);
 

Re: [PATCH 2/3] x86/cpu: fix unbootable VMs by inlining memcmp in hypervisor_cpuid_base

[Thomas Gleixner]

On Fri, Aug 02 2024 at 11:50, Alexey Dobriyan wrote:

Please amend functions with '()' in the subject line and the change log
consistently.

> diff --git a/arch/x86/include/asm/cpuid.h b/arch/x86/include/asm/cpuid.h
> index 6b122a31da06..3eca7824430e 100644
> --- a/arch/x86/include/asm/cpuid.h
> +++ b/arch/x86/include/asm/cpuid.h
> @@ -196,7 +196,20 @@ static inline uint32_t hypervisor_cpuid_base(const char *sig, uint32_t leaves)
>  	for_each_possible_hypervisor_cpuid_base(base) {
>  		cpuid(base, &eax, &signature[0], &signature[1], &signature[2]);
>  
> -		if (!memcmp(sig, signature, 12) &&
> +		/*
> +		 * FIXME rewrite cpuid comparators to accept uint32_t[3].

Which comparators?

Thanks,

        tglx

Re: [PATCH 3/3] xen, pvh: fix unbootable VMs by inlining memset in xen_prepare_pvh

[Thomas Gleixner]

On Fri, Aug 02 2024 at 11:53, Alexey Dobriyan wrote:
> If this memset() is not inlined than PVH early boot code can call
> into KASAN-instrumented memset() which results in unbootable VMs.
>
> Ubuntu's 22.04.4 LTS gcc version 11.4.0 (Ubuntu 11.4.0-1ubuntu1~22.04)
> doesn't inline this memset but inlines __builtin_memset.

memset() ......

Re: [PATCH 2/3] x86/cpu: fix unbootable VMs by inlining memcmp in hypervisor_cpuid_base

[Nikolay Borisov]

On 2.08.24 г. 11:50 ч., Alexey Dobriyan wrote:
> If this memcmp() is not inlined then PVH early boot code can call
> into KASAN-instrumented memcmp() which results in unbootable VMs:
> 
> 	pvh_start_xen
> 	xen_prepare_pvh
> 	xen_cpuid_base
> 	hypervisor_cpuid_base
> 	memcmp
> 
> Ubuntu's gcc version 11.4.0 (Ubuntu 11.4.0-1ubuntu1~22.04) inlines
> memcmp with patch and the bug is partially fixed.
> 
> Leave FIXME just in case someone cares enough to compare 3 pairs of
> integers like 3 pairs of integers.
> 
> Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
> ---
> 
>   arch/x86/include/asm/cpuid.h | 15 ++++++++++++++-
>   1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/cpuid.h b/arch/x86/include/asm/cpuid.h
> index 6b122a31da06..3eca7824430e 100644
> --- a/arch/x86/include/asm/cpuid.h
> +++ b/arch/x86/include/asm/cpuid.h
> @@ -196,7 +196,20 @@ static inline uint32_t hypervisor_cpuid_base(const char *sig, uint32_t leaves)
>   	for_each_possible_hypervisor_cpuid_base(base) {
>   		cpuid(base, &eax, &signature[0], &signature[1], &signature[2]);
>   
> -		if (!memcmp(sig, signature, 12) &&
> +		/*
> +		 * FIXME rewrite cpuid comparators to accept uint32_t[3].
> +		 *
> +		 * This memcmp()
> +		 * a) is called from PVH early boot code
> +		 *    before instrumentation is set up,
> +		 * b) may be compiled to "call memcmp" (not inlined),
> +		 * c) memcmp() itself may be instrumented.
> +		 *
> +		 * Any combination of 2 is fine, but all 3 aren't.
> +		 *
> +		 * Force inline this function call.
> +		 */
> +		if (!__builtin_memcmp(sig, signature, 12) &&

Instead of putting this giant FIXME, why not simply do the comparison as 
ints, i.e ((uint32_t)&sig[0]) == signature1 && ((uitn32_t)&sig[4]) == 
signature2 && ((uint32_t)&sig[8] == signature_3  and be done with it?

>   		    (leaves == 0 || ((eax - base) >= leaves)))
>   			return base;
>   	}
> 

Re: [PATCH 2/3] x86/cpu: fix unbootable VMs by inlining memcmp in hypervisor_cpuid_base

[Thomas Gleixner]

On Fri, Aug 02 2024 at 16:25, Nikolay Borisov wrote:
> On 2.08.24 г. 11:50 ч., Alexey Dobriyan wrote:
>> If this memcmp() is not inlined then PVH early boot code can call
>> into KASAN-instrumented memcmp() which results in unbootable VMs:
>> 
>> 	pvh_start_xen
>> 	xen_prepare_pvh
>> 	xen_cpuid_base
>> 	hypervisor_cpuid_base
>> 	memcmp
>> 
>> Ubuntu's gcc version 11.4.0 (Ubuntu 11.4.0-1ubuntu1~22.04) inlines
>> memcmp with patch and the bug is partially fixed.
>> 
>> Leave FIXME just in case someone cares enough to compare 3 pairs of
>> integers like 3 pairs of integers.
>> 
>> Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
>> ---
>> 
>>   arch/x86/include/asm/cpuid.h | 15 ++++++++++++++-
>>   1 file changed, 14 insertions(+), 1 deletion(-)
>> 
>> diff --git a/arch/x86/include/asm/cpuid.h b/arch/x86/include/asm/cpuid.h
>> index 6b122a31da06..3eca7824430e 100644
>> --- a/arch/x86/include/asm/cpuid.h
>> +++ b/arch/x86/include/asm/cpuid.h
>> @@ -196,7 +196,20 @@ static inline uint32_t hypervisor_cpuid_base(const char *sig, uint32_t leaves)
>>   	for_each_possible_hypervisor_cpuid_base(base) {
>>   		cpuid(base, &eax, &signature[0], &signature[1], &signature[2]);
>>   
>> -		if (!memcmp(sig, signature, 12) &&
>> +		/*
>> +		 * FIXME rewrite cpuid comparators to accept uint32_t[3].
>> +		 *
>> +		 * This memcmp()
>> +		 * a) is called from PVH early boot code
>> +		 *    before instrumentation is set up,
>> +		 * b) may be compiled to "call memcmp" (not inlined),
>> +		 * c) memcmp() itself may be instrumented.
>> +		 *
>> +		 * Any combination of 2 is fine, but all 3 aren't.
>> +		 *
>> +		 * Force inline this function call.
>> +		 */
>> +		if (!__builtin_memcmp(sig, signature, 12) &&
>
> Instead of putting this giant FIXME, why not simply do the comparison as 
> ints, i.e ((uint32_t)&sig[0]) == signature1 && ((uitn32_t)&sig[4]) == 
> signature2 && ((uint32_t)&sig[8] == signature_3  and be done with it?

Because a smart compiler might turn it into a memcmp() :