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

["Lorenzo Stoakes (ARM)" <ljs@kernel.org>]

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