| Message ID | 20250318091904.52903-5-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/ubsan: fix ubsan on clang + code fixes | expand |
On 18/03/2025 9:19 am, Roger Pau Monne wrote: > UBSAN complains with: > > UBSAN: Undefined behaviour in arch/x86/mm/shadow/private.h:515:30 > pointer operation overflowed ffff82e000000000 to ffff82dfffffffe0 > [...] > Xen call trace: > [<ffff82d040303882>] R common/ubsan/ubsan.c#ubsan_epilogue+0xa/0xc0 > [<ffff82d040304cc3>] F lib/xxhash64.c#__ubsan_handle_pointer_overflow+0xcb/0x100 > [<ffff82d040471c5d>] F arch/x86/mm/shadow/guest_2.c#sh_page_fault__guest_2+0x1e350 > [<ffff82d0403b216b>] F lib/xxhash64.c#svm_vmexit_handler+0xdf3/0x2450 > [<ffff82d0402049c0>] F lib/xxhash64.c#svm_stgi_label+0x5/0x15 Something is definitely wonky in this backtrace. > > Fix by moving the call to mfn_to_page() after the check of whether the > passed gmfn is valid. This avoid the call to mfn_to_page() with an > INVALID_MFN parameter. > > While there make the page local variable const, it's not modified by the > function. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> Whatever is wonky in the backtrace isn't related to this patch, so Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>, but the backtrace does want fixing.
On 18.03.2025 13:53, Andrew Cooper wrote: > On 18/03/2025 9:19 am, Roger Pau Monne wrote: >> UBSAN complains with: >> >> UBSAN: Undefined behaviour in arch/x86/mm/shadow/private.h:515:30 >> pointer operation overflowed ffff82e000000000 to ffff82dfffffffe0 >> [...] >> Xen call trace: >> [<ffff82d040303882>] R common/ubsan/ubsan.c#ubsan_epilogue+0xa/0xc0 >> [<ffff82d040304cc3>] F lib/xxhash64.c#__ubsan_handle_pointer_overflow+0xcb/0x100 >> [<ffff82d040471c5d>] F arch/x86/mm/shadow/guest_2.c#sh_page_fault__guest_2+0x1e350 >> [<ffff82d0403b216b>] F lib/xxhash64.c#svm_vmexit_handler+0xdf3/0x2450 >> [<ffff82d0402049c0>] F lib/xxhash64.c#svm_stgi_label+0x5/0x15 > > Something is definitely wonky in this backtrace. > >> >> Fix by moving the call to mfn_to_page() after the check of whether the >> passed gmfn is valid. This avoid the call to mfn_to_page() with an >> INVALID_MFN parameter. >> >> While there make the page local variable const, it's not modified by the >> function. >> >> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > Whatever is wonky in the backtrace isn't related to this patch, so > Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>, but the backtrace > does want fixing. Right, but the fix may need to be in the tool chain. I'd be curious what the symbol table looks like that this was created from. Roger, was this linked with GNU ld or LLVM? Are the filename anomalies also visible in the corresponding xen-syms.map? Jan
On Tue, Mar 18, 2025 at 12:53:30PM +0000, Andrew Cooper wrote: > On 18/03/2025 9:19 am, Roger Pau Monne wrote: > > UBSAN complains with: > > > > UBSAN: Undefined behaviour in arch/x86/mm/shadow/private.h:515:30 > > pointer operation overflowed ffff82e000000000 to ffff82dfffffffe0 > > [...] > > Xen call trace: > > [<ffff82d040303882>] R common/ubsan/ubsan.c#ubsan_epilogue+0xa/0xc0 > > [<ffff82d040304cc3>] F lib/xxhash64.c#__ubsan_handle_pointer_overflow+0xcb/0x100 > > [<ffff82d040471c5d>] F arch/x86/mm/shadow/guest_2.c#sh_page_fault__guest_2+0x1e350 > > [<ffff82d0403b216b>] F lib/xxhash64.c#svm_vmexit_handler+0xdf3/0x2450 > > [<ffff82d0402049c0>] F lib/xxhash64.c#svm_stgi_label+0x5/0x15 > > Something is definitely wonky in this backtrace. Oh, yes, it's a TODO I have pending when using LLVM LD. I sent a fix very long time ago, but it was quite ugly. > > > > Fix by moving the call to mfn_to_page() after the check of whether the > > passed gmfn is valid. This avoid the call to mfn_to_page() with an > > INVALID_MFN parameter. > > > > While there make the page local variable const, it's not modified by the > > function. > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > Whatever is wonky in the backtrace isn't related to this patch, so > Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>, but the backtrace > does want fixing. I can get the proper backtrace using clang + GNU LD. Thanks, Roger.
On Tue, Mar 18, 2025 at 03:36:45PM +0100, Jan Beulich wrote: > On 18.03.2025 13:53, Andrew Cooper wrote: > > On 18/03/2025 9:19 am, Roger Pau Monne wrote: > >> UBSAN complains with: > >> > >> UBSAN: Undefined behaviour in arch/x86/mm/shadow/private.h:515:30 > >> pointer operation overflowed ffff82e000000000 to ffff82dfffffffe0 > >> [...] > >> Xen call trace: > >> [<ffff82d040303882>] R common/ubsan/ubsan.c#ubsan_epilogue+0xa/0xc0 > >> [<ffff82d040304cc3>] F lib/xxhash64.c#__ubsan_handle_pointer_overflow+0xcb/0x100 > >> [<ffff82d040471c5d>] F arch/x86/mm/shadow/guest_2.c#sh_page_fault__guest_2+0x1e350 > >> [<ffff82d0403b216b>] F lib/xxhash64.c#svm_vmexit_handler+0xdf3/0x2450 > >> [<ffff82d0402049c0>] F lib/xxhash64.c#svm_stgi_label+0x5/0x15 > > > > Something is definitely wonky in this backtrace. > > > >> > >> Fix by moving the call to mfn_to_page() after the check of whether the > >> passed gmfn is valid. This avoid the call to mfn_to_page() with an > >> INVALID_MFN parameter. > >> > >> While there make the page local variable const, it's not modified by the > >> function. > >> > >> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > > > Whatever is wonky in the backtrace isn't related to this patch, so > > Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>, but the backtrace > > does want fixing. > > Right, but the fix may need to be in the tool chain. I'd be curious what > the symbol table looks like that this was created from. Roger, was this > linked with GNU ld or LLVM? Are the filename anomalies also visible in > the corresponding xen-syms.map? It's with LLVM LD, it's this issue: https://lore.kernel.org/xen-devel/20220505142137.51306-1-roger.pau@citrix.com/ I need to refresh that patch and resend. Sorry, I got so used to those wonky filenames in the backtraces that I no longer notice. Thanks, Roger.
diff --git a/xen/arch/x86/mm/shadow/private.h b/xen/arch/x86/mm/shadow/private.h index a5fc3a7676eb..cef9dbef2e77 100644 --- a/xen/arch/x86/mm/shadow/private.h +++ b/xen/arch/x86/mm/shadow/private.h @@ -512,13 +512,14 @@ static inline unsigned long __backpointer(const struct page_info *sp) static inline int sh_mfn_is_a_page_table(mfn_t gmfn) { - struct page_info *page = mfn_to_page(gmfn); + const struct page_info *page; struct domain *owner; unsigned long type_info; if ( !mfn_valid(gmfn) ) return 0; + page = mfn_to_page(gmfn); owner = page_get_owner(page); if ( owner && shadow_mode_refcounts(owner) && (page->count_info & PGC_shadowed_pt) )
UBSAN complains with: UBSAN: Undefined behaviour in arch/x86/mm/shadow/private.h:515:30 pointer operation overflowed ffff82e000000000 to ffff82dfffffffe0 [...] Xen call trace: [<ffff82d040303882>] R common/ubsan/ubsan.c#ubsan_epilogue+0xa/0xc0 [<ffff82d040304cc3>] F lib/xxhash64.c#__ubsan_handle_pointer_overflow+0xcb/0x100 [<ffff82d040471c5d>] F arch/x86/mm/shadow/guest_2.c#sh_page_fault__guest_2+0x1e350 [<ffff82d0403b216b>] F lib/xxhash64.c#svm_vmexit_handler+0xdf3/0x2450 [<ffff82d0402049c0>] F lib/xxhash64.c#svm_stgi_label+0x5/0x15 Fix by moving the call to mfn_to_page() after the check of whether the passed gmfn is valid. This avoid the call to mfn_to_page() with an INVALID_MFN parameter. While there make the page local variable const, it's not modified by the function. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Changes since v1: - New in this version. --- xen/arch/x86/mm/shadow/private.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)