Re: A possible pointer_overflow in xen-4.13

* Re: A possible pointer_overflow in xen-4.13
       [not found] <tencent_A17CA7BA63F6E47B3FE7B1AC54E55B2A3609@qq.com>
@ 2021-06-26 13:50 ` Andrew Cooper
  2021-07-07  2:32   ` =?gb18030?B?UnJvYWNo?=
  0 siblings, 1 reply; 5+ messages in thread
From: Andrew Cooper @ 2021-06-26 13:50 UTC (permalink / raw)
  To: Rroach, xen-devel

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=gb18030, Size: 3121 bytes --]

On 26/06/2021 14:29, Rroach wrote:
> Hi, I compile Xen-4.13 with CONFIG_UBSAN, and try test it. However,
> during testing, xl dmesg got the output as shown below.
>
> It seems that there is a potential pointer overflow within
> arch/x86/pv/emul-priv-op.c:131 where xen try to execute instruction
> ''' APPEND_CALL(save_guest_gprs) '''£¬where APPEND_CALL try to add an
> offset on *p without proper checking.
>
> I compiled xen-4.13 by clang-9, with following instructions: '''
> export CONFIG_UBSAN=y ''' && ''' make clang=y debug=y ''' . Do you
> have any idea what going on here?

You say Xen 4.13, but APPEND_CALL() doesn't exist there.0„2 I added it in
4.14 when I rewrote this mess to be compatible with CET by not using a
ROP gadget.0„2 Your backtrace says 4.15 unstable which means its an old
staging build (not that that is going to have any effect on this
specific issue).

The fact that it continued executing correctly means the calculation did
the right thing, whether or not UBSAN was happy.0„2 The displacement will
end up negative as the stub we're writing is numerically higher than
{load,save}_guest_gprs(), which I guess means that f - stub_va will
underflow.

I'm very confused as to why UBSAN reports against save_guest_gprs()
considering that load_guest_gprs() when through the exact same logic a
few instructions earlier.

Either way, does this make the problem go away?

diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c
index 11467a1e3a..be41bced76 100644
--- a/xen/arch/x86/pv/emul-priv-op.c
+++ b/xen/arch/x86/pv/emul-priv-op.c
@@ -98,7 +98,7 @@ static io_emul_stub_t *io_emul_stub_setup(struct
priv_op_ctxt *ctxt, u8 opcode,
0„2#define APPEND_BUFF(b) ({ memcpy(p, b, sizeof(b)); p += sizeof(b); })
0„2#define APPEND_CALL(f)0„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„2 \
0„20„20„20„2 ({0„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„2 \
-0„20„20„20„20„20„20„2 long disp = (long)(f) - (stub_va + p - ctxt->io_emul_stub + 5); \
+0„20„20„20„20„20„20„2 long disp = (long)(f) - (long)(stub_va + p - ctxt->io_emul_stub
+ 5); \
0„20„20„20„20„20„20„20„2 BUG_ON((int32_t)disp != disp);0„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„2 \
0„20„20„20„20„20„20„20„2 *p++ = 0xe8;0„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„2 \
0„20„20„20„20„20„20„20„2 *(int32_t *)p = disp; p += 4;0„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„20„2 \

~Andrew

[-- Attachment #2: Type: text/html, Size: 4634 bytes --]

^ permalink raw reply related	[flat|nested] 5+ messages in thread