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

[Stefano Stabellini <sstabellini@kernel.org>]

On Thu, 16 Sep 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@arm.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

> ---
> Changes in v4:
> - arm64_ftr_set_value
> 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  | 117 +++++++++++++++++++++++++++++++
>  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, 158 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 152086dc9d..58596495a8 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
> @@ -512,3 +523,109 @@ 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++)
> +	{
> +		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;
> +
> +		*cur_reg = arm64_ftr_set_value(ftrp, *cur_reg,
> +							arm64_ftr_safe_value(ftrp, new_field, cur_field));
> +	}
> +
> +	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
> +	}
> +}
> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> index c9f2827d56..60c0e82fc5 100644
> --- a/xen/arch/arm/smpboot.c
> +++ b/xen/arch/arm/smpboot.c
> @@ -318,15 +318,26 @@ void start_secondary(void)
>       * is manually specified for all domains). Better to park them for
>       * now.
>       */
> -    if ( !opt_hmp_unsafe &&
> -         current_cpu_data.midr.bits != system_cpuinfo.midr.bits )
> +    if ( current_cpu_data.midr.bits != system_cpuinfo.midr.bits )
>      {
> -        printk(XENLOG_ERR
> -               "CPU%u MIDR (0x%"PRIregister") does not match boot CPU MIDR (0x%"PRIregister"),\n"
> -               XENLOG_ERR "disable cpu (see big.LITTLE.txt under docs/).\n",
> -               smp_processor_id(), current_cpu_data.midr.bits,
> -               system_cpuinfo.midr.bits);
> -        stop_cpu();
> +        if ( !opt_hmp_unsafe )
> +        {
> +            printk(XENLOG_ERR
> +                   "CPU%u MIDR (0x%"PRIregister") does not match boot CPU MIDR (0x%"PRIregister"),\n"
> +                   XENLOG_ERR "disable cpu (see big.LITTLE.txt under docs/).\n",
> +                   smp_processor_id(), current_cpu_data.midr.bits,
> +                   system_cpuinfo.midr.bits);
> +            stop_cpu();
> +        }
> +        else
> +        {
> +            printk(XENLOG_ERR
> +                   "CPU%u MIDR (0x%"PRIregister") does not match boot CPU MIDR (0x%"PRIregister"),\n"
> +                   XENLOG_ERR "hmp-unsafe turned on so tainting Xen and keep core on!!\n",
> +                   smp_processor_id(), current_cpu_data.midr.bits,
> +                   system_cpuinfo.midr.bits);
> +            add_taint(TAINT_CPU_OUT_OF_SPEC);
> +         }
>      }
>  
>      if ( dcache_line_bytes != read_dcache_line_bytes() )
> @@ -337,6 +348,13 @@ void start_secondary(void)
>          stop_cpu();
>      }
>  
> +    /*
> +     * system features must be updated only if we do not stop the core or
> +     * we might disable features due to a non used core (for example when
> +     * booting on big cores on a big.LITTLE system with hmp_unsafe)
> +     */
> +    update_system_features(&current_cpu_data);
> +
>      mmu_init_secondary_cpu();
>  
>      gic_init_secondary_cpu();
> diff --git a/xen/common/kernel.c b/xen/common/kernel.c
> index d77756a81e..e119e5401f 100644
> --- a/xen/common/kernel.c
> +++ b/xen/common/kernel.c
> @@ -327,6 +327,7 @@ unsigned int tainted;
>   *  'H' - HVM forced emulation prefix is permitted.
>   *  'M' - Machine had a machine check experience.
>   *  'U' - Platform is unsecure (usually due to an errata on the platform).
> + *  'S' - Out of spec CPU (One core has a feature incompatible with others).
>   *
>   *      The string is overwritten by the next call to print_taint().
>   */
> @@ -334,12 +335,13 @@ char *print_tainted(char *str)
>  {
>      if ( tainted )
>      {
> -        snprintf(str, TAINT_STRING_MAX_LEN, "Tainted: %c%c%c%c%c",
> +        snprintf(str, TAINT_STRING_MAX_LEN, "Tainted: %c%c%c%c%c%c",
>                   tainted & TAINT_MACHINE_UNSECURE ? 'U' : ' ',
>                   tainted & TAINT_MACHINE_CHECK ? 'M' : ' ',
>                   tainted & TAINT_SYNC_CONSOLE ? 'C' : ' ',
>                   tainted & TAINT_ERROR_INJECT ? 'E' : ' ',
> -                 tainted & TAINT_HVM_FEP ? 'H' : ' ');
> +                 tainted & TAINT_HVM_FEP ? 'H' : ' ',
> +                 tainted & TAINT_CPU_OUT_OF_SPEC ? 'S' : ' ');
>      }
>      else
>      {
> diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h
> index 8f2b8e7830..52cb3133e0 100644
> --- a/xen/include/asm-arm/cpufeature.h
> +++ b/xen/include/asm-arm/cpufeature.h
> @@ -330,6 +330,15 @@ extern struct cpuinfo_arm system_cpuinfo;
>  
>  extern void identify_cpu(struct cpuinfo_arm *);
>  
> +#ifdef CONFIG_ARM_64
> +extern void update_system_features(const struct cpuinfo_arm *);
> +#else
> +static inline void update_system_features(const struct cpuinfo_arm *cpuinfo)
> +{
> +    /* Not supported on arm32 */
> +}
> +#endif
> +
>  extern struct cpuinfo_arm cpu_data[];
>  #define current_cpu_data cpu_data[smp_processor_id()]
>  
> diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
> index 1198c7c0b2..c6987973bf 100644
> --- a/xen/include/xen/lib.h
> +++ b/xen/include/xen/lib.h
> @@ -192,6 +192,7 @@ uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c);
>  #define TAINT_ERROR_INJECT              (1u << 2)
>  #define TAINT_HVM_FEP                   (1u << 3)
>  #define TAINT_MACHINE_UNSECURE          (1u << 4)
> +#define TAINT_CPU_OUT_OF_SPEC           (1u << 5)
>  extern unsigned int tainted;
>  #define TAINT_STRING_MAX_LEN            20
>  extern char *print_tainted(char *str);
> -- 
> 2.17.1
>