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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

Jan


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