Use arm64 cpu feature sanitization to TAIN Xen if different DCZID values
are found (ftr_dczid is using only STRICT method).
In this case actual memory being cleaned by DC ZVA operations would be
different depending on the cores which could make a guest zeroing too
much or too little memory if it is merged between CPUs.

We could, on processor supporting it, trap access to DCZID_EL0 register
using HFGRTR_EL2 register but this would not solve the case where a
process is being migrated during a copy or if it cached the value of the
register.

Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
---
Change in v3: none
Change in v2: Patch introduced in v2
---
 xen/arch/arm/arm64/cpufeature.c  | 14 +++++++++++---
 xen/arch/arm/cpufeature.c        |  2 ++
 xen/include/asm-arm/cpufeature.h |  8 ++++++++
 3 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/arm64/cpufeature.c b/xen/arch/arm/arm64/cpufeature.c
index 61f629ebaa..b1936ef1d6 100644
--- a/xen/arch/arm/arm64/cpufeature.c
+++ b/xen/arch/arm/arm64/cpufeature.c
@@ -329,14 +329,11 @@ 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),
@@ -592,6 +589,17 @@ void update_system_features(const struct cpuinfo_arm *new)
 
 	SANITIZE_ID_REG(zfr64, 0, aa64zfr0);
 
+	/*
+	 * Comment from Linux:
+	 * Userspace may perform DC ZVA instructions. Mismatched block sizes
+	 * could result in too much or too little memory being zeroed if a
+	 * process is preempted and migrated between CPUs.
+	 *
+	 * ftr_dczid is using STRICT comparison so we will taint Xen if different
+	 * values are found.
+	 */
+	SANITIZE_REG(dczid, 0, dczid);
+
 	if ( cpu_feature64_has_el0_32(&system_cpuinfo) )
 	{
 		SANITIZE_ID_REG(pfr32, 0, pfr0);
diff --git a/xen/arch/arm/cpufeature.c b/xen/arch/arm/cpufeature.c
index f600a611bd..113f20f601 100644
--- a/xen/arch/arm/cpufeature.c
+++ b/xen/arch/arm/cpufeature.c
@@ -125,6 +125,8 @@ void identify_cpu(struct cpuinfo_arm *c)
 
     c->zfr64.bits[0] = READ_SYSREG(ID_AA64ZFR0_EL1);
 
+    c->dczid.bits[0] = READ_SYSREG(DCZID_EL0);
+
     aarch32_el0 = cpu_feature64_has_el0_32(c);
 #endif
 
diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h
index 52cb3133e0..5219fd3bab 100644
--- a/xen/include/asm-arm/cpufeature.h
+++ b/xen/include/asm-arm/cpufeature.h
@@ -259,6 +259,14 @@ struct cpuinfo_arm {
         register_t bits[1];
     } zfr64;
 
+    /*
+     * DCZID is only used to check for incoherent values between cores
+     * and taint Xen in this case
+     */
+    struct {
+        register_t bits[1];
+    } dczid;
+
 #endif
 
     /*
-- 
2.17.1

On Wed, 25 Aug 2021, Bertrand Marquis wrote:
> Use arm64 cpu feature sanitization to TAIN Xen if different DCZID values
                                        ^  TAINT


> are found (ftr_dczid is using only STRICT method).
> In this case actual memory being cleaned by DC ZVA operations would be
> different depending on the cores which could make a guest zeroing too
> much or too little memory if it is merged between CPUs.
> 
> We could, on processor supporting it, trap access to DCZID_EL0 register
               ^ processors

> using HFGRTR_EL2 register but this would not solve the case where a
> process is being migrated during a copy or if it cached the value of the
> register.
> 
> Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
> ---
> Change in v3: none
> Change in v2: Patch introduced in v2
> ---
>  xen/arch/arm/arm64/cpufeature.c  | 14 +++++++++++---
>  xen/arch/arm/cpufeature.c        |  2 ++
>  xen/include/asm-arm/cpufeature.h |  8 ++++++++
>  3 files changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/cpufeature.c b/xen/arch/arm/arm64/cpufeature.c
> index 61f629ebaa..b1936ef1d6 100644
> --- a/xen/arch/arm/arm64/cpufeature.c
> +++ b/xen/arch/arm/arm64/cpufeature.c
> @@ -329,14 +329,11 @@ 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),
> @@ -592,6 +589,17 @@ void update_system_features(const struct cpuinfo_arm *new)
>  
>  	SANITIZE_ID_REG(zfr64, 0, aa64zfr0);
>  
> +	/*
> +	 * Comment from Linux:

I don't know if I would keep or remove "Comment from Linux"

In any case:

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


Also I gave a quick test of the series on a ZCU102 and it worked fine.


> +	 * Userspace may perform DC ZVA instructions. Mismatched block sizes
> +	 * could result in too much or too little memory being zeroed if a
> +	 * process is preempted and migrated between CPUs.
> +	 *
> +	 * ftr_dczid is using STRICT comparison so we will taint Xen if different
> +	 * values are found.
> +	 */
> +	SANITIZE_REG(dczid, 0, dczid);
> +
>  	if ( cpu_feature64_has_el0_32(&system_cpuinfo) )
>  	{
>  		SANITIZE_ID_REG(pfr32, 0, pfr0);
> diff --git a/xen/arch/arm/cpufeature.c b/xen/arch/arm/cpufeature.c
> index f600a611bd..113f20f601 100644
> --- a/xen/arch/arm/cpufeature.c
> +++ b/xen/arch/arm/cpufeature.c
> @@ -125,6 +125,8 @@ void identify_cpu(struct cpuinfo_arm *c)
>  
>      c->zfr64.bits[0] = READ_SYSREG(ID_AA64ZFR0_EL1);
>  
> +    c->dczid.bits[0] = READ_SYSREG(DCZID_EL0);
> +
>      aarch32_el0 = cpu_feature64_has_el0_32(c);
>  #endif
>  
> diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h
> index 52cb3133e0..5219fd3bab 100644
> --- a/xen/include/asm-arm/cpufeature.h
> +++ b/xen/include/asm-arm/cpufeature.h
> @@ -259,6 +259,14 @@ struct cpuinfo_arm {
>          register_t bits[1];
>      } zfr64;
>  
> +    /*
> +     * DCZID is only used to check for incoherent values between cores
> +     * and taint Xen in this case
> +     */
> +    struct {
> +        register_t bits[1];
> +    } dczid;
> +
>  #endif
>  
>      /*
> -- 
> 2.17.1
> 

> On 3 Sep 2021, at 23:49, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> On Wed, 25 Aug 2021, Bertrand Marquis wrote:
>> Use arm64 cpu feature sanitization to TAIN Xen if different DCZID values
>                                        ^  TAINT
> 
> 
>> are found (ftr_dczid is using only STRICT method).
>> In this case actual memory being cleaned by DC ZVA operations would be
>> different depending on the cores which could make a guest zeroing too
>> much or too little memory if it is merged between CPUs.
>> 
>> We could, on processor supporting it, trap access to DCZID_EL0 register
>               ^ processors

Could those typos be fixed during commit ?

> 
>> using HFGRTR_EL2 register but this would not solve the case where a
>> process is being migrated during a copy or if it cached the value of the
>> register.
>> 
>> Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
>> ---
>> Change in v3: none
>> Change in v2: Patch introduced in v2
>> ---
>> xen/arch/arm/arm64/cpufeature.c  | 14 +++++++++++---
>> xen/arch/arm/cpufeature.c        |  2 ++
>> xen/include/asm-arm/cpufeature.h |  8 ++++++++
>> 3 files changed, 21 insertions(+), 3 deletions(-)
>> 
>> diff --git a/xen/arch/arm/arm64/cpufeature.c b/xen/arch/arm/arm64/cpufeature.c
>> index 61f629ebaa..b1936ef1d6 100644
>> --- a/xen/arch/arm/arm64/cpufeature.c
>> +++ b/xen/arch/arm/arm64/cpufeature.c
>> @@ -329,14 +329,11 @@ 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),
>> @@ -592,6 +589,17 @@ void update_system_features(const struct cpuinfo_arm *new)
>> 
>> 	SANITIZE_ID_REG(zfr64, 0, aa64zfr0);
>> 
>> +	/*
>> +	 * Comment from Linux:
> 
> I don't know if I would keep or remove "Comment from Linux"

I added that because the comment itself does not really apply to Xen.
I could have rephrased the comment/
Anyway I have no objection to remove that statement.

Do I need to send a v2 for that ?

Cheers
Bertrand


> 
> In any case:
> 
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> 
> 
> Also I gave a quick test of the series on a ZCU102 and it worked fine.
> 
> 
>> +	 * Userspace may perform DC ZVA instructions. Mismatched block sizes
>> +	 * could result in too much or too little memory being zeroed if a
>> +	 * process is preempted and migrated between CPUs.
>> +	 *
>> +	 * ftr_dczid is using STRICT comparison so we will taint Xen if different
>> +	 * values are found.
>> +	 */
>> +	SANITIZE_REG(dczid, 0, dczid);
>> +
>> 	if ( cpu_feature64_has_el0_32(&system_cpuinfo) )
>> 	{
>> 		SANITIZE_ID_REG(pfr32, 0, pfr0);
>> diff --git a/xen/arch/arm/cpufeature.c b/xen/arch/arm/cpufeature.c
>> index f600a611bd..113f20f601 100644
>> --- a/xen/arch/arm/cpufeature.c
>> +++ b/xen/arch/arm/cpufeature.c
>> @@ -125,6 +125,8 @@ void identify_cpu(struct cpuinfo_arm *c)
>> 
>>     c->zfr64.bits[0] = READ_SYSREG(ID_AA64ZFR0_EL1);
>> 
>> +    c->dczid.bits[0] = READ_SYSREG(DCZID_EL0);
>> +
>>     aarch32_el0 = cpu_feature64_has_el0_32(c);
>> #endif
>> 
>> diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h
>> index 52cb3133e0..5219fd3bab 100644
>> --- a/xen/include/asm-arm/cpufeature.h
>> +++ b/xen/include/asm-arm/cpufeature.h
>> @@ -259,6 +259,14 @@ struct cpuinfo_arm {
>>         register_t bits[1];
>>     } zfr64;
>> 
>> +    /*
>> +     * DCZID is only used to check for incoherent values between cores
>> +     * and taint Xen in this case
>> +     */
>> +    struct {
>> +        register_t bits[1];
>> +    } dczid;
>> +
>> #endif
>> 
>>     /*
>> -- 
>> 2.17.1

On Mon, 6 Sep 2021, Bertrand Marquis wrote:
> > On 3 Sep 2021, at 23:49, Stefano Stabellini <sstabellini@kernel.org> wrote:
> > 
> > On Wed, 25 Aug 2021, Bertrand Marquis wrote:
> >> Use arm64 cpu feature sanitization to TAIN Xen if different DCZID values
> >                                        ^  TAINT
> > 
> > 
> >> are found (ftr_dczid is using only STRICT method).
> >> In this case actual memory being cleaned by DC ZVA operations would be
> >> different depending on the cores which could make a guest zeroing too
> >> much or too little memory if it is merged between CPUs.
> >> 
> >> We could, on processor supporting it, trap access to DCZID_EL0 register
> >               ^ processors
> 
> Could those typos be fixed during commit ?

Yes they can

 
> >> using HFGRTR_EL2 register but this would not solve the case where a
> >> process is being migrated during a copy or if it cached the value of the
> >> register.
> >> 
> >> Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com>
> >> ---
> >> Change in v3: none
> >> Change in v2: Patch introduced in v2
> >> ---
> >> xen/arch/arm/arm64/cpufeature.c  | 14 +++++++++++---
> >> xen/arch/arm/cpufeature.c        |  2 ++
> >> xen/include/asm-arm/cpufeature.h |  8 ++++++++
> >> 3 files changed, 21 insertions(+), 3 deletions(-)
> >> 
> >> diff --git a/xen/arch/arm/arm64/cpufeature.c b/xen/arch/arm/arm64/cpufeature.c
> >> index 61f629ebaa..b1936ef1d6 100644
> >> --- a/xen/arch/arm/arm64/cpufeature.c
> >> +++ b/xen/arch/arm/arm64/cpufeature.c
> >> @@ -329,14 +329,11 @@ 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),
> >> @@ -592,6 +589,17 @@ void update_system_features(const struct cpuinfo_arm *new)
> >> 
> >> 	SANITIZE_ID_REG(zfr64, 0, aa64zfr0);
> >> 
> >> +	/*
> >> +	 * Comment from Linux:
> > 
> > I don't know if I would keep or remove "Comment from Linux"
> 
> I added that because the comment itself does not really apply to Xen.
> I could have rephrased the comment/
> Anyway I have no objection to remove that statement.
> 
> Do I need to send a v2 for that ?

No you don't need to resend just for that or the typos. However if you
are going to resend, then an update would be nice :-)