Re: [Xen-devel] [PATCH 1/4] x86/vmx: Don't leak host syscall MSR state into HVM guests

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> From: Andrew Cooper [mailto:andrew.cooper3@xxxxxxxxxx]
> Sent: Monday, February 20, 2017 7:17 PM
> To: Tian, Kevin
> Cc: Jan Beulich; Suravee Suthikulpanit; Nakajima, Jun; Xen-devel; Boris 
> Ostrovsky
> Subject: Re: [PATCH 1/4] x86/vmx: Don't leak host syscall MSR state into HVM 
> guests
> 
> On 14/02/17 10:13, Jan Beulich wrote:
> >>>> On 14.02.17 at 09:40, <kevin.tian@xxxxxxxxx> wrote:
> >>>  From: Andrew Cooper [mailto:amc96@xxxxxxxxxxxxxxxx] On Behalf Of Andrew
> Cooper
> >>> Sent: Tuesday, February 14, 2017 4:19 PM
> >>>
> >>> On 14/02/2017 08:04, Tian, Kevin wrote:
> >>>>> From: Andrew Cooper [mailto:amc96@xxxxxxxxxxxxxxxx] On Behalf Of Andrew
> >>> Cooper
> >>>>> Sent: Tuesday, February 14, 2017 3:59 PM
> >>>>>
> >>>>> On 14/02/2017 02:52, Tian, Kevin wrote:
> >>>>>>> From: Andrew Cooper [mailto:andrew.cooper3@xxxxxxxxxx]
> >>>>>>> Sent: Monday, February 13, 2017 10:32 PM
> >>>>>>>
> >>>>>>> hvm_hw_cpu->msr_flags is in fact the VMX dirty bitmap of MSRs needing 
> >>>>>>> to
> be
> >>>>>>> restored when switching into guest context.  It should never have 
> >>>>>>> been part
> of
> >>>>>>> the migration state to start with, and Xen must not make any 
> >>>>>>> decisions based
> >>>>>>> on the value seen during restore.
> >>>>>>>
> >>>>>>> Identify it as obsolete in the header files, consistently save it as 
> >>>>>>> zero and
> >>>>>>> ignore it on restore.
> >>>>>>>
> >>>>>>> The MSRs must be considered dirty during VMCS creation to cause the 
> >>>>>>> proper
> >>>>>>> defaults of 0 to be visible to the guest.
> >>>>>>>
> >>>>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> >>>>>> Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx>, with one small comment.
> >>>>>>
> >>>>>> the effect of this patch should be more than not leaking syscall MSR.
> >>>>>> what about making the subject clearer when check-in?
> >>>>> What other effects do you think are going on here?  Yes, we now context
> >>>>> switch the MSRs right from the start of the domain, but that is
> >>>>> necessary to avoid leaking them.
> >>>>>
> >>>> If just looking at this patch, it's for general MSR save/restore policy,
> >>>> nothing specific to syscall MSR.
> >>> The only three MSRs which use this infrastructure are LSTAR, STAR and
> >>> FMASK.  What if I were to clarify that in the first paragraph?
> >> I meant the subject line (talk about syscall MSR leakage) mismatches the
> >> commit message (for general MSR load)
> > I'm with Andrew here: The title seems perfectly fine to me, considering
> > that the generic mechanism is only used for the syscall MSRs. Hence I
> > would think his offer to clarify the change in the first paragraph of the
> > commit message ought to suffice. Otherwise, may I ask that you make
> > a concrete suggestion as to what you'd like to see?
> 
> Kevin: Do you have any other concerns?
> 
> ~Andrew

fine to me.

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