Hi Jeremy, 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. 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. 345 pci_seg_supported = false; regards, dan carpenter
On Fri, 2012-09-14 at 14:24 +0300, Dan Carpenter wrote:> Hi Jeremy,Jeremy doesn't work on Xen much any more. Adding Konrad and the xen-devel@ list.> 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!> > 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?> > 345 pci_seg_supported = false; > > regards, > dan carpenter > _______________________________________________ > Virtualization mailing list > Virtualization at lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/virtualization >-- Ian Campbell Current Noise: Therapy? - Femtex Riffle West Virginia is so small that the Boy Scout had to double as the town drunk.
>>> On 15.10.12 at 12:27, Ian Campbell <Ian.Campbell@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
>>> On 15.10.12 at 12:27, Ian Campbell <Ian.Campbell@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
Seemingly Similar Threads
- memory corruption in HYPERVISOR_physdev_op()
- memory corruption in HYPERVISOR_physdev_op()
- [PATCH] EHCI/Xen: propagate controller reset information to hypervisor
- [PATCH 2/3 v3] Refactor MSI restore call-chain to drop unnecessary argument
- [PATCH] PVH: remove code to map iomem from guest