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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 15.10.12 at 12:58, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
> On Mon, 2012-10-15 at 11:48 +0100, Jan Beulich wrote:
>> >>> 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)?
> 
> 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


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