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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 05/07/18 10:08, Jan Beulich wrote:
>>>> 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.

Ok, in which case I'll also move the max vs default discussion into the
previous patch.

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

With this diff:

diff --git a/xen/include/xen/libx86/policies.h
b/xen/include/xen/libx86/policies.h
index 21e0a40..03fe6dd 100644
--- a/xen/include/xen/libx86/policies.h
+++ b/xen/include/xen/libx86/policies.h
@@ -7,9 +7,9 @@
 
 struct policy_group
 {
-    struct cpuid_policy *cp;
-    struct msr_domain_policy *dp;
-    struct msr_vcpu_policy *vp;
+    const struct cpuid_policy *cp;
+    const struct msr_domain_policy *dp;
+    const struct msr_vcpu_policy *vp;
 };
 
 /*

The resulting compiler complains are

domctl.c: In function ‘update_domain_cpumsr_policy’:
domctl.c:355:15: error: passing argument 1 of ‘x86_cpuid_copy_from_buffer’ 
discards ‘const’ qualifier from pointer target type [-Werror]
               new.cp, xdpc->cpuid_policy, xdpc->nr_leaves,
               ^
In file included from /local/xen.git/xen/include/asm/cpuid.h:11:0,
                 from /local/xen.git/xen/include/asm/cpufeature.h:10,
                 from /local/xen.git/xen/include/asm/processor.h:14,
                 from /local/xen.git/xen/include/asm/system.h:6,
                 from /local/xen.git/xen/include/xen/list.h:11,
                 from /local/xen.git/xen/include/xen/mm.h:50,
                 from domctl.c:9:
/local/xen.git/xen/include/xen/libx86/cpuid.h:257:5: note: expected ‘struct 
cpuid_policy *’ but argument is of type ‘const struct cpuid_policy *’
 int x86_cpuid_copy_from_buffer(struct cpuid_policy *p,
     ^
domctl.c:360:15: error: passing argument 1 of ‘x86_msr_copy_from_buffer’ 
discards ‘const’ qualifier from pointer target type [-Werror]
               new.dp, new.vp,
               ^
In file included from /local/xen.git/xen/include/xen/libx86/policies.h:6:0,
                 from /local/xen.git/xen/include/asm/cpuid.h:12,
                 from /local/xen.git/xen/include/asm/cpufeature.h:10,
                 from /local/xen.git/xen/include/asm/processor.h:14,
                 from /local/xen.git/xen/include/asm/system.h:6,
                 from /local/xen.git/xen/include/xen/list.h:11,
                 from /local/xen.git/xen/include/xen/mm.h:50,
                 from domctl.c:9:
/local/xen.git/xen/include/xen/libx86/msr.h:63:5: note: expected ‘struct 
msr_domain_policy *’ but argument is of type ‘const struct msr_domain_policy *’
 int x86_msr_copy_from_buffer(struct msr_domain_policy *dp,
     ^
domctl.c:360:23: error: passing argument 2 of ‘x86_msr_copy_from_buffer’ 
discards ‘const’ qualifier from pointer target type [-Werror]
               new.dp, new.vp,
                       ^
In file included from /local/xen.git/xen/include/xen/libx86/policies.h:6:0,
                 from /local/xen.git/xen/include/asm/cpuid.h:12,
                 from /local/xen.git/xen/include/asm/cpufeature.h:10,
                 from /local/xen.git/xen/include/asm/processor.h:14,
                 from /local/xen.git/xen/include/asm/system.h:6,
                 from /local/xen.git/xen/include/xen/list.h:11,
                 from /local/xen.git/xen/include/xen/mm.h:50,
                 from domctl.c:9:
/local/xen.git/xen/include/xen/libx86/msr.h:63:5: note: expected ‘struct 
msr_vcpu_policy *’ but argument is of type ‘const struct msr_vcpu_policy *’
 int x86_msr_copy_from_buffer(struct msr_domain_policy *dp,
     ^
domctl.c:373:84: error: assignment discards ‘const’ qualifier from pointer 
target type [-Werror]
     SWAP(new.cp, d->arch.cpuid);
                                                                                
    ^
domctl.c:374:80: error: assignment discards ‘const’ qualifier from pointer 
target type [-Werror]
     SWAP(new.dp, d->arch.msr);
                                                                                
^
domctl.c:375:80: error: assignment discards ‘const’ qualifier from pointer 
target type [-Werror]
     SWAP(new.vp, v->arch.msr);
                                                                                
^
domctl.c:391:11: error: passing argument 1 of ‘xfree’ discards ‘const’ 
qualifier from pointer target type [-Werror]
     xfree(new.cp);
           ^
In file included from /local/xen.git/xen/include/xen/lib.h:7:0,
                 from domctl.c:8:
/local/xen.git/xen/include/xen/xmalloc.h:34:13: note: expected ‘void *’ but 
argument is of type ‘const struct cpuid_policy *’
 extern void xfree(void *);
             ^
domctl.c:392:11: error: passing argument 1 of ‘xfree’ discards ‘const’ 
qualifier from pointer target type [-Werror]
     xfree(new.dp);
           ^
In file included from /local/xen.git/xen/include/xen/lib.h:7:0,
                 from domctl.c:8:
/local/xen.git/xen/include/xen/xmalloc.h:34:13: note: expected ‘void *’ but 
argument is of type ‘const struct msr_domain_policy *’
 extern void xfree(void *);
             ^
domctl.c:393:11: error: passing argument 1 of ‘xfree’ discards ‘const’ 
qualifier from pointer target type [-Werror]
     xfree(new.vp);
           ^
In file included from /local/xen.git/xen/include/xen/lib.h:7:0,
                 from domctl.c:8:
/local/xen.git/xen/include/xen/xmalloc.h:34:13: note: expected ‘void *’ but 
argument is of type ‘const struct msr_vcpu_policy *’
 extern void xfree(void *);
             ^
cc1: all warnings being treated as errors


Furthermore, while x86_policies_are_compatible() isn't intended to
modify the policies, the other important function for levelling
(x86_calculate_compatible_policy(a, b, out)) will write to its parameter.

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

"No its not half a gadget.  Its a full gadget".

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

Hmm - I suppose "cpu policy" does logically cover any and all of the
main core, without including any of the uncore or chipset, which matches
the intended purpose.

I'll see about applying this naming scheme consistently across the series.

Julien/Stefano: Are you liable to want something like this on ARM?

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

Right, but possibly the only thing worse than an interface version of
questionable utility is inconsistent ABI expectations across different
subops of an otherwise consistent hypercall.

In principle I'd like to improve the ABI expectations of these ops, but
a) we need something much better than this suggestion, and b) I'm not
going to get diverted into fixing that rats nest as part of this series.

~Andrew

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