Re: [Xen-devel] [PATCH 09/13] x86/sysctl: Implement XEN_SYSCTL_get_cpumsr_policy

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.07.18 at 19:57, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 04/07/18 10:43, Jan Beulich wrote:
>>> --- a/xen/arch/x86/sysctl.c
>>> +++ b/xen/arch/x86/sysctl.c
>>> @@ -32,22 +32,32 @@
>>>  #include <asm/cpuid.h>
>>>  
>>>  const struct policy_group system_policies[] = {
>>> -    {
>>> +    [ XEN_SYSCTL_cpumsr_policy_raw ] = {
>> Aha - this clarifies a question I had on the earlier patch. But it would
>> be nice if that other patch was self contained also in the way of
>> allowing readers to understand the intentions.
> 
> One thing I could do is introduce the XEN_SYSCTL_cpumsr_policy_* defines
> in the previous patch?  I don't want to merge the two patches as that is
> too many moving parts to review in a single patch.

I think this would help, yes.

>> And with this I now
>> wonder whether the pointers in struct policy_group shouldn't all
>> be const qualified.
> 
> Unfortunately that doesn't work with the logic to create a policy_group
> for an individual domain during audit.

I don't understand: Is x86_policies_are_compatible() supposed to
alter the policies? Otherwise, if you maintained local separate
pointers in update_domain_cpumsr_policy(), using "new" just for
the purpose of passing to x86_policies_are_compatible(), all
should be fine and it would be crystal clear that anyone handed
a group won't alter anything the group refers to.

>>> @@ -318,6 +328,74 @@ long arch_do_sysctl(
>>>          break;
>>>      }
>>>  
>>> +    case XEN_SYSCTL_get_cpumsr_policy:
>>> +    {
>>> +        const struct policy_group *group;
>>> +
>>> +        /* Bad policy index? */
>>> +        if ( sysctl->u.cpumsr_policy.index >= ARRAY_SIZE(system_policies) )
>>> +        {
>>> +            ret = -EINVAL;
>>> +            break;
>>> +        }
>>> +        group = &system_policies[sysctl->u.cpumsr_policy.index];
>> Isn't this introducing at least half of a Spectre v1 gadget?
> 
> Nope :(
> 
> It's both halves of the Spectre gadget, when you account for the
> dereference when calling x86_*_copy_to_buffer() slightly lower.
> 
> I suppose we want to port the Linux array nospec lookup logic so we can
> protect the clearly-visible gadgets.

I'm confused: You first say "nope", but the rest of your response reads
as if you meant "yes.".

>>> --- a/xen/include/public/sysctl.h
>>> +++ b/xen/include/public/sysctl.h
>>> @@ -1063,6 +1063,43 @@ struct xen_sysctl_set_parameter {
>>>      uint16_t pad[3];                        /* IN: MUST be zero. */
>>>  };
>>>  
>>> +#if defined(__i386__) || defined(__x86_64__)
>>> +/*
>>> + * XEN_SYSCTL_get_cpumsr_policy (x86 specific)
>> Perhaps express the "x86 specific" also in the opcode name? And make
>> more obvious that this is about CPUID and MSRs at the same time? E.g.
>> XEN_SYSCTL_x86_get_cpuid_msr_policy?
>>
>> I'm sure you have reasons to munge it all into a single operation.
> 
> (Answering in reverse order)
> 
> The get operations don't strictly need to be a single operation.  The
> set operation specifically must be a single operation, and the getters
> have an interface to match.
> 
> As for naming, cpumsr_policy wasn't chosen by me, but I can't think of
> anything better.  The code is currently consistent and, while I'm open
> to a rename, it will impact large quantities of the series.
> 
> One concern I have if we end up with a new block of information.  I was
> hoping for a generic name, but simply "policy" on its own is too
> generic.  cpumsr is, I believe, a contraction of cpuid_msr to avoid
> excessive code volume.
> 
> Suggestions welcome.

To cover potential future additions, why not XEN_SYSCTL_get_cpu_policy?
That's neither misleading by abbreviating too much, nor more specific than
we need it to be. However, in this case it might be worthwhile to consider
adding in "x86", as ARM might plausibly want something similar at some
point. Otoh the same name (but different structure contents) could be
used for both.

>>> +struct xen_sysctl_cpumsr_policy {
>>> +#define XEN_SYSCTL_cpumsr_policy_raw          0
>>> +#define XEN_SYSCTL_cpumsr_policy_host         1
>>> +#define XEN_SYSCTL_cpumsr_policy_pv_max       2
>>> +#define XEN_SYSCTL_cpumsr_policy_hvm_max      3
>>> +#define XEN_SYSCTL_cpumsr_policy_pv_default   4
>>> +#define XEN_SYSCTL_cpumsr_policy_hvm_default  5
>>> +    uint32_t index;       /* IN: Which policy to query? */
>>> +    uint32_t nr_leaves;   /* IN/OUT: Number of leaves in/written to
>>> +                           * 'cpuid_policy', or the maximum number of 
>>> leaves if
>>> +                           * any of the guest handles is NULL.
>>> +                           * NB. All policies come from the same space,
>>> +                           * so have the same maximum length. */
>>> +    uint32_t nr_msrs;     /* IN/OUT: Number of MSRs in/written to
>>> +                           * 'msr_domain_policy', or the maximum number of 
>>> MSRs
>>> +                           * if any of the guest handles is NULL.
>>> +                           * NB. All policies come from the same space,
>>> +                           * so have the same maximum length. */
>>> +    XEN_GUEST_HANDLE_64(xen_cpuid_leaf_t) cpuid_policy; /* OUT: */
>> Explicit padding (checked to be zero in the handler) above here
>> please.
> 
> Why?  SYSCTLs are unstable and we don't perform similar checks for other
> subops.

You don't like the interface version bumps anyway, as being only
partly useful. If you added and checked explicit padding, no bump
would be needed once the field gains meaning.

Jan


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