Re: [PATCH v2 10/13] libx86: introduce a helper to deserialise msr_policy objects

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 16/07/18 12:36, Jan Beulich wrote:
>>>> On 13.07.18 at 22:03, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/common/libx86/msr.c
>> +++ b/xen/common/libx86/msr.c
>> @@ -45,6 +45,57 @@ int x86_msr_copy_to_buffer(const struct msr_policy *p,
>>      return 0;
>>  }
>>  
>> +int x86_msr_copy_from_buffer(struct msr_policy *p,
>> +                             const msr_entry_buffer_t msrs, uint32_t nr_msrs,
>> +                             uint32_t *err_msr)
>> +{
>> +    unsigned int i;
>> +    xen_msr_entry_t data;
>> +
>> +    /*
>> +     * A well formed caller is expected pass an array with entries in order,
>> +     * and without any repetitions.  However, due to per-vendor differences,
>> +     * and in the case of upgrade or levelled scenarios, we typically expect
>> +     * fewer than MAX entries to be passed.
>> +     *
>> +     * Detecting repeated entries is prohibitively complicated, so we don't
>> +     * bother.  That said, one way or another if more than MAX entries are
>> +     * passed, something is wrong.
>> +     */
>> +    if ( nr_msrs > MSR_MAX_SERIALISED_ENTRIES )
>> +        return -E2BIG;
>> +
>> +    for ( i = 0; i < nr_msrs; i++ )
>> +    {
>> +        if ( copy_from_buffer_offset(&data, msrs, i, 1) )
>> +            return -EFAULT;
>> +
>> +        if ( data.flags ) /* .flags MBZ */
>> +            goto err;
>> +
>> +        switch ( data.idx )
>> +        {
>> +        case MSR_INTEL_PLATFORM_INFO:
>> +            if ( data.val > ~0u )
> I suppose this is to guard against truncation. I think it would be
> more obvious (and future proof) if you used
> (typeof(p->plaform_info.raw))~0,

ITYM ~((typeof(p->plaform_info.raw)0) ...

>  or an intermediate variable
> of that type, or data.val >> (sizeof(p->plaform_info.raw) * 8),
> some of which would likely even trigger a compiler warning once
> the policy field was grown to uint64_t.

... but this is probably better.

>
>> +                goto err;
>> +
>> +            p->plaform_info.raw = data.val;
> No other sanity checking?

Correct.  This is a data marshalling function, not an auditing function.

The auditing functions are also needed for in-place modification to an
existing policy.

~Andrew

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