Re: [Xen-devel] memory corruption in HYPERVISOR_physdev_op()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 15.10.12 at 12:27, Ian Campbell <Ian.Campbell@xxxxxxxxxx> 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


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel