All of lore.kernel.org
 help / color / mirror / Atom feed
* Re:  A possible pointer_overflow in xen-4.13
@ 2021-07-07 15:54 =?gb18030?B?UnJvYWNo?=
  2021-07-07 15:59 ` Jan Beulich
  0 siblings, 1 reply; 5+ messages in thread
From: =?gb18030?B?UnJvYWNo?= @ 2021-07-07 15:54 UTC (permalink / raw)
  To: =?gb18030?B?SmFuIEJldWxpY2g=?=, =?gb18030?B?eGVuLWRldmVs?=

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

Hi, sorry about the late respond. I tried your suggestion, it works. I'm kind of surprised too, since such problem should exposed long time ago. 


I looked deep into your suggestion. I believe you were right about it, since p - ctxt->io_emul_stub  won't overflow and the pointer overflow is likely to happen  in  stub_va + p or ctxt->io_emul_stub. 


Andrew's suggestion works perhaps it the long variable allows the expression to store more bytes, however in long term it may not be a solid solution. So alternative should we take both of the advise that using
+   long disp = (long)(f) - (long)(stub_va + (p - ctxt->io_emul_stub) + 5); \
as a fix patch


Best regards
Franklin


------------------ Original ------------------
From: &nbsp;"Jan Beulich";<jbeulich@suse.com&gt;;
Send time:&nbsp;Wednesday, Jul 7, 2021 3:55 PM
To:&nbsp;"Rroach"<2284696125@qq.com&gt;; 
Cc:&nbsp;"xen-devel"<xen-devel@lists.xenproject.org&gt;; "Andrew Cooper"<andrew.cooper3@citrix.com&gt;; 
Subject: &nbsp;Re: A possible pointer_overflow in xen-4.13



On 07.07.2021 04:32, Rroach wrote:
&gt; After patching it, this works fine and UBSAN dose not have any error report about it.

Which I'm surprised about, because in Andrew's suggestion (sorry,
need to reproduce it manually, because quoting your HTML mail is
rendering unreadable results, as you can see below if you view it
as plain text)

--- 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,
&nbsp;#define APPEND_BUFF(b) ({ memcpy(p, b, sizeof(b)); p += sizeof(b); })
&nbsp;#define APPEND_CALL(f)&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; \
&nbsp;&nbsp;&nbsp; ({&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; \
-&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; long disp = (long)(f) - (stub_va + p - ctxt-&gt;io_emul_stub + 5); \
+&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; long disp = (long)(f) - (long)(stub_va + p - ctxt-&gt;io_emul_stub + 5); \

there is still a possible pointer overflow afaict, unlike in the
suggestion I had given:

&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; long disp = (long)(f) - (stub_va + (p - ctxt-&gt;io_emul_stub) + 5); \

This because of expression evaluation order, which I understand would
match the fully parenthesized

&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; long disp = (long)(f) - (long)(((stub_va + p) - ctxt-&gt;io_emul_stub) + 5); \

where both stub_va + p and the subsequent subtraction of ctxt-&gt;io_emul_stub
are liable to overflow. Whereas p - ctxt-&gt;io_emul_stub won't overflow, as
p starts out from ctxt-&gt;io_emul_stub and then gets incremented by a few bytes.

Would you mind giving the alternative suggestion a try as well?

Jan

&gt; ------------------&amp;nbsp;Original&amp;nbsp;------------------
&gt; From: &amp;nbsp;"Andrew Cooper";<andrew.cooper3@citrix.com&amp;gt;;
&gt; Send time:&amp;nbsp;Saturday, Jun 26, 2021 9:50 PM
&gt; To:&amp;nbsp;"Rroach"<2284696125@qq.com&amp;gt;; "xen-devel"<xen-devel@lists.xenproject.org&amp;gt;; 
&gt; 
&gt; Subject: &amp;nbsp;Re: A possible pointer_overflow in xen-4.13
&gt; 
&gt; 
&gt; 
&gt;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; On 26/06/2021 14:29, Rroach wrote:
&gt;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; 
&gt;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; Hi, I compile Xen-4.13 with CONFIG_UBSAN, and try test&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; it. However, during testing, xl dmesg got the output as&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; shown below.
&gt;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; 
&gt;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; 
&gt;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; It seems that there is a potential pointer overflow&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; within arch/x86/pv/emul-priv-op.c:131 where xen try to&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; execute instruction ''' APPEND_CALL(save_guest_gprs)&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; '''£¬where APPEND_CALL try to add an offset on *p without&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; proper checking.
&gt;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; 
&gt;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; 
&gt;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; I compiled xen-4.13 by clang-9, with following&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; instructions: ''' export CONFIG_UBSAN=y ''' &amp;amp;&amp;amp; '''&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; make clang=y debug=y ''' . Do you have any idea what going&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; on here?
&gt;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; 
&gt;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; 
&gt;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; You say Xen 4.13, but APPEND_CALL() doesn't exist&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; there.&amp;nbsp; I added it in 4.14 when I rewrote this mess to be&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; compatible with CET by not using a ROP gadget.&amp;nbsp; Your backtrace&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; says 4.15 unstable which means its an old staging build (not that&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; that is going to have any effect on this specific issue).
&gt;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; 
&gt;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; The fact that it continued executing correctly means the&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; calculation did the right thing, whether or not UBSAN was happy.&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; The displacement will end up negative as the stub we're writing is&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; numerically higher than {load,save}_guest_gprs(), which I guess&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; means that f - stub_va will underflow.
&gt;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; 
&gt;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; I'm very confused as to why UBSAN reports against&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; save_guest_gprs() considering that load_guest_gprs() when through&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; the exact same logic a few instructions earlier.
&gt;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; 
&gt;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; Either way, does this make the problem go away?
&gt;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; 
&gt;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; diff --git a/xen/arch/x86/pv/emul-priv-op.c&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; b/xen/arch/x86/pv/emul-priv-op.c
&gt;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; index 11467a1e3a..be41bced76 100644
&gt;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; --- a/xen/arch/x86/pv/emul-priv-op.c
&gt;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; +++ b/xen/arch/x86/pv/emul-priv-op.c
&gt;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; @@ -98,7 +98,7 @@ static io_emul_stub_t *io_emul_stub_setup(struct&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; priv_op_ctxt *ctxt, u8 opcode,
&gt;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; &amp;nbsp;#define APPEND_BUFF(b) ({ memcpy(p, b, sizeof(b)); p +=&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; sizeof(b); })
&gt;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; &amp;nbsp;#define&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; APPEND_CALL(f)&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp; &amp;nbsp; \
&gt;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; &amp;nbsp;&amp;nbsp;&amp;nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; ({&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; \
&gt;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; -&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp; &amp;nbsp; long disp = (long)(f) - (stub_va + p -&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; ctxt-&amp;gt;io_emul_stub + 5); \
&gt;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; +&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp; &amp;nbsp; long disp = (long)(f) - (long)(stub_va + p -&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; ctxt-&amp;gt;io_emul_stub + 5); \
&gt;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; &amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp; &amp;nbsp; BUG_ON((int32_t)disp !=&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; disp);&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp; &amp;nbsp; \
&gt;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; &amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp; &amp;nbsp; *p++ =&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; 0xe8;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp; &amp;nbsp; \
&gt;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; &amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp; &amp;nbsp; *(int32_t *)p = disp; p +=&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; 4;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp;&amp;nbsp; &amp;nbsp; \
&gt;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; 
&gt;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; ~Andrew
&gt;

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

^ permalink raw reply	[flat|nested] 5+ messages in thread
[parent not found: <tencent_A17CA7BA63F6E47B3FE7B1AC54E55B2A3609@qq.com>]

end of thread, other threads:[~2021-07-07 15:59 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-07 15:54 A possible pointer_overflow in xen-4.13 =?gb18030?B?UnJvYWNo?=
2021-07-07 15:59 ` Jan Beulich
     [not found] <tencent_A17CA7BA63F6E47B3FE7B1AC54E55B2A3609@qq.com>
2021-06-26 13:50 ` Andrew Cooper
2021-07-07  2:32   ` =?gb18030?B?UnJvYWNo?=
2021-07-07  7:55     ` Jan Beulich

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.