memory corruption in HYPERVISOR_physdev_op()

memory corruption in HYPERVISOR_physdev_op()

[Dan Carpenter]

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

Re: memory corruption in HYPERVISOR_physdev_op()

[Ian Campbell]

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@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.

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

[Jan Beulich]

>>> 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

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

[Ian Campbell]

On Mon, 2012-10-15 at 11:48 +0100, Jan Beulich wrote:
> >>> 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)?

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.

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

[Jan Beulich]

>>> On 15.10.12 at 12:58, Ian Campbell <Ian.Campbell@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.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)?
> 
> 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

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

[Jan Beulich]

>>> 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

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

[Jan Beulich]

>>> On 16.10.12 at 11:07, "Jan Beulich" <JBeulich@suse.com> wrote:
>>>> 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)?

Actually, the problem isn't tied to new additions of sub-hypercalls
(I was wrongly implying this from the example originally provided),
and should be visible e.g. on any use of EVTCHNOP_unmask.

Jan