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

["Jan Beulich" <JBeulich@suse.com>]

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

> 2. Even the user page is r/o, the kernel can still use it to update the 
> system time info when WP is disabled.

The kernel can - for its own accesses to the page - do whatever
it wants. It can't, however, control Xen's updating of it, and Xen
won't do that with CR0.WP clear (leaving aside Aravindh's PV
mem-access work, where this option is currently being discussed).

Jan