Re: [PATCH v3 4/7] xen/arm: Sanitize cpuinfo ID registers fields

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Stefano,

> On 3 Sep 2021, at 23:48, Stefano Stabellini <sstabellini@xxxxxxxxxx> wrote:
> 
> On Wed, 25 Aug 2021, Bertrand Marquis wrote:
>> Define a sanitize_cpu function to be called on secondary cores to
>> sanitize the system cpuinfo structure.
>> 
>> The safest value is taken when possible and the system is marked tainted
>> if we encounter values which are incompatible with each other.
>> 
>> Call the update_system_features function on all secondary cores that are
>> kept running and taint the system if different midr are found between
>> cores but hmp-unsafe=true was passed on Xen command line.
>> 
>> This is only supported on arm64 so update_system_features is an empty
>> static inline on arm32.
>> 
>> The patch is adding a new TAINT_CPU_OUT_OF_SPEC to warn the user if
>> Xen is running on a system with features differences between cores which
>> are not supported.
>> 
>> The patch is disabling CTR_EL0, DCZID_EL0 and ZCRusing #if 0 with a TODO
>> as this patch is not handling sanitization of those registers.
>> CTR_EL0/DCZID will be handled in a future patch to properly handle
>> different cache attributes when possible.
>> ZCR should be sanitize once we add support for SVE in Xen.
>> 
>> Signed-off-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
> 
> Great patch! It looks good I only have a couple of minor questions
> below.

Thanks
> 
> 
>> ---
>> Changes in v3:
>> - in case of different midr but hmp-unsafe passed on the command line,
>> enable all cores anyway but taint Xen with CPU_OUT_OF_SPEC.
>> - use current core info to sanitize cpu only if we keep it on
>> Changes in v2:
>> - add compilation of cpufeature.c in this patch instead of previous one
>> - remove functions reused from linux code and moved to header
>> - rename sanitize_cpu to update_system_features
>> - change to Linux coding style
>> - remove dev comments
>> - surround currently not used Linux structures with #if 0 and adapt the
>> commit message
>> - add missing aa64dfr1 register
>> - add TODO for CTR, DCZID and ZCR
>> - add CPU_OUT_OF_SPEC support to print_taint
>> - use system_cpuinfo instead of boot_cpu_data
>> ---
>> xen/arch/arm/arm64/Makefile      |   1 +
>> xen/arch/arm/arm64/cpufeature.c  | 121 +++++++++++++++++++++++++++++++
>> xen/arch/arm/smpboot.c           |  34 +++++++--
>> xen/common/kernel.c              |   6 +-
>> xen/include/asm-arm/cpufeature.h |   9 +++
>> xen/include/xen/lib.h            |   1 +
>> 6 files changed, 162 insertions(+), 10 deletions(-)
>> 
>> diff --git a/xen/arch/arm/arm64/Makefile b/xen/arch/arm/arm64/Makefile
>> index 40642ff574..701d66883d 100644
>> --- a/xen/arch/arm/arm64/Makefile
>> +++ b/xen/arch/arm/arm64/Makefile
>> @@ -1,6 +1,7 @@
>> obj-y += lib/
>> 
>> obj-y += cache.o
>> +obj-y += cpufeature.o
>> obj-$(CONFIG_HARDEN_BRANCH_PREDICTOR) += bpi.o
>> obj-$(CONFIG_EARLY_PRINTK) += debug.o
>> obj-y += domctl.o
>> diff --git a/xen/arch/arm/arm64/cpufeature.c 
>> b/xen/arch/arm/arm64/cpufeature.c
>> index 5777e33e5c..61f629ebaa 100644
>> --- a/xen/arch/arm/arm64/cpufeature.c
>> +++ b/xen/arch/arm/arm64/cpufeature.c
>> @@ -275,6 +275,9 @@ static const struct arm64_ftr_bits ftr_id_aa64mmfr2[] = {
>>      ARM64_FTR_END,
>> };
>> 
>> +#if 0
>> +/* TODO: use this to sanitize the cache line size among cores */
>> +
>> static const struct arm64_ftr_bits ftr_ctr[] = {
>>      ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_EXACT, 31, 1, 1), /* RES1 */
>>      ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, CTR_DIC_SHIFT, 
>> 1, 1),
>> @@ -291,6 +294,7 @@ static const struct arm64_ftr_bits ftr_ctr[] = {
>>      ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, 
>> CTR_IMINLINE_SHIFT, 4, 0),
>>      ARM64_FTR_END,
>> };
>> +#endif
>> 
>> static const struct arm64_ftr_bits ftr_id_mmfr0[] = {
>>      S_ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, 
>> ID_MMFR0_INNERSHR_SHIFT, 4, 0xf),
>> @@ -325,11 +329,14 @@ static const struct arm64_ftr_bits ftr_mvfr2[] = {
>>      ARM64_FTR_END,
>> };
>> 
>> +#if 0
>> +/* TODO: handle this when sanitizing cache related registers */
>> static const struct arm64_ftr_bits ftr_dczid[] = {
>>      ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_EXACT, DCZID_DZP_SHIFT, 1, 
>> 1),
>>      ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, DCZID_BS_SHIFT, 
>> 4, 0),
>>      ARM64_FTR_END,
>> };
>> +#endif
>> 
>> static const struct arm64_ftr_bits ftr_id_isar0[] = {
>>      ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, 
>> ID_ISAR0_DIVIDE_SHIFT, 4, 0),
>> @@ -444,11 +451,15 @@ static const struct arm64_ftr_bits ftr_id_dfr1[] = {
>>      ARM64_FTR_END,
>> };
>> 
>> +#if 0
>> +/* TODO: use this to sanitize SVE once we support it */
>> +
>> static const struct arm64_ftr_bits ftr_zcr[] = {
>>      ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE,
>>              ZCR_ELx_LEN_SHIFT, ZCR_ELx_LEN_SIZE, 0),        /* LEN */
>>      ARM64_FTR_END,
>> };
>> +#endif
>> 
>> /*
>>  * Common ftr bits for a 32bit register with all hidden, strict
>> @@ -502,3 +513,113 @@ static s64 arm64_ftr_safe_value(const struct 
>> arm64_ftr_bits *ftrp, s64 new,
>>  * End of imported linux structures and code
>>  */
>> 
>> +static void sanitize_reg(u64 *cur_reg, u64 new_reg, const char *reg_name,
>> +                                            const struct arm64_ftr_bits 
>> *ftrp)
>> +{
>> +    int taint = 0;
>> +    u64 old_reg = *cur_reg;
>> +
>> +    for (;ftrp->width != 0;ftrp++)
>> +    {
>> +            u64 mask;
>> +            s64 cur_field = arm64_ftr_value(ftrp, *cur_reg);
>> +            s64 new_field = arm64_ftr_value(ftrp, new_reg);
>> +
>> +            if (cur_field == new_field)
>> +                    continue;
>> +
>> +            if (ftrp->strict)
>> +                    taint = 1;
>> +
>> +            mask = arm64_ftr_mask(ftrp);
>> +
>> +            *cur_reg &= ~mask;
>> +            *cur_reg |= (arm64_ftr_safe_value(ftrp, new_field, cur_field)
>> +                                    << ftrp->shift) & mask;
> 
> I wonder why you haven't also imported arm64_ftr_set_value?  This seems
> to be the open-coded version of it.

You are right I could have used it instead (no idea why I did not).

I will modify that and send an update.

Good finding :-)

> 
> 
>> +    }
>> +
>> +    if (old_reg != new_reg)
>> +            printk(XENLOG_DEBUG "SANITY DIF: %s 0x%"PRIx64" -> 
>> 0x%"PRIx64"\n",
>> +                            reg_name, old_reg, new_reg);
>> +    if (old_reg != *cur_reg)
>> +            printk(XENLOG_DEBUG "SANITY FIX: %s 0x%"PRIx64" -> 
>> 0x%"PRIx64"\n",
>> +                            reg_name, old_reg, *cur_reg);
>> +
>> +    if (taint)
>> +    {
>> +            printk(XENLOG_WARNING "SANITY CHECK: Unexpected variation in 
>> %s.\n",
>> +                            reg_name);
>> +            add_taint(TAINT_CPU_OUT_OF_SPEC);
>> +    }
>> +}
>> +
>> +
>> +/*
>> + * This function should be called on secondary cores to sanitize the boot 
>> cpu
>> + * cpuinfo.
>> + */
>> +void update_system_features(const struct cpuinfo_arm *new)
>> +{
>> +
>> +#define SANITIZE_REG(field, num, reg)  \
>> +    sanitize_reg(&system_cpuinfo.field.bits[num], new->field.bits[num], \
>> +                             #reg, ftr_##reg)
>> +
>> +#define SANITIZE_ID_REG(field, num, reg)  \
>> +    sanitize_reg(&system_cpuinfo.field.bits[num], new->field.bits[num], \
>> +                            #reg, ftr_id_##reg)
>> +
>> +#define SANITIZE_RAZ_REG(field, num, reg)  \
>> +    sanitize_reg(&system_cpuinfo.field.bits[num], new->field.bits[num], \
>> +                            #reg, ftr_raz)
>> +
>> +#define SANITIZE_GENERIC_REG(field, num, reg)  \
>> +    sanitize_reg(&system_cpuinfo.field.bits[num], new->field.bits[num], \
>> +                            #reg, ftr_generic_32bits)
>> +
>> +    SANITIZE_ID_REG(pfr64, 0, aa64pfr0);
>> +    SANITIZE_ID_REG(pfr64, 1, aa64pfr1);
>> +
>> +    SANITIZE_ID_REG(dbg64, 0, aa64dfr0);
>> +    SANITIZE_RAZ_REG(dbg64, 1, aa64dfr1);
>> +
>> +    SANITIZE_ID_REG(mm64, 0, aa64mmfr0);
>> +    SANITIZE_ID_REG(mm64, 1, aa64mmfr1);
>> +    SANITIZE_ID_REG(mm64, 2, aa64mmfr2);
>> +
>> +    SANITIZE_ID_REG(isa64, 0, aa64isar0);
>> +    SANITIZE_ID_REG(isa64, 1, aa64isar1);
>> +
>> +    SANITIZE_ID_REG(zfr64, 0, aa64zfr0);
>> +
>> +    if ( cpu_feature64_has_el0_32(&system_cpuinfo) )
>> +    {
>> +            SANITIZE_ID_REG(pfr32, 0, pfr0);
>> +            SANITIZE_ID_REG(pfr32, 1, pfr1);
>> +            SANITIZE_ID_REG(pfr32, 2, pfr2);
>> +
>> +            SANITIZE_ID_REG(dbg32, 0, dfr0);
>> +            SANITIZE_ID_REG(dbg32, 1, dfr1);
>> +
>> +            SANITIZE_ID_REG(mm32, 0, mmfr0);
>> +            SANITIZE_GENERIC_REG(mm32, 1, mmfr1);
>> +            SANITIZE_GENERIC_REG(mm32, 2, mmfr2);
>> +            SANITIZE_GENERIC_REG(mm32, 3, mmfr3);
>> +            SANITIZE_ID_REG(mm32, 4, mmfr4);
>> +            SANITIZE_ID_REG(mm32, 5, mmfr5);
>> +
>> +            SANITIZE_ID_REG(isa32, 0, isar0);
>> +            SANITIZE_GENERIC_REG(isa32, 1, isar1);
>> +            SANITIZE_GENERIC_REG(isa32, 2, isar2);
>> +            SANITIZE_GENERIC_REG(isa32, 3, isar3);
>> +            SANITIZE_ID_REG(isa32, 4, isar4);
>> +            SANITIZE_ID_REG(isa32, 5, isar5);
>> +            SANITIZE_ID_REG(isa32, 6, isar6);
>> +
>> +            SANITIZE_GENERIC_REG(mvfr, 0, mvfr0);
>> +            SANITIZE_GENERIC_REG(mvfr, 1, mvfr1);
>> +#ifndef MVFR2_MAYBE_UNDEFINED
>> +            SANITIZE_REG(mvfr, 2, mvfr2);
>> +#endif
>> +    }
>> +}
> 
> Looking at the list of registers we are sanitizing here we are only
> missing aux32 and aux64 compared to struct cpuinfo_arm. Is that because
> there is nothing to sanitize there?

Aux registers have no standard bit definition and as such cannot be treated.
Linux is doing the same.

Cheers
Bertrand