KVM: arm64: Fix guest feature sanitization and pKVM state synchronization

When CONFIG_ARM64_POE is disabled, KVM does not save/restore POR_EL1. However, ID_AA64MMFR3_EL1 sanitisation currently exposes the feature to guests whenever the hardware supports it, ignoring the host kernel configuration.

If a guest detects this feature and attempts to use it, the host will fail to context-switch POR_EL1, potentially leading to state corruption.

Fix this by masking ID_AA64MMFR3_EL1.S1POE and preventing KVM from advertising the feature when the host does not support it, i.e., system_supports_poe() is false.

Fixes 70ed7238297f ("KVM: arm64: Sanitise ID_AA64MMFR3_EL1")

Signed-off-by Fuad Tabba <[email protected]>

---
 arch/arm64/include/asm/kvm_host.h | 3 ++-
 arch/arm64/kvm/sys_regs.c         | 3 +++
 2 files changed, 5 insertions(+), 1 deletion(-)

arch/arm64/include/asm/kvm_host.h +0 -0

index ac7f970c7883..7af72ca749a6 100644

arch/arm64/include/asm/kvm_host.h +0 -0

 #define kvm_has_ras(k)					\
 	(kvm_has_feat((k), ID_AA64PFR0_EL1, RAS, IMP))

arch/arm64/kvm/sys_regs.c +0 -0

index 88a57ca36d96..237e8bd1cf29 100644

Hi Fuad,

Quoted 5 lines · Fuad Tabba

When CONFIG_ARM64_POE is disabled, KVM does not save/restore POR_EL1. However, ID_AA64MMFR3_EL1 sanitisation currently exposes the feature to guests whenever the hardware supports it, ignoring the host kernel configuration.

This is the umpteenth time we get caught by this. PAN was the latest instance until this one. Maybe an approach would be to have a default override when a config option is not enabled, so that KVM is consistent with the rest of the kernel?

Quoted 22 lines

If a guest detects this feature and attempts to use it, the host will fail to context-switch POR_EL1, potentially leading to state corruption.

Fix this by masking ID_AA64MMFR3_EL1.S1POE and preventing KVM from advertising the feature when the host does not support it, i.e., system_supports_poe() is false.

Fixes 70ed7238297f ("KVM: arm64: Sanitise ID_AA64MMFR3_EL1")

Signed-off-by Fuad Tabba <[email protected]>

---
 arch/arm64/include/asm/kvm_host.h | 3 ++-
 arch/arm64/kvm/sys_regs.c         | 3 +++
 2 files changed, 5 insertions(+), 1 deletion(-)

arch/arm64/include/asm/kvm_host.h +0 -0

index ac7f970c7883..7af72ca749a6 100644
@@ -1592,7 +1592,8 @@ void kvm_set_vm_id_reg(struct kvm *kvm, u32 reg, u64 val);
      (kvm_has_feat((k), ID_AA64MMFR3_EL1, S1PIE, IMP))
 

arch/arm64/include/asm/kvm_host.h void kvm_set_vm_id_reg(struct kvm *kvm, u32 reg, u64 val); · line 1596

Quoted 4 lines

arch/arm64/include/asm/kvm_host.h +2 -1

 #define kvm_has_s1poe(k)                             \
-     (kvm_has_feat((k), ID_AA64MMFR3_EL1, S1POE, IMP))
+     (system_supports_poe() &&                       \
+      kvm_has_feat((k), ID_AA64MMFR3_EL1, S1POE, IMP))

Why do we need to further key this on system_supports_poe()? I can see this is a potential optimisation, but I don't think this is part of the minimal fix.

Quoted 11 lines

arch/arm64/include/asm/kvm_host.h +0 -0

 #define kvm_has_ras(k)                                       \
      (kvm_has_feat((k), ID_AA64PFR0_EL1, RAS, IMP))

arch/arm64/kvm/sys_regs.c +0 -0

index 88a57ca36d96..237e8bd1cf29 100644
@@ -1816,6 +1816,9 @@ static u64 __kvm_read_sanitised_id_reg(const struct kvm_vcpu *vcpu,
                     ID_AA64MMFR3_EL1_SCTLRX |
                     ID_AA64MMFR3_EL1_S1POE |

arch/arm64/kvm/sys_regs.c static u64 __kvm_read_sanitised_id_reg(const struct kvm_vcpu *vcpu, · line 1821

Quoted 4 lines

arch/arm64/kvm/sys_regs.c +3 -0

                     ID_AA64MMFR3_EL1_S1PIE;
+
+             if (!system_supports_poe())
+                     val &= ~ID_AA64MMFR3_EL1_S1POE;

How about S1PIE? It seems to have a similar problem, in the sense that it has extra state. But I guess because we don't put it behind a config option, we context-switch it anyway and all is good?

Thanks,

        M.

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

Hi Marc,

Quoted 11 lines · Marc Zyngier

Hi Fuad,

Quoted 5 lines · Fuad Tabba

When CONFIG_ARM64_POE is disabled, KVM does not save/restore POR_EL1. However, ID_AA64MMFR3_EL1 sanitisation currently exposes the feature to guests whenever the hardware supports it, ignoring the host kernel configuration.

Quoted 4 lines

This is the umpteenth time we get caught by this. PAN was the latest instance until this one. Maybe an approach would be to have a default override when a config option is not enabled, so that KVM is consistent with the rest of the kernel?

I spoke to Will about this, and one thing he'll look into is whether this value in struct arm64_ftr_reg can be made consistent with the cpu configuration itself (in cpufeature.c itself) . This would avoid the problem altogether if possible. The question is whether the kernel needs to somehow know that a certain feature exists even if it's disabled in the config...

If he thinks it's not doable at that level, I'll look into alternatives to make it correct by construction.

Quoted 27 lines · Fuad Tabba

Hi Marc,

Quoted 11 lines · Marc Zyngier

Hi Fuad,

Quoted 5 lines · Fuad Tabba

When CONFIG_ARM64_POE is disabled, KVM does not save/restore POR_EL1. However, ID_AA64MMFR3_EL1 sanitisation currently exposes the feature to guests whenever the hardware supports it, ignoring the host kernel configuration.

Quoted 4 lines

This is the umpteenth time we get caught by this. PAN was the latest instance until this one. Maybe an approach would be to have a default override when a config option is not enabled, so that KVM is consistent with the rest of the kernel?

I spoke to Will about this, and one thing he'll look into is whether this value in struct arm64_ftr_reg can be made consistent with the cpu configuration itself (in cpufeature.c itself) . This would avoid the problem altogether if possible. The question is whether the kernel needs to somehow know that a certain feature exists even if it's disabled in the config...

Quoted 2 lines

If he thinks it's not doable at that level, I'll look into alternatives to make it correct by construction.

What I currently have for that is rather ugly:

arch/arm64/include/asm/cpufeature.h +1 -0

index 72f39cecce93a..3bde0ad5ea972 100644
@@ -971,6 +971,7 @@ struct arm64_ftr_reg *get_arm64_ftr_reg(u32 sys_id);
 extern struct arm64_ftr_override id_aa64mmfr0_override;
 extern struct arm64_ftr_override id_aa64mmfr1_override;
 extern struct arm64_ftr_override id_aa64mmfr2_override;
+extern struct arm64_ftr_override id_aa64mmfr3_override;
 extern struct arm64_ftr_override id_aa64pfr0_override;
 extern struct arm64_ftr_override id_aa64pfr1_override;
 extern struct arm64_ftr_override id_aa64zfr0_override;

arch/arm64/kernel/cpufeature.c +3 -1

index 1a7eec542675b..32069da9651bf 100644
@@ -778,6 +778,7 @@ static const struct arm64_ftr_bits ftr_raz[] = {
 struct arm64_ftr_override __read_mostly id_aa64mmfr0_override;
 struct arm64_ftr_override __read_mostly id_aa64mmfr1_override;
 struct arm64_ftr_override __read_mostly id_aa64mmfr2_override;
+struct arm64_ftr_override __read_mostly id_aa64mmfr3_override;
 struct arm64_ftr_override __read_mostly id_aa64pfr0_override;
 struct arm64_ftr_override __read_mostly id_aa64pfr1_override;
 struct arm64_ftr_override __read_mostly id_aa64zfr0_override;

@@ -850,7 +851,8 @@ static const struct __ftr_reg_entry {
                               &id_aa64mmfr1_override),
        ARM64_FTR_REG_OVERRIDE(SYS_ID_AA64MMFR2_EL1, ftr_id_aa64mmfr2,
                               &id_aa64mmfr2_override),
-       ARM64_FTR_REG(SYS_ID_AA64MMFR3_EL1, ftr_id_aa64mmfr3),
+       ARM64_FTR_REG_OVERRIDE(SYS_ID_AA64MMFR3_EL1, ftr_id_aa64mmfr3,
+                              &id_aa64mmfr3_override),
        ARM64_FTR_REG(SYS_ID_AA64MMFR4_EL1, ftr_id_aa64mmfr4),
 
        /* Op1 = 0, CRn = 10, CRm = 4 */

arch/arm64/kernel/image-vars.h +1 -0

index 85bc629270bd9..202e165a4680c 100644
@@ -51,6 +51,7 @@ PI_EXPORT_SYM(id_aa64isar2_override);
 PI_EXPORT_SYM(id_aa64mmfr0_override);
 PI_EXPORT_SYM(id_aa64mmfr1_override);
 PI_EXPORT_SYM(id_aa64mmfr2_override);
+PI_EXPORT_SYM(id_aa64mmfr3_override);
 PI_EXPORT_SYM(id_aa64pfr0_override);
 PI_EXPORT_SYM(id_aa64pfr1_override);
 PI_EXPORT_SYM(id_aa64smfr0_override);

arch/arm64/kernel/pi/idreg-override.c +24 -0

index e5ea280452c3b..b8dbe02e53171 100644
@@ -24,10 +24,12 @@
 static u64 __boot_status __initdata;
 
 typedef bool filter_t(u64 val);
+typedef void cfg_override_t(struct arm64_ftr_override *);
 
 struct ftr_set_desc {
        char                            name[FTR_DESC_NAME_LEN];
        PREL64(struct arm64_ftr_override, override);
+       PREL64(cfg_override_t,          cfg_override);
        struct {
                char                    name[FTR_DESC_FIELD_LEN];
                u8                      shift;

@@ -106,6 +108,22 @@ static const struct ftr_set_desc mmfr2 __prel64_initconst = {
        },
 };
 
+static void __init cfg_mmfr3_override(struct arm64_ftr_override *override)
+{
+#ifndef CONFIG_ARM64_POE
+       override->mask |= ID_AA64MMFR3_EL1_S1POE_MASK;
+#endif
+}
+
+static const struct ftr_set_desc mmfr3 __prel64_initconst = {
+       .name           = "id_aa64mmfr3",
+       .override       = &id_aa64mmfr3_override,
+       .cfg_override   = cfg_mmfr3_override,
+       .fields         = {
+               {}
+       },
+};
+
 static bool __init pfr0_sve_filter(u64 val)
 {
        /*

@@ -221,6 +239,7 @@ PREL64(const struct ftr_set_desc, reg) regs[] __prel64_initconst = {
        { &mmfr0        },
        { &mmfr1        },
        { &mmfr2        },
+       { &mmfr3        },
        { &pfr0         },
        { &pfr1         },
        { &isar1        },

@@ -398,14 +417,19 @@ void __init init_feature_override(u64 boot_status, const void *fdt,
 {
        struct arm64_ftr_override *override;
        const struct ftr_set_desc *reg;
+       cfg_override_t *cfg_override;
        int i;
 
        for (i = 0; i < ARRAY_SIZE(regs); i++) {
                reg = prel64_pointer(regs[i].reg);
                override = prel64_pointer(reg->override);
+               cfg_override = prel64_pointer(reg->cfg_override);
 
                override->val  = 0;
                override->mask = 0;
+
+               if (cfg_override)
+                       cfg_override(override);
        }
 
        __boot_status = boot_status;

which works, but is not super friendly.

Looking at the arm64_ftr_reg structure, this could work if FTR_VISIBLE_IF_IS_ENABLED() didn't simply put "HIDDEN" when the feature is not present, but forced things to be disabled altogether. The problem is that "HIDDEN" means not shown to userspace, and that we have plenty of HIDDEN features that must make it into KVM.

I'll have a think.

        M.

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

Ouch! Yeah, no easy solutions here. For now, as a fix to be able to backport, would you like me to respin this without the change to kvm_has_s1poe(), or with that change as a separate patch?

In protected mode, the hypervisor maintains a separate instance of the kvm structure for each VM. For non-protected VMs, this structure is initialized from the host's kvm state.

Currently, pkvm_init_features_from_host() copies the KVM_ARCH_FLAG_ID_REGS_INITIALIZED flag from the host without the underlying id_regs data being initialized. This results in the hypervisor seeing the flag as set while the ID registers remain zeroed.

Consequently, kvm_has_feat() checks at EL2 fail (return 0) for non-protected VMs. This breaks logic that relies on feature detection, such as ctxt_has_tcrx() for TCR2_EL1 support. As a result, certain system registers (e.g., TCR2_EL1, PIR_EL1, POR_EL1) are not saved/restored during the world switch, which could lead to state corruption.

Fix this by explicitly copying the ID registers from the host kvm to the hypervisor kvm for non-protected VMs during vCPU initialization, since we trust the host with its non-protected guests' features. Also ensure KVM_ARCH_FLAG_ID_REGS_INITIALIZED is cleared initially in pkvm_init_features_from_host so that vm_copy_id_regs can properly initialize them and set the flag once done.

Fixes 41d6028e28bd ("KVM: arm64: Convert the SVE guest vcpu flag to a vm flag")

Signed-off-by Fuad Tabba <[email protected]>

---
 arch/arm64/kvm/hyp/nvhe/pkvm.c | 37 ++++++++++++++++++++++++++++++++--
 1 file changed, 35 insertions(+), 2 deletions(-)

arch/arm64/kvm/hyp/nvhe/pkvm.c +0 -0

index 12b2acfbcfd1..267854ed29c8 100644

arch/arm64/kvm/hyp/nvhe/pkvm.c +4 -0

 		bitmap_copy(kvm->arch.vcpu_features,

@@ -471,6 +473,36 @@ static int pkvm_vcpu_init_sve(struct pkvm_hyp_vcpu *hyp_vcpu, struct kvm_vcpu *h
 	return ret;
 }
 
+static int vm_copy_id_regs(struct pkvm_hyp_vcpu *hyp_vcpu)
+{
+	struct pkvm_hyp_vm *hyp_vm = pkvm_hyp_vcpu_to_hyp_vm(hyp_vcpu);
+	const struct kvm *host_kvm = hyp_vm->host_kvm;

Quoted 35 lines · Fuad Tabba

In protected mode, the hypervisor maintains a separate instance of the kvm structure for each VM. For non-protected VMs, this structure is initialized from the host's kvm state.

Currently, pkvm_init_features_from_host() copies the KVM_ARCH_FLAG_ID_REGS_INITIALIZED flag from the host without the underlying id_regs data being initialized. This results in the hypervisor seeing the flag as set while the ID registers remain zeroed.

Consequently, kvm_has_feat() checks at EL2 fail (return 0) for non-protected VMs. This breaks logic that relies on feature detection, such as ctxt_has_tcrx() for TCR2_EL1 support. As a result, certain system registers (e.g., TCR2_EL1, PIR_EL1, POR_EL1) are not saved/restored during the world switch, which could lead to state corruption.

Fix this by explicitly copying the ID registers from the host kvm to the hypervisor kvm for non-protected VMs during vCPU initialization, since we trust the host with its non-protected guests' features. Also ensure KVM_ARCH_FLAG_ID_REGS_INITIALIZED is cleared initially in pkvm_init_features_from_host so that vm_copy_id_regs can properly initialize them and set the flag once done.

Fixes 41d6028e28bd ("KVM: arm64: Convert the SVE guest vcpu flag to a vm flag")

Signed-off-by Fuad Tabba <[email protected]>

---
 arch/arm64/kvm/hyp/nvhe/pkvm.c | 37 ++++++++++++++++++++++++++++++++--
 1 file changed, 35 insertions(+), 2 deletions(-)

arch/arm64/kvm/hyp/nvhe/pkvm.c +0 -0

index 12b2acfbcfd1..267854ed29c8 100644

arch/arm64/kvm/hyp/nvhe/pkvm.c static void pkvm_init_features_from_host(struct pkvm_hyp_vm *hyp_vm, const struc · line 349

Quoted 6 lines

arch/arm64/kvm/hyp/nvhe/pkvm.c +2 -0

@@ -344,6 +344,8 @@ static void pkvm_init_features_from_host(struct pkvm_hyp_vm *hyp_vm, const struc
 
      /* No restrictions for non-protected VMs. */
      if (!kvm_vm_is_protected(kvm)) {
+             clear_bit(KVM_ARCH_FLAG_ID_REGS_INITIALIZED, &host_arch_flags);
+
              hyp_vm->kvm.arch.flags = host_arch_flags;

Can't you just have

                hyp_vm->kvm.arch.flags &= ~BIT_ULL(KVM_ARCH_FLAG_ID_REGS_INITIALIZED);

since there are no atomicity requirements here?

Quoted 16 lines

arch/arm64/kvm/hyp/nvhe/pkvm.c +10 -0

              bitmap_copy(kvm->arch.vcpu_features,

@@ -471,6 +473,36 @@ static int pkvm_vcpu_init_sve(struct pkvm_hyp_vcpu *hyp_vcpu, struct kvm_vcpu *h
      return ret;
 }
 
+static int vm_copy_id_regs(struct pkvm_hyp_vcpu *hyp_vcpu)
+{
+     struct pkvm_hyp_vm *hyp_vm = pkvm_hyp_vcpu_to_hyp_vm(hyp_vcpu);
+     const struct kvm *host_kvm = hyp_vm->host_kvm;
+     struct kvm *kvm = &hyp_vm->kvm;
+
+     if (!test_bit(KVM_ARCH_FLAG_ID_REGS_INITIALIZED, &host_kvm->arch.flags))
+             return -EINVAL;
+
+     if (test_bit(KVM_ARCH_FLAG_ID_REGS_INITIALIZED, &kvm->arch.flags))

arch/arm64/kvm/hyp/nvhe/pkvm.c static int pkvm_vcpu_init_sve(struct pkvm_hyp_vcpu *hyp_vcpu, struct kvm_vcpu *h · line 489

Quoted 4 lines

arch/arm64/kvm/hyp/nvhe/pkvm.c +4 -0

+             return 0;
+
+     memcpy(kvm->arch.id_regs, host_kvm->arch.id_regs, sizeof(kvm->arch.id_regs));
+     set_bit(KVM_ARCH_FLAG_ID_REGS_INITIALIZED, &kvm->arch.flags);

This looks a bit odd. Can you have another vcpu doing this in parallel? You seem to be holding vm_table_lock at this stage, so that's probably OK, but I'd have expected something like:

        if (test_and_set_bit(KVM_ARCH_FLAG_ID_REGS_INITIALIZED, &kvm->arch.flags))
                return 0;

        memcpy(kvm->arch.id_regs, host_kvm->arch.id_regs, sizeof(kvm->arch.id_regs));

which makes the intent slightly clearer.

Thanks,

        M.

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