Re: [Xen-devel] memory corruption in HYPERVISOR_physdev_op()
On 15.10.12 at 12:27, Ian Campbell ian.campb...@citrix.com 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! Doesn't the same problem also exist for HYPERVISOR_event_channel_op() then, at least theoretically (EVTCHNOP_reset is apparently the only addition here so far, but is being used by the tools only afaics)? Jan ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [Xen-devel] memory corruption in HYPERVISOR_physdev_op()
On 15.10.12 at 12:27, Ian Campbell ian.campb...@citrix.com 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)? 399 } 400 return rc; 401 } One example of this is in xen_initdom_restore_msi_irqs(). arch/x86/pci/xen.c 337 struct physdev_pci_device restore_ext; 338 339 restore_ext.seg = pci_domain_nr(dev-bus); 340 restore_ext.bus = dev-bus-number; 341 restore_ext.devfn = dev-devfn; 342 ret = HYPERVISOR_physdev_op(PHYSDEVOP_restore_msi_ext, 343 restore_ext); There are only 4 bytes here. 344 if (ret == -ENOSYS) ^^ If we hit this condition, we have corrupted some memory. 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)) { Supposedly because as long as the argument passed to the function is in memory accessed by the local CPU only and doesn't overlap with storage used for rc (e.g. living in a register), there's no corruption possible afaict - the second memcpy() would just copy back what the first one obtained from there. 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). Jan ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [Xen-devel] memory corruption in HYPERVISOR_physdev_op()
On Mon, 2012-10-15 at 11:48 +0100, Jan Beulich wrote: On 15.10.12 at 12:27, Ian Campbell ian.campb...@citrix.com 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? 399 } 400 return rc; 401 } One example of this is in xen_initdom_restore_msi_irqs(). arch/x86/pci/xen.c 337 struct physdev_pci_device restore_ext; 338 339 restore_ext.seg = pci_domain_nr(dev-bus); 340 restore_ext.bus = dev-bus-number; 341 restore_ext.devfn = dev-devfn; 342 ret = HYPERVISOR_physdev_op(PHYSDEVOP_restore_msi_ext, 343 restore_ext); There are only 4 bytes here. 344 if (ret == -ENOSYS) ^^ If we hit this condition, we have corrupted some memory. 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. Supposedly because as long as the argument passed to the function is in memory accessed by the local CPU only and doesn't overlap with storage used for rc (e.g. living in a register), there's no corruption possible afaict - the second memcpy() would just copy back what the first one obtained from there. 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. Ian. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [Xen-devel] memory corruption in HYPERVISOR_physdev_op()
On 15.10.12 at 12:58, Ian Campbell ian.campb...@citrix.com wrote: On Mon, 2012-10-15 at 11:48 +0100, Jan Beulich wrote: On 15.10.12 at 12:27, Ian Campbell ian.campb...@citrix.com 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 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization