* Re: [PATCH v2] KVM: arm64: Only enable S1PIE and S1POE with the features they imply
2026-10-18 8:09 [PATCH v2] KVM: arm64: Only enable S1PIE and S1POE with the features they imply Fuad Tabba
@ 2026-10-18 8:29 ` sashiko-bot
2026-10-18 15:15 ` Fuad Tabba
2026-10-18 17:37 ` Marc Zyngier
1 sibling, 1 reply; 7+ messages in thread
From: sashiko-bot @ 2026-10-18 8:29 UTC (permalink / raw)
To: Fuad Tabba, kvmarm; +Cc: Oliver Upton, Marc Zyngier
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] KVM: arm64: incomplete S1PIE/S1POE implied-feature checks in kvm_has_s1p*()
--
commit 1d4d0fe97a6aa6ec7a4e6a93ff7c048e9f3bf806
Author: Fuad Tabba <fuad.tabba@linux.dev>
KVM: arm64: Only enable S1PIE and S1POE with the features they imply
This commit restricts the fine-grained trap enablement of S1PIE and S1POE
registers in hfgrtr_feat_map and hfgwtr_feat_map to guests that also have
the architecturally implied features (TCR2 and ATS1A, plus HPDS for S1POE).
Link: https://lore.kernel.org/r/20260930-arm64-gcs-v21-2-3556644cd927@kernel.org
Link: https://lore.kernel.org/r/86fqyn2che.wl-maz@kernel.org
> diff --git a/arch/arm64/kvm/config.c b/arch/arm64/kvm/config.c
> index e93919cadf860..5cb2b759b68d6 100644
> --- a/arch/arm64/kvm/config.c
> +++ b/arch/arm64/kvm/config.c
> @@ -205,6 +205,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 +281,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));
> +}
> +
[Severity: High]
Should these implied-feature checks also be folded into kvm_has_s1pie() and
kvm_has_s1poe() in arch/arm64/include/asm/kvm_host.h, as well as the other
S1PIE and S1POE checks in KVM?
Right now, feat_s1pie() and feat_s1poe() are only wired into
hfgrtr_feat_map[] and hfgwtr_feat_map[], which populate
kvm->arch.fgu[HFGRTR_GROUP] for the EL1/EL0 registers, while
kvm_has_s1pie() and kvm_has_s1poe() still only check ID_AA64MMFR3_EL1:
arch/arm64/include/asm/kvm_host.h:
#define kvm_has_s1pie(k) \
(kvm_has_feat((k), ID_AA64MMFR3_EL1, S1PIE, IMP))
#define kvm_has_s1poe(k) \
(system_supports_poe() && \
kvm_has_feat((k), ID_AA64MMFR3_EL1, S1POE, IMP))
In a nested virtualization guest running at vEL2 when TCR2 is hidden,
accesses to PIR_EL2, PIRE0_EL2, and POR_EL2 trap via HCR_EL2.NV rather than
fine-grained traps. Because encoding_to_fgt[] in triage_sysreg_trap() only
maps the EL1/EL0 encodings, kvm_handle_sys_reg() -> perform_access() falls
back to sysreg_hidden() -> s1pie_el2_visibility() / s1poe_el2_visibility():
arch/arm64/kvm/sys_regs.c:
static unsigned int s1poe_visibility(const struct kvm_vcpu *vcpu,
const struct sys_reg_desc *rd)
{
if (kvm_has_s1poe(vcpu->kvm))
return 0;
return REG_HIDDEN;
}
Because kvm_has_s1pie() and kvm_has_s1poe() return true even when TCR2 is
hidden, perform_access() does not inject an UNDEF. Instead, on VHE,
access_rw() -> vcpu_read_sys_reg() / vcpu_write_sys_reg() ->
locate_register() maps PIR_EL2, PIRE0_EL2, and POR_EL2 to the hardware
SYS_PIR_EL12, SYS_PIRE0_EL12, and SYS_POR_EL12 registers. However,
__sysreg_save_vel2_state() and __sysreg_restore_vel2_state() in
arch/arm64/kvm/hyp/vhe/sysreg-sr.c skip saving and restoring them when
!ctxt_has_tcrx():
if (ctxt_has_tcrx(&vcpu->arch.ctxt)) {
__vcpu_assign_sys_reg(vcpu, TCR2_EL2, read_sysreg_el1(SYS_TCR2));
if (ctxt_has_s1pie(&vcpu->arch.ctxt)) {
__vcpu_assign_sys_reg(vcpu, PIRE0_EL2, read_sysreg_el1(SYS_PIRE0));
__vcpu_assign_sys_reg(vcpu, PIR_EL2, read_sysreg_el1(SYS_PIR));
}
if (ctxt_has_s1poe(&vcpu->arch.ctxt))
__vcpu_assign_sys_reg(vcpu, POR_EL2, read_sysreg_el1(SYS_POR));
}
Won't a nested guest with S1PIE or S1POE and without TCR2 still leak and
corrupt the physical PIR_EL1, PIRE0_EL1, and POR_EL1 registers across
guests on VHE via PIR_EL2, PIRE0_EL2, and POR_EL2?
Similarly, if userspace enables TCR2 alongside S1PIE or S1POE while hiding
ATS1A or HPDS, leaving kvm_has_s1pie() and kvm_has_s1poe() unchanged means:
- s1pie_visibility() and s1poe_visibility() keep the registers visible to
userspace and vEL2, and ctxt_has_s1pie() / ctxt_has_s1poe() still context
switch them.
- s1pie_enabled() and compute_s1poe() in arch/arm64/kvm/at.c still treat
S1PIE and S1POE as enabled during software AT walks.
- tcr2_el2_feat_map[] in arch/arm64/kvm/config.c still uses FEAT_S1POE and
FEAT_S1PIE for TCR2_EL2_POE | TCR2_EL2_E0POE and TCR2_EL2_PIE instead of
feat_s1poe and feat_s1pie.
- __activate_cptr_traps_vhe() in arch/arm64/kvm/hyp/include/hyp/switch.h
still checks raw kvm_has_feat(vcpu->kvm, ID_AA64MMFR3_EL1, S1POE, IMP)
when propagating CPACR_EL1_E0POE.
> static bool feat_sme_smps(struct kvm *kvm)
> {
> /*
> @@ -393,10 +413,10 @@ static const struct reg_bits_to_feat_map hfgrtr_feat_map[] = {
> NEEDS_FEAT(HFGRTR_EL2_nS2POR_EL1, FEAT_S2POE),
> NEEDS_FEAT(HFGRTR_EL2_nPOR_EL1 |
> HFGRTR_EL2_nPOR_EL0,
> - FEAT_S1POE),
> + feat_s1poe),
> NEEDS_FEAT(HFGRTR_EL2_nPIR_EL1 |
> HFGRTR_EL2_nPIRE0_EL1,
> - FEAT_S1PIE),
> + feat_s1pie),
> NEEDS_FEAT(HFGRTR_EL2_nRCWMASK_EL1, FEAT_THE),
> NEEDS_FEAT(HFGRTR_EL2_nTPIDR2_EL0 |
> HFGRTR_EL2_nSMPRI_EL1,
> @@ -474,10 +494,10 @@ static const struct reg_bits_to_feat_map hfgwtr_feat_map[] = {
> NEEDS_FEAT(HFGWTR_EL2_nS2POR_EL1, FEAT_S2POE),
> NEEDS_FEAT(HFGWTR_EL2_nPOR_EL1 |
> HFGWTR_EL2_nPOR_EL0,
> - FEAT_S1POE),
> + feat_s1poe),
> NEEDS_FEAT(HFGWTR_EL2_nPIR_EL1 |
> HFGWTR_EL2_nPIRE0_EL1,
> - FEAT_S1PIE),
> + feat_s1pie),
> NEEDS_FEAT(HFGWTR_EL2_nRCWMASK_EL1, FEAT_THE),
> NEEDS_FEAT(HFGWTR_EL2_nTPIDR2_EL0 |
> HFGWTR_EL2_nSMPRI_EL1,
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261018080921.3942517-1-fuad.tabba@linux.dev?part=1
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] KVM: arm64: Only enable S1PIE and S1POE with the features they imply
2026-10-18 8:09 [PATCH v2] KVM: arm64: Only enable S1PIE and S1POE with the features they imply Fuad Tabba
2026-10-18 8:29 ` sashiko-bot
@ 2026-10-18 17:37 ` Marc Zyngier
2026-10-18 18:52 ` Fuad Tabba
1 sibling, 1 reply; 7+ messages in thread
From: Marc Zyngier @ 2026-10-18 17:37 UTC (permalink / raw)
To: Fuad Tabba
Cc: Oliver Upton, kvmarm, linux-arm-kernel, Joey Gouly, Steffen Eiden,
Suzuki K Poulose, Zenghui Yu, Catalin Marinas, Will Deacon,
Mark Rutland, Mark Brown, Lorenzo Stoakes, Fuad Tabba,
linux-kernel
On Sun, 18 Oct 2026 09:09:20 +0100,
Fuad Tabba <fuad.tabba@linux.dev> wrote:
>
> The world switch skips PIR_EL1, PIRE0_EL1 and POR_EL1 for a guest
> without TCR2, but KVM doesn't trap them for a guest with S1PIE or
> S1POE. Userspace can hide TCR2 while leaving either visible, and such a
> guest then accesses registers that are neither trapped nor switched.
>
> Under VHE, that leaks values between guests. POR_EL1, which the host
> kernel doesn't use, leaks between guests in every mode. On nVHE and
> hVHE, pKVM included, the host's own EL1 registers are saved and restored
> according to the running guest's features, so the host kernel resumes
> with its stage-1 permissions taken from the guest's PIR_EL1 and
> PIRE0_EL1.
>
> As per the architecture, S1PIE and S1POE imply TCR2 and ATS1A, and
> S1POE also implies HPDS. Only enable each for a guest that has the
> features it implies. A guest that has S1PIE or S1POE without them still
> sees the feature in its ID registers, but its accesses to that
> feature's 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
> Suggested-by: Marc Zyngier <maz@kernel.org>
> Link: https://lore.kernel.org/r/86fqyn2che.wl-maz@kernel.org
> Cc: stable@vger.kernel.org
> Signed-off-by: Fuad Tabba <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 Linux 7.3-rc3 (fd73f4a665989).
>
> arch/arm64/kvm/config.c | 28 ++++++++++++++++++++++++----
> 1 file changed, 24 insertions(+), 4 deletions(-)
I'm afraid that this only addresses part of the problem. Ultimately,
the idregs themselves require some sanitisation because KVM uses this
as the source of truth. I was pointing this in my reply to Broonie,
and this is (probably) what sashiko is blabbing on about.
I hacked something that could be used as the bases for such
sanitisation, although it is quite crude and doesn't cover all of the
possible requirements, see below. I've only boot-tested it in a nested
guest, and I'm not even sure it actually works (my test box doesn't
have these features).
M.
From 6632cbcebbf4b384f209e988deab0924f03dfff8 Mon Sep 17 00:00:00 2001
From: Marc Zyngier <maz@kernel.org>
Date: Sun, 18 Oct 2026 12:07:09 +0100
Subject: [PATCH] KVM: arm64: Sanitise ID_AA64MMFR3_EL1.S1P[IO]E
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
arch/arm64/include/asm/kvm_host.h | 1 +
arch/arm64/kvm/config.c | 59 +++++++++++++++++++++++++++++++
arch/arm64/kvm/sys_regs.c | 2 ++
3 files changed, 62 insertions(+)
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 6be90f8e19813..ab68ddca234c4 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -1742,6 +1742,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 6d2650427c5fc..f037a288c9d46 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
@@ -1319,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 |
@@ -1428,6 +1452,41 @@ 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 4f18ed2793c33..0687b78cd7c2b 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.
--
2.47.3
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply related [flat|nested] 7+ messages in thread