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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 17.07.18 at 12:17, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 16/07/18 12:36, Jan Beulich wrote:
>>>>> On 13.07.18 at 22:03, <andrew.cooper3@xxxxxxxxxx> 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) ...

Both have the same effect afaict, due to ~0 being signed int.

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

Right, but the primary problem with understanding whether the lack
of checking here is a problem is the lack of a caller of this function.
As I think I did say in the earlier reply - it matters quite a bit where p
points.

Jan



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