Re: [Xen-devel] xc_evtchn_status fails with EFAULT on HVM, the same on PV works

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 13/01/2017 20:32, Marek Marczykowski-Górecki wrote:
> On Fri, Jan 13, 2017 at 07:54:01PM +0000, Andrew Cooper wrote:
>> On 13/01/17 19:40, Marek Marczykowski-Górecki wrote:
>>> On Fri, Jan 13, 2017 at 07:27:20PM +0000, Andrew Cooper wrote:
>>>> On 13/01/17 18:59, Marek Marczykowski-Górecki wrote:
>>>>> On Fri, Jan 13, 2017 at 06:37:06PM +0000, Andrew Cooper wrote:
>>>>>> On 13/01/17 18:32, Marek Marczykowski-Górecki wrote:
>>>>>>> On Fri, Jan 13, 2017 at 06:15:35PM +0000, Andrew Cooper wrote:
>>>>>>>> Can you get the result of this piece of debugging in the failure case?
>>>>>>> I've got this:
>>>>>>> ** d4v0 CFG(24, 00007f794bd07004, 1) = 24
>>>>>> Silly question (and I really hope the answer isn't yes, but I have a
>>>>>> sinking feeling it is).
>>>>>>
>>>>>> Is the guest in question using SMAP? If so, does disabling SMAP fix the
>>>>>> problem?
>>>>> How can I check that? If looking at 0x200000 CR4 bit in `xl debug-keys
>>>>> v` output enough, then yes - it's enabled. And booting hypervisor with
>>>>> smap=0 "fixed" the problem.
>>>> :(, although now I think about it, there might be a quick option.
>>>>
>>>>> So, what would be the correct solution? I'd prefer not to disable SMAP
>>>>> "just" for this reason...
>>>> For the quick option, the privcmd driver in Linux needs a stac()/clac()
>>>> pair around the actual hypercall invocation, to whitelist userspace
>>>> memory accesses as being ok.
>>>>
>>>> Something like this (completely untested)
>>>>
>>>> andrewcoop@andrewcoop:/local/linux.git/arch/x86$ git diff
>>>> diff --git a/arch/x86/include/asm/xen/hypercall.h
>>>> b/arch/x86/include/asm/xen/hypercall.h
>>>> index a12a047..e1b2af9e 100644
>>>> --- a/arch/x86/include/asm/xen/hypercall.h
>>>> +++ b/arch/x86/include/asm/xen/hypercall.h
>>>> @@ -214,10 +214,12 @@ privcmd_call(unsigned call,
>>>>         __HYPERCALL_DECLS;
>>>>         __HYPERCALL_5ARG(a1, a2, a3, a4, a5);
>>>>  
>>>> +       stac();
>>>>         asm volatile("call *%[call]"
>>>>                      : __HYPERCALL_5PARAM
>>>>                      : [call] "a" (&hypercall_page[call])
>>>>                      : __HYPERCALL_CLOBBER5);
>>>> +       clac();
>>>>  
>>>>         return (long)__res;
>>>>  }
>>> Is there any option to do that from hypervisor side? For example somehow
>>> modify copy_from_guest/copy_to_guest? I'm not always controlling the VM
>>> kernel (for example sometimes I need to cope with the one from Debian
>>> stable).
>> Not really.  (I mean - there is, but it involves deliberately breaking
>> SMAP support in Xen.)
>>
>> This is a guest kernel bug.  The guest kernel has used SMAP to say
>> "please disallow all userspace accesses", and Xen is respecting this
>> when trying to follow the pointer in the hypercall.
> Hmm, if that's the case, isn't the above patch also effectively breaking
> SMAP?

No.

> I see the purpose of SMAP is to prevent _accidental_ access to
> arbitrary, guest controlled memory.

This is correct.  The point of SMAP is to prevent accidental access to
user memory.

The ABI of the privcmd hypercall ioctl() is to pass values straight to
the hypervisor, and it is a fact that most of these hypercalls contain
pointers to hypercall buffers, which reside in userspace.

(There is a separate thread ongoing questioning whether the ABI is
appropriate, but irrespective of those results, this is how the ABI
currently behaves.  A concerned person might note that anyone with an
open privcmd fd can issue any hypercall, including the ones only
intended for kernels, such as set_trap_table, update_va_mapping and iret.)

Therefore, it is an expected consequence that a hypercall passed blindly
from a userspace agent contains userspace pointers which Xen must follow
to complete the hypercall successfully.

> For example because of some memory
> corruption bug in the hypervisor. If such a bug could be triggered using
> a hypercall, then the above patch also makes SMAP useless.  Patching
> copy_from_guest/copy_to_guest on the other hand would only allow such
> guest controlled memory access when hypervisor is really supposed to
> access guest memory.

I don't follow this argument.

In a native situation, SMAP raises #PF if hardware tries to follow a
user pointer outside of an area whitelisted by the kernel.  (The
copy_*_guest path suppresses #PF, opting instead to return -EFAULT.)

The choice of supervisor vs user in a pagewalk is a single bit, and Xen
(for better or worse, but realistically as a consequence of SMAP being
~10 years younger than HVM guests) accesses pointers from hypercalls as
a supervisor entity when walking the pagetables.  The key point here is
that Xen must emulate the hardware pagewalker when trying to find the
data being pointed to.

If Xen has a bug causing spurious accesses to HVM guests, then all bets
are already off wrt memory corruption, because real hardware isn't used
to make the access.

As indicated before, we could deliberately break the Xen pagewalker to
ignore SMAP.  However, that would be identical to "pretend the guest
actually whitelisted this access".  This would fix your symptoms, but
open up a hole in the case that userspace somehow managed to trick Xen
into thinking a legitimate hypercall had been made, at which point Xen
would ignore the last level of protection that SMAP was supposed to offer.

>> This bug doesn't
>> manifest for PV guests, and you are probably the first person to do any
>> serious hypercall work from HVM userspace.
> That's interesting. I'm just trying to use slightly modified
> libxenchan...

And I believe that you are first person (in 6 years of being a part of
upstream Xen) who I have positively identified as having used
libxenchan.  (There have been a few comments along the line of "oh yeah
- there is that libxenchan thing which might be worth looking at".)

It does raise some questions though.  Linux has (what is supposed to be
a) perfectly good /dev/xen/evtchn, for interacting with event channels. 
Why is libxenchan open-coding the hypercalls using a lower level
interface?  Can it be modified to use the higher level interfaces?

~Andrew

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