[PATCH v3] KVM: arm64: Sanitise ID_AA64MMFR3_EL1.S1P[IO]E

[PATCH v3] KVM: arm64: Sanitise ID_AA64MMFR3_EL1.S1P[IO]E

[Fuad Tabba]

From: Marc Zyngier <maz@kernel.org>

KVM uses the VM's ID registers as the source of truth for what the
guest has, but userspace can expose S1PIE or S1POE without the features
they imply. As per the architecture, both imply TCR2 and ATS1A, and
S1POE also implies HPDS. With TCR2 hidden, the world switch skips
PIR_EL1, PIRE0_EL1 and POR_EL1 while KVM doesn't trap them, so the guest
accesses registers that are neither trapped nor switched. Under VHE,
that leaks values between guests. On nVHE and hVHE, pKVM included, the
host kernel resumes with the guest's PIR_EL1 and PIRE0_EL1.

On first run, clear S1PIE and S1POE from the VM's ID registers when the
features they imply are missing, before anything derives state from
them. The guest and userspace then see neither feature, and the guest's
accesses to their registers UNDEF.

Fixes: 70ed7238297fb ("KVM: arm64: Sanitise ID_AA64MMFR3_EL1")
Fixes: d4a89e5aee23e ("KVM: arm64: Expose S1PIE to guests")
Reported-by: Mark Brown <broonie@kernel.org>
Link: https://lore.kernel.org/r/20260930-arm64-gcs-v21-2-3556644cd927@kernel.org
Link: https://lore.kernel.org/r/8633u229lq.wl-maz@kernel.org
Cc: stable@vger.kernel.org
Signed-off-by: Marc Zyngier <maz@kernel.org>
Co-developed-by: Fuad Tabba <fuad.tabba@linux.dev>
Signed-off-by: Fuad Tabba <fuad.tabba@linux.dev>
---
Changes in v3:
- Sanitise the ID registers instead of changing the trap tables, using
  Marc's patch (Marc, Sashiko)
- Rebased on kvmarm-7.4
- Link to v2: https://lore.kernel.org/r/20261018080921.3942517-1-fuad.tabba@linux.dev

Changes in v2:
- Also require FEAT_HPDS for S1POE (Marc)
- Link to v1: https://lore.kernel.org/r/20261015153041.3770032-1-fuad.tabba@linux.dev

Based on kvmarm-7.4 (dd0bc00e3e920; also applies to kvmarm/fixes
2ea3a929fe01e).

 arch/arm64/include/asm/kvm_host.h |  1 +
 arch/arm64/kvm/config.c           | 78 +++++++++++++++++++++++++++++++
 arch/arm64/kvm/sys_regs.c         |  2 +
 3 files changed, 81 insertions(+)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 1c6632216e327..695c389047c91 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -1741,6 +1741,7 @@ static inline bool kvm_arch_has_irq_bypass(void)
 	return true;
 }
 
+void kvm_idregs_enforce_dependencies(struct kvm *kvm);
 void compute_fgu(struct kvm *kvm, enum fgt_group_id fgt);
 struct resx get_reg_fixed_bits(struct kvm *kvm, enum vcpu_sysreg reg);
 void check_feature_map(void);
diff --git a/arch/arm64/kvm/config.c b/arch/arm64/kvm/config.c
index 1053676551aff..d856c0b5c3907 100644
--- a/arch/arm64/kvm/config.c
+++ b/arch/arm64/kvm/config.c
@@ -59,6 +59,7 @@ struct reg_feat_map_desc {
 	const struct reg_bits_to_feat_map feat_map;
 	const struct reg_bits_to_feat_map *bit_feat_map;
 	const unsigned int		  bit_feat_map_sz;
+	const unsigned int		  idreg_idx;
 };
 
 #define __NEEDS_FEAT_3(m, f, w, id, fld, lim)		\
@@ -130,6 +131,21 @@ struct reg_feat_map_desc {
 		.bit_feat_map_sz	= ARRAY_SIZE(m),		\
 	}
 
+/*
+ * Declare the dependency between the features advertised in an id
+ * register, and the set of other features they depend on. This
+ * generates a struct reg_feat_map_desc.
+ */
+#define DECLARE_IDREG_FEAT_MAP(n, r, m, f)				\
+	struct reg_feat_map_desc n = {					\
+		.name			= #r,				\
+		.feat_map		= NEEDS_FEAT(~(r##_RES0 |	\
+						       r##_RES1), f),	\
+		.bit_feat_map		= m,				\
+		.bit_feat_map_sz	= ARRAY_SIZE(m),		\
+		.idreg_idx		= IDREG_IDX(SYS_##r),		\
+	}
+
 #define FEAT_SPE		ID_AA64DFR0_EL1, PMSVer, IMP
 #define FEAT_BRBE		ID_AA64DFR0_EL1, BRBE, IMP
 #define FEAT_TRC_SR		ID_AA64DFR0_EL1, TraceVer, IMP
@@ -205,6 +221,7 @@ struct reg_feat_map_desc {
 #define FEAT_MEC		ID_AA64MMFR3_EL1, MEC, IMP
 #define FEAT_HAFT		ID_AA64MMFR1_EL1, HAFDBS, HAFT
 #define FEAT_HDBSS		ID_AA64MMFR1_EL1, HAFDBS, HDBSS
+#define FEAT_HPDS		ID_AA64MMFR1_EL1, HPDS, IMP
 #define FEAT_HPDS2		ID_AA64MMFR1_EL1, HPDS, HPDS2
 #define FEAT_BTI		ID_AA64PFR1_EL1, BT, IMP
 #define FEAT_ExS		ID_AA64MMFR0_EL1, EXS, IMP
@@ -280,6 +297,25 @@ static bool feat_anerr(struct kvm *kvm)
 		kvm_has_feat(kvm, ID_AA64MMFR3_EL1, SNERR, FEAT_ANERR));
 }
 
+/*
+ * S1PIE and S1POE imply TCR2 and ATS1A, and S1POE also implies HPDS, but
+ * userspace can set them independently.
+ */
+static bool feat_s1pie(struct kvm *kvm)
+{
+	return (kvm_has_feat(kvm, FEAT_S1PIE) &&
+		kvm_has_feat(kvm, FEAT_TCR2) &&
+		kvm_has_feat(kvm, FEAT_ATS1A));
+}
+
+static bool feat_s1poe(struct kvm *kvm)
+{
+	return (kvm_has_feat(kvm, FEAT_S1POE) &&
+		kvm_has_feat(kvm, FEAT_TCR2) &&
+		kvm_has_feat(kvm, FEAT_ATS1A) &&
+		kvm_has_feat(kvm, FEAT_HPDS));
+}
+
 static bool feat_sme_smps(struct kvm *kvm)
 {
 	/*
@@ -1299,6 +1335,14 @@ static const struct reg_bits_to_feat_map vtcr_el2_feat_map[] = {
 static const DECLARE_FEAT_MAP(vtcr_el2_desc, VTCR_EL2,
 			      vtcr_el2_feat_map, FEAT_AA64EL2);
 
+static const struct reg_bits_to_feat_map id_aa64mmfr3_el1_feat_map[] = {
+	NEEDS_FEAT(ID_AA64MMFR3_EL1_S1PIE, feat_s1pie),
+	NEEDS_FEAT(ID_AA64MMFR3_EL1_S1POE, feat_s1poe),
+};
+
+static const DECLARE_IDREG_FEAT_MAP(id_aa64mmfr3_el1_desc, ID_AA64MMFR3_EL1,
+				    id_aa64mmfr3_el1_feat_map, FEAT_AA64EL1);
+
 static const struct reg_bits_to_feat_map ich_hfgrtr_feat_map[] = {
 	NEEDS_FEAT(ICH_HFGRTR_EL2_ICC_APR_EL1 |
 		   ICH_HFGRTR_EL2_ICC_IDRn_EL1 |
@@ -1408,6 +1452,40 @@ void __init check_feature_map(void)
 	check_reg_desc(&ich_hfgitr_desc);
 }
 
+static const struct reg_feat_map_desc *idreg_feat_maps[] = {
+	&id_aa64mmfr3_el1_desc,
+};
+
+static bool idreg_deps(struct kvm *kvm, const struct reg_feat_map_desc *r)
+{
+	struct resx resx = {};
+	u64 val;
+
+	for (int i = 0; i < r->bit_feat_map_sz; i++) {
+		if ((r->bit_feat_map[i].flags & CALL_FUNC) &&
+		    !r->bit_feat_map[i].match(kvm))
+			resx.res0 |= reg_feat_map_bits(&r->bit_feat_map[i]);
+	}
+
+	val = kvm->arch.id_regs[r->idreg_idx];
+	kvm->arch.id_regs[r->idreg_idx] &= ~resx.res0;
+	kvm->arch.id_regs[r->idreg_idx] |= resx.res1;
+
+	return val != kvm->arch.id_regs[r->idreg_idx];
+}
+
+void kvm_idregs_enforce_dependencies(struct kvm *kvm)
+{
+	bool again;
+
+	do {
+		again = false;
+
+		for (int i = 0; i < ARRAY_SIZE(idreg_feat_maps); i++)
+			again |= idreg_deps(kvm, idreg_feat_maps[i]);
+	} while (again);
+}
+
 static bool idreg_feat_match(struct kvm *kvm, const struct reg_bits_to_feat_map *map)
 {
 	u64 regval = kvm->arch.id_regs[map->regidx];
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index e9cf6cc340349..f10905347f3a1 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -5872,6 +5872,8 @@ static int kvm_vm_finalize_sys_regs(struct kvm *kvm)
 	if (kvm_id_regs_final(kvm))
 		return 0;
 
+	kvm_idregs_enforce_dependencies(kvm);
+
 	/*
 	 * This hacks into the ID registers, so only perform it when the
 	 * first vcpu runs, or the kvm_set_vm_id_reg() helper will scream.

base-commit: dd0bc00e3e9201fd49b62ff9cc07890c89003afe
-- 
2.47.3

Re: [PATCH v3] KVM: arm64: Sanitise ID_AA64MMFR3_EL1.S1P[IO]E

[Lorenzo Stoakes (ARM)]

On Tue, Oct 20, 2026 at 07:43:06AM +0100, Fuad Tabba wrote:
> From: Marc Zyngier <maz@kernel.org>
>
> KVM uses the VM's ID registers as the source of truth for what the
> guest has, but userspace can expose S1PIE or S1POE without the features
> they imply. As per the architecture, both imply TCR2 and ATS1A, and
> S1POE also implies HPDS. With TCR2 hidden, the world switch skips
> PIR_EL1, PIRE0_EL1 and POR_EL1 while KVM doesn't trap them, so the guest
> accesses registers that are neither trapped nor switched. Under VHE,
> that leaks values between guests. On nVHE and hVHE, pKVM included, the
> host kernel resumes with the guest's PIR_EL1 and PIRE0_EL1.
>
> On first run, clear S1PIE and S1POE from the VM's ID registers when the
> features they imply are missing, before anything derives state from
> them. The guest and userspace then see neither feature, and the guest's
> accesses to their registers UNDEF.
>
> Fixes: 70ed7238297fb ("KVM: arm64: Sanitise ID_AA64MMFR3_EL1")
> Fixes: d4a89e5aee23e ("KVM: arm64: Expose S1PIE to guests")

I guess kernels that are missing 19868b67e3e7 and 2ce80f624032 will need
that stuff ported too?

> Reported-by: Mark Brown <broonie@kernel.org>
> Link: https://lore.kernel.org/r/20260930-arm64-gcs-v21-2-3556644cd927@kernel.org
> Link: https://lore.kernel.org/r/8633u229lq.wl-maz@kernel.org
> Cc: stable@vger.kernel.org
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> Co-developed-by: Fuad Tabba <fuad.tabba@linux.dev>
> Signed-off-by: Fuad Tabba <fuad.tabba@linux.dev>

Thanks for helping this along Fuad, nice to see all of this come together
:)

So it seems like this is a much nicer solution to the problem overall -
rather than having e.g. kvm_has_s1pie() -> feat_s1pie(), enforce the
dependency by applying res0/1 to the id reg in
kvm_idregs_enforce_dependencies().

And I see that it uses the guaranteed correct ordering from commit 19868b67e3e7
("KVM: arm64: Finalize guest-wide sysregs prior to per-vCPU sysregs") also.

Nits below but the logic looks sound to me so:

Reviewed-by: Lorenzo Stoakes (ARM) <ljs@kernel.org>

For the GCS series, when Mark respins, I'm guessing it'll be something like:

+static bool feat_gcs(struct kvm *kvm)
+{
+	return kvm_has_feat(kvm, FEAT_GCS) && feat_s1pie(kvm);
+}

...

+static const struct reg_bits_to_feat_map id_aa64pfr1_el1_feat_map[] = {
+	NEEDS_FEAT(ID_AA64PFR1_EL1_GCS, feat_gcs),
+};

...

+static const DECLARE_IDREG_FEAT_MAP(id_aa64pfr1_el1_desc, ID_AA64PFR1_EL1,
+                                    id_aa64pfr1_el1_feat_map, FEAT_AA64EL1);

(or with my suggestion below, DECLARE_FEAT_MAP())

...

static const struct reg_feat_map_desc *idreg_feat_maps[] = {
	&id_aa64mmfr3_el1_desc,
+	&id_aa64pfr1_el1_desc,
};

Right?

> ---
> Changes in v3:
> - Sanitise the ID registers instead of changing the trap tables, using
>   Marc's patch (Marc, Sashiko)
> - Rebased on kvmarm-7.4
> - Link to v2: https://lore.kernel.org/r/20261018080921.3942517-1-fuad.tabba@linux.dev
>
> Changes in v2:
> - Also require FEAT_HPDS for S1POE (Marc)
> - Link to v1: https://lore.kernel.org/r/20261015153041.3770032-1-fuad.tabba@linux.dev
>
> Based on kvmarm-7.4 (dd0bc00e3e920; also applies to kvmarm/fixes
> 2ea3a929fe01e).
>
>  arch/arm64/include/asm/kvm_host.h |  1 +
>  arch/arm64/kvm/config.c           | 78 +++++++++++++++++++++++++++++++
>  arch/arm64/kvm/sys_regs.c         |  2 +
>  3 files changed, 81 insertions(+)
>
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 1c6632216e327..695c389047c91 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -1741,6 +1741,7 @@ static inline bool kvm_arch_has_irq_bypass(void)
>  	return true;
>  }
>
> +void kvm_idregs_enforce_dependencies(struct kvm *kvm);
>  void compute_fgu(struct kvm *kvm, enum fgt_group_id fgt);
>  struct resx get_reg_fixed_bits(struct kvm *kvm, enum vcpu_sysreg reg);
>  void check_feature_map(void);
> diff --git a/arch/arm64/kvm/config.c b/arch/arm64/kvm/config.c
> index 1053676551aff..d856c0b5c3907 100644
> --- a/arch/arm64/kvm/config.c
> +++ b/arch/arm64/kvm/config.c
> @@ -59,6 +59,7 @@ struct reg_feat_map_desc {
>  	const struct reg_bits_to_feat_map feat_map;
>  	const struct reg_bits_to_feat_map *bit_feat_map;
>  	const unsigned int		  bit_feat_map_sz;
> +	const unsigned int		  idreg_idx;

This field feels a bit hazardous - 0 is a valid index and now things that
don't declare it are setting a valid value.

That feels like establishing a bit of a footgun for later.

It'd be nice to have something that, when 0, means not present.

Maybe just store the register directly like:

	const u32			reg;

And...

>  };
>
>  #define __NEEDS_FEAT_3(m, f, w, id, fld, lim)		\
> @@ -130,6 +131,21 @@ struct reg_feat_map_desc {
>  		.bit_feat_map_sz	= ARRAY_SIZE(m),		\
>  	}
>
> +/*
> + * Declare the dependency between the features advertised in an id
> + * register, and the set of other features they depend on. This
> + * generates a struct reg_feat_map_desc.
> + */
> +#define DECLARE_IDREG_FEAT_MAP(n, r, m, f)				\
> +	struct reg_feat_map_desc n = {					\
> +		.name			= #r,				\
> +		.feat_map		= NEEDS_FEAT(~(r##_RES0 |	\
> +						       r##_RES1), f),	\
> +		.bit_feat_map		= m,				\
> +		.bit_feat_map_sz	= ARRAY_SIZE(m),		\
> +		.idreg_idx		= IDREG_IDX(SYS_##r),		\
> +	}
> +

...Here you could avoid the duplication and just update DECLARE_FEAT_MAP()
to do:

+		.reg			= SYS_##r,		\

And when you need to check for the feature you can use
is_feature_id_reg(r->reg) [that's static in sys_regs.c and would need to be
exported].


>  #define FEAT_SPE		ID_AA64DFR0_EL1, PMSVer, IMP
>  #define FEAT_BRBE		ID_AA64DFR0_EL1, BRBE, IMP
>  #define FEAT_TRC_SR		ID_AA64DFR0_EL1, TraceVer, IMP
> @@ -205,6 +221,7 @@ struct reg_feat_map_desc {
>  #define FEAT_MEC		ID_AA64MMFR3_EL1, MEC, IMP
>  #define FEAT_HAFT		ID_AA64MMFR1_EL1, HAFDBS, HAFT
>  #define FEAT_HDBSS		ID_AA64MMFR1_EL1, HAFDBS, HDBSS
> +#define FEAT_HPDS		ID_AA64MMFR1_EL1, HPDS, IMP
>  #define FEAT_HPDS2		ID_AA64MMFR1_EL1, HPDS, HPDS2
>  #define FEAT_BTI		ID_AA64PFR1_EL1, BT, IMP
>  #define FEAT_ExS		ID_AA64MMFR0_EL1, EXS, IMP
> @@ -280,6 +297,25 @@ static bool feat_anerr(struct kvm *kvm)
>  		kvm_has_feat(kvm, ID_AA64MMFR3_EL1, SNERR, FEAT_ANERR));
>  }
>
> +/*
> + * S1PIE and S1POE imply TCR2 and ATS1A, and S1POE also implies HPDS, but
> + * userspace can set them independently.
> + */
> +static bool feat_s1pie(struct kvm *kvm)
> +{
> +	return (kvm_has_feat(kvm, FEAT_S1PIE) &&
> +		kvm_has_feat(kvm, FEAT_TCR2) &&
> +		kvm_has_feat(kvm, FEAT_ATS1A));
> +}
> +
> +static bool feat_s1poe(struct kvm *kvm)
> +{
> +	return (kvm_has_feat(kvm, FEAT_S1POE) &&
> +		kvm_has_feat(kvm, FEAT_TCR2) &&
> +		kvm_has_feat(kvm, FEAT_ATS1A) &&
> +		kvm_has_feat(kvm, FEAT_HPDS));
> +}
> +

Kinda neat to have a super-readable, clear definition of the dependencies
here like this :)

>  static bool feat_sme_smps(struct kvm *kvm)
>  {
>  	/*
> @@ -1299,6 +1335,14 @@ static const struct reg_bits_to_feat_map vtcr_el2_feat_map[] = {
>  static const DECLARE_FEAT_MAP(vtcr_el2_desc, VTCR_EL2,
>  			      vtcr_el2_feat_map, FEAT_AA64EL2);
>
> +static const struct reg_bits_to_feat_map id_aa64mmfr3_el1_feat_map[] = {
> +	NEEDS_FEAT(ID_AA64MMFR3_EL1_S1PIE, feat_s1pie),
> +	NEEDS_FEAT(ID_AA64MMFR3_EL1_S1POE, feat_s1poe),
> +};
> +
> +static const DECLARE_IDREG_FEAT_MAP(id_aa64mmfr3_el1_desc, ID_AA64MMFR3_EL1,
> +				    id_aa64mmfr3_el1_feat_map, FEAT_AA64EL1);
> +
>  static const struct reg_bits_to_feat_map ich_hfgrtr_feat_map[] = {
>  	NEEDS_FEAT(ICH_HFGRTR_EL2_ICC_APR_EL1 |
>  		   ICH_HFGRTR_EL2_ICC_IDRn_EL1 |
> @@ -1408,6 +1452,40 @@ void __init check_feature_map(void)
>  	check_reg_desc(&ich_hfgitr_desc);
>  }
>
> +static const struct reg_feat_map_desc *idreg_feat_maps[] = {
> +	&id_aa64mmfr3_el1_desc,
> +};
> +
> +static bool idreg_deps(struct kvm *kvm, const struct reg_feat_map_desc *r)

To me this reads confusingly - idreg_apply_deps() would be clearer I think.

> +{
> +	struct resx resx = {};
> +	u64 val;
> +
> +	for (int i = 0; i < r->bit_feat_map_sz; i++) {
> +		if ((r->bit_feat_map[i].flags & CALL_FUNC) &&

What if it isn't a CALL_FUNC feature?

Could this loop just be replaced with compute_resx_bits()?

> +		    !r->bit_feat_map[i].match(kvm))
> +			resx.res0 |= reg_feat_map_bits(&r->bit_feat_map[i]);

OK, neat, this e.g. invokes feat_s1pie, feat_s1poe for aa64mmfr3_el1.


> +	}
> +
> +	val = kvm->arch.id_regs[r->idreg_idx];
> +	kvm->arch.id_regs[r->idreg_idx] &= ~resx.res0;
> +	kvm->arch.id_regs[r->idreg_idx] |= resx.res1;

I see from the previous review that res1 is for future proofing, but
redundant (but harmless) atm.

> +
> +	return val != kvm->arch.id_regs[r->idreg_idx];
> +}
> +
> +void kvm_idregs_enforce_dependencies(struct kvm *kvm)
> +{
> +	bool again;
> +
> +	do {
> +		again = false;
> +
> +		for (int i = 0; i < ARRAY_SIZE(idreg_feat_maps); i++)
> +			again |= idreg_deps(kvm, idreg_feat_maps[i]);
> +	} while (again);

This is pretty confusing at first glance, so - iterate through all
idreg_feat_maps, if any changes happened, then loop again.

Could it be e.g.:

static bool idreg_apply_all_deps(struct kvm *kvm)
{
	bool changed = false;

	for (int i = 0; i < ARRAY_SIZE(idreg_feat_maps); i++)
		changed |= idreg_apply_deps(kvm, idreg_feat_maps[i]);
	return changed;
}

void kvm_idregs_enforce_dependencies(struct kvm *kvm)
{
	/* Propagate chained dependencies. */
	while (idreg_apply_all_deps(kvm))
		;
}

> +}
> +
>  static bool idreg_feat_match(struct kvm *kvm, const struct reg_bits_to_feat_map *map)
>  {
>  	u64 regval = kvm->arch.id_regs[map->regidx];
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index e9cf6cc340349..f10905347f3a1 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -5872,6 +5872,8 @@ static int kvm_vm_finalize_sys_regs(struct kvm *kvm)
>  	if (kvm_id_regs_final(kvm))
>  		return 0;
>
> +	kvm_idregs_enforce_dependencies(kvm);
> +
>  	/*
>  	 * This hacks into the ID registers, so only perform it when the
>  	 * first vcpu runs, or the kvm_set_vm_id_reg() helper will scream.
>
> base-commit: dd0bc00e3e9201fd49b62ff9cc07890c89003afe
> --
> 2.47.3
>

--
Cheers, Lorenzo