| Message ID | c1868fce1bcb0bcddf7bc786be166007f91f2f67.1629897306.git.bertrand.marquis@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: Sanitize cpuinfo | expand |
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 :-)
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 /*
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(-)