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

["Wu, Feng" <feng.wu@xxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: Friday, July 25, 2014 4:32 PM
> To: Wu, Feng
> Cc: linux@xxxxxxxxxxxxxx; xen-devel@xxxxxxxxxxxxx; konrad.wilk@xxxxxxxxxx;
> keir@xxxxxxx; tim@xxxxxxx
> Subject: RE: [PATCH 2/2] x86/hvm: honor guest's option when updating
> secondary system time for guest
> 
> >>> On 25.07.14 at 10:03, <feng.wu@xxxxxxxxx> wrote:
> 
> >
> >> -----Original Message-----
> >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> >> Sent: Friday, July 25, 2014 3:40 PM
> >> To: Wu, Feng
> >> Cc: linux@xxxxxxxxxxxxxx; xen-devel@xxxxxxxxxxxxx;
> konrad.wilk@xxxxxxxxxx;
> >> keir@xxxxxxx; tim@xxxxxxx
> >> 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@xxxxxxxxx> wrote:
> >>
> >> >
> >> >> -----Original Message-----
> >> >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> >> >> Sent: Friday, July 25, 2014 3:26 PM
> >> >> To: Wu, Feng
> >> >> Cc: linux@xxxxxxxxxxxxxx; xen-devel@xxxxxxxxxxxxx;
> >> konrad.wilk@xxxxxxxxxx;
> >> >> keir@xxxxxxx; tim@xxxxxxx
> >> >> 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@xxxxxxxxx> wrote:
> >> >>
> >> >> >
> >> >> >> -----Original Message-----
> >> >> >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> >> >> >> Sent: Wednesday, July 23, 2014 8:19 PM
> >> >> >> To: Wu, Feng
> >> >> >> Cc: linux@xxxxxxxxxxxxxx; xen-devel@xxxxxxxxxxxxx;
> >> >> konrad.wilk@xxxxxxxxxx;
> >> >> >> keir@xxxxxxx; tim@xxxxxxx
> >> >> >> 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@xxxxxxxxx> 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.
> 
> >> > 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).
> >
> > Just like my comments above, my point is in some case, guest may pass a
> > user page (r/o or r/w) to get the time information, I doubt whether the SMAP
> > checking should be done for this?
> 
> If we want to allow kernels to expose the time data r/w to user mode,
> then yes, we would need to avoid the SMAP check. But as said above,
> I don't think helping kernels to be insecure is of any particular value.

Okay, thanks a lot for your comments! It makes sense. I got your point. I will 
do the
SMAP checking for both of the two cases (runstate area and system time 
information).

Thanks,
Feng

> 
> Jan

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