Re: [PATCH v6 02/12] libx86: introduce helper to fetch cpuid leaf

[Andrew Cooper <amc96@xxxxxxxx>]

On 17/01/2022 09:48, Roger Pau Monne wrote:
> Introduce a helper based on the current Xen guest_cpuid code in order
> to fetch a cpuid leaf from a policy. The newly introduced function in
> cpuid.c should not be directly called and instead the provided
> x86_cpuid_get_leaf macro should be used that will properly deal with
> const and non-const inputs.
>
> Also add a test to check that the introduced helper doesn't go over
> the bounds of the policy.
>
> Note the code in x86_cpuid_copy_from_buffer is not switched to use the
> new function because of the boundary checks against the max fields of
> the policy, which might not be properly set at the point where
> x86_cpuid_copy_from_buffer get called, for example when filling an
> empty policy from scratch.

Filling an empty policy from scratch will be fine, because we always
ascend through leaves.  This also matches the chronology of how CPUID
developed.

The most likely case to go wrong is enabling an optional feature above
max_leaf, and getting the bump to max_leaf out of order.  That said, I
suspect such logic would be working on an object, rather than a list.

The important point is that x86_cpuid_copy_from_buffer() is deliberately
invariant to the order of entries for compatibility reasons, even if we
don't expect it to matter in practice.

>
> Suggested-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> Changes since v4:
>  - Rename _x86_cpuid_get_leaf to x86_cpuid_get_leaf_const.
>
> Changes since v3:
>  - New in this version.
> ---
> Regarding safety of the usage of array_access_nospec to obtain a
> pointer to an element of an array, there are already other instances
> of this usage, for example in viridian_time_wrmsr, so I would assume
> this is fine.

It's a bit of a weird construct, and both GCC and Clang could generate
better code, but it does look to be safe.

> ---
>  tools/tests/cpu-policy/test-cpu-policy.c | 75 ++++++++++++++++++++++++
>  xen/arch/x86/cpuid.c                     | 55 +++--------------
>  xen/include/xen/lib/x86/cpuid.h          | 19 ++++++
>  xen/lib/x86/cpuid.c                      | 52 ++++++++++++++++
>  4 files changed, 153 insertions(+), 48 deletions(-)
>
> diff --git a/tools/tests/cpu-policy/test-cpu-policy.c 
> b/tools/tests/cpu-policy/test-cpu-policy.c
> index ed450a0997..3f777fc1fc 100644
> --- a/tools/tests/cpu-policy/test-cpu-policy.c
> +++ b/tools/tests/cpu-policy/test-cpu-policy.c
> @@ -570,6 +570,80 @@ static void test_cpuid_out_of_range_clearing(void)
>      }
>  }
>  
> +static void test_cpuid_get_leaf_failure(void)
> +{
> +    static const struct test {
> +        struct cpuid_policy p;
> +        const char *name;
> +        uint32_t leaf, subleaf;
> +    } tests[] = {
> +        /* Bound checking logic. */
> +        {
> +            .name = "Basic max leaf >= array size",
> +            .p = {
> +                .basic.max_leaf = CPUID_GUEST_NR_BASIC,
> +            },
> +        },
> +        {
> +            .name = "Feature max leaf >= array size",
> +            .p = {
> +                .basic.max_leaf = CPUID_GUEST_NR_BASIC - 1,
> +                .feat.max_subleaf = CPUID_GUEST_NR_FEAT,
> +            },
> +            .leaf = 0x00000007,
> +        },
> +        {
> +            .name = "Extended max leaf >= array size",
> +            .p = {
> +                .extd.max_leaf = 0x80000000 + CPUID_GUEST_NR_EXTD,
> +            },
> +            .leaf = 0x80000000,
> +        },
> +
> +        {
> +            .name = "Basic leaf >= max leaf",
> +            .p = {
> +                .basic.max_leaf = CPUID_GUEST_NR_BASIC - 1,
> +            },
> +            .leaf = CPUID_GUEST_NR_BASIC,
> +        },
> +        {
> +            .name = "Feature leaf >= max leaf",
> +            .p = {
> +                .basic.max_leaf = CPUID_GUEST_NR_BASIC - 1,
> +                .feat.max_subleaf = CPUID_GUEST_NR_FEAT - 1,
> +            },
> +            .leaf = 0x00000007,
> +            .subleaf = CPUID_GUEST_NR_FEAT,
> +        },
> +        {
> +            .name = "Extended leaf >= max leaf",
> +            .p = {
> +                .extd.max_leaf = 0x80000000 + CPUID_GUEST_NR_EXTD - 1,
> +            },
> +            .leaf = 0x80000000 + CPUID_GUEST_NR_EXTD,
> +        },
> +    };
> +    const struct cpuid_policy pc;
> +    const struct cpuid_leaf *lc;
> +    struct cpuid_policy p;
> +    struct cpuid_leaf *l;
> +
> +    /* Constness build test. */
> +    lc = x86_cpuid_get_leaf(&pc, 0, 0);
> +    l = x86_cpuid_get_leaf(&p, 0, 0);
> +
> +    printf("Testing CPUID get leaf bound checking:\n");
> +
> +    for ( size_t i = 0; i < ARRAY_SIZE(tests); ++i )
> +    {
> +        const struct test *t = &tests[i];

memdup().  It is important for tests which potentially reach out of
bounds, so ASAN can work.


That said, you're only testing half of the boundary cases.  Perhaps more
important important is the case where max_leaf is really out of legal
bounds.  Further, it is also important to check the non-NULL cases too.

It would probably be better to have a single cpuid_policy object, and a
list of pointers (perhaps offsets) to interesting max_leaf fields, along
with their relevant compile time bounds.  That way, you can try all the
interesting max_leaf values (0, limit-1, limit, ~0) and check
NULL/non-NULLness of the answer with a simple min() calculation.

> diff --git a/xen/include/xen/lib/x86/cpuid.h b/xen/include/xen/lib/x86/cpuid.h
> index a4d254ea96..050cd4f9d1 100644
> --- a/xen/include/xen/lib/x86/cpuid.h
> +++ b/xen/include/xen/lib/x86/cpuid.h
> @@ -431,6 +431,25 @@ int x86_cpuid_copy_from_buffer(struct cpuid_policy 
> *policy,
>                                 uint32_t nr_entries, uint32_t *err_leaf,
>                                 uint32_t *err_subleaf);
>  
> +/**
> + * Get a cpuid leaf from a policy object.
> + *
> + * @param policy      The cpuid_policy object.
> + * @param leaf        The leaf index.
> + * @param subleaf     The subleaf index.
> + * @returns a pointer to the requested leaf or NULL in case of error.
> + *
> + * The function will perform out of bound checks. Do not call this function
> + * directly and instead use x86_cpuid_get_leaf that will deal with both const
> + * and non-const policies returning a pointer with constness matching that of
> + * the input.
> + */
> +const struct cpuid_leaf *x86_cpuid_get_leaf_const(const struct cpuid_policy 
> *p,
> +                                                  uint32_t leaf,
> +                                                  uint32_t subleaf);

Examples like this demonstrate obviously why

const struct cpuid_leaf *x86_cpuid_get_leaf_const(
    const struct cpuid_policy *p, uint32_t leaf, uint32_t subleaf);

is a better layout.

> +#define x86_cpuid_get_leaf(p, l, s) \
> +    ((__typeof__(&(p)->basic.raw[0]))x86_cpuid_get_leaf_const(p, l, s))

You can drop the outermost brackets.

~Andrew