From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [Xen-devel] memory corruption in HYPERVISOR_physdev_op() Date: Mon, 15 Oct 2012 13:46:49 +0100 Message-ID: <507C21D902000078000A1630@nat28.tlf.novell.com> References: <20120914112427.GA1454@elgon.mountain> <1350296868.18058.24.camel@zakaz.uk.xensource.com> <507C062002000078000A1574@nat28.tlf.novell.com> <1350298696.18058.33.camel@zakaz.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1350298696.18058.33.camel@zakaz.uk.xensource.com> Content-Disposition: inline List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: virtualization-bounces@lists.linux-foundation.org Errors-To: virtualization-bounces@lists.linux-foundation.org To: Ian Campbell Cc: Jeremy Fitzhardinge , Konrad Rzeszutek Wilk , xen-devel , Tang Liang , "virtualization@lists.linux-foundation.org" , Dan Carpenter List-Id: virtualization@lists.linuxfoundation.org >>> On 15.10.12 at 12:58, Ian Campbell wrote: > On Mon, 2012-10-15 at 11:48 +0100, Jan Beulich wrote: >> >>> On 15.10.12 at 12:27, Ian Campbell wrote: >> > On Fri, 2012-09-14 at 14:24 +0300, Dan Carpenter wrote: >> >> My static analyzer complains about potential memory corruption in >> >> HYPERVISOR_physdev_op() >> >> >> >> arch/x86/include/asm/xen/hypercall.h >> >> 389 static inline int >> >> 390 HYPERVISOR_physdev_op(int cmd, void *arg) >> >> 391 { >> >> 392 int rc = _hypercall2(int, physdev_op, cmd, arg); >> >> 393 if (unlikely(rc == -ENOSYS)) { >> >> 394 struct physdev_op op; >> >> 395 op.cmd = cmd; >> >> 396 memcpy(&op.u, arg, sizeof(op.u)); >> >> 397 rc = _hypercall1(int, physdev_op_compat, &op); >> >> 398 memcpy(arg, &op.u, sizeof(op.u)); >> >> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ >> >> Some of the arg buffers are not as large as sizeof(op.u) which is either >> >> 12 or 16 depending on the size of longs in struct physdev_apic. >> > >> > Nasty! >> >> Wasn't it that pv-ops expects Xen 4.0.1 or newer anyway? If so, >> what does this code exist for in the first place (it's framed by >> #if CONFIG_XEN_COMPAT <= 0x030002 in the Xenified kernel)? > > I think the 4.0.1 or newer requirement is for dom0 only. I guess physdev > op is only used in dom0 though? Or does passthrough need it? No, it's only platform_op that is Dom0-only. >> > I can see the memory corruption but how does it relate to ret == >> > -ENOSYS? >> >> The (supposedly) corrupting code site inside an >> >> if (unlikely(rc == -ENOSYS)) { > > Ah, for some reason I assumed this was in the eventual caller, even > though it was staring me right in the face in the full quote. I think Dan's reference was to an eventual caller - it would see the -ENOSYS, as the compat call wouldn't return anything else than the modern one, and the modern one (to enter the code in question) must have returned -ENOSYS. >> Fixing this other than by removing the broken code would be >> pretty hard I'm afraid (and I tend to leave the code untouched >> altogether in the Xenified tree). > > Given that it is compat code the list of subops which needs to supported > in this case is small and finite so a simple lookup table or even switch > stmt for the size might be an option. Ugly, particularly for an inline function. But possible of course. Jan