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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 05.07.18 at 16:08, <andrew.cooper3@xxxxxxxxxx> wrote:
> 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:
>>>> 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,
>                ^

As said - you'd need local variables being pointers to non-const, and
"new" should be used solely for handing the entire pack to the
auditing function.

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

Hmm, that's unfortunate. I'd really like to have a way to make sure
(not just by commentary) that checking functions can't alter their
inputs. I agree though that having a second container structure
doesn't look very appealing.

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

Right - as I hadn't checked further, I did write "at least".

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

I can't help a), but as to b) you certainly shouldn't, yet my frequently
made remark applies here perfectly well: Let's not use bad examples
as excuse to widen the badness. As to consistency, it only took me
looking at the first few lines of sysctl.h to find padding fields in
xen_sysctl_readconsole (I didn't go look whether there's also checking
for them to be zero on input, but the latest for the live patching
sysctl you'll find that padding is validated to be zero).

Jan


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