Re: [PATCH 2/2] x86/hvm: honor guest's option when updating secondary system time for guest

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 25/07/14 09:31, Jan Beulich wrote:
>>>> On 25.07.14 at 10:03, <feng.wu@intel.com> wrote:
>>> -----Original Message-----
>>> From: Jan Beulich [mailto:JBeulich@suse.com]
>>> Sent: Friday, July 25, 2014 3:40 PM
>>> To: Wu, Feng
>>> Cc: linux@eikelenboom.it; xen-devel@lists.xen.org; konrad.wilk@oracle.com;
>>> keir@xen.org; tim@xen.org 
>>> Subject: RE: [PATCH 2/2] x86/hvm: honor guest's option when updating
>>> secondary system time for guest
>>>
>>>>>> On 25.07.14 at 09:33, <feng.wu@intel.com> wrote:
>>>>> -----Original Message-----
>>>>> From: Jan Beulich [mailto:JBeulich@suse.com]
>>>>> Sent: Friday, July 25, 2014 3:26 PM
>>>>> To: Wu, Feng
>>>>> Cc: linux@eikelenboom.it; xen-devel@lists.xen.org;
>>> konrad.wilk@oracle.com;
>>>>> keir@xen.org; tim@xen.org 
>>>>> Subject: RE: [PATCH 2/2] x86/hvm: honor guest's option when updating
>>>>> secondary system time for guest
>>>>>
>>>>>>>> On 25.07.14 at 06:30, <feng.wu@intel.com> wrote:
>>>>>>> -----Original Message-----
>>>>>>> From: Jan Beulich [mailto:JBeulich@suse.com]
>>>>>>> Sent: Wednesday, July 23, 2014 8:19 PM
>>>>>>> To: Wu, Feng
>>>>>>> Cc: linux@eikelenboom.it; xen-devel@lists.xen.org;
>>>>> konrad.wilk@oracle.com;
>>>>>>> keir@xen.org; tim@xen.org 
>>>>>>> Subject: Re: [PATCH 2/2] x86/hvm: honor guest's option when updating
>>>>>>> secondary system time for guest
>>>>>>>
>>>>>>>>>> On 08.07.14 at 01:18, <feng.wu@intel.com> wrote:
>>>>>>>> --- a/xen/include/public/vcpu.h
>>>>>>>> +++ b/xen/include/public/vcpu.h
>>>>>>>> @@ -227,6 +227,16 @@ struct vcpu_register_time_memory_area {
>>>>>>>>  typedef struct vcpu_register_time_memory_area
>>>>>>>> vcpu_register_time_memory_area_t;
>>>>>>>>  DEFINE_XEN_GUEST_HANDLE(vcpu_register_time_memory_area_t);
>>>>>>>>
>>>>>>>> +/*
>>>>>>>> + * Flags to tell Xen whether we need to do the SMAP check when
>>>>> updating
>>>>>>>> + * the secondary copy of the vcpu time when SMAP is enabled. Since
>>> the
>>>>>>>> + * memory location for the secondary copy of the vcpu time may be
>>>>> mapped
>>>>>>>> + * into userspace by guests intendedly, we let the guest to determine
>>>>>>>> + * whether the check is needed. The default behavior of hypevisor is
>>>>>>>> + * not doing the check.
>>>>>>>> + */
>>>>>>>> +#define VCPUOP_enable_smap_check_vcpu_time_memory_area
>>> 14
>>>>>>> I think the new op to be
>>> VCPUOP_register_vcpu_time_memory_area_smap,
>>>>>>> identical to VCPUOP_register_vcpu_time_memory_area apart from also
>>>>>>> setting the flag, would be more natural. But considering what I just
>>> wrote
>>>>>>> in the reply to Tim I guess we can expect a nun-user mapping to be
>>>>>>> presented here anyway, i.e. we wouldn't need to new operation at all.
>>>>>> Do you mean since the user-paging is r/o, guest will pass a r/w kernel page
>>>>>> to
>>>>>> Xen for updating the system time. So we don't need to do the SMAP check
>>>>>> in this case?
>>>>> If the user page is r/o, it's VA obviously can't be used for updating by
>>>>> Xen. Hence the kernel has to provide a r/w mapped VA. That should be
>>>>> subject to SMAP checking (consistent with the runstate area handling),
>>>>> to make sure it's not a user accessible mapping.
>>>> But there are two possible problems here:
>>>> 1. Is it possible that guest passes a user r/w page to update the system
>>>> time information?
>>> The guest kernel may pass a page that was originally mapped r/w-
>>> kernel, but the mapping later gets (say inadvertently) changed to
>>> r/w-user. That's why I think the SMAP checking ought to be done
>>> here.
>> But in another case, as what we discussed before, if the guest intended to
>> pass a user r/w page to get the time information. Do we need to do the
>> SMAP checking for this?
> The question is whether we should bother supporting such insecure
> kernels.

Thinking about this some more, how can we possibly know whether writing
to the nominated page is even safe?  At the point of update, there is no
knowledge about the current cr3 in use.

As far as I can see, the only safe actions Xen can take is refuse to
ever write to user mappings, and insist that any OS using this feature
provide a supervisor mapping to Xen (and guarantee it will never swap
the target of that mapping) and give user mappings of the same gpa to
any of its own userspace which wants access.  This then avoids any
concerns wrt SMAP.

~Andrew