KVM: arm64: Only enable S1PIE and S1POE with the features they imply
Fuad Tabba
reviewerPATCH v2 1/1 KVM: arm64: Only enable S1PIE and S1POE with the features they imply
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.
---
Changes in v2:
- Also require FEAT_HPDS for S1POE (Marc)
- Link to v1: https://lore.kernel.org/r/[email protected]
Based on Linux 7.3-rc3 (fd73f4a665989).
arch/arm64/kvm/config.c | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-)
arch/arm64/kvm/config.c +24 -4
@@ -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 )); +} +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,
Signature · 1 line
2.39.5
[email protected]
AI reviewThank 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*() --
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 <[email protected]>
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).
Quoted 25 lines
arch/arm64/kvm/config.c +10
-0@@ -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 ));
arch/arm64/kvm/config.c static bool feat_anerr(struct kvm *kvm) · line 302
Quoted 10 lines
arch/arm64/kvm/config.c +10
-0+} + +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.
Quoted 29 lines
arch/arm64/kvm/config.c +4
-4static 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,
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Fuad Tabba
reviewerSashiko, I see you're on Argon now... way to go!
On Sun, 18 Oct 2026 09:29:22 +0100, [email protected] wrote:
[...]
Quoted 3 lines
Won't a nested guest with S1PIE or S1POE and without TCR2 still leak and corrupt the physical
PIR_EL1,PIRE0_EL1, andPOR_EL1registers across guests on VHE viaPIR_EL2,PIRE0_EL2, andPOR_EL2?
Yes, these trap through HCR_EL2.NV and no FGT bit covers them, so the FGU misses them. v3 will UNDEF them along with their EL1 counterparts.
Quoted 2 lines
Similarly, if userspace enables TCR2 alongside S1PIE or S1POE while hiding ATS1A or HPDS, leaving
kvm_has_s1pie()andkvm_has_s1poe()unchanged means:
With TCR2 present, the world switch saves and restores all of these registers, so nothing leaks. The rest is KVM's handling of an ID register combination the architecture doesn't allow, which this stable fix leaves alone.
Cheers,
/fuad
Marc Zyngier
maintainerQuoted 32 lines · Fuad Tabba
The world switch skips
PIR_EL1,PIRE0_EL1andPOR_EL1for 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'sPIR_EL1andPIRE0_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 <[email protected]>Suggested-by Marc Zyngier <[email protected]>Signed-off-by Fuad Tabba <[email protected]>---
Changes in v2:
- Also requireFEAT_HPDSfor S1POE (Marc)
- Link to v1: https://lore.kernel.org/r/[email protected]
Quoted 4 lines
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 <[email protected]>
Date: Sun, 18 Oct 2026 12:07:09 +0100
Subject: [PATCH] KVM: arm64: Sanitise ID_AA64MMFR3_EL1.S1P[IO]E
--- 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(+)
arch/arm64/include/asm/kvm_host.h +1 -0
@@ -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 );
arch/arm64/kvm/config.c +59 -0
83 lines not on this page · raw message
arch/arm64/kvm/sys_regs.c +2 -0
@@ -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.
Signature · 5 lines
2.47.3 -- Without deviation from the norm, progress is not possible.
Fuad Tabba
reviewerHi Marc,
On Sun, 18 Oct 2026 18:37:37 +0100, Marc Zyngier <[email protected]> wrote:
[...]
Quoted 4 lines
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.
It seems so. The trap tables were only one of the places that read S1PIE and S1POE. Sashiko found another: PIR_EL2, PIRE0_EL2 and POR_EL2 accessed from vEL2 have no FGT bit, so accesses to them reached the hardware registers.
Quoted 5 lines
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).
I'll send your patch as v3, rebased on -rc3 with you as the author, once I've tested it. I'm dropping v2's trap table changes, since they're redundant once the ID registers are sanitised. I made two changes to your patch. The sanitisation now runs before kvm_init_nv_sysregs(), so the nested RESx masks are computed from the sanitised values. I dropped the res1 half, which isn't set by anything.
Cheers,
/fuad
Marc Zyngier
maintainerQuoted 15 lines · Fuad Tabba
Hi Marc,
On Sun, 18 Oct 2026 18:37:37 +0100, Marc Zyngier <[email protected]> wrote:
[...]Quoted 4 lines
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.
It seems so. The trap tables were only one of the places that read S1PIE and S1POE. Sashiko found another:
PIR_EL2,PIRE0_EL2andPOR_EL2accessed from vEL2 have no FGT bit, so accesses to them reached the hardware registers.
Quoted 7 lines
Quoted 5 lines
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).
I'll send your patch as v3, rebased on -rc3 with you as the author,
Please at least add yourself as a co-author.
Quoted 5 lines
once I've tested it. I'm dropping v2's trap table changes, since they're redundant once the ID registers are sanitised. I made two changes to your patch. The sanitisation now runs before
kvm_init_nv_sysregs(), so the nested RESx masks are computed from the sanitised values.
Isn't that already the case? At least with kvmarm-7.4, kvm_vm_finalize_sys_regs() is called before kvm_init_nv_sysregs().
Quoted 1 line
I dropped the res1 half, which isn't set by anything.
I'd like to keep this, as it will eventually get used, just like the recompute loop is a bit useless for now, but will become useful in the future as dependencies get recomputed.
The way I imagine this to work is that each sanitised field entry will point to a subtable containing the encoding of the field and the a function that checks for the dependency. That's not required for now, as S1P[IO]E is only a boolean, but I expect some features to eventually need something more fine-grained.
Thanks,
M.
Signature · 1 line
Without deviation from the norm, progress is not possible.
Fuad Tabba
reviewerHi Marc,
On Mon, 19 Oct 2026 08:31:18 +0100, Marc Zyngier <[email protected]> wrote:
[...]
Quoted 3 lines
Quoted 1 line
I'll send your patch as v3, rebased on -rc3 with you as the author,
Please at least add yourself as a co-author.
Will do, thanks.
Quoted 8 lines
Quoted 5 lines
once I've tested it. I'm dropping v2's trap table changes, since they're redundant once the ID registers are sanitised. I made two changes to your patch. The sanitisation now runs before
kvm_init_nv_sysregs(), so the nested RESx masks are computed from the sanitised values.Isn't that already the case? At least with kvmarm-7.4,
kvm_vm_finalize_sys_regs()is called beforekvm_init_nv_sysregs().
It is. -rc3 doesn't have Mark's split of kvm_finalize_sys_regs(), so kvm_init_nv_sysregs() still runs there first. I'll rebase v3 on kvmarm-7.4 and keep the call where you had it.
Quoted 5 lines
Quoted 1 line
I dropped the res1 half, which isn't set by anything.
I'd like to keep this, as it will eventually get used, just like the recompute loop is a bit useless for now, but will become useful in the future as dependencies get recomputed.
Ack.
Cheers,
/fuad