Re: [Xen-devel] [PATCH 13/13] x86/domctl: Implement XEN_DOMCTL_set_cpumsr_policy

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 04/07/18 11:18, Wei Liu wrote:
> On Tue, Jul 03, 2018 at 09:55:26PM +0100, Andrew Cooper wrote:
>> From: Sergey Dyasli <sergey.dyasli@xxxxxxxxxx>
>>
>> This hypercall allows the toolstack to present one combined CPUID and MSR
>> policy for a domain, which can be audited in one go by Xen, which is 
>> necessary
>> for correctness of the auditing.
>>
>> A stub x86_policies_are_compatible() function is introduced, although at
>> present it will always fail the hypercall.
>>
>> The hypercall ABI allows for update of individual CPUID or MSR entries, so
>> begins by duplicating the existing policy (for which a helper is introduced),
>> merging the toolstack data, then checking compatibility of the result.
>>
>> The system PV/HVM max policy is used for the compatiblity check.
>>
>> Signed-off-by: Sergey Dyasli <sergey.dyasli@xxxxxxxxxx>
>> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> ---
>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>> CC: Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
>> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
>> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>> CC: Sergey Dyasli <sergey.dyasli@xxxxxxxxxx>
>> CC: Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>
>>
>> One awkard corner case is re-deserialising of the vcpu msrs.  The correct fix
>> would be to allocate a buffer, copy the MSRs list, then deserialise from 
>> that,
>> but trips the bounds checks in the copy_from_guest() helpers.  The compat 
>> XLAT
>> are would work, but would require that we allocate it even for 64bit PV
>> guests.
> I'm not sure I follow this. The issue isn't obvious from looking at the
> code.
>
>> ---
>> +    /* Merge the (now audited) vCPU MSRs into every other msr_vcpu_policy. 
>> */
>> +    for ( ; v; v = v->next_in_list )
>> +    {
>> +        /* XXX - Figure out how to avoid a TOCTOU race here.  XLAT area? */
> What is the TOCTOU race here?

In the lines you snipped...

>
>> +        if ( (ret = x86_msr_copy_from_buffer(
>> +                  NULL, v->arch.msr, xdpc->msr_policy, xdpc->nr_msrs, 
>> NULL)) )
>

xdpc->msr_policy is a pointer into toolstack userspace, which we re-read.

The problem is that we can't copy this up front to a hypervisor buffer
(which would avoid the TOCTOU race), without tripping the access_ok()
check in copy_from_guest().

However, Jan and Sergey (IRL) have suggested an alternative approach
which will work for now.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel