[PATCH v7 01/38] KVM: arm64: Introduce a validation function for an ID register

[Reiji Watanabe]

Hi Oliver,

On Tue, May 3, 2022 at 11:35 PM Oliver Upton <oupton at google.com> wrote:
>
> On Mon, Apr 18, 2022 at 11:55:07PM -0700, Reiji Watanabe wrote:
> > Introduce arm64_check_features(), which does a basic validity checking
> > of an ID register value against the register's limit value, which is
> > generally the host's sanitized value.
> >
> > This function will be used by the following patches to check if an ID
> > register value that userspace tries to set for a guest can be supported
> > on the host.
> >
> > Signed-off-by: Reiji Watanabe <reijiw at google.com>
> > ---
> >  arch/arm64/include/asm/cpufeature.h |  1 +
> >  arch/arm64/kernel/cpufeature.c      | 52 +++++++++++++++++++++++++++++
> >  2 files changed, 53 insertions(+)
> >
> > diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
> > index c62e7e5e2f0c..7a009d4e18a6 100644
> > --- a/arch/arm64/include/asm/cpufeature.h
> > +++ b/arch/arm64/include/asm/cpufeature.h
> > @@ -634,6 +634,7 @@ void check_local_cpu_capabilities(void);
> >
> >  u64 read_sanitised_ftr_reg(u32 id);
> >  u64 __read_sysreg_by_encoding(u32 sys_id);
> > +int arm64_check_features(const struct arm64_ftr_bits *ftrp, u64 val, u64 limit);
> >
> >  static inline bool cpu_supports_mixed_endian_el0(void)
> >  {
> > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> > index d72c4b4d389c..dbbc69745f22 100644
> > --- a/arch/arm64/kernel/cpufeature.c
> > +++ b/arch/arm64/kernel/cpufeature.c
> > @@ -3239,3 +3239,55 @@ ssize_t cpu_show_meltdown(struct device *dev, struct device_attribute *attr,
> >               return sprintf(buf, "Vulnerable\n");
> >       }
> >  }
> > +
> > +/**
> > + * arm64_check_features() - Check if a feature register value constitutes
> > + * a subset of features indicated by @limit.
> > + *
> > + * @ftrp: Pointer to an array of arm64_ftr_bits. It must be terminated by
> > + * an item whose width field is zero.
> > + * @val: The feature register value to check
> > + * @limit: The limit value of the feature register
> > + *
> > + * This function will check if each feature field of @val is the "safe" value
> > + * against @limit based on @ftrp[], each of which specifies the target field
> > + * (shift, width), whether or not the field is for a signed value (sign),
> > + * how the field is determined to be "safe" (type), and the safe value
> > + * (safe_val) when type == FTR_EXACT (safe_val won't be used by this
> > + * function when type != FTR_EXACT). Any other fields in arm64_ftr_bits
> > + * won't be used by this function. If a field value in @val is the same
> > + * as the one in @limit, it is always considered the safe value regardless
> > + * of the type. For register fields that are not in @ftrp[], only the value
> > + * in @limit is considered the safe value.
> > + *
> > + * Return: 0 if all the fields are safe. Otherwise, return negative errno.
> > + */
> > +int arm64_check_features(const struct arm64_ftr_bits *ftrp, u64 val, u64 limit)
> > +{
> > +     u64 mask = 0;
> > +
> > +     for (; ftrp->width; ftrp++) {
> > +             s64 f_val, f_lim, safe_val;
> > +
> > +             f_val = arm64_ftr_value(ftrp, val);
> > +             f_lim = arm64_ftr_value(ftrp, limit);
> > +             mask |= arm64_ftr_mask(ftrp);
> > +
> > +             if (f_val == f_lim)
> > +                     safe_val = f_val;
> > +             else
> > +                     safe_val = arm64_ftr_safe_value(ftrp, f_val, f_lim);
> > +
> > +             if (safe_val != f_val)
> > +                     return -E2BIG;
> > +     }
> > +
> > +     /*
> > +      * For fields that are not indicated in ftrp, values in limit are the
> > +      * safe values.
> > +      */
> > +     if ((val & ~mask) != (limit & ~mask))
> > +             return -E2BIG;
>
> This bit is interesting. Apologies if I paged out relevant context. What
> features are we trying to limit that exist outside of an arm64_ftr_bits
> definition? I'll follow the series and see if I figure out later :-P
>
> Generally speaking, though, it seems to me that we'd prefer to have an
> arm64_ftr_bits struct plumbed up for whatever hits this case.

I'm sorry that I completely overlooked this until now...

This code is not currently used by this series since KVM will fill
any statically undefined fields as a lower safe unsigned field.

But, considering that arm64_ftr_bits that are defined in cpufeature.c
doesn't have all bits definitions, I wanted to have the function
handle such arm64_ftr_bits as well (the code is basically to make
sure that undefined fields are 0).

Thanks,
Reiji