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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

> +                goto err;
> +
> +            p->plaform_info.raw = data.val;

No other sanity checking? If the implication is that the policy
stored into isn't an instance attached to a domain (but some
intermediate object), and sanity checking is going to be done
later, then storing and then comparing the values would perhaps
be even more obvious an approach above.

The intentions here are hard to judge, because both this
function and its CPUID counterpart appear to remain unused
by the end of the series.

Jan



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