| Message ID | 304592cf-e4a7-4ba1-baa6-4941c60f0e3c@p183 (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] xen, pvh: fix unbootable VMs (PVH + KASAN) | expand |
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
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; > } >
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() :
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; }
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(-)