Re: [PATCH] x86/shadow: Fix UBSAN in hvm_emulate_insn_fetch

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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@xxxxxxxxxx>
> ---
>  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