[PATCH 0/2] KVM: arm64: Control visibility of S1PIE related sysregs to userspace

[PATCH 0/2] KVM: arm64: Control visibility of S1PIE related sysregs to userspace

[Mark Brown]

While looking at the KVM system register code I noticed that we do not
have visibility operations for TCR2 or the S1PIE registers, I may be
missing some reason why they are not required but in case I'm not I
figured the most direct way to ask was to propose adding the operations.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
Mark Brown (2):
      KVM: arm64: Hide TCR2_EL1 from userspace when disabled for guests
      KVM: arm64: Hide S1PIE registers from userspace when disabled for guests

 arch/arm64/include/asm/kvm_host.h |  6 ++++++
 arch/arm64/kvm/sys_regs.c         | 31 ++++++++++++++++++++++++++-----
 2 files changed, 32 insertions(+), 5 deletions(-)
---
base-commit: 6e4436539ae182dc86d57d13849862bcafaa4709
change-id: 20240820-kvm-arm64-hide-pie-regs-2236f70703ab

Best regards,
-- 
Mark Brown <broonie@kernel.org>

[PATCH 1/2] KVM: arm64: Hide TCR2_EL1 from userspace when disabled for guests

[Mark Brown]

When the guest does not support FEAT_TCR2 we should not allow any access
to it in order to ensure that we do not create spurious issues with guest
migration. Add a visibility operation for it.

Fixes: fbff56068232 ("KVM: arm64: Save/restore TCR2_EL1")
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 arch/arm64/include/asm/kvm_host.h |  3 +++
 arch/arm64/kvm/sys_regs.c         | 14 ++++++++++++--
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index a33f5996ca9f..47675ef13676 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -1473,4 +1473,7 @@ void kvm_set_vm_id_reg(struct kvm *kvm, u32 reg, u64 val);
 		(pa + pi + pa3) == 1;					\
 	})
 
+#define kvm_has_tcr2(k)				\
+	(kvm_has_feat((k), ID_AA64MMFR3_EL1, TCRX, IMP))
+
 #endif /* __ARM64_KVM_HOST_H__ */
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index c90324060436..aab689ea8992 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -2255,6 +2255,15 @@ static bool access_zcr_el2(struct kvm_vcpu *vcpu,
 	return true;
 }
 
+static unsigned int tcr2_visibility(const struct kvm_vcpu *vcpu,
+				    const struct sys_reg_desc *rd)
+{
+	if (kvm_has_tcr2(vcpu->kvm))
+		return 0;
+
+	return REG_HIDDEN;
+}
+
 /*
  * Architected system registers.
  * Important: Must be sorted ascending by Op0, Op1, CRn, CRm, Op2
@@ -2438,7 +2447,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 	{ SYS_DESC(SYS_TTBR0_EL1), access_vm_reg, reset_unknown, TTBR0_EL1 },
 	{ SYS_DESC(SYS_TTBR1_EL1), access_vm_reg, reset_unknown, TTBR1_EL1 },
 	{ SYS_DESC(SYS_TCR_EL1), access_vm_reg, reset_val, TCR_EL1, 0 },
-	{ SYS_DESC(SYS_TCR2_EL1), access_vm_reg, reset_val, TCR2_EL1, 0 },
+	{ SYS_DESC(SYS_TCR2_EL1), access_vm_reg, reset_val, TCR2_EL1, 0,
+	  .visibility = tcr2_visibility },
 
 	PTRAUTH_KEY(APIA),
 	PTRAUTH_KEY(APIB),
@@ -4558,7 +4568,7 @@ void kvm_calculate_traps(struct kvm_vcpu *vcpu)
 		if (kvm_has_feat(kvm, ID_AA64ISAR2_EL1, MOPS, IMP))
 			vcpu->arch.hcrx_el2 |= (HCRX_EL2_MSCEn | HCRX_EL2_MCE2);
 
-		if (kvm_has_feat(kvm, ID_AA64MMFR3_EL1, TCRX, IMP))
+		if (kvm_has_tcr2(kvm))
 			vcpu->arch.hcrx_el2 |= HCRX_EL2_TCR2En;
 	}
 

-- 
2.39.2

[PATCH 2/2] KVM: arm64: Hide S1PIE registers from userspace when disabled for guests

[Mark Brown]

When the guest does not support S1PIE we should not allow any access
to the system registers it adds in order to ensure that we do not create
spurious issues with guest migration. Add a visibility operation for these
registers.

Fixes: 86f9de9db178 ("KVM: arm64: Save/restore PIE registers")
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 arch/arm64/include/asm/kvm_host.h |  3 +++
 arch/arm64/kvm/sys_regs.c         | 17 ++++++++++++++---
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 47675ef13676..38bfa6e10ba5 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -1476,4 +1476,7 @@ void kvm_set_vm_id_reg(struct kvm *kvm, u32 reg, u64 val);
 #define kvm_has_tcr2(k)				\
 	(kvm_has_feat((k), ID_AA64MMFR3_EL1, TCRX, IMP))
 
+#define kvm_has_s1pie(k)				\
+	(kvm_has_feat((k), ID_AA64MMFR3_EL1, S1PIE, IMP))
+
 #endif /* __ARM64_KVM_HOST_H__ */
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index aab689ea8992..73daa33a43b4 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -2264,6 +2264,15 @@ static unsigned int tcr2_visibility(const struct kvm_vcpu *vcpu,
 	return REG_HIDDEN;
 }
 
+static unsigned int s1pie_visibility(const struct kvm_vcpu *vcpu,
+				     const struct sys_reg_desc *rd)
+{
+	if (kvm_has_s1pie(vcpu->kvm))
+		return 0;
+
+	return REG_HIDDEN;
+}
+
 /*
  * Architected system registers.
  * Important: Must be sorted ascending by Op0, Op1, CRn, CRm, Op2
@@ -2500,8 +2509,10 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 	{ SYS_DESC(SYS_PMMIR_EL1), trap_raz_wi },
 
 	{ SYS_DESC(SYS_MAIR_EL1), access_vm_reg, reset_unknown, MAIR_EL1 },
-	{ SYS_DESC(SYS_PIRE0_EL1), NULL, reset_unknown, PIRE0_EL1 },
-	{ SYS_DESC(SYS_PIR_EL1), NULL, reset_unknown, PIR_EL1 },
+	{ SYS_DESC(SYS_PIRE0_EL1), NULL, reset_unknown, PIRE0_EL1,
+	  .visibility = s1pie_visibility },
+	{ SYS_DESC(SYS_PIR_EL1), NULL, reset_unknown, PIR_EL1,
+	  .visibility = s1pie_visibility },
 	{ SYS_DESC(SYS_AMAIR_EL1), access_vm_reg, reset_amair_el1, AMAIR_EL1 },
 
 	{ SYS_DESC(SYS_LORSA_EL1), trap_loregion },
@@ -4610,7 +4621,7 @@ void kvm_calculate_traps(struct kvm_vcpu *vcpu)
 						HFGITR_EL2_TLBIRVAAE1OS	|
 						HFGITR_EL2_TLBIRVAE1OS);
 
-	if (!kvm_has_feat(kvm, ID_AA64MMFR3_EL1, S1PIE, IMP))
+	if (!kvm_has_s1pie(kvm))
 		kvm->arch.fgu[HFGxTR_GROUP] |= (HFGxTR_EL2_nPIRE0_EL1 |
 						HFGxTR_EL2_nPIR_EL1);
 

-- 
2.39.2

Re: [PATCH 0/2] KVM: arm64: Control visibility of S1PIE related sysregs to userspace

[Marc Zyngier]

On Wed, 21 Aug 2024 14:07:14 +0100,
Mark Brown <broonie@kernel.org> wrote:
> 
> While looking at the KVM system register code I noticed that we do not
> have visibility operations for TCR2 or the S1PIE registers, I may be
> missing some reason why they are not required but in case I'm not I
> figured the most direct way to ask was to propose adding the operations.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
> Mark Brown (2):
>       KVM: arm64: Hide TCR2_EL1 from userspace when disabled for guests
>       KVM: arm64: Hide S1PIE registers from userspace when disabled for guests
> 
>  arch/arm64/include/asm/kvm_host.h |  6 ++++++
>  arch/arm64/kvm/sys_regs.c         | 31 ++++++++++++++++++++++++++-----
>  2 files changed, 32 insertions(+), 5 deletions(-)

If you are going to do this, please add it on top of [1], and handle
the corresponding EL2 registers.

Thanks,

	M.

[1] https://lore.kernel.org/20240813144738.2048302-1-maz@kernel.org

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 0/2] KVM: arm64: Control visibility of S1PIE related sysregs to userspace

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 436 bytes --]

On Wed, Aug 21, 2024 at 02:46:25PM +0100, Marc Zyngier wrote:
> Mark Brown <broonie@kernel.org> wrote:

> > Mark Brown (2):
> >       KVM: arm64: Hide TCR2_EL1 from userspace when disabled for guests
> >       KVM: arm64: Hide S1PIE registers from userspace when disabled for guests

> If you are going to do this, please add it on top of [1], and handle
> the corresponding EL2 registers.

OK.  To confirm, this is a desirable change?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/2] KVM: arm64: Control visibility of S1PIE related sysregs to userspace

[Marc Zyngier]

On Wed, 21 Aug 2024 15:01:54 +0100,
Mark Brown <broonie@kernel.org> wrote:
> 
> [1  <text/plain; us-ascii (7bit)>]
> On Wed, Aug 21, 2024 at 02:46:25PM +0100, Marc Zyngier wrote:
> > Mark Brown <broonie@kernel.org> wrote:
> 
> > > Mark Brown (2):
> > >       KVM: arm64: Hide TCR2_EL1 from userspace when disabled for guests
> > >       KVM: arm64: Hide S1PIE registers from userspace when disabled for guests
> 
> > If you are going to do this, please add it on top of [1], and handle
> > the corresponding EL2 registers.
> 
> OK.  To confirm, this is a desirable change?

Yes.

Ultimately, we need to revisit the way we deal with visibility, as
adding a myriad of helpers checking a combination of features doesn't
scale. That information should exist as a static table, just like the
trap bits.

But until then, that's probably the way.

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 0/2] KVM: arm64: Control visibility of S1PIE related sysregs to userspace

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 452 bytes --]

On Wed, Aug 21, 2024 at 03:45:20PM +0100, Marc Zyngier wrote:

> Ultimately, we need to revisit the way we deal with visibility, as
> adding a myriad of helpers checking a combination of features doesn't
> scale. That information should exist as a static table, just like the
> trap bits.

Indeed, I was wondering about just adding a description of the relevant
ID register field to the sys_regs table.

> But until then, that's probably the way.

OK.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/2] KVM: arm64: Control visibility of S1PIE related sysregs to userspace

[Marc Zyngier]

On Wed, 21 Aug 2024 16:19:59 +0100,
Mark Brown <broonie@kernel.org> wrote:
> 
> On Wed, Aug 21, 2024 at 03:45:20PM +0100, Marc Zyngier wrote:
> 
> > Ultimately, we need to revisit the way we deal with visibility, as
> > adding a myriad of helpers checking a combination of features doesn't
> > scale. That information should exist as a static table, just like the
> > trap bits.
> 
> Indeed, I was wondering about just adding a description of the relevant
> ID register field to the sys_regs table.

You'd need more than that.

How would you express the visibility of TCR2_EL2? It depends on both
ID_AA64PFR0_EL1.EL2==1 *and* ID_AA64MMFR3_EL1.TCRX==1. So it cannot be
just a single tuple { idreg, field, value }. It needs to be an
arbitrary conjunction of those.

The good news is that it is a much smaller table than the monster trap
routing table: it is "enum vcpu_sysreg" plus things that are
synthesised (anything with a .get_user callback).

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 0/2] KVM: arm64: Control visibility of S1PIE related sysregs to userspace

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1629 bytes --]

On Wed, Aug 21, 2024 at 05:40:06PM +0100, Marc Zyngier wrote:
> Mark Brown <broonie@kernel.org> wrote:

> > Indeed, I was wondering about just adding a description of the relevant
> > ID register field to the sys_regs table.

> You'd need more than that.

> How would you express the visibility of TCR2_EL2? It depends on both
> ID_AA64PFR0_EL1.EL2==1 *and* ID_AA64MMFR3_EL1.TCRX==1. So it cannot be
> just a single tuple { idreg, field, value }. It needs to be an
> arbitrary conjunction of those.

I haven't done an audit for fun cases to see how viable things are, for
the EL2 cases I'd just have an encoding based check for EL2 rather than
explicitly enumerating the ID register for each EL2.  That seemed
quicker and less error prone.  

The other cases I'm aware of are more along the lines of features
restricting the values other features/idregs can have (eg, for SME the
information in ID_AA64PFR1_EL1.SME can also be gleaned from
ID_AA64SMFR0_EL1.SMEver).  For those I'm not sure if visibility checks
are the best approach, if we should audit the registers when starting
the guest to make sure they're self consistent or if we should just pick
the most directly relevant register and rely on userspace to enforce
consistancy.  If there's others more like the EL2 case that wouldn't be
viable though and an approach like you suggest would be better, like I
say I've not actually looked yet.

> The good news is that it is a much smaller table than the monster trap
> routing table: it is "enum vcpu_sysreg" plus things that are
> synthesised (anything with a .get_user callback).

Yes.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/2] KVM: arm64: Control visibility of S1PIE related sysregs to userspace

[Marc Zyngier]

On Thu, 22 Aug 2024 16:30:42 +0100,
Mark Brown <broonie@kernel.org> wrote:
> 
> [1  <text/plain; us-ascii (quoted-printable)>]
> On Wed, Aug 21, 2024 at 05:40:06PM +0100, Marc Zyngier wrote:
> > Mark Brown <broonie@kernel.org> wrote:
> 
> > > Indeed, I was wondering about just adding a description of the relevant
> > > ID register field to the sys_regs table.
> 
> > You'd need more than that.
> 
> > How would you express the visibility of TCR2_EL2? It depends on both
> > ID_AA64PFR0_EL1.EL2==1 *and* ID_AA64MMFR3_EL1.TCRX==1. So it cannot be
> > just a single tuple { idreg, field, value }. It needs to be an
> > arbitrary conjunction of those.
> 
> I haven't done an audit for fun cases to see how viable things are, for
> the EL2 cases I'd just have an encoding based check for EL2 rather than
> explicitly enumerating the ID register for each EL2.  That seemed
> quicker and less error prone.

Sure, you can do that. Or rather, you can do that *right now*. But
that's not what the architecture says (there is no statement saying
that Op1==4 for an EL2 register). So the forward-looking way to do it
is to match the full encoding of a register against the properties
that define its existence.  Anything else is a short lived hack, and I
don't care much for them.

But a simple way to deal with that is to encode a property that checks
a vcpu creation property (EL2, PAuth...).

> 
> The other cases I'm aware of are more along the lines of features
> restricting the values other features/idregs can have (eg, for SME the
> information in ID_AA64PFR1_EL1.SME can also be gleaned from
> ID_AA64SMFR0_EL1.SMEver).

Well, they don't quite advertise the same thing. If you decode the
feature specification, you get:

(FEAT_SME <-> (AArch64 ID_AA64PFR1_EL1.SME >= 1))
(FEAT_SME2 --> (AArch64 ID_AA64SMFR0_EL1.SMEver >= 1))
(FEAT_SME2 --> (AArch64 ID_AA64PFR1_EL1.SME >= 2))
(((AArch64 ID_AA64SMFR0_EL1.SMEver >= 1) || (AArch64 ID_AA64PFR1_EL1.SME >= 2)) --> FEAT_SME2)
(FEAT_SME2p1 <-> (AArch64 ID_AA64SMFR0_EL1.SMEver >= 2))

So SME isn't really advertised in SMEver, SME2 is advertised in both
(and it is enough that one advertises SME2 for the feature to be
present), and SME2p1 is only advertised in SMEver.

That's what we need to implement. Yes, this part of the architecture
is... interesting.

> For those I'm not sure if visibility checks
> are the best approach, if we should audit the registers when starting
> the guest to make sure they're self consistent or if we should just pick
> the most directly relevant register and rely on userspace to enforce
> consistancy.

We definitely rely on userspace to enforce consistency. If userspace
messes up, it's "garbage in, garbage out".

We enforce the sysregs visible by the guest and userspace based on
that, and if that means evaluating 10 registers because there is a
terrible set of combinations (such as PAuth-like features), so be it.
We can always find ways to accelerate that.

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 0/2] KVM: arm64: Control visibility of S1PIE related sysregs to userspace

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2584 bytes --]

On Thu, Aug 22, 2024 at 06:44:08PM +0100, Marc Zyngier wrote:
> Mark Brown <broonie@kernel.org> wrote:

> > I haven't done an audit for fun cases to see how viable things are, for
> > the EL2 cases I'd just have an encoding based check for EL2 rather than
> > explicitly enumerating the ID register for each EL2.  That seemed
> > quicker and less error prone.

> Sure, you can do that. Or rather, you can do that *right now*. But
> that's not what the architecture says (there is no statement saying
> that Op1==4 for an EL2 register). So the forward-looking way to do it
> is to match the full encoding of a register against the properties
> that define its existence.  Anything else is a short lived hack, and I
> don't care much for them.

Oh, well - I had thought that was a case of me not having found the rule
rather than the rule not existing given how consistent the scheme is
currently but yes, if a rule definiteively doesn't exist then I agree
that making one up in software is a bad idea.

> > The other cases I'm aware of are more along the lines of features
> > restricting the values other features/idregs can have (eg, for SME the
> > information in ID_AA64PFR1_EL1.SME can also be gleaned from
> > ID_AA64SMFR0_EL1.SMEver).

> Well, they don't quite advertise the same thing. If you decode the
> feature specification, you get:

> (FEAT_SME <-> (AArch64 ID_AA64PFR1_EL1.SME >= 1))
> (FEAT_SME2 --> (AArch64 ID_AA64SMFR0_EL1.SMEver >= 1))
> (FEAT_SME2 --> (AArch64 ID_AA64PFR1_EL1.SME >= 2))
> (((AArch64 ID_AA64SMFR0_EL1.SMEver >= 1) || (AArch64 ID_AA64PFR1_EL1.SME >= 2)) --> FEAT_SME2)
> (FEAT_SME2p1 <-> (AArch64 ID_AA64SMFR0_EL1.SMEver >= 2))

> So SME isn't really advertised in SMEver, SME2 is advertised in both
> (and it is enough that one advertises SME2 for the feature to be
> present), and SME2p1 is only advertised in SMEver.

> That's what we need to implement. Yes, this part of the architecture
> is... interesting.

Yes, it's not a 1:1 mapping but you can for example identify the
presence of ZT0 via either SME or SMEVer since either implies FEAT_SME2
which like you say makes things a bit interesting.

> > For those I'm not sure if visibility checks
> > are the best approach, if we should audit the registers when starting
> > the guest to make sure they're self consistent or if we should just pick
> > the most directly relevant register and rely on userspace to enforce
> > consistancy.

> We definitely rely on userspace to enforce consistency. If userspace
> messes up, it's "garbage in, garbage out".

It's definitely the simpler approach.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]