| Message ID | 91bd66eae876b940b09b3b650a21614e42ab0469.1744721279.git.teddy.astie@vates.tech (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/shadow: Fix UBSAN in hvm_emulate_insn_fetch | expand |
On 15.04.2025 14:49, Teddy Astie wrote: > UBSAN complains when trying memcpy with a NULL pointer even if it's going to > copy zero bytes (which are the only cases where a NULL pointer is used). If this really was a problem, I think we'd need to go through and find all instances. However, ... > Fix this by only doing the memcpy if the pointer is non-NULL. > > (XEN) ================================================================================ > (XEN) UBSAN: Undefined behaviour in arch/x86/mm/shadow/hvm.c:168:5 > (XEN) null pointer passed as argument 1, declared with nonnull attribute ... it can only be the compiler who has added the nonnull attribute; we use it only in very few (other) places. Personally I find it absurd to forbid NULL here when the size is zero. Yet I agree that the spec can be interpreted this way. Jan
On 15/04/2025 1:49 pm, Teddy Astie wrote: > UBSAN complains when trying memcpy with a NULL pointer even if it's going to > copy zero bytes (which are the only cases where a NULL pointer is used). > Fix this by only doing the memcpy if the pointer is non-NULL. Which compiler are you using? (Just so there's a record. These reports are version-sensitive.) > > (XEN) ================================================================================ > (XEN) UBSAN: Undefined behaviour in arch/x86/mm/shadow/hvm.c:168:5 > (XEN) null pointer passed as argument 1, declared with nonnull attribute Interestingly, this isn't a Xen annotation. It must be coming from the builtin. And what we really want is nonnull_if_nonzero, but I bet that's not available. > (XEN) ----[ Xen-4.21-unstable x86_64 debug=y ubsan=y Not tainted ]---- > (XEN) CPU: 0 > (XEN) RIP: e008:[<ffff82d0402f789c>] common/ubsan/ubsan.c#ubsan_epilogue+0xa/0xd2 > (XEN) RFLAGS: 0000000000010046 CONTEXT: hypervisor (d1v0) > (XEN) rax: ffff82d040a82eb0 rbx: ffff83021b6e7808 rcx: 000000000000c458 > (XEN) rdx: ffff83021b6e7fd0 rsi: 000000000000000a rdi: ffff83021b6e7808 > (XEN) rbp: ffff83021b6e77f8 rsp: ffff83021b6e77e8 r8: 00000000ffffffff > (XEN) r9: 00000000ffffffff r10: 0000000000000000 r11: 0000000000000000 > (XEN) r12: 000000000000004d r13: 0000000000000000 r14: ffff82d040a82eb0 > (XEN) r15: 00000000002ffddc cr0: 0000000080050033 cr4: 00000000001526e0 > (XEN) cr3: 000000021b7f4000 cr2: 0000000000000000 > (XEN) fsb: 0000000000000000 gsb: 0000000000000000 gss: 0000000000000000 > (XEN) ds: 0000 es: 0000 fs: 0000 gs: 0000 ss: 0000 cs: e008 > (XEN) Xen code around <ffff82d0402f789c> (common/ubsan/ubsan.c#ubsan_epilogue+0xa/0xd2): > (XEN) 89 e5 41 54 53 48 89 fb <0f> 0b 48 8d 3d 1b cf 36 00 e8 b4 94 00 00 48 85 > (XEN) Xen stack trace from rsp=ffff83021b6e77e8: > (XEN) ffff82d040a82ea0 000000000000004d ffff83021b6e7820 ffff82d0402f8435 > (XEN) 0000000000000202 ffff83021b6e7d25 0000000000000000 ffff83021b6e7858 > (XEN) ffff82d040455cb6 00000000002ffddc ffff83021b6e7ef8 ffff83021fbf1010 > (XEN) 0000000000000000 0000000000000000 ffff83021b6e7bd8 ffff82d0405b562b > (XEN) ffffffff00200033 ffffffff0020874b 00007cfde4918743 ffff83021b6e7af0 > (XEN) 0000000000000003 000000000000000a 0000000000000000 0000000440661f40 > (XEN) ffffffff00000000 0000000000000000 00007cfd000000e8 ffff83021b6e79a8 > (XEN) ffff83021b6e7980 ffff82d040d3fa90 00000000a00000e8 ffff82d0406904a0 > (XEN) ffff83021b6e7cd8 ffff8302159963f0 ffff83021b6e7998 ffff82d04052f592 > (XEN) fffffffa0000000a ffff83021b6e7b21 393082d040661f40 0000001000000033 > (XEN) ffffffff00307b39 ffffffffe491868b ffffffff00200d00 00007cfde491867b > (XEN) ffff83021b6e7bb8 0000000000000003 0000000000000001 0000000000000000 > (XEN) 0000000000000000 0000000000000067 ffff8302159963f0 aaaaaaaaaaaaaaaa > (XEN) aaaaaaaaaaaaaaaa aaaaaaaaaaaaaaaa aaaaaaaaaaaaaaaa aaaaaaaaaaaaaaaa > (XEN) aaaaaaaaaaaaaaaa aaaaaaaaaaaaaaaa aaaaaaaaaaaaaaaa ffff83021b7fc008 > (XEN) 00000000000002ff ffff8302159963f0 0000000000000000 ffff830215994000 > (XEN) 0000000715994000 0000000000000000 0000000000000003 0000000000000000 > (XEN) 0000000000000000 8086000000008086 0000000000000000 0000000000000000 > (XEN) 0000000400000002 00000000002ffddc 0000000000000000 8086000000008086 > (XEN) 0000000000000000 0000000000000000 ffffffffffffffff 000000000000001f > (XEN) Xen call trace: > (XEN) [<ffff82d0402f789c>] R common/ubsan/ubsan.c#ubsan_epilogue+0xa/0xd2 > (XEN) [<ffff82d0402f8435>] F __ubsan_handle_nonnull_arg+0x7c/0xb3 > (XEN) [<ffff82d040455cb6>] F arch/x86/mm/shadow/hvm.c#hvm_emulate_insn_fetch+0xfb/0x100 > (XEN) [<ffff82d0405b562b>] F x86_emulate+0x17f6b/0x3b8e3 > (XEN) [<ffff82d0405dce4f>] F x86_emulate_wrapper+0x87/0x216 > (XEN) [<ffff82d040489847>] F arch/x86/mm/shadow/guest_4.c#sh_page_fault__guest_4+0x908/0x3b34 > (XEN) [<ffff82d0403ca3ac>] F vmx_vmexit_handler+0x1691/0x3439 > (XEN) [<ffff82d040204683>] F vmx_asm_vmexit_handler+0x103/0x220 > (XEN) > (XEN) ================================================================================ For this, we normally abbreviate it to just the relevant information, so in this case: (XEN) UBSAN: Undefined behaviour in arch/x86/mm/shadow/hvm.c:168:5 (XEN) null pointer passed as argument 1, declared with nonnull attribute (XEN) ----[ Xen-4.21-unstable x86_64 debug=y ubsan=y Not tainted ]---- (XEN) CPU: 0 (XEN) RIP: e008:[<ffff82d0402f789c>] common/ubsan/ubsan.c#ubsan_epilogue+0xa/0xd2 ... (XEN) Xen call trace: (XEN) [<ffff82d0402f789c>] R common/ubsan/ubsan.c#ubsan_epilogue+0xa/0xd2 (XEN) [<ffff82d0402f8435>] F __ubsan_handle_nonnull_arg+0x7c/0xb3 (XEN) [<ffff82d040455cb6>] F arch/x86/mm/shadow/hvm.c#hvm_emulate_insn_fetch+0xfb/0x100 (XEN) [<ffff82d0405b562b>] F x86_emulate+0x17f6b/0x3b8e3 (XEN) [<ffff82d0405dce4f>] F x86_emulate_wrapper+0x87/0x216 (XEN) [<ffff82d040489847>] F arch/x86/mm/shadow/guest_4.c#sh_page_fault__guest_4+0x908/0x3b34 (XEN) [<ffff82d0403ca3ac>] F vmx_vmexit_handler+0x1691/0x3439 (XEN) [<ffff82d040204683>] F vmx_asm_vmexit_handler+0x103/0x220 which is rather less voluminous. > > Signed-off-by: Teddy Astie <teddy.astie@vates.tech> > --- > xen/arch/x86/mm/shadow/hvm.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/x86/mm/shadow/hvm.c b/xen/arch/x86/mm/shadow/hvm.c > index 114957a3e1..298dd0f229 100644 > --- a/xen/arch/x86/mm/shadow/hvm.c > +++ b/xen/arch/x86/mm/shadow/hvm.c > @@ -165,7 +165,8 @@ hvm_emulate_insn_fetch(unsigned long offset, > hvm_access_insn_fetch, sh_ctxt); > > /* Hit the cache. Simple memcpy. */ > - memcpy(p_data, &sh_ctxt->insn_buf[insn_off], bytes); > + if ( p_data ) > + memcpy(p_data, &sh_ctxt->insn_buf[insn_off], bytes); Do you know precisely which condition is being hit? ~Andrew
On 15/04/2025 2:00 pm, Jan Beulich wrote: > On 15.04.2025 14:49, Teddy Astie wrote: >> UBSAN complains when trying memcpy with a NULL pointer even if it's going to >> copy zero bytes (which are the only cases where a NULL pointer is used). > If this really was a problem, I think we'd need to go through and find all > instances. However, ... > >> Fix this by only doing the memcpy if the pointer is non-NULL. >> >> (XEN) ================================================================================ >> (XEN) UBSAN: Undefined behaviour in arch/x86/mm/shadow/hvm.c:168:5 >> (XEN) null pointer passed as argument 1, declared with nonnull attribute > ... it can only be the compiler who has added the nonnull attribute; we > use it only in very few (other) places. > > Personally I find it absurd to forbid NULL here when the size is zero. Yet > I agree that the spec can be interpreted this way. https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3322.pdf This is being proposed for fixing in C2Y, because lots of people think it's absurd. However, until we can raise our -std, I think we're stuck with the current behaviour. GCC-15 introduces the nonnull_if_nonzero attribute specifically for memcpy() etc, but I don't see how we could make use of it in this case. ~Andrew
Le 15/04/2025 à 15:02, Andrew Cooper a écrit : > Which compiler are you using? (Just so there's a record. These reports > are version-sensitive.) It's GCC 14.2.0. > > Do you know precisely which condition is being hit? It occurs when booting OVMF (PVH and HVM) with HAP=0. It doesn't seem to occur with SeaBIOS or PVH GRUB though. Teddy Teddy Astie | Vates XCP-ng Developer XCP-ng & Xen Orchestra - Vates solutions web: https://vates.tech
diff --git a/xen/arch/x86/mm/shadow/hvm.c b/xen/arch/x86/mm/shadow/hvm.c index 114957a3e1..298dd0f229 100644 --- a/xen/arch/x86/mm/shadow/hvm.c +++ b/xen/arch/x86/mm/shadow/hvm.c @@ -165,7 +165,8 @@ hvm_emulate_insn_fetch(unsigned long offset, hvm_access_insn_fetch, sh_ctxt); /* Hit the cache. Simple memcpy. */ - memcpy(p_data, &sh_ctxt->insn_buf[insn_off], bytes); + if ( p_data ) + memcpy(p_data, &sh_ctxt->insn_buf[insn_off], bytes); return X86EMUL_OKAY; }
UBSAN complains when trying memcpy with a NULL pointer even if it's going to copy zero bytes (which are the only cases where a NULL pointer is used). Fix this by only doing the memcpy if the pointer is non-NULL. (XEN) ================================================================================ (XEN) UBSAN: Undefined behaviour in arch/x86/mm/shadow/hvm.c:168:5 (XEN) null pointer passed as argument 1, declared with nonnull attribute (XEN) ----[ Xen-4.21-unstable x86_64 debug=y ubsan=y Not tainted ]---- (XEN) CPU: 0 (XEN) RIP: e008:[<ffff82d0402f789c>] common/ubsan/ubsan.c#ubsan_epilogue+0xa/0xd2 (XEN) RFLAGS: 0000000000010046 CONTEXT: hypervisor (d1v0) (XEN) rax: ffff82d040a82eb0 rbx: ffff83021b6e7808 rcx: 000000000000c458 (XEN) rdx: ffff83021b6e7fd0 rsi: 000000000000000a rdi: ffff83021b6e7808 (XEN) rbp: ffff83021b6e77f8 rsp: ffff83021b6e77e8 r8: 00000000ffffffff (XEN) r9: 00000000ffffffff r10: 0000000000000000 r11: 0000000000000000 (XEN) r12: 000000000000004d r13: 0000000000000000 r14: ffff82d040a82eb0 (XEN) r15: 00000000002ffddc cr0: 0000000080050033 cr4: 00000000001526e0 (XEN) cr3: 000000021b7f4000 cr2: 0000000000000000 (XEN) fsb: 0000000000000000 gsb: 0000000000000000 gss: 0000000000000000 (XEN) ds: 0000 es: 0000 fs: 0000 gs: 0000 ss: 0000 cs: e008 (XEN) Xen code around <ffff82d0402f789c> (common/ubsan/ubsan.c#ubsan_epilogue+0xa/0xd2): (XEN) 89 e5 41 54 53 48 89 fb <0f> 0b 48 8d 3d 1b cf 36 00 e8 b4 94 00 00 48 85 (XEN) Xen stack trace from rsp=ffff83021b6e77e8: (XEN) ffff82d040a82ea0 000000000000004d ffff83021b6e7820 ffff82d0402f8435 (XEN) 0000000000000202 ffff83021b6e7d25 0000000000000000 ffff83021b6e7858 (XEN) ffff82d040455cb6 00000000002ffddc ffff83021b6e7ef8 ffff83021fbf1010 (XEN) 0000000000000000 0000000000000000 ffff83021b6e7bd8 ffff82d0405b562b (XEN) ffffffff00200033 ffffffff0020874b 00007cfde4918743 ffff83021b6e7af0 (XEN) 0000000000000003 000000000000000a 0000000000000000 0000000440661f40 (XEN) ffffffff00000000 0000000000000000 00007cfd000000e8 ffff83021b6e79a8 (XEN) ffff83021b6e7980 ffff82d040d3fa90 00000000a00000e8 ffff82d0406904a0 (XEN) ffff83021b6e7cd8 ffff8302159963f0 ffff83021b6e7998 ffff82d04052f592 (XEN) fffffffa0000000a ffff83021b6e7b21 393082d040661f40 0000001000000033 (XEN) ffffffff00307b39 ffffffffe491868b ffffffff00200d00 00007cfde491867b (XEN) ffff83021b6e7bb8 0000000000000003 0000000000000001 0000000000000000 (XEN) 0000000000000000 0000000000000067 ffff8302159963f0 aaaaaaaaaaaaaaaa (XEN) aaaaaaaaaaaaaaaa aaaaaaaaaaaaaaaa aaaaaaaaaaaaaaaa aaaaaaaaaaaaaaaa (XEN) aaaaaaaaaaaaaaaa aaaaaaaaaaaaaaaa aaaaaaaaaaaaaaaa ffff83021b7fc008 (XEN) 00000000000002ff ffff8302159963f0 0000000000000000 ffff830215994000 (XEN) 0000000715994000 0000000000000000 0000000000000003 0000000000000000 (XEN) 0000000000000000 8086000000008086 0000000000000000 0000000000000000 (XEN) 0000000400000002 00000000002ffddc 0000000000000000 8086000000008086 (XEN) 0000000000000000 0000000000000000 ffffffffffffffff 000000000000001f (XEN) Xen call trace: (XEN) [<ffff82d0402f789c>] R common/ubsan/ubsan.c#ubsan_epilogue+0xa/0xd2 (XEN) [<ffff82d0402f8435>] F __ubsan_handle_nonnull_arg+0x7c/0xb3 (XEN) [<ffff82d040455cb6>] F arch/x86/mm/shadow/hvm.c#hvm_emulate_insn_fetch+0xfb/0x100 (XEN) [<ffff82d0405b562b>] F x86_emulate+0x17f6b/0x3b8e3 (XEN) [<ffff82d0405dce4f>] F x86_emulate_wrapper+0x87/0x216 (XEN) [<ffff82d040489847>] F arch/x86/mm/shadow/guest_4.c#sh_page_fault__guest_4+0x908/0x3b34 (XEN) [<ffff82d0403ca3ac>] F vmx_vmexit_handler+0x1691/0x3439 (XEN) [<ffff82d040204683>] F vmx_asm_vmexit_handler+0x103/0x220 (XEN) (XEN) ================================================================================ Signed-off-by: Teddy Astie <teddy.astie@vates.tech> --- xen/arch/x86/mm/shadow/hvm.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)