* [PATCH v3 0/2] KVM: arm/arm64: Add VCPU workarounds firmware register
@ 2019-02-22 12:18 ` Andre Przywara
0 siblings, 0 replies; 20+ messages in thread
From: Andre Przywara @ 2019-02-22 12:18 UTC (permalink / raw)
To: Marc Zyngier, Christoffer Dall
Cc: linux-arm-kernel, Steven Price, kvmarm, kvm, Dave Martin
Hi,
Aside from a rebase against kvm-arm/next, this update introduces a new
UNAFFECTED value for WORKAROUND_1, which is not used at the moment, but
will become useful with some patches extending the host kernel workaround
code [1]. We add the value to the ABI already.
Also we now require an exact match for the two WORKAROUND_1 levels, as
we can't allow a guest to migrate from a system with an effectively
unknown workaround state to a system which requires an active guest
workaround.
Now tested with actual migration, see below.
Cheers,
Andre
-----------------------------
Workarounds for Spectre variant 2 or 4 vulnerabilities require some help
from the firmware, so KVM implements an interface to provide that for
guests. When such a guest is migrated, we want to make sure we don't
loose the protection the guest relies on.
This introduces two new firmware registers in KVM's GET/SET_ONE_REG
interface, so userland can save the level of protection implemented by
the hypervisor and used by the guest. Upon restoring these registers,
we make sure we don't downgrade and reject any values that would mean
weaker protection.
The protection level is encoding in the lower 4 bits, with smaller
values indicating weaker protection.
Patch 1 implements the two firmware registers, patch 2 adds the
documentation.
ARM(32) is a bit of a pain (again), as the firmware register interface
is shared, but 32-bit does not implement all the workarounds.
For now I stuffed two wrappers into kvm_emulate.h, which doesn't sound
like the best solution. Happy to hear about better solutions.
This has been tested with migration between two Juno systems. Out of the
box they advertise identical workaround levels, and migration succeeds.
However when disabling the A57 cluster on one system, WORKAROUND_1 is
not needed and the host kernel disables this. Migration between the two
now fails, as expected.
Please have a look and comment!
Cheers,
Andre
[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2019-January/627791.html
Changelog:
v1 .. v2:
- complete rework of WORKAROUND_2 presentation to use a linear scale,
dropping the complicated comparison routine
v2 .. v3:
- rebase against latest kvm-arm/next
- introduce UNAFFECTED value for WORKAROUND_1
- require exact match for WORKAROUND_1 levels
Andre Przywara (2):
KVM: arm/arm64: Add save/restore support for firmware workaround state
KVM: doc: add API documentation on the KVM_REG_ARM_WORKAROUNDS
register
Documentation/virtual/kvm/arm/psci.txt | 24 +++++
arch/arm/include/asm/kvm_emulate.h | 10 +++
arch/arm/include/uapi/asm/kvm.h | 10 +++
arch/arm64/include/asm/kvm_emulate.h | 14 +++
arch/arm64/include/uapi/asm/kvm.h | 9 ++
virt/kvm/arm/psci.c | 119 +++++++++++++++++++++----
6 files changed, 170 insertions(+), 16 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v3 0/2] KVM: arm/arm64: Add VCPU workarounds firmware register
@ 2019-02-22 12:18 ` Andre Przywara
0 siblings, 0 replies; 20+ messages in thread
From: Andre Przywara @ 2019-02-22 12:18 UTC (permalink / raw)
To: Marc Zyngier, Christoffer Dall
Cc: linux-arm-kernel, Steven Price, kvmarm, kvm, Dave Martin
Hi,
Aside from a rebase against kvm-arm/next, this update introduces a new
UNAFFECTED value for WORKAROUND_1, which is not used at the moment, but
will become useful with some patches extending the host kernel workaround
code [1]. We add the value to the ABI already.
Also we now require an exact match for the two WORKAROUND_1 levels, as
we can't allow a guest to migrate from a system with an effectively
unknown workaround state to a system which requires an active guest
workaround.
Now tested with actual migration, see below.
Cheers,
Andre
-----------------------------
Workarounds for Spectre variant 2 or 4 vulnerabilities require some help
from the firmware, so KVM implements an interface to provide that for
guests. When such a guest is migrated, we want to make sure we don't
loose the protection the guest relies on.
This introduces two new firmware registers in KVM's GET/SET_ONE_REG
interface, so userland can save the level of protection implemented by
the hypervisor and used by the guest. Upon restoring these registers,
we make sure we don't downgrade and reject any values that would mean
weaker protection.
The protection level is encoding in the lower 4 bits, with smaller
values indicating weaker protection.
Patch 1 implements the two firmware registers, patch 2 adds the
documentation.
ARM(32) is a bit of a pain (again), as the firmware register interface
is shared, but 32-bit does not implement all the workarounds.
For now I stuffed two wrappers into kvm_emulate.h, which doesn't sound
like the best solution. Happy to hear about better solutions.
This has been tested with migration between two Juno systems. Out of the
box they advertise identical workaround levels, and migration succeeds.
However when disabling the A57 cluster on one system, WORKAROUND_1 is
not needed and the host kernel disables this. Migration between the two
now fails, as expected.
Please have a look and comment!
Cheers,
Andre
[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2019-January/627791.html
Changelog:
v1 .. v2:
- complete rework of WORKAROUND_2 presentation to use a linear scale,
dropping the complicated comparison routine
v2 .. v3:
- rebase against latest kvm-arm/next
- introduce UNAFFECTED value for WORKAROUND_1
- require exact match for WORKAROUND_1 levels
Andre Przywara (2):
KVM: arm/arm64: Add save/restore support for firmware workaround state
KVM: doc: add API documentation on the KVM_REG_ARM_WORKAROUNDS
register
Documentation/virtual/kvm/arm/psci.txt | 24 +++++
arch/arm/include/asm/kvm_emulate.h | 10 +++
arch/arm/include/uapi/asm/kvm.h | 10 +++
arch/arm64/include/asm/kvm_emulate.h | 14 +++
arch/arm64/include/uapi/asm/kvm.h | 9 ++
virt/kvm/arm/psci.c | 119 +++++++++++++++++++++----
6 files changed, 170 insertions(+), 16 deletions(-)
--
2.17.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v3 1/2] KVM: arm/arm64: Add save/restore support for firmware workaround state
2019-02-22 12:18 ` Andre Przywara
@ 2019-02-22 12:18 ` Andre Przywara
-1 siblings, 0 replies; 20+ messages in thread
From: Andre Przywara @ 2019-02-22 12:18 UTC (permalink / raw)
To: Marc Zyngier, Christoffer Dall
Cc: linux-arm-kernel, Steven Price, kvmarm, kvm, Dave Martin
KVM implements the firmware interface for mitigating cache speculation
vulnerabilities. Guests may use this interface to ensure mitigation is
active.
If we want to migrate such a guest to a host with a different support
level for those workarounds, migration might need to fail, to ensure that
critical guests don't loose their protection.
Introduce a way for userland to save and restore the workarounds state.
On restoring we do checks that make sure we don't downgrade our
mitigation level.
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
arch/arm/include/asm/kvm_emulate.h | 10 +++
arch/arm/include/uapi/asm/kvm.h | 10 +++
arch/arm64/include/asm/kvm_emulate.h | 14 ++++
arch/arm64/include/uapi/asm/kvm.h | 9 ++
virt/kvm/arm/psci.c | 119 +++++++++++++++++++++++----
5 files changed, 146 insertions(+), 16 deletions(-)
diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
index 8927cae7c966..663a02d7e6f4 100644
--- a/arch/arm/include/asm/kvm_emulate.h
+++ b/arch/arm/include/asm/kvm_emulate.h
@@ -283,6 +283,16 @@ static inline unsigned long kvm_vcpu_get_mpidr_aff(struct kvm_vcpu *vcpu)
return vcpu_cp15(vcpu, c0_MPIDR) & MPIDR_HWID_BITMASK;
}
+static inline bool kvm_arm_get_vcpu_workaround_2_flag(struct kvm_vcpu *vcpu)
+{
+ return false;
+}
+
+static inline void kvm_arm_set_vcpu_workaround_2_flag(struct kvm_vcpu *vcpu,
+ bool flag)
+{
+}
+
static inline void kvm_vcpu_set_be(struct kvm_vcpu *vcpu)
{
*vcpu_cpsr(vcpu) |= PSR_E_BIT;
diff --git a/arch/arm/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h
index 4602464ebdfb..ba4d2afe65e3 100644
--- a/arch/arm/include/uapi/asm/kvm.h
+++ b/arch/arm/include/uapi/asm/kvm.h
@@ -214,6 +214,16 @@ struct kvm_vcpu_events {
#define KVM_REG_ARM_FW_REG(r) (KVM_REG_ARM | KVM_REG_SIZE_U64 | \
KVM_REG_ARM_FW | ((r) & 0xffff))
#define KVM_REG_ARM_PSCI_VERSION KVM_REG_ARM_FW_REG(0)
+#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1 KVM_REG_ARM_FW_REG(1)
+#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_NOT_AVAIL 0
+#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_AVAIL 1
+#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_UNAFFECTED 2
+#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2 KVM_REG_ARM_FW_REG(2)
+#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_AVAIL 0
+#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_UNKNOWN 1
+#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_AVAIL 2
+#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_UNAFFECTED 3
+#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_ENABLED (1U << 4)
/* Device Control API: ARM VGIC */
#define KVM_DEV_ARM_VGIC_GRP_ADDR 0
diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
index d3842791e1c4..c00c17c9adb6 100644
--- a/arch/arm64/include/asm/kvm_emulate.h
+++ b/arch/arm64/include/asm/kvm_emulate.h
@@ -348,6 +348,20 @@ static inline unsigned long kvm_vcpu_get_mpidr_aff(struct kvm_vcpu *vcpu)
return vcpu_read_sys_reg(vcpu, MPIDR_EL1) & MPIDR_HWID_BITMASK;
}
+static inline bool kvm_arm_get_vcpu_workaround_2_flag(struct kvm_vcpu *vcpu)
+{
+ return vcpu->arch.workaround_flags & VCPU_WORKAROUND_2_FLAG;
+}
+
+static inline void kvm_arm_set_vcpu_workaround_2_flag(struct kvm_vcpu *vcpu,
+ bool flag)
+{
+ if (flag)
+ vcpu->arch.workaround_flags |= VCPU_WORKAROUND_2_FLAG;
+ else
+ vcpu->arch.workaround_flags &= ~VCPU_WORKAROUND_2_FLAG;
+}
+
static inline void kvm_vcpu_set_be(struct kvm_vcpu *vcpu)
{
if (vcpu_mode_is_32bit(vcpu)) {
diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
index 97c3478ee6e7..367e96fe654e 100644
--- a/arch/arm64/include/uapi/asm/kvm.h
+++ b/arch/arm64/include/uapi/asm/kvm.h
@@ -225,6 +225,15 @@ struct kvm_vcpu_events {
#define KVM_REG_ARM_FW_REG(r) (KVM_REG_ARM64 | KVM_REG_SIZE_U64 | \
KVM_REG_ARM_FW | ((r) & 0xffff))
#define KVM_REG_ARM_PSCI_VERSION KVM_REG_ARM_FW_REG(0)
+#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1 KVM_REG_ARM_FW_REG(1)
+#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_NOT_AVAIL 0
+#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_AVAIL 1
+#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2 KVM_REG_ARM_FW_REG(2)
+#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_AVAIL 0
+#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_UNKNOWN 1
+#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_AVAIL 2
+#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_UNAFFECTED 3
+#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_ENABLED (1U << 4)
/* Device Control API: ARM VGIC */
#define KVM_DEV_ARM_VGIC_GRP_ADDR 0
diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c
index 9b73d3ad918a..97d2d13756f6 100644
--- a/virt/kvm/arm/psci.c
+++ b/virt/kvm/arm/psci.c
@@ -445,42 +445,97 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
int kvm_arm_get_fw_num_regs(struct kvm_vcpu *vcpu)
{
- return 1; /* PSCI version */
+ return 3; /* PSCI version and two workaround registers */
}
int kvm_arm_copy_fw_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
{
- if (put_user(KVM_REG_ARM_PSCI_VERSION, uindices))
+ if (put_user(KVM_REG_ARM_PSCI_VERSION, uindices++))
return -EFAULT;
+ if (put_user(KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1, uindices++))
+ return -EFAULT;
+
+ if (put_user(KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2, uindices++))
+ return -EFAULT;
+
+ return 0;
+}
+
+#define KVM_REG_FEATURE_LEVEL_WIDTH 4
+#define KVM_REG_FEATURE_LEVEL_MASK (BIT(KVM_REG_FEATURE_LEVEL_WIDTH) - 1)
+
+/*
+ * Convert the workaround level into an easy-to-compare number, where higher
+ * values mean better protection.
+ */
+static int get_kernel_wa_level(u64 regid)
+{
+ switch (regid) {
+ case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1:
+ if (kvm_arm_harden_branch_predictor())
+ return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_AVAIL;
+ else
+ return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_NOT_AVAIL;
+ case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2:
+ switch (kvm_arm_have_ssbd()) {
+ case KVM_SSBD_FORCE_DISABLE:
+ return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_AVAIL;
+ case KVM_SSBD_KERNEL:
+ return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_AVAIL;
+ case KVM_SSBD_FORCE_ENABLE:
+ case KVM_SSBD_MITIGATED:
+ return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_UNAFFECTED;
+ case KVM_SSBD_UNKNOWN:
+ default:
+ return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_UNKNOWN;
+ }
+ }
+
return 0;
}
int kvm_arm_get_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
{
- if (reg->id == KVM_REG_ARM_PSCI_VERSION) {
- void __user *uaddr = (void __user *)(long)reg->addr;
- u64 val;
+ void __user *uaddr = (void __user *)(long)reg->addr;
+ u64 val;
+ switch (reg->id) {
+ case KVM_REG_ARM_PSCI_VERSION:
val = kvm_psci_version(vcpu, vcpu->kvm);
- if (copy_to_user(uaddr, &val, KVM_REG_SIZE(reg->id)))
- return -EFAULT;
-
- return 0;
+ break;
+ case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1:
+ val = get_kernel_wa_level(reg->id) & KVM_REG_FEATURE_LEVEL_MASK;
+ break;
+ case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2:
+ val = get_kernel_wa_level(reg->id) & KVM_REG_FEATURE_LEVEL_MASK;
+ if (kvm_arm_have_ssbd() == KVM_SSBD_KERNEL &&
+ kvm_arm_get_vcpu_workaround_2_flag(vcpu))
+ val |= KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_ENABLED;
+ break;
+ default:
+ return -EINVAL;
}
- return -EINVAL;
+ if (copy_to_user(uaddr, &val, KVM_REG_SIZE(reg->id)))
+ return -EFAULT;
+
+ return 0;
}
int kvm_arm_set_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
{
- if (reg->id == KVM_REG_ARM_PSCI_VERSION) {
- void __user *uaddr = (void __user *)(long)reg->addr;
- bool wants_02;
- u64 val;
+ void __user *uaddr = (void __user *)(long)reg->addr;
+ u64 val;
+ int wa_level;
+
+ if (copy_from_user(&val, uaddr, KVM_REG_SIZE(reg->id)))
+ return -EFAULT;
- if (copy_from_user(&val, uaddr, KVM_REG_SIZE(reg->id)))
- return -EFAULT;
+ switch (reg->id) {
+ case KVM_REG_ARM_PSCI_VERSION:
+ {
+ bool wants_02;
wants_02 = test_bit(KVM_ARM_VCPU_PSCI_0_2, vcpu->arch.features);
@@ -497,6 +552,38 @@ int kvm_arm_set_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
vcpu->kvm->arch.psci_version = val;
return 0;
}
+ break;
+ }
+
+ case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1:
+ wa_level = val & KVM_REG_FEATURE_LEVEL_MASK;
+
+ /* For now we only accept the very same workaround level. */
+ if (get_kernel_wa_level(reg->id) != wa_level)
+ return -EINVAL;
+
+ return 0;
+
+ case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2:
+ wa_level = val & KVM_REG_FEATURE_LEVEL_MASK;
+
+ if (get_kernel_wa_level(reg->id) < wa_level)
+ return -EINVAL;
+
+ if (kvm_arm_have_ssbd() != KVM_SSBD_KERNEL)
+ return 0;
+
+ switch (wa_level) {
+ case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_AVAIL:
+ kvm_arm_set_vcpu_workaround_2_flag(vcpu,
+ val & KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_ENABLED);
+ break;
+ case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_UNAFFECTED:
+ kvm_arm_set_vcpu_workaround_2_flag(vcpu, true);
+ break;
+ }
+
+ return 0;
}
return -EINVAL;
--
2.17.1
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v3 1/2] KVM: arm/arm64: Add save/restore support for firmware workaround state
@ 2019-02-22 12:18 ` Andre Przywara
0 siblings, 0 replies; 20+ messages in thread
From: Andre Przywara @ 2019-02-22 12:18 UTC (permalink / raw)
To: Marc Zyngier, Christoffer Dall
Cc: linux-arm-kernel, Steven Price, kvmarm, kvm, Dave Martin
KVM implements the firmware interface for mitigating cache speculation
vulnerabilities. Guests may use this interface to ensure mitigation is
active.
If we want to migrate such a guest to a host with a different support
level for those workarounds, migration might need to fail, to ensure that
critical guests don't loose their protection.
Introduce a way for userland to save and restore the workarounds state.
On restoring we do checks that make sure we don't downgrade our
mitigation level.
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
arch/arm/include/asm/kvm_emulate.h | 10 +++
arch/arm/include/uapi/asm/kvm.h | 10 +++
arch/arm64/include/asm/kvm_emulate.h | 14 ++++
arch/arm64/include/uapi/asm/kvm.h | 9 ++
virt/kvm/arm/psci.c | 119 +++++++++++++++++++++++----
5 files changed, 146 insertions(+), 16 deletions(-)
diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
index 8927cae7c966..663a02d7e6f4 100644
--- a/arch/arm/include/asm/kvm_emulate.h
+++ b/arch/arm/include/asm/kvm_emulate.h
@@ -283,6 +283,16 @@ static inline unsigned long kvm_vcpu_get_mpidr_aff(struct kvm_vcpu *vcpu)
return vcpu_cp15(vcpu, c0_MPIDR) & MPIDR_HWID_BITMASK;
}
+static inline bool kvm_arm_get_vcpu_workaround_2_flag(struct kvm_vcpu *vcpu)
+{
+ return false;
+}
+
+static inline void kvm_arm_set_vcpu_workaround_2_flag(struct kvm_vcpu *vcpu,
+ bool flag)
+{
+}
+
static inline void kvm_vcpu_set_be(struct kvm_vcpu *vcpu)
{
*vcpu_cpsr(vcpu) |= PSR_E_BIT;
diff --git a/arch/arm/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h
index 4602464ebdfb..ba4d2afe65e3 100644
--- a/arch/arm/include/uapi/asm/kvm.h
+++ b/arch/arm/include/uapi/asm/kvm.h
@@ -214,6 +214,16 @@ struct kvm_vcpu_events {
#define KVM_REG_ARM_FW_REG(r) (KVM_REG_ARM | KVM_REG_SIZE_U64 | \
KVM_REG_ARM_FW | ((r) & 0xffff))
#define KVM_REG_ARM_PSCI_VERSION KVM_REG_ARM_FW_REG(0)
+#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1 KVM_REG_ARM_FW_REG(1)
+#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_NOT_AVAIL 0
+#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_AVAIL 1
+#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_UNAFFECTED 2
+#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2 KVM_REG_ARM_FW_REG(2)
+#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_AVAIL 0
+#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_UNKNOWN 1
+#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_AVAIL 2
+#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_UNAFFECTED 3
+#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_ENABLED (1U << 4)
/* Device Control API: ARM VGIC */
#define KVM_DEV_ARM_VGIC_GRP_ADDR 0
diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
index d3842791e1c4..c00c17c9adb6 100644
--- a/arch/arm64/include/asm/kvm_emulate.h
+++ b/arch/arm64/include/asm/kvm_emulate.h
@@ -348,6 +348,20 @@ static inline unsigned long kvm_vcpu_get_mpidr_aff(struct kvm_vcpu *vcpu)
return vcpu_read_sys_reg(vcpu, MPIDR_EL1) & MPIDR_HWID_BITMASK;
}
+static inline bool kvm_arm_get_vcpu_workaround_2_flag(struct kvm_vcpu *vcpu)
+{
+ return vcpu->arch.workaround_flags & VCPU_WORKAROUND_2_FLAG;
+}
+
+static inline void kvm_arm_set_vcpu_workaround_2_flag(struct kvm_vcpu *vcpu,
+ bool flag)
+{
+ if (flag)
+ vcpu->arch.workaround_flags |= VCPU_WORKAROUND_2_FLAG;
+ else
+ vcpu->arch.workaround_flags &= ~VCPU_WORKAROUND_2_FLAG;
+}
+
static inline void kvm_vcpu_set_be(struct kvm_vcpu *vcpu)
{
if (vcpu_mode_is_32bit(vcpu)) {
diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
index 97c3478ee6e7..367e96fe654e 100644
--- a/arch/arm64/include/uapi/asm/kvm.h
+++ b/arch/arm64/include/uapi/asm/kvm.h
@@ -225,6 +225,15 @@ struct kvm_vcpu_events {
#define KVM_REG_ARM_FW_REG(r) (KVM_REG_ARM64 | KVM_REG_SIZE_U64 | \
KVM_REG_ARM_FW | ((r) & 0xffff))
#define KVM_REG_ARM_PSCI_VERSION KVM_REG_ARM_FW_REG(0)
+#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1 KVM_REG_ARM_FW_REG(1)
+#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_NOT_AVAIL 0
+#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_AVAIL 1
+#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2 KVM_REG_ARM_FW_REG(2)
+#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_AVAIL 0
+#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_UNKNOWN 1
+#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_AVAIL 2
+#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_UNAFFECTED 3
+#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_ENABLED (1U << 4)
/* Device Control API: ARM VGIC */
#define KVM_DEV_ARM_VGIC_GRP_ADDR 0
diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c
index 9b73d3ad918a..97d2d13756f6 100644
--- a/virt/kvm/arm/psci.c
+++ b/virt/kvm/arm/psci.c
@@ -445,42 +445,97 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
int kvm_arm_get_fw_num_regs(struct kvm_vcpu *vcpu)
{
- return 1; /* PSCI version */
+ return 3; /* PSCI version and two workaround registers */
}
int kvm_arm_copy_fw_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
{
- if (put_user(KVM_REG_ARM_PSCI_VERSION, uindices))
+ if (put_user(KVM_REG_ARM_PSCI_VERSION, uindices++))
return -EFAULT;
+ if (put_user(KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1, uindices++))
+ return -EFAULT;
+
+ if (put_user(KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2, uindices++))
+ return -EFAULT;
+
+ return 0;
+}
+
+#define KVM_REG_FEATURE_LEVEL_WIDTH 4
+#define KVM_REG_FEATURE_LEVEL_MASK (BIT(KVM_REG_FEATURE_LEVEL_WIDTH) - 1)
+
+/*
+ * Convert the workaround level into an easy-to-compare number, where higher
+ * values mean better protection.
+ */
+static int get_kernel_wa_level(u64 regid)
+{
+ switch (regid) {
+ case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1:
+ if (kvm_arm_harden_branch_predictor())
+ return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_AVAIL;
+ else
+ return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_NOT_AVAIL;
+ case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2:
+ switch (kvm_arm_have_ssbd()) {
+ case KVM_SSBD_FORCE_DISABLE:
+ return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_AVAIL;
+ case KVM_SSBD_KERNEL:
+ return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_AVAIL;
+ case KVM_SSBD_FORCE_ENABLE:
+ case KVM_SSBD_MITIGATED:
+ return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_UNAFFECTED;
+ case KVM_SSBD_UNKNOWN:
+ default:
+ return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_UNKNOWN;
+ }
+ }
+
return 0;
}
int kvm_arm_get_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
{
- if (reg->id == KVM_REG_ARM_PSCI_VERSION) {
- void __user *uaddr = (void __user *)(long)reg->addr;
- u64 val;
+ void __user *uaddr = (void __user *)(long)reg->addr;
+ u64 val;
+ switch (reg->id) {
+ case KVM_REG_ARM_PSCI_VERSION:
val = kvm_psci_version(vcpu, vcpu->kvm);
- if (copy_to_user(uaddr, &val, KVM_REG_SIZE(reg->id)))
- return -EFAULT;
-
- return 0;
+ break;
+ case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1:
+ val = get_kernel_wa_level(reg->id) & KVM_REG_FEATURE_LEVEL_MASK;
+ break;
+ case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2:
+ val = get_kernel_wa_level(reg->id) & KVM_REG_FEATURE_LEVEL_MASK;
+ if (kvm_arm_have_ssbd() == KVM_SSBD_KERNEL &&
+ kvm_arm_get_vcpu_workaround_2_flag(vcpu))
+ val |= KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_ENABLED;
+ break;
+ default:
+ return -EINVAL;
}
- return -EINVAL;
+ if (copy_to_user(uaddr, &val, KVM_REG_SIZE(reg->id)))
+ return -EFAULT;
+
+ return 0;
}
int kvm_arm_set_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
{
- if (reg->id == KVM_REG_ARM_PSCI_VERSION) {
- void __user *uaddr = (void __user *)(long)reg->addr;
- bool wants_02;
- u64 val;
+ void __user *uaddr = (void __user *)(long)reg->addr;
+ u64 val;
+ int wa_level;
+
+ if (copy_from_user(&val, uaddr, KVM_REG_SIZE(reg->id)))
+ return -EFAULT;
- if (copy_from_user(&val, uaddr, KVM_REG_SIZE(reg->id)))
- return -EFAULT;
+ switch (reg->id) {
+ case KVM_REG_ARM_PSCI_VERSION:
+ {
+ bool wants_02;
wants_02 = test_bit(KVM_ARM_VCPU_PSCI_0_2, vcpu->arch.features);
@@ -497,6 +552,38 @@ int kvm_arm_set_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
vcpu->kvm->arch.psci_version = val;
return 0;
}
+ break;
+ }
+
+ case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1:
+ wa_level = val & KVM_REG_FEATURE_LEVEL_MASK;
+
+ /* For now we only accept the very same workaround level. */
+ if (get_kernel_wa_level(reg->id) != wa_level)
+ return -EINVAL;
+
+ return 0;
+
+ case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2:
+ wa_level = val & KVM_REG_FEATURE_LEVEL_MASK;
+
+ if (get_kernel_wa_level(reg->id) < wa_level)
+ return -EINVAL;
+
+ if (kvm_arm_have_ssbd() != KVM_SSBD_KERNEL)
+ return 0;
+
+ switch (wa_level) {
+ case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_AVAIL:
+ kvm_arm_set_vcpu_workaround_2_flag(vcpu,
+ val & KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_ENABLED);
+ break;
+ case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_UNAFFECTED:
+ kvm_arm_set_vcpu_workaround_2_flag(vcpu, true);
+ break;
+ }
+
+ return 0;
}
return -EINVAL;
--
2.17.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v3 2/2] KVM: doc: add API documentation on the KVM_REG_ARM_WORKAROUNDS register
2019-02-22 12:18 ` Andre Przywara
@ 2019-02-22 12:18 ` Andre Przywara
-1 siblings, 0 replies; 20+ messages in thread
From: Andre Przywara @ 2019-02-22 12:18 UTC (permalink / raw)
To: Marc Zyngier, Christoffer Dall
Cc: linux-arm-kernel, Steven Price, kvmarm, kvm, Dave Martin
Add documentation for the newly defined firmware registers to save and
restore any vulnerability migitation status.
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
Documentation/virtual/kvm/arm/psci.txt | 24 ++++++++++++++++++++++++
1 file changed, 24 insertions(+)
diff --git a/Documentation/virtual/kvm/arm/psci.txt b/Documentation/virtual/kvm/arm/psci.txt
index aafdab887b04..fe8930765cc7 100644
--- a/Documentation/virtual/kvm/arm/psci.txt
+++ b/Documentation/virtual/kvm/arm/psci.txt
@@ -28,3 +28,27 @@ The following register is defined:
- Allows any PSCI version implemented by KVM and compatible with
v0.2 to be set with SET_ONE_REG
- Affects the whole VM (even if the register view is per-vcpu)
+
+* KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1:
+ Holds the state of the firmware controlled workaround to mitigate
+ CVE-2017-5715, as described under SMCCC_ARCH_WORKAROUND_1 in [1].
+ Accepted values are:
+ KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_NOT_AVAIL: Workaround not available.
+ The mitigation status for the guest is unknown.
+ KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_AVAIL: Workaround active for the guest.
+ KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_UNAFFECTED: The workaround is not
+ available and not needed.
+
+* KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2:
+ Holds the state of the firmware controlled workaround to mitigate
+ CVE-2018-3639, as described under SMCCC_ARCH_WORKAROUND_2 in [1].
+ Accepted values are:
+ KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_AVAIL: Workaround not available.
+ KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_UNKNOWN: Workaround state unknown.
+ KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_AVAIL: Workaround available, and can
+ be disabled by a vCPU. If KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_ENABLED is
+ set, it is active for this vCPU.
+ KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_UNAFFECTED: Workaround always active
+ or not needed.
+
+[1] https://developer.arm.com/-/media/developer/pdf/ARM_DEN_0070A_Firmware_interfaces_for_mitigating_CVE-2017-5715.pdf
--
2.17.1
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v3 2/2] KVM: doc: add API documentation on the KVM_REG_ARM_WORKAROUNDS register
@ 2019-02-22 12:18 ` Andre Przywara
0 siblings, 0 replies; 20+ messages in thread
From: Andre Przywara @ 2019-02-22 12:18 UTC (permalink / raw)
To: Marc Zyngier, Christoffer Dall
Cc: linux-arm-kernel, Steven Price, kvmarm, kvm, Dave Martin
Add documentation for the newly defined firmware registers to save and
restore any vulnerability migitation status.
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
Documentation/virtual/kvm/arm/psci.txt | 24 ++++++++++++++++++++++++
1 file changed, 24 insertions(+)
diff --git a/Documentation/virtual/kvm/arm/psci.txt b/Documentation/virtual/kvm/arm/psci.txt
index aafdab887b04..fe8930765cc7 100644
--- a/Documentation/virtual/kvm/arm/psci.txt
+++ b/Documentation/virtual/kvm/arm/psci.txt
@@ -28,3 +28,27 @@ The following register is defined:
- Allows any PSCI version implemented by KVM and compatible with
v0.2 to be set with SET_ONE_REG
- Affects the whole VM (even if the register view is per-vcpu)
+
+* KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1:
+ Holds the state of the firmware controlled workaround to mitigate
+ CVE-2017-5715, as described under SMCCC_ARCH_WORKAROUND_1 in [1].
+ Accepted values are:
+ KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_NOT_AVAIL: Workaround not available.
+ The mitigation status for the guest is unknown.
+ KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_AVAIL: Workaround active for the guest.
+ KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_UNAFFECTED: The workaround is not
+ available and not needed.
+
+* KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2:
+ Holds the state of the firmware controlled workaround to mitigate
+ CVE-2018-3639, as described under SMCCC_ARCH_WORKAROUND_2 in [1].
+ Accepted values are:
+ KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_AVAIL: Workaround not available.
+ KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_UNKNOWN: Workaround state unknown.
+ KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_AVAIL: Workaround available, and can
+ be disabled by a vCPU. If KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_ENABLED is
+ set, it is active for this vCPU.
+ KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_UNAFFECTED: Workaround always active
+ or not needed.
+
+[1] https://developer.arm.com/-/media/developer/pdf/ARM_DEN_0070A_Firmware_interfaces_for_mitigating_CVE-2017-5715.pdf
--
2.17.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v3 1/2] KVM: arm/arm64: Add save/restore support for firmware workaround state
2019-02-22 12:18 ` Andre Przywara
@ 2019-02-22 17:16 ` Steven Price
-1 siblings, 0 replies; 20+ messages in thread
From: Steven Price @ 2019-02-22 17:16 UTC (permalink / raw)
To: Andre Przywara, Marc Zyngier, Christoffer Dall
Cc: Dave Martin, kvmarm, linux-arm-kernel, kvm
On 22/02/2019 12:18, Andre Przywara wrote:
> KVM implements the firmware interface for mitigating cache speculation
> vulnerabilities. Guests may use this interface to ensure mitigation is
> active.
> If we want to migrate such a guest to a host with a different support
> level for those workarounds, migration might need to fail, to ensure that
> critical guests don't loose their protection.
>
> Introduce a way for userland to save and restore the workarounds state.
> On restoring we do checks that make sure we don't downgrade our
> mitigation level.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
> arch/arm/include/asm/kvm_emulate.h | 10 +++
> arch/arm/include/uapi/asm/kvm.h | 10 +++
> arch/arm64/include/asm/kvm_emulate.h | 14 ++++
> arch/arm64/include/uapi/asm/kvm.h | 9 ++
> virt/kvm/arm/psci.c | 119 +++++++++++++++++++++++----
> 5 files changed, 146 insertions(+), 16 deletions(-)
>
> diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
> index 8927cae7c966..663a02d7e6f4 100644
> --- a/arch/arm/include/asm/kvm_emulate.h
> +++ b/arch/arm/include/asm/kvm_emulate.h
> @@ -283,6 +283,16 @@ static inline unsigned long kvm_vcpu_get_mpidr_aff(struct kvm_vcpu *vcpu)
> return vcpu_cp15(vcpu, c0_MPIDR) & MPIDR_HWID_BITMASK;
> }
>
> +static inline bool kvm_arm_get_vcpu_workaround_2_flag(struct kvm_vcpu *vcpu)
> +{
> + return false;
> +}
> +
> +static inline void kvm_arm_set_vcpu_workaround_2_flag(struct kvm_vcpu *vcpu,
> + bool flag)
> +{
> +}
> +
> static inline void kvm_vcpu_set_be(struct kvm_vcpu *vcpu)
> {
> *vcpu_cpsr(vcpu) |= PSR_E_BIT;
> diff --git a/arch/arm/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h
> index 4602464ebdfb..ba4d2afe65e3 100644
> --- a/arch/arm/include/uapi/asm/kvm.h
> +++ b/arch/arm/include/uapi/asm/kvm.h
> @@ -214,6 +214,16 @@ struct kvm_vcpu_events {
> #define KVM_REG_ARM_FW_REG(r) (KVM_REG_ARM | KVM_REG_SIZE_U64 | \
> KVM_REG_ARM_FW | ((r) & 0xffff))
> #define KVM_REG_ARM_PSCI_VERSION KVM_REG_ARM_FW_REG(0)
> +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1 KVM_REG_ARM_FW_REG(1)
> +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_NOT_AVAIL 0
> +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_AVAIL 1
> +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_UNAFFECTED 2
Thanks for adding the UNAFFECTED state for WORKAROUND_1 - this means the
ABI at least can deal with migration to a host which supports but
doesn't need the workaround. I'm happy for the actual support for this
to be added later if/when it's needed.
Reviewed-by: Steven Price <steven.price@arm.com>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 1/2] KVM: arm/arm64: Add save/restore support for firmware workaround state
@ 2019-02-22 17:16 ` Steven Price
0 siblings, 0 replies; 20+ messages in thread
From: Steven Price @ 2019-02-22 17:16 UTC (permalink / raw)
To: Andre Przywara, Marc Zyngier, Christoffer Dall
Cc: Dave Martin, kvmarm, linux-arm-kernel, kvm
On 22/02/2019 12:18, Andre Przywara wrote:
> KVM implements the firmware interface for mitigating cache speculation
> vulnerabilities. Guests may use this interface to ensure mitigation is
> active.
> If we want to migrate such a guest to a host with a different support
> level for those workarounds, migration might need to fail, to ensure that
> critical guests don't loose their protection.
>
> Introduce a way for userland to save and restore the workarounds state.
> On restoring we do checks that make sure we don't downgrade our
> mitigation level.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
> arch/arm/include/asm/kvm_emulate.h | 10 +++
> arch/arm/include/uapi/asm/kvm.h | 10 +++
> arch/arm64/include/asm/kvm_emulate.h | 14 ++++
> arch/arm64/include/uapi/asm/kvm.h | 9 ++
> virt/kvm/arm/psci.c | 119 +++++++++++++++++++++++----
> 5 files changed, 146 insertions(+), 16 deletions(-)
>
> diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
> index 8927cae7c966..663a02d7e6f4 100644
> --- a/arch/arm/include/asm/kvm_emulate.h
> +++ b/arch/arm/include/asm/kvm_emulate.h
> @@ -283,6 +283,16 @@ static inline unsigned long kvm_vcpu_get_mpidr_aff(struct kvm_vcpu *vcpu)
> return vcpu_cp15(vcpu, c0_MPIDR) & MPIDR_HWID_BITMASK;
> }
>
> +static inline bool kvm_arm_get_vcpu_workaround_2_flag(struct kvm_vcpu *vcpu)
> +{
> + return false;
> +}
> +
> +static inline void kvm_arm_set_vcpu_workaround_2_flag(struct kvm_vcpu *vcpu,
> + bool flag)
> +{
> +}
> +
> static inline void kvm_vcpu_set_be(struct kvm_vcpu *vcpu)
> {
> *vcpu_cpsr(vcpu) |= PSR_E_BIT;
> diff --git a/arch/arm/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h
> index 4602464ebdfb..ba4d2afe65e3 100644
> --- a/arch/arm/include/uapi/asm/kvm.h
> +++ b/arch/arm/include/uapi/asm/kvm.h
> @@ -214,6 +214,16 @@ struct kvm_vcpu_events {
> #define KVM_REG_ARM_FW_REG(r) (KVM_REG_ARM | KVM_REG_SIZE_U64 | \
> KVM_REG_ARM_FW | ((r) & 0xffff))
> #define KVM_REG_ARM_PSCI_VERSION KVM_REG_ARM_FW_REG(0)
> +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1 KVM_REG_ARM_FW_REG(1)
> +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_NOT_AVAIL 0
> +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_AVAIL 1
> +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_UNAFFECTED 2
Thanks for adding the UNAFFECTED state for WORKAROUND_1 - this means the
ABI at least can deal with migration to a host which supports but
doesn't need the workaround. I'm happy for the actual support for this
to be added later if/when it's needed.
Reviewed-by: Steven Price <steven.price@arm.com>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 2/2] KVM: doc: add API documentation on the KVM_REG_ARM_WORKAROUNDS register
2019-02-22 12:18 ` Andre Przywara
@ 2019-02-22 17:16 ` Steven Price
-1 siblings, 0 replies; 20+ messages in thread
From: Steven Price @ 2019-02-22 17:16 UTC (permalink / raw)
To: Andre Przywara, Marc Zyngier, Christoffer Dall
Cc: Dave Martin, kvmarm, linux-arm-kernel, kvm
On 22/02/2019 12:18, Andre Przywara wrote:
> Add documentation for the newly defined firmware registers to save and
> restore any vulnerability migitation status.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
> Documentation/virtual/kvm/arm/psci.txt | 24 ++++++++++++++++++++++++
> 1 file changed, 24 insertions(+)
>
> diff --git a/Documentation/virtual/kvm/arm/psci.txt b/Documentation/virtual/kvm/arm/psci.txt
> index aafdab887b04..fe8930765cc7 100644
> --- a/Documentation/virtual/kvm/arm/psci.txt
> +++ b/Documentation/virtual/kvm/arm/psci.txt
> @@ -28,3 +28,27 @@ The following register is defined:
> - Allows any PSCI version implemented by KVM and compatible with
> v0.2 to be set with SET_ONE_REG
> - Affects the whole VM (even if the register view is per-vcpu)
> +
> +* KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1:
> + Holds the state of the firmware controlled workaround to mitigate
> + CVE-2017-5715, as described under SMCCC_ARCH_WORKAROUND_1 in [1].
> + Accepted values are:
> + KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_NOT_AVAIL: Workaround not available.
> + The mitigation status for the guest is unknown.
> + KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_AVAIL: Workaround active for the guest.
> + KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_UNAFFECTED: The workaround is not
> + available and not needed.
This definition of "unaffected" doesn't match the definition in ARM
DEN-0070A - specifically a value of "1" means:
SMCCC_ARCH_WORKAROUND_1 can be invoked safely on all PEs in the
system.
The PE on which SMCCC_ARCH_FEATURES is called does not require
firmware mitigation for CVE-2017-5715.
i.e. the workaround *is* available - just not needed.
This difference is important if/when we allow migration from an "AVAIL"
host to an "UNAFFECTED" host - the workaround calls can still be made
even though they are redundant.
Steve
> +
> +* KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2:
> + Holds the state of the firmware controlled workaround to mitigate
> + CVE-2018-3639, as described under SMCCC_ARCH_WORKAROUND_2 in [1].
> + Accepted values are:
> + KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_AVAIL: Workaround not available.
> + KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_UNKNOWN: Workaround state unknown.
> + KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_AVAIL: Workaround available, and can
> + be disabled by a vCPU. If KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_ENABLED is
> + set, it is active for this vCPU.
> + KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_UNAFFECTED: Workaround always active
> + or not needed.
> +
> +[1] https://developer.arm.com/-/media/developer/pdf/ARM_DEN_0070A_Firmware_interfaces_for_mitigating_CVE-2017-5715.pdf
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 2/2] KVM: doc: add API documentation on the KVM_REG_ARM_WORKAROUNDS register
@ 2019-02-22 17:16 ` Steven Price
0 siblings, 0 replies; 20+ messages in thread
From: Steven Price @ 2019-02-22 17:16 UTC (permalink / raw)
To: Andre Przywara, Marc Zyngier, Christoffer Dall
Cc: Dave Martin, kvmarm, linux-arm-kernel, kvm
On 22/02/2019 12:18, Andre Przywara wrote:
> Add documentation for the newly defined firmware registers to save and
> restore any vulnerability migitation status.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
> Documentation/virtual/kvm/arm/psci.txt | 24 ++++++++++++++++++++++++
> 1 file changed, 24 insertions(+)
>
> diff --git a/Documentation/virtual/kvm/arm/psci.txt b/Documentation/virtual/kvm/arm/psci.txt
> index aafdab887b04..fe8930765cc7 100644
> --- a/Documentation/virtual/kvm/arm/psci.txt
> +++ b/Documentation/virtual/kvm/arm/psci.txt
> @@ -28,3 +28,27 @@ The following register is defined:
> - Allows any PSCI version implemented by KVM and compatible with
> v0.2 to be set with SET_ONE_REG
> - Affects the whole VM (even if the register view is per-vcpu)
> +
> +* KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1:
> + Holds the state of the firmware controlled workaround to mitigate
> + CVE-2017-5715, as described under SMCCC_ARCH_WORKAROUND_1 in [1].
> + Accepted values are:
> + KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_NOT_AVAIL: Workaround not available.
> + The mitigation status for the guest is unknown.
> + KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_AVAIL: Workaround active for the guest.
> + KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_UNAFFECTED: The workaround is not
> + available and not needed.
This definition of "unaffected" doesn't match the definition in ARM
DEN-0070A - specifically a value of "1" means:
SMCCC_ARCH_WORKAROUND_1 can be invoked safely on all PEs in the
system.
The PE on which SMCCC_ARCH_FEATURES is called does not require
firmware mitigation for CVE-2017-5715.
i.e. the workaround *is* available - just not needed.
This difference is important if/when we allow migration from an "AVAIL"
host to an "UNAFFECTED" host - the workaround calls can still be made
even though they are redundant.
Steve
> +
> +* KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2:
> + Holds the state of the firmware controlled workaround to mitigate
> + CVE-2018-3639, as described under SMCCC_ARCH_WORKAROUND_2 in [1].
> + Accepted values are:
> + KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_AVAIL: Workaround not available.
> + KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_UNKNOWN: Workaround state unknown.
> + KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_AVAIL: Workaround available, and can
> + be disabled by a vCPU. If KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_ENABLED is
> + set, it is active for this vCPU.
> + KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_UNAFFECTED: Workaround always active
> + or not needed.
> +
> +[1] https://developer.arm.com/-/media/developer/pdf/ARM_DEN_0070A_Firmware_interfaces_for_mitigating_CVE-2017-5715.pdf
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 1/2] KVM: arm/arm64: Add save/restore support for firmware workaround state
2019-02-22 12:18 ` Andre Przywara
@ 2019-02-22 17:22 ` Dave Martin
-1 siblings, 0 replies; 20+ messages in thread
From: Dave Martin @ 2019-02-22 17:22 UTC (permalink / raw)
To: Andre Przywara; +Cc: kvm, Marc Zyngier, Steven Price, kvmarm, linux-arm-kernel
On Fri, Feb 22, 2019 at 12:18:17PM +0000, Andre Przywara wrote:
> KVM implements the firmware interface for mitigating cache speculation
> vulnerabilities. Guests may use this interface to ensure mitigation is
> active.
> If we want to migrate such a guest to a host with a different support
> level for those workarounds, migration might need to fail, to ensure that
> critical guests don't loose their protection.
>
> Introduce a way for userland to save and restore the workarounds state.
> On restoring we do checks that make sure we don't downgrade our
> mitigation level.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
> arch/arm/include/asm/kvm_emulate.h | 10 +++
> arch/arm/include/uapi/asm/kvm.h | 10 +++
> arch/arm64/include/asm/kvm_emulate.h | 14 ++++
> arch/arm64/include/uapi/asm/kvm.h | 9 ++
> virt/kvm/arm/psci.c | 119 +++++++++++++++++++++++----
> 5 files changed, 146 insertions(+), 16 deletions(-)
>
> diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
> index 8927cae7c966..663a02d7e6f4 100644
> --- a/arch/arm/include/asm/kvm_emulate.h
> +++ b/arch/arm/include/asm/kvm_emulate.h
> @@ -283,6 +283,16 @@ static inline unsigned long kvm_vcpu_get_mpidr_aff(struct kvm_vcpu *vcpu)
> return vcpu_cp15(vcpu, c0_MPIDR) & MPIDR_HWID_BITMASK;
> }
>
> +static inline bool kvm_arm_get_vcpu_workaround_2_flag(struct kvm_vcpu *vcpu)
> +{
> + return false;
> +}
> +
> +static inline void kvm_arm_set_vcpu_workaround_2_flag(struct kvm_vcpu *vcpu,
> + bool flag)
> +{
> +}
> +
> static inline void kvm_vcpu_set_be(struct kvm_vcpu *vcpu)
> {
> *vcpu_cpsr(vcpu) |= PSR_E_BIT;
> diff --git a/arch/arm/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h
> index 4602464ebdfb..ba4d2afe65e3 100644
> --- a/arch/arm/include/uapi/asm/kvm.h
> +++ b/arch/arm/include/uapi/asm/kvm.h
> @@ -214,6 +214,16 @@ struct kvm_vcpu_events {
> #define KVM_REG_ARM_FW_REG(r) (KVM_REG_ARM | KVM_REG_SIZE_U64 | \
> KVM_REG_ARM_FW | ((r) & 0xffff))
> #define KVM_REG_ARM_PSCI_VERSION KVM_REG_ARM_FW_REG(0)
> +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1 KVM_REG_ARM_FW_REG(1)
> +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_NOT_AVAIL 0
> +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_AVAIL 1
> +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_UNAFFECTED 2
> +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2 KVM_REG_ARM_FW_REG(2)
> +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_AVAIL 0
> +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_UNKNOWN 1
> +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_AVAIL 2
> +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_UNAFFECTED 3
> +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_ENABLED (1U << 4)
Why 4? How could the system be less affected than "UNAFFECTED"? (And
if not, why do we need space to insert extra values?) Mixing read-only
system-description info with mutable guest state in this register feels
a bit odd, but we probably don't win much by making it a separate
register either.
Possibly we could drop the _REG from the #defines that are not reg IDs,
since KVM_REG_* are all reg IDs today.
But it works either way.
Without a clear definition of what these values mean, I worry about ABI
drift. But if we don't expect these to evolve it's probably low-risk.
> /* Device Control API: ARM VGIC */
> #define KVM_DEV_ARM_VGIC_GRP_ADDR 0
> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> index d3842791e1c4..c00c17c9adb6 100644
> --- a/arch/arm64/include/asm/kvm_emulate.h
> +++ b/arch/arm64/include/asm/kvm_emulate.h
> @@ -348,6 +348,20 @@ static inline unsigned long kvm_vcpu_get_mpidr_aff(struct kvm_vcpu *vcpu)
> return vcpu_read_sys_reg(vcpu, MPIDR_EL1) & MPIDR_HWID_BITMASK;
> }
>
> +static inline bool kvm_arm_get_vcpu_workaround_2_flag(struct kvm_vcpu *vcpu)
> +{
> + return vcpu->arch.workaround_flags & VCPU_WORKAROUND_2_FLAG;
> +}
> +
> +static inline void kvm_arm_set_vcpu_workaround_2_flag(struct kvm_vcpu *vcpu,
> + bool flag)
> +{
> + if (flag)
> + vcpu->arch.workaround_flags |= VCPU_WORKAROUND_2_FLAG;
> + else
> + vcpu->arch.workaround_flags &= ~VCPU_WORKAROUND_2_FLAG;
> +}
> +
> static inline void kvm_vcpu_set_be(struct kvm_vcpu *vcpu)
> {
> if (vcpu_mode_is_32bit(vcpu)) {
> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> index 97c3478ee6e7..367e96fe654e 100644
> --- a/arch/arm64/include/uapi/asm/kvm.h
> +++ b/arch/arm64/include/uapi/asm/kvm.h
> @@ -225,6 +225,15 @@ struct kvm_vcpu_events {
> #define KVM_REG_ARM_FW_REG(r) (KVM_REG_ARM64 | KVM_REG_SIZE_U64 | \
> KVM_REG_ARM_FW | ((r) & 0xffff))
> #define KVM_REG_ARM_PSCI_VERSION KVM_REG_ARM_FW_REG(0)
> +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1 KVM_REG_ARM_FW_REG(1)
> +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_NOT_AVAIL 0
> +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_AVAIL 1
> +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2 KVM_REG_ARM_FW_REG(2)
> +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_AVAIL 0
> +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_UNKNOWN 1
> +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_AVAIL 2
> +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_UNAFFECTED 3
> +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_ENABLED (1U << 4)
>
> /* Device Control API: ARM VGIC */
> #define KVM_DEV_ARM_VGIC_GRP_ADDR 0
> diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c
> index 9b73d3ad918a..97d2d13756f6 100644
> --- a/virt/kvm/arm/psci.c
> +++ b/virt/kvm/arm/psci.c
> @@ -445,42 +445,97 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
>
> int kvm_arm_get_fw_num_regs(struct kvm_vcpu *vcpu)
> {
> - return 1; /* PSCI version */
> + return 3; /* PSCI version and two workaround registers */
Meh. But this is no worse than the way we do it elsewhere.
> }
>
> int kvm_arm_copy_fw_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
> {
> - if (put_user(KVM_REG_ARM_PSCI_VERSION, uindices))
> + if (put_user(KVM_REG_ARM_PSCI_VERSION, uindices++))
> return -EFAULT;
>
> + if (put_user(KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1, uindices++))
> + return -EFAULT;
> +
> + if (put_user(KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2, uindices++))
> + return -EFAULT;
> +
> + return 0;
> +}
> +
> +#define KVM_REG_FEATURE_LEVEL_WIDTH 4
> +#define KVM_REG_FEATURE_LEVEL_MASK (BIT(KVM_REG_FEATURE_LEVEL_WIDTH) - 1)
Nit: Maybe use GENMASK?
> +
> +/*
> + * Convert the workaround level into an easy-to-compare number, where higher
> + * values mean better protection.
> + */
> +static int get_kernel_wa_level(u64 regid)
> +{
> + switch (regid) {
> + case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1:
> + if (kvm_arm_harden_branch_predictor())
> + return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_AVAIL;
> + else
> + return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_NOT_AVAIL;
> + case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2:
> + switch (kvm_arm_have_ssbd()) {
> + case KVM_SSBD_FORCE_DISABLE:
> + return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_AVAIL;
> + case KVM_SSBD_KERNEL:
> + return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_AVAIL;
> + case KVM_SSBD_FORCE_ENABLE:
> + case KVM_SSBD_MITIGATED:
> + return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_UNAFFECTED;
> + case KVM_SSBD_UNKNOWN:
> + default:
> + return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_UNKNOWN;
> + }
> + }
> +
> return 0;
> }
>
> int kvm_arm_get_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> {
> - if (reg->id == KVM_REG_ARM_PSCI_VERSION) {
> - void __user *uaddr = (void __user *)(long)reg->addr;
> - u64 val;
> + void __user *uaddr = (void __user *)(long)reg->addr;
(Why (long), I wonder? Anyway, no bother.)
> + u64 val;
>
> + switch (reg->id) {
> + case KVM_REG_ARM_PSCI_VERSION:
> val = kvm_psci_version(vcpu, vcpu->kvm);
> - if (copy_to_user(uaddr, &val, KVM_REG_SIZE(reg->id)))
> - return -EFAULT;
> -
> - return 0;
> + break;
> + case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1:
> + val = get_kernel_wa_level(reg->id) & KVM_REG_FEATURE_LEVEL_MASK;
> + break;
> + case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2:
> + val = get_kernel_wa_level(reg->id) & KVM_REG_FEATURE_LEVEL_MASK;
> + if (kvm_arm_have_ssbd() == KVM_SSBD_KERNEL &&
> + kvm_arm_get_vcpu_workaround_2_flag(vcpu))
> + val |= KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_ENABLED;
> + break;
> + default:
> + return -EINVAL;
Hmmm, it could be more reasonable to return -ENOENT. But this precedent
may be established now.
Anyway, userspace is unlikely to make any useful distinction between
these two errors.
> }
>
> - return -EINVAL;
You could try a BUILD_BUG_ON(KVM_REG_SIZE(reg->id) == sizeof(val)).
I suspect gcc may not be quite bright enough to spot that condition is
compiletime-constant, but if it does then so much the better.
Not sure it's worth a runtime check though.
> + if (copy_to_user(uaddr, &val, KVM_REG_SIZE(reg->id)))
> + return -EFAULT;
> +
> + return 0;
> }
>
> int kvm_arm_set_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> {
> - if (reg->id == KVM_REG_ARM_PSCI_VERSION) {
> - void __user *uaddr = (void __user *)(long)reg->addr;
> - bool wants_02;
> - u64 val;
> + void __user *uaddr = (void __user *)(long)reg->addr;
> + u64 val;
> + int wa_level;
> +
> + if (copy_from_user(&val, uaddr, KVM_REG_SIZE(reg->id)))
> + return -EFAULT;
>
> - if (copy_from_user(&val, uaddr, KVM_REG_SIZE(reg->id)))
> - return -EFAULT;
> + switch (reg->id) {
> + case KVM_REG_ARM_PSCI_VERSION:
> + {
> + bool wants_02;
>
> wants_02 = test_bit(KVM_ARM_VCPU_PSCI_0_2, vcpu->arch.features);
>
> @@ -497,6 +552,38 @@ int kvm_arm_set_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> vcpu->kvm->arch.psci_version = val;
> return 0;
> }
> + break;
> + }
> +
> + case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1:
> + wa_level = val & KVM_REG_FEATURE_LEVEL_MASK;
Should we require the other bits to be zero?
Providing we never add anything else to this register we will get away
with it, but it would be cleaner to police the extra bits here and in
similar places.
> +
> + /* For now we only accept the very same workaround level. */
> + if (get_kernel_wa_level(reg->id) != wa_level)
> + return -EINVAL;
> +
> + return 0;
> +
> + case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2:
> + wa_level = val & KVM_REG_FEATURE_LEVEL_MASK;
Ditto...
> +
> + if (get_kernel_wa_level(reg->id) < wa_level)
> + return -EINVAL;
Is the SMCCC_ARCH_WORKAROUND_2 interface definitely always available to
the guest even when in the get_kernel_wa_level() == _UNAFFECTED case?
With wa_level == _AVAIL, the guest assumes that the interface is there.
(This may be fine; I'm just not so aware of the history.)
> +
> + if (kvm_arm_have_ssbd() != KVM_SSBD_KERNEL)
> + return 0;
(val & KVM_REG_FEATURE_LEVEL_MASK) could still be _NOT_AVAIL or _UNKNOWN
here, yet we still attempt to call kvm_arm_set_vcpu_workaround_2_flag()
based on the user's KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_ENABLED bit.
Should we? I'm not clear on what the sematics should be for this case.
> +
> + switch (wa_level) {
> + case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_AVAIL:
> + kvm_arm_set_vcpu_workaround_2_flag(vcpu,
> + val & KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_ENABLED);
> + break;
> + case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_UNAFFECTED:
> + kvm_arm_set_vcpu_workaround_2_flag(vcpu, true);
> + break;
> + }
This has the odd effect that userspace can bypass PSCI/SMCCC and twiddle
this bit directly via KVM_SET_ONE_REG. I'm not sure this matters, and
it may even be useful. A bit unexpected though. Are there any pitfalls
here I've not thought of?
> +
> + return 0;
The logic overall feels a bit fragile and arbitrary, though given what
it is describing, we may not be able to do a whole lot better.
Do we trust the user-supplied val more than we should?
[...]
Overall, the code looks reasonable; my comments are pretty much nits.
Cheers
---Dave
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 1/2] KVM: arm/arm64: Add save/restore support for firmware workaround state
@ 2019-02-22 17:22 ` Dave Martin
0 siblings, 0 replies; 20+ messages in thread
From: Dave Martin @ 2019-02-22 17:22 UTC (permalink / raw)
To: Andre Przywara
Cc: kvm, Marc Zyngier, Christoffer Dall, Steven Price, kvmarm,
linux-arm-kernel
On Fri, Feb 22, 2019 at 12:18:17PM +0000, Andre Przywara wrote:
> KVM implements the firmware interface for mitigating cache speculation
> vulnerabilities. Guests may use this interface to ensure mitigation is
> active.
> If we want to migrate such a guest to a host with a different support
> level for those workarounds, migration might need to fail, to ensure that
> critical guests don't loose their protection.
>
> Introduce a way for userland to save and restore the workarounds state.
> On restoring we do checks that make sure we don't downgrade our
> mitigation level.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
> arch/arm/include/asm/kvm_emulate.h | 10 +++
> arch/arm/include/uapi/asm/kvm.h | 10 +++
> arch/arm64/include/asm/kvm_emulate.h | 14 ++++
> arch/arm64/include/uapi/asm/kvm.h | 9 ++
> virt/kvm/arm/psci.c | 119 +++++++++++++++++++++++----
> 5 files changed, 146 insertions(+), 16 deletions(-)
>
> diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
> index 8927cae7c966..663a02d7e6f4 100644
> --- a/arch/arm/include/asm/kvm_emulate.h
> +++ b/arch/arm/include/asm/kvm_emulate.h
> @@ -283,6 +283,16 @@ static inline unsigned long kvm_vcpu_get_mpidr_aff(struct kvm_vcpu *vcpu)
> return vcpu_cp15(vcpu, c0_MPIDR) & MPIDR_HWID_BITMASK;
> }
>
> +static inline bool kvm_arm_get_vcpu_workaround_2_flag(struct kvm_vcpu *vcpu)
> +{
> + return false;
> +}
> +
> +static inline void kvm_arm_set_vcpu_workaround_2_flag(struct kvm_vcpu *vcpu,
> + bool flag)
> +{
> +}
> +
> static inline void kvm_vcpu_set_be(struct kvm_vcpu *vcpu)
> {
> *vcpu_cpsr(vcpu) |= PSR_E_BIT;
> diff --git a/arch/arm/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h
> index 4602464ebdfb..ba4d2afe65e3 100644
> --- a/arch/arm/include/uapi/asm/kvm.h
> +++ b/arch/arm/include/uapi/asm/kvm.h
> @@ -214,6 +214,16 @@ struct kvm_vcpu_events {
> #define KVM_REG_ARM_FW_REG(r) (KVM_REG_ARM | KVM_REG_SIZE_U64 | \
> KVM_REG_ARM_FW | ((r) & 0xffff))
> #define KVM_REG_ARM_PSCI_VERSION KVM_REG_ARM_FW_REG(0)
> +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1 KVM_REG_ARM_FW_REG(1)
> +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_NOT_AVAIL 0
> +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_AVAIL 1
> +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_UNAFFECTED 2
> +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2 KVM_REG_ARM_FW_REG(2)
> +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_AVAIL 0
> +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_UNKNOWN 1
> +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_AVAIL 2
> +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_UNAFFECTED 3
> +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_ENABLED (1U << 4)
Why 4? How could the system be less affected than "UNAFFECTED"? (And
if not, why do we need space to insert extra values?) Mixing read-only
system-description info with mutable guest state in this register feels
a bit odd, but we probably don't win much by making it a separate
register either.
Possibly we could drop the _REG from the #defines that are not reg IDs,
since KVM_REG_* are all reg IDs today.
But it works either way.
Without a clear definition of what these values mean, I worry about ABI
drift. But if we don't expect these to evolve it's probably low-risk.
> /* Device Control API: ARM VGIC */
> #define KVM_DEV_ARM_VGIC_GRP_ADDR 0
> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> index d3842791e1c4..c00c17c9adb6 100644
> --- a/arch/arm64/include/asm/kvm_emulate.h
> +++ b/arch/arm64/include/asm/kvm_emulate.h
> @@ -348,6 +348,20 @@ static inline unsigned long kvm_vcpu_get_mpidr_aff(struct kvm_vcpu *vcpu)
> return vcpu_read_sys_reg(vcpu, MPIDR_EL1) & MPIDR_HWID_BITMASK;
> }
>
> +static inline bool kvm_arm_get_vcpu_workaround_2_flag(struct kvm_vcpu *vcpu)
> +{
> + return vcpu->arch.workaround_flags & VCPU_WORKAROUND_2_FLAG;
> +}
> +
> +static inline void kvm_arm_set_vcpu_workaround_2_flag(struct kvm_vcpu *vcpu,
> + bool flag)
> +{
> + if (flag)
> + vcpu->arch.workaround_flags |= VCPU_WORKAROUND_2_FLAG;
> + else
> + vcpu->arch.workaround_flags &= ~VCPU_WORKAROUND_2_FLAG;
> +}
> +
> static inline void kvm_vcpu_set_be(struct kvm_vcpu *vcpu)
> {
> if (vcpu_mode_is_32bit(vcpu)) {
> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> index 97c3478ee6e7..367e96fe654e 100644
> --- a/arch/arm64/include/uapi/asm/kvm.h
> +++ b/arch/arm64/include/uapi/asm/kvm.h
> @@ -225,6 +225,15 @@ struct kvm_vcpu_events {
> #define KVM_REG_ARM_FW_REG(r) (KVM_REG_ARM64 | KVM_REG_SIZE_U64 | \
> KVM_REG_ARM_FW | ((r) & 0xffff))
> #define KVM_REG_ARM_PSCI_VERSION KVM_REG_ARM_FW_REG(0)
> +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1 KVM_REG_ARM_FW_REG(1)
> +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_NOT_AVAIL 0
> +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_AVAIL 1
> +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2 KVM_REG_ARM_FW_REG(2)
> +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_AVAIL 0
> +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_UNKNOWN 1
> +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_AVAIL 2
> +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_UNAFFECTED 3
> +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_ENABLED (1U << 4)
>
> /* Device Control API: ARM VGIC */
> #define KVM_DEV_ARM_VGIC_GRP_ADDR 0
> diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c
> index 9b73d3ad918a..97d2d13756f6 100644
> --- a/virt/kvm/arm/psci.c
> +++ b/virt/kvm/arm/psci.c
> @@ -445,42 +445,97 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
>
> int kvm_arm_get_fw_num_regs(struct kvm_vcpu *vcpu)
> {
> - return 1; /* PSCI version */
> + return 3; /* PSCI version and two workaround registers */
Meh. But this is no worse than the way we do it elsewhere.
> }
>
> int kvm_arm_copy_fw_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
> {
> - if (put_user(KVM_REG_ARM_PSCI_VERSION, uindices))
> + if (put_user(KVM_REG_ARM_PSCI_VERSION, uindices++))
> return -EFAULT;
>
> + if (put_user(KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1, uindices++))
> + return -EFAULT;
> +
> + if (put_user(KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2, uindices++))
> + return -EFAULT;
> +
> + return 0;
> +}
> +
> +#define KVM_REG_FEATURE_LEVEL_WIDTH 4
> +#define KVM_REG_FEATURE_LEVEL_MASK (BIT(KVM_REG_FEATURE_LEVEL_WIDTH) - 1)
Nit: Maybe use GENMASK?
> +
> +/*
> + * Convert the workaround level into an easy-to-compare number, where higher
> + * values mean better protection.
> + */
> +static int get_kernel_wa_level(u64 regid)
> +{
> + switch (regid) {
> + case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1:
> + if (kvm_arm_harden_branch_predictor())
> + return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_AVAIL;
> + else
> + return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_NOT_AVAIL;
> + case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2:
> + switch (kvm_arm_have_ssbd()) {
> + case KVM_SSBD_FORCE_DISABLE:
> + return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_AVAIL;
> + case KVM_SSBD_KERNEL:
> + return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_AVAIL;
> + case KVM_SSBD_FORCE_ENABLE:
> + case KVM_SSBD_MITIGATED:
> + return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_UNAFFECTED;
> + case KVM_SSBD_UNKNOWN:
> + default:
> + return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_UNKNOWN;
> + }
> + }
> +
> return 0;
> }
>
> int kvm_arm_get_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> {
> - if (reg->id == KVM_REG_ARM_PSCI_VERSION) {
> - void __user *uaddr = (void __user *)(long)reg->addr;
> - u64 val;
> + void __user *uaddr = (void __user *)(long)reg->addr;
(Why (long), I wonder? Anyway, no bother.)
> + u64 val;
>
> + switch (reg->id) {
> + case KVM_REG_ARM_PSCI_VERSION:
> val = kvm_psci_version(vcpu, vcpu->kvm);
> - if (copy_to_user(uaddr, &val, KVM_REG_SIZE(reg->id)))
> - return -EFAULT;
> -
> - return 0;
> + break;
> + case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1:
> + val = get_kernel_wa_level(reg->id) & KVM_REG_FEATURE_LEVEL_MASK;
> + break;
> + case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2:
> + val = get_kernel_wa_level(reg->id) & KVM_REG_FEATURE_LEVEL_MASK;
> + if (kvm_arm_have_ssbd() == KVM_SSBD_KERNEL &&
> + kvm_arm_get_vcpu_workaround_2_flag(vcpu))
> + val |= KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_ENABLED;
> + break;
> + default:
> + return -EINVAL;
Hmmm, it could be more reasonable to return -ENOENT. But this precedent
may be established now.
Anyway, userspace is unlikely to make any useful distinction between
these two errors.
> }
>
> - return -EINVAL;
You could try a BUILD_BUG_ON(KVM_REG_SIZE(reg->id) == sizeof(val)).
I suspect gcc may not be quite bright enough to spot that condition is
compiletime-constant, but if it does then so much the better.
Not sure it's worth a runtime check though.
> + if (copy_to_user(uaddr, &val, KVM_REG_SIZE(reg->id)))
> + return -EFAULT;
> +
> + return 0;
> }
>
> int kvm_arm_set_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> {
> - if (reg->id == KVM_REG_ARM_PSCI_VERSION) {
> - void __user *uaddr = (void __user *)(long)reg->addr;
> - bool wants_02;
> - u64 val;
> + void __user *uaddr = (void __user *)(long)reg->addr;
> + u64 val;
> + int wa_level;
> +
> + if (copy_from_user(&val, uaddr, KVM_REG_SIZE(reg->id)))
> + return -EFAULT;
>
> - if (copy_from_user(&val, uaddr, KVM_REG_SIZE(reg->id)))
> - return -EFAULT;
> + switch (reg->id) {
> + case KVM_REG_ARM_PSCI_VERSION:
> + {
> + bool wants_02;
>
> wants_02 = test_bit(KVM_ARM_VCPU_PSCI_0_2, vcpu->arch.features);
>
> @@ -497,6 +552,38 @@ int kvm_arm_set_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> vcpu->kvm->arch.psci_version = val;
> return 0;
> }
> + break;
> + }
> +
> + case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1:
> + wa_level = val & KVM_REG_FEATURE_LEVEL_MASK;
Should we require the other bits to be zero?
Providing we never add anything else to this register we will get away
with it, but it would be cleaner to police the extra bits here and in
similar places.
> +
> + /* For now we only accept the very same workaround level. */
> + if (get_kernel_wa_level(reg->id) != wa_level)
> + return -EINVAL;
> +
> + return 0;
> +
> + case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2:
> + wa_level = val & KVM_REG_FEATURE_LEVEL_MASK;
Ditto...
> +
> + if (get_kernel_wa_level(reg->id) < wa_level)
> + return -EINVAL;
Is the SMCCC_ARCH_WORKAROUND_2 interface definitely always available to
the guest even when in the get_kernel_wa_level() == _UNAFFECTED case?
With wa_level == _AVAIL, the guest assumes that the interface is there.
(This may be fine; I'm just not so aware of the history.)
> +
> + if (kvm_arm_have_ssbd() != KVM_SSBD_KERNEL)
> + return 0;
(val & KVM_REG_FEATURE_LEVEL_MASK) could still be _NOT_AVAIL or _UNKNOWN
here, yet we still attempt to call kvm_arm_set_vcpu_workaround_2_flag()
based on the user's KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_ENABLED bit.
Should we? I'm not clear on what the sematics should be for this case.
> +
> + switch (wa_level) {
> + case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_AVAIL:
> + kvm_arm_set_vcpu_workaround_2_flag(vcpu,
> + val & KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_ENABLED);
> + break;
> + case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_UNAFFECTED:
> + kvm_arm_set_vcpu_workaround_2_flag(vcpu, true);
> + break;
> + }
This has the odd effect that userspace can bypass PSCI/SMCCC and twiddle
this bit directly via KVM_SET_ONE_REG. I'm not sure this matters, and
it may even be useful. A bit unexpected though. Are there any pitfalls
here I've not thought of?
> +
> + return 0;
The logic overall feels a bit fragile and arbitrary, though given what
it is describing, we may not be able to do a whole lot better.
Do we trust the user-supplied val more than we should?
[...]
Overall, the code looks reasonable; my comments are pretty much nits.
Cheers
---Dave
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 2/2] KVM: doc: add API documentation on the KVM_REG_ARM_WORKAROUNDS register
2019-02-22 12:18 ` Andre Przywara
@ 2019-02-22 17:27 ` Dave Martin
-1 siblings, 0 replies; 20+ messages in thread
From: Dave Martin @ 2019-02-22 17:27 UTC (permalink / raw)
To: Andre Przywara; +Cc: kvm, Marc Zyngier, Steven Price, kvmarm, linux-arm-kernel
On Fri, Feb 22, 2019 at 12:18:18PM +0000, Andre Przywara wrote:
> Add documentation for the newly defined firmware registers to save and
> restore any vulnerability migitation status.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
> Documentation/virtual/kvm/arm/psci.txt | 24 ++++++++++++++++++++++++
> 1 file changed, 24 insertions(+)
>
> diff --git a/Documentation/virtual/kvm/arm/psci.txt b/Documentation/virtual/kvm/arm/psci.txt
> index aafdab887b04..fe8930765cc7 100644
> --- a/Documentation/virtual/kvm/arm/psci.txt
> +++ b/Documentation/virtual/kvm/arm/psci.txt
> @@ -28,3 +28,27 @@ The following register is defined:
> - Allows any PSCI version implemented by KVM and compatible with
> v0.2 to be set with SET_ONE_REG
> - Affects the whole VM (even if the register view is per-vcpu)
Doh, ignore my comments about undescribed semantics in the last patch.
> +
> +* KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1:
> + Holds the state of the firmware controlled workaround to mitigate
> + CVE-2017-5715, as described under SMCCC_ARCH_WORKAROUND_1 in [1].
> + Accepted values are:
> + KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_NOT_AVAIL: Workaround not available.
> + The mitigation status for the guest is unknown.
> + KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_AVAIL: Workaround active for the guest.
> + KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_UNAFFECTED: The workaround is not
> + available and not needed.
> +
> +* KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2:
> + Holds the state of the firmware controlled workaround to mitigate
> + CVE-2018-3639, as described under SMCCC_ARCH_WORKAROUND_2 in [1].
> + Accepted values are:
> + KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_AVAIL: Workaround not available.
> + KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_UNKNOWN: Workaround state unknown.
> + KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_AVAIL: Workaround available, and can
> + be disabled by a vCPU. If KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_ENABLED is
> + set, it is active for this vCPU.
Ah, so this is really not supposed to be set except for _AVAIL?
> + KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_UNAFFECTED: Workaround always active
> + or not needed.
But is the workaround available (i.e., SMCCC interface implemented?)
The code assumes it is, IIUC -- i.e., it will happily upgrade _AVAIL
to _UNAFFECTED. That's bad if it means the SMCCC call disappears under
the guest's feet.
(Not saying it does, just that I'm not sure.)
For KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1, we apply an == check, avoiding
this subtlety.
Cheers
---Dave
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 2/2] KVM: doc: add API documentation on the KVM_REG_ARM_WORKAROUNDS register
@ 2019-02-22 17:27 ` Dave Martin
0 siblings, 0 replies; 20+ messages in thread
From: Dave Martin @ 2019-02-22 17:27 UTC (permalink / raw)
To: Andre Przywara
Cc: kvm, Marc Zyngier, Christoffer Dall, Steven Price, kvmarm,
linux-arm-kernel
On Fri, Feb 22, 2019 at 12:18:18PM +0000, Andre Przywara wrote:
> Add documentation for the newly defined firmware registers to save and
> restore any vulnerability migitation status.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
> Documentation/virtual/kvm/arm/psci.txt | 24 ++++++++++++++++++++++++
> 1 file changed, 24 insertions(+)
>
> diff --git a/Documentation/virtual/kvm/arm/psci.txt b/Documentation/virtual/kvm/arm/psci.txt
> index aafdab887b04..fe8930765cc7 100644
> --- a/Documentation/virtual/kvm/arm/psci.txt
> +++ b/Documentation/virtual/kvm/arm/psci.txt
> @@ -28,3 +28,27 @@ The following register is defined:
> - Allows any PSCI version implemented by KVM and compatible with
> v0.2 to be set with SET_ONE_REG
> - Affects the whole VM (even if the register view is per-vcpu)
Doh, ignore my comments about undescribed semantics in the last patch.
> +
> +* KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1:
> + Holds the state of the firmware controlled workaround to mitigate
> + CVE-2017-5715, as described under SMCCC_ARCH_WORKAROUND_1 in [1].
> + Accepted values are:
> + KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_NOT_AVAIL: Workaround not available.
> + The mitigation status for the guest is unknown.
> + KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_AVAIL: Workaround active for the guest.
> + KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_UNAFFECTED: The workaround is not
> + available and not needed.
> +
> +* KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2:
> + Holds the state of the firmware controlled workaround to mitigate
> + CVE-2018-3639, as described under SMCCC_ARCH_WORKAROUND_2 in [1].
> + Accepted values are:
> + KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_AVAIL: Workaround not available.
> + KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_UNKNOWN: Workaround state unknown.
> + KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_AVAIL: Workaround available, and can
> + be disabled by a vCPU. If KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_ENABLED is
> + set, it is active for this vCPU.
Ah, so this is really not supposed to be set except for _AVAIL?
> + KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_UNAFFECTED: Workaround always active
> + or not needed.
But is the workaround available (i.e., SMCCC interface implemented?)
The code assumes it is, IIUC -- i.e., it will happily upgrade _AVAIL
to _UNAFFECTED. That's bad if it means the SMCCC call disappears under
the guest's feet.
(Not saying it does, just that I'm not sure.)
For KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1, we apply an == check, avoiding
this subtlety.
Cheers
---Dave
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 1/2] KVM: arm/arm64: Add save/restore support for firmware workaround state
2019-02-22 17:16 ` Steven Price
@ 2019-02-22 17:36 ` Andre Przywara
-1 siblings, 0 replies; 20+ messages in thread
From: Andre Przywara @ 2019-02-22 17:36 UTC (permalink / raw)
To: Steven Price; +Cc: kvm, Marc Zyngier, kvmarm, Dave Martin, linux-arm-kernel
On Fri, 22 Feb 2019 17:16:12 +0000
Steven Price <steven.price@arm.com> wrote:
> On 22/02/2019 12:18, Andre Przywara wrote:
> > KVM implements the firmware interface for mitigating cache speculation
> > vulnerabilities. Guests may use this interface to ensure mitigation is
> > active.
> > If we want to migrate such a guest to a host with a different support
> > level for those workarounds, migration might need to fail, to ensure that
> > critical guests don't loose their protection.
> >
> > Introduce a way for userland to save and restore the workarounds state.
> > On restoring we do checks that make sure we don't downgrade our
> > mitigation level.
> >
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > ---
> > arch/arm/include/asm/kvm_emulate.h | 10 +++
> > arch/arm/include/uapi/asm/kvm.h | 10 +++
> > arch/arm64/include/asm/kvm_emulate.h | 14 ++++
> > arch/arm64/include/uapi/asm/kvm.h | 9 ++
> > virt/kvm/arm/psci.c | 119 +++++++++++++++++++++++----
> > 5 files changed, 146 insertions(+), 16 deletions(-)
> >
> > diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
> > index 8927cae7c966..663a02d7e6f4 100644
> > --- a/arch/arm/include/asm/kvm_emulate.h
> > +++ b/arch/arm/include/asm/kvm_emulate.h
> > @@ -283,6 +283,16 @@ static inline unsigned long kvm_vcpu_get_mpidr_aff(struct kvm_vcpu *vcpu)
> > return vcpu_cp15(vcpu, c0_MPIDR) & MPIDR_HWID_BITMASK;
> > }
> >
> > +static inline bool kvm_arm_get_vcpu_workaround_2_flag(struct kvm_vcpu *vcpu)
> > +{
> > + return false;
> > +}
> > +
> > +static inline void kvm_arm_set_vcpu_workaround_2_flag(struct kvm_vcpu *vcpu,
> > + bool flag)
> > +{
> > +}
> > +
> > static inline void kvm_vcpu_set_be(struct kvm_vcpu *vcpu)
> > {
> > *vcpu_cpsr(vcpu) |= PSR_E_BIT;
> > diff --git a/arch/arm/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h
> > index 4602464ebdfb..ba4d2afe65e3 100644
> > --- a/arch/arm/include/uapi/asm/kvm.h
> > +++ b/arch/arm/include/uapi/asm/kvm.h
> > @@ -214,6 +214,16 @@ struct kvm_vcpu_events {
> > #define KVM_REG_ARM_FW_REG(r) (KVM_REG_ARM | KVM_REG_SIZE_U64 | \
> > KVM_REG_ARM_FW | ((r) & 0xffff))
> > #define KVM_REG_ARM_PSCI_VERSION KVM_REG_ARM_FW_REG(0)
> > +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1 KVM_REG_ARM_FW_REG(1)
> > +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_NOT_AVAIL 0
> > +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_AVAIL 1
> > +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_UNAFFECTED 2
>
> Thanks for adding the UNAFFECTED state for WORKAROUND_1 - this means the
> ABI at least can deal with migration to a host which supports but
> doesn't need the workaround. I'm happy for the actual support for this
> to be added later if/when it's needed.
Thanks. Actually we *can't* do anything right now, because the host kernel only provides this "requires w/a or not" state, so for the current kernel we will never see UNAFFECTED. If QEMU wants to set UNAFFECTED because the source kernel had it, we naturally deny it, as future code would do as well. So I think this solution is forward compatible.
> Reviewed-by: Steven Price <steven.price@arm.com>
Thanks for that!
Andre.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 1/2] KVM: arm/arm64: Add save/restore support for firmware workaround state
@ 2019-02-22 17:36 ` Andre Przywara
0 siblings, 0 replies; 20+ messages in thread
From: Andre Przywara @ 2019-02-22 17:36 UTC (permalink / raw)
To: Steven Price
Cc: kvm, Marc Zyngier, Christoffer Dall, kvmarm, Dave Martin,
linux-arm-kernel
On Fri, 22 Feb 2019 17:16:12 +0000
Steven Price <steven.price@arm.com> wrote:
> On 22/02/2019 12:18, Andre Przywara wrote:
> > KVM implements the firmware interface for mitigating cache speculation
> > vulnerabilities. Guests may use this interface to ensure mitigation is
> > active.
> > If we want to migrate such a guest to a host with a different support
> > level for those workarounds, migration might need to fail, to ensure that
> > critical guests don't loose their protection.
> >
> > Introduce a way for userland to save and restore the workarounds state.
> > On restoring we do checks that make sure we don't downgrade our
> > mitigation level.
> >
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > ---
> > arch/arm/include/asm/kvm_emulate.h | 10 +++
> > arch/arm/include/uapi/asm/kvm.h | 10 +++
> > arch/arm64/include/asm/kvm_emulate.h | 14 ++++
> > arch/arm64/include/uapi/asm/kvm.h | 9 ++
> > virt/kvm/arm/psci.c | 119 +++++++++++++++++++++++----
> > 5 files changed, 146 insertions(+), 16 deletions(-)
> >
> > diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
> > index 8927cae7c966..663a02d7e6f4 100644
> > --- a/arch/arm/include/asm/kvm_emulate.h
> > +++ b/arch/arm/include/asm/kvm_emulate.h
> > @@ -283,6 +283,16 @@ static inline unsigned long kvm_vcpu_get_mpidr_aff(struct kvm_vcpu *vcpu)
> > return vcpu_cp15(vcpu, c0_MPIDR) & MPIDR_HWID_BITMASK;
> > }
> >
> > +static inline bool kvm_arm_get_vcpu_workaround_2_flag(struct kvm_vcpu *vcpu)
> > +{
> > + return false;
> > +}
> > +
> > +static inline void kvm_arm_set_vcpu_workaround_2_flag(struct kvm_vcpu *vcpu,
> > + bool flag)
> > +{
> > +}
> > +
> > static inline void kvm_vcpu_set_be(struct kvm_vcpu *vcpu)
> > {
> > *vcpu_cpsr(vcpu) |= PSR_E_BIT;
> > diff --git a/arch/arm/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h
> > index 4602464ebdfb..ba4d2afe65e3 100644
> > --- a/arch/arm/include/uapi/asm/kvm.h
> > +++ b/arch/arm/include/uapi/asm/kvm.h
> > @@ -214,6 +214,16 @@ struct kvm_vcpu_events {
> > #define KVM_REG_ARM_FW_REG(r) (KVM_REG_ARM | KVM_REG_SIZE_U64 | \
> > KVM_REG_ARM_FW | ((r) & 0xffff))
> > #define KVM_REG_ARM_PSCI_VERSION KVM_REG_ARM_FW_REG(0)
> > +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1 KVM_REG_ARM_FW_REG(1)
> > +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_NOT_AVAIL 0
> > +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_AVAIL 1
> > +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_UNAFFECTED 2
>
> Thanks for adding the UNAFFECTED state for WORKAROUND_1 - this means the
> ABI at least can deal with migration to a host which supports but
> doesn't need the workaround. I'm happy for the actual support for this
> to be added later if/when it's needed.
Thanks. Actually we *can't* do anything right now, because the host kernel only provides this "requires w/a or not" state, so for the current kernel we will never see UNAFFECTED. If QEMU wants to set UNAFFECTED because the source kernel had it, we naturally deny it, as future code would do as well. So I think this solution is forward compatible.
> Reviewed-by: Steven Price <steven.price@arm.com>
Thanks for that!
Andre.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 1/2] KVM: arm/arm64: Add save/restore support for firmware workaround state
2019-02-22 17:22 ` Dave Martin
@ 2019-02-22 18:32 ` Andre Przywara
-1 siblings, 0 replies; 20+ messages in thread
From: Andre Przywara @ 2019-02-22 18:32 UTC (permalink / raw)
To: Dave Martin; +Cc: kvm, Marc Zyngier, Steven Price, kvmarm, linux-arm-kernel
On Fri, 22 Feb 2019 17:22:37 +0000
Dave Martin <Dave.Martin@arm.com> wrote:
Hi Dave,
thanks for having a look!
> On Fri, Feb 22, 2019 at 12:18:17PM +0000, Andre Przywara wrote:
> > KVM implements the firmware interface for mitigating cache speculation
> > vulnerabilities. Guests may use this interface to ensure mitigation is
> > active.
> > If we want to migrate such a guest to a host with a different support
> > level for those workarounds, migration might need to fail, to ensure that
> > critical guests don't loose their protection.
> >
> > Introduce a way for userland to save and restore the workarounds state.
> > On restoring we do checks that make sure we don't downgrade our
> > mitigation level.
> >
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > ---
> > arch/arm/include/asm/kvm_emulate.h | 10 +++
> > arch/arm/include/uapi/asm/kvm.h | 10 +++
> > arch/arm64/include/asm/kvm_emulate.h | 14 ++++
> > arch/arm64/include/uapi/asm/kvm.h | 9 ++
> > virt/kvm/arm/psci.c | 119 +++++++++++++++++++++++----
> > 5 files changed, 146 insertions(+), 16 deletions(-)
> >
> > diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
> > index 8927cae7c966..663a02d7e6f4 100644
> > --- a/arch/arm/include/asm/kvm_emulate.h
> > +++ b/arch/arm/include/asm/kvm_emulate.h
> > @@ -283,6 +283,16 @@ static inline unsigned long kvm_vcpu_get_mpidr_aff(struct kvm_vcpu *vcpu)
> > return vcpu_cp15(vcpu, c0_MPIDR) & MPIDR_HWID_BITMASK;
> > }
> >
> > +static inline bool kvm_arm_get_vcpu_workaround_2_flag(struct kvm_vcpu *vcpu)
> > +{
> > + return false;
> > +}
> > +
> > +static inline void kvm_arm_set_vcpu_workaround_2_flag(struct kvm_vcpu *vcpu,
> > + bool flag)
> > +{
> > +}
> > +
> > static inline void kvm_vcpu_set_be(struct kvm_vcpu *vcpu)
> > {
> > *vcpu_cpsr(vcpu) |= PSR_E_BIT;
> > diff --git a/arch/arm/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h
> > index 4602464ebdfb..ba4d2afe65e3 100644
> > --- a/arch/arm/include/uapi/asm/kvm.h
> > +++ b/arch/arm/include/uapi/asm/kvm.h
> > @@ -214,6 +214,16 @@ struct kvm_vcpu_events {
> > #define KVM_REG_ARM_FW_REG(r) (KVM_REG_ARM | KVM_REG_SIZE_U64 | \
> > KVM_REG_ARM_FW | ((r) & 0xffff))
> > #define KVM_REG_ARM_PSCI_VERSION KVM_REG_ARM_FW_REG(0)
> > +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1 KVM_REG_ARM_FW_REG(1)
> > +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_NOT_AVAIL 0
> > +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_AVAIL 1
> > +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_UNAFFECTED 2
> > +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2 KVM_REG_ARM_FW_REG(2)
> > +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_AVAIL 0
> > +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_UNKNOWN 1
> > +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_AVAIL 2
> > +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_UNAFFECTED 3
> > +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_ENABLED (1U << 4)
>
> Why 4? How could the system be less affected than "UNAFFECTED"? (And
> if not, why do we need space to insert extra values?) Mixing read-only
> system-description info with mutable guest state in this register feels
> a bit odd, but we probably don't win much by making it a separate
> register either.
For workaround 2 the guest has the ability to opt out of the (costly)
mitigation. This bit 4 holds that state for the VCPU. As this is somewhat orthogonal to the *level*, I didn't want to fiddle this bit into the encoding.
> Possibly we could drop the _REG from the #defines that are not reg IDs,
> since KVM_REG_* are all reg IDs today.
>
> But it works either way.
>
>
> Without a clear definition of what these values mean, I worry about ABI
> drift. But if we don't expect these to evolve it's probably low-risk.
See the Documentation patch. I found it odd to have it first in the
series, describing something that is not yet implemented.
> > /* Device Control API: ARM VGIC */
> > #define KVM_DEV_ARM_VGIC_GRP_ADDR 0
> > diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> > index d3842791e1c4..c00c17c9adb6 100644
> > --- a/arch/arm64/include/asm/kvm_emulate.h
> > +++ b/arch/arm64/include/asm/kvm_emulate.h
> > @@ -348,6 +348,20 @@ static inline unsigned long kvm_vcpu_get_mpidr_aff(struct kvm_vcpu *vcpu)
> > return vcpu_read_sys_reg(vcpu, MPIDR_EL1) & MPIDR_HWID_BITMASK;
> > }
> >
> > +static inline bool kvm_arm_get_vcpu_workaround_2_flag(struct kvm_vcpu *vcpu)
> > +{
> > + return vcpu->arch.workaround_flags & VCPU_WORKAROUND_2_FLAG;
> > +}
> > +
> > +static inline void kvm_arm_set_vcpu_workaround_2_flag(struct kvm_vcpu *vcpu,
> > + bool flag)
> > +{
> > + if (flag)
> > + vcpu->arch.workaround_flags |= VCPU_WORKAROUND_2_FLAG;
> > + else
> > + vcpu->arch.workaround_flags &= ~VCPU_WORKAROUND_2_FLAG;
> > +}
> > +
> > static inline void kvm_vcpu_set_be(struct kvm_vcpu *vcpu)
> > {
> > if (vcpu_mode_is_32bit(vcpu)) {
> > diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> > index 97c3478ee6e7..367e96fe654e 100644
> > --- a/arch/arm64/include/uapi/asm/kvm.h
> > +++ b/arch/arm64/include/uapi/asm/kvm.h
> > @@ -225,6 +225,15 @@ struct kvm_vcpu_events {
> > #define KVM_REG_ARM_FW_REG(r) (KVM_REG_ARM64 | KVM_REG_SIZE_U64 | \
> > KVM_REG_ARM_FW | ((r) & 0xffff))
> > #define KVM_REG_ARM_PSCI_VERSION KVM_REG_ARM_FW_REG(0)
> > +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1 KVM_REG_ARM_FW_REG(1)
> > +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_NOT_AVAIL 0
> > +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_AVAIL 1
> > +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2 KVM_REG_ARM_FW_REG(2)
> > +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_AVAIL 0
> > +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_UNKNOWN 1
> > +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_AVAIL 2
> > +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_UNAFFECTED 3
> > +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_ENABLED (1U << 4)
> >
> > /* Device Control API: ARM VGIC */
> > #define KVM_DEV_ARM_VGIC_GRP_ADDR 0
> > diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c
> > index 9b73d3ad918a..97d2d13756f6 100644
> > --- a/virt/kvm/arm/psci.c
> > +++ b/virt/kvm/arm/psci.c
> > @@ -445,42 +445,97 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
> >
> > int kvm_arm_get_fw_num_regs(struct kvm_vcpu *vcpu)
> > {
> > - return 1; /* PSCI version */
> > + return 3; /* PSCI version and two workaround registers */
>
> Meh. But this is no worse than the way we do it elsewhere.
>
> > }
> >
> > int kvm_arm_copy_fw_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
> > {
> > - if (put_user(KVM_REG_ARM_PSCI_VERSION, uindices))
> > + if (put_user(KVM_REG_ARM_PSCI_VERSION, uindices++))
> > return -EFAULT;
> >
> > + if (put_user(KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1, uindices++))
> > + return -EFAULT;
> > +
> > + if (put_user(KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2, uindices++))
> > + return -EFAULT;
> > +
> > + return 0;
> > +}
> > +
> > +#define KVM_REG_FEATURE_LEVEL_WIDTH 4
> > +#define KVM_REG_FEATURE_LEVEL_MASK (BIT(KVM_REG_FEATURE_LEVEL_WIDTH) - 1)
>
> Nit: Maybe use GENMASK?
Yeah, but it would need to be GENMASK(KVM_REG_FEATURE_LEVEL_WIDTH - 1, 0),
which is not really more readable, also breaks 80 columns ;-)
> > +
> > +/*
> > + * Convert the workaround level into an easy-to-compare number, where
> > higher
> > + * values mean better protection.
> > + */
> > +static int get_kernel_wa_level(u64 regid)
> > +{
> > + switch (regid) {
> > + case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1:
> > + if (kvm_arm_harden_branch_predictor())
> > + return
> > KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_AVAIL;
> > + else
> > + return
> > KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_NOT_AVAIL;
> > + case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2:
> > + switch (kvm_arm_have_ssbd()) {
> > + case KVM_SSBD_FORCE_DISABLE:
> > + return
> > KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_AVAIL;
> > + case KVM_SSBD_KERNEL:
> > + return
> > KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_AVAIL;
> > + case KVM_SSBD_FORCE_ENABLE:
> > + case KVM_SSBD_MITIGATED:
> > + return
> > KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_UNAFFECTED;
> > + case KVM_SSBD_UNKNOWN:
> > + default:
> > + return
> > KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_UNKNOWN;
> > + }
> > + }
> > +
> > return 0;
> > }
> >
> > int kvm_arm_get_fw_reg(struct kvm_vcpu *vcpu, const struct
> > kvm_one_reg *reg) {
> > - if (reg->id == KVM_REG_ARM_PSCI_VERSION) {
> > - void __user *uaddr = (void __user *)(long)reg->addr;
> > - u64 val;
> > + void __user *uaddr = (void __user *)(long)reg->addr;
>
> (Why (long), I wonder? Anyway, no bother.)
Just copied from above. You need some cast to a pointer-sized integer for
32-bit. uintptr_t doesn't seem too popular in the kernel.
> > + u64 val;
> >
> > + switch (reg->id) {
> > + case KVM_REG_ARM_PSCI_VERSION:
> > val = kvm_psci_version(vcpu, vcpu->kvm);
> > - if (copy_to_user(uaddr, &val, KVM_REG_SIZE(reg->id)))
> > - return -EFAULT;
> > -
> > - return 0;
> > + break;
> > + case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1:
> > + val = get_kernel_wa_level(reg->id) &
> > KVM_REG_FEATURE_LEVEL_MASK;
> > + break;
> > + case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2:
> > + val = get_kernel_wa_level(reg->id) &
> > KVM_REG_FEATURE_LEVEL_MASK;
> > + if (kvm_arm_have_ssbd() == KVM_SSBD_KERNEL &&
> > + kvm_arm_get_vcpu_workaround_2_flag(vcpu))
> > + val |=
> > KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_ENABLED;
> > + break;
> > + default:
> > + return -EINVAL;
>
> Hmmm, it could be more reasonable to return -ENOENT. But this precedent
> may be established now.
Good point. For non-existent sysregs we return -ENOENT, indeed. So I think
we should follow this here as well. Don't think it's a regression to
switch now.
> Anyway, userspace is unlikely to make any useful distinction between
> these two errors.
>
> > }
> >
> > - return -EINVAL;
>
> You could try a BUILD_BUG_ON(KVM_REG_SIZE(reg->id) == sizeof(val)).
>
> I suspect gcc may not be quite bright enough to spot that condition is
> compiletime-constant, but if it does then so much the better.
>
> Not sure it's worth a runtime check though.
Do we rely on them being 64 bit, actually? Not sure we would be checking
something useful here.
> > + if (copy_to_user(uaddr, &val, KVM_REG_SIZE(reg->id)))
> > + return -EFAULT;
> > +
> > + return 0;
> > }
> >
> > int kvm_arm_set_fw_reg(struct kvm_vcpu *vcpu, const struct
> > kvm_one_reg *reg) {
> > - if (reg->id == KVM_REG_ARM_PSCI_VERSION) {
> > - void __user *uaddr = (void __user *)(long)reg->addr;
> > - bool wants_02;
> > - u64 val;
> > + void __user *uaddr = (void __user *)(long)reg->addr;
> > + u64 val;
> > + int wa_level;
> > +
> > + if (copy_from_user(&val, uaddr, KVM_REG_SIZE(reg->id)))
> > + return -EFAULT;
> >
> > - if (copy_from_user(&val, uaddr,
> > KVM_REG_SIZE(reg->id)))
> > - return -EFAULT;
> > + switch (reg->id) {
> > + case KVM_REG_ARM_PSCI_VERSION:
> > + {
> > + bool wants_02;
> >
> > wants_02 = test_bit(KVM_ARM_VCPU_PSCI_0_2,
> > vcpu->arch.features);
> > @@ -497,6 +552,38 @@ int kvm_arm_set_fw_reg(struct kvm_vcpu *vcpu,
> > const struct kvm_one_reg *reg) vcpu->kvm->arch.psci_version = val;
> > return 0;
> > }
> > + break;
> > + }
> > +
> > + case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1:
> > + wa_level = val & KVM_REG_FEATURE_LEVEL_MASK;
>
> Should we require the other bits to be zero?
>
> Providing we never add anything else to this register we will get away
> with it, but it would be cleaner to police the extra bits here and in
> similar places.
Mmh, I remember we are strict about this in other places, and I think I
had some check in an earlier version, so might indeed be useful to have.
> > +
> > + /* For now we only accept the very same workaround
> > level. */
> > + if (get_kernel_wa_level(reg->id) != wa_level)
> > + return -EINVAL;
> > +
> > + return 0;
> > +
> > + case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2:
> > + wa_level = val & KVM_REG_FEATURE_LEVEL_MASK;
>
> Ditto...
>
> > +
> > + if (get_kernel_wa_level(reg->id) < wa_level)
> > + return -EINVAL;
>
> Is the SMCCC_ARCH_WORKAROUND_2 interface definitely always available to
> the guest even when in the get_kernel_wa_level() == _UNAFFECTED case?
>
> With wa_level == _AVAIL, the guest assumes that the interface is there.
>
> (This may be fine; I'm just not so aware of the history.)
If I understand arch/arm64/kvm/hyp/hyp-entry.S:el1_hvc_guest correctly, we
do.
> > +
> > + if (kvm_arm_have_ssbd() != KVM_SSBD_KERNEL)
> > + return 0;
>
> (val & KVM_REG_FEATURE_LEVEL_MASK) could still be _NOT_AVAIL or _UNKNOWN
> here, yet we still attempt to call kvm_arm_set_vcpu_workaround_2_flag()
> based on the user's KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_ENABLED bit.
Not sure I understand. If wa_level is not _AVAIL or _UNAFFECTED, we just
fall through the switch/case below without doing anything. So what do I
miss here?
> Should we? I'm not clear on what the sematics should be for this case.
>
> > +
> > + switch (wa_level) {
> > + case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_AVAIL:
> > + kvm_arm_set_vcpu_workaround_2_flag(vcpu,
> > + val &
> > KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_ENABLED);
> > + break;
> > + case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_UNAFFECTED:
> > + kvm_arm_set_vcpu_workaround_2_flag(vcpu,
> > true);
> > + break;
> > + }
>
> This has the odd effect that userspace can bypass PSCI/SMCCC and twiddle
> this bit directly via KVM_SET_ONE_REG. I'm not sure this matters, and
> it may even be useful. A bit unexpected though. Are there any pitfalls
> here I've not thought of?
I think that userland can generally have more "power" than the guest,
which is fine, since it's under the host's admin control. Is there any
threat scenario would should be worried about? And what could be the
solution?
> > +
> > + return 0;
>
> The logic overall feels a bit fragile and arbitrary, though given what
> it is describing, we may not be able to do a whole lot better.
Given that it was a lot worse before, I am now quite happy with it ;-)
> Do we trust the user-supplied val more than we should?
I think we allow QEMU to treat its guest badly ...
> [...]
>
> Overall, the code looks reasonable; my comments are pretty much nits.
Thanks,
Andre.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 1/2] KVM: arm/arm64: Add save/restore support for firmware workaround state
@ 2019-02-22 18:32 ` Andre Przywara
0 siblings, 0 replies; 20+ messages in thread
From: Andre Przywara @ 2019-02-22 18:32 UTC (permalink / raw)
To: Dave Martin
Cc: kvm, Marc Zyngier, Christoffer Dall, Steven Price, kvmarm,
linux-arm-kernel
On Fri, 22 Feb 2019 17:22:37 +0000
Dave Martin <Dave.Martin@arm.com> wrote:
Hi Dave,
thanks for having a look!
> On Fri, Feb 22, 2019 at 12:18:17PM +0000, Andre Przywara wrote:
> > KVM implements the firmware interface for mitigating cache speculation
> > vulnerabilities. Guests may use this interface to ensure mitigation is
> > active.
> > If we want to migrate such a guest to a host with a different support
> > level for those workarounds, migration might need to fail, to ensure that
> > critical guests don't loose their protection.
> >
> > Introduce a way for userland to save and restore the workarounds state.
> > On restoring we do checks that make sure we don't downgrade our
> > mitigation level.
> >
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > ---
> > arch/arm/include/asm/kvm_emulate.h | 10 +++
> > arch/arm/include/uapi/asm/kvm.h | 10 +++
> > arch/arm64/include/asm/kvm_emulate.h | 14 ++++
> > arch/arm64/include/uapi/asm/kvm.h | 9 ++
> > virt/kvm/arm/psci.c | 119 +++++++++++++++++++++++----
> > 5 files changed, 146 insertions(+), 16 deletions(-)
> >
> > diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
> > index 8927cae7c966..663a02d7e6f4 100644
> > --- a/arch/arm/include/asm/kvm_emulate.h
> > +++ b/arch/arm/include/asm/kvm_emulate.h
> > @@ -283,6 +283,16 @@ static inline unsigned long kvm_vcpu_get_mpidr_aff(struct kvm_vcpu *vcpu)
> > return vcpu_cp15(vcpu, c0_MPIDR) & MPIDR_HWID_BITMASK;
> > }
> >
> > +static inline bool kvm_arm_get_vcpu_workaround_2_flag(struct kvm_vcpu *vcpu)
> > +{
> > + return false;
> > +}
> > +
> > +static inline void kvm_arm_set_vcpu_workaround_2_flag(struct kvm_vcpu *vcpu,
> > + bool flag)
> > +{
> > +}
> > +
> > static inline void kvm_vcpu_set_be(struct kvm_vcpu *vcpu)
> > {
> > *vcpu_cpsr(vcpu) |= PSR_E_BIT;
> > diff --git a/arch/arm/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h
> > index 4602464ebdfb..ba4d2afe65e3 100644
> > --- a/arch/arm/include/uapi/asm/kvm.h
> > +++ b/arch/arm/include/uapi/asm/kvm.h
> > @@ -214,6 +214,16 @@ struct kvm_vcpu_events {
> > #define KVM_REG_ARM_FW_REG(r) (KVM_REG_ARM | KVM_REG_SIZE_U64 | \
> > KVM_REG_ARM_FW | ((r) & 0xffff))
> > #define KVM_REG_ARM_PSCI_VERSION KVM_REG_ARM_FW_REG(0)
> > +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1 KVM_REG_ARM_FW_REG(1)
> > +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_NOT_AVAIL 0
> > +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_AVAIL 1
> > +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_UNAFFECTED 2
> > +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2 KVM_REG_ARM_FW_REG(2)
> > +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_AVAIL 0
> > +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_UNKNOWN 1
> > +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_AVAIL 2
> > +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_UNAFFECTED 3
> > +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_ENABLED (1U << 4)
>
> Why 4? How could the system be less affected than "UNAFFECTED"? (And
> if not, why do we need space to insert extra values?) Mixing read-only
> system-description info with mutable guest state in this register feels
> a bit odd, but we probably don't win much by making it a separate
> register either.
For workaround 2 the guest has the ability to opt out of the (costly)
mitigation. This bit 4 holds that state for the VCPU. As this is somewhat orthogonal to the *level*, I didn't want to fiddle this bit into the encoding.
> Possibly we could drop the _REG from the #defines that are not reg IDs,
> since KVM_REG_* are all reg IDs today.
>
> But it works either way.
>
>
> Without a clear definition of what these values mean, I worry about ABI
> drift. But if we don't expect these to evolve it's probably low-risk.
See the Documentation patch. I found it odd to have it first in the
series, describing something that is not yet implemented.
> > /* Device Control API: ARM VGIC */
> > #define KVM_DEV_ARM_VGIC_GRP_ADDR 0
> > diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> > index d3842791e1c4..c00c17c9adb6 100644
> > --- a/arch/arm64/include/asm/kvm_emulate.h
> > +++ b/arch/arm64/include/asm/kvm_emulate.h
> > @@ -348,6 +348,20 @@ static inline unsigned long kvm_vcpu_get_mpidr_aff(struct kvm_vcpu *vcpu)
> > return vcpu_read_sys_reg(vcpu, MPIDR_EL1) & MPIDR_HWID_BITMASK;
> > }
> >
> > +static inline bool kvm_arm_get_vcpu_workaround_2_flag(struct kvm_vcpu *vcpu)
> > +{
> > + return vcpu->arch.workaround_flags & VCPU_WORKAROUND_2_FLAG;
> > +}
> > +
> > +static inline void kvm_arm_set_vcpu_workaround_2_flag(struct kvm_vcpu *vcpu,
> > + bool flag)
> > +{
> > + if (flag)
> > + vcpu->arch.workaround_flags |= VCPU_WORKAROUND_2_FLAG;
> > + else
> > + vcpu->arch.workaround_flags &= ~VCPU_WORKAROUND_2_FLAG;
> > +}
> > +
> > static inline void kvm_vcpu_set_be(struct kvm_vcpu *vcpu)
> > {
> > if (vcpu_mode_is_32bit(vcpu)) {
> > diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> > index 97c3478ee6e7..367e96fe654e 100644
> > --- a/arch/arm64/include/uapi/asm/kvm.h
> > +++ b/arch/arm64/include/uapi/asm/kvm.h
> > @@ -225,6 +225,15 @@ struct kvm_vcpu_events {
> > #define KVM_REG_ARM_FW_REG(r) (KVM_REG_ARM64 | KVM_REG_SIZE_U64 | \
> > KVM_REG_ARM_FW | ((r) & 0xffff))
> > #define KVM_REG_ARM_PSCI_VERSION KVM_REG_ARM_FW_REG(0)
> > +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1 KVM_REG_ARM_FW_REG(1)
> > +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_NOT_AVAIL 0
> > +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_AVAIL 1
> > +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2 KVM_REG_ARM_FW_REG(2)
> > +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_AVAIL 0
> > +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_UNKNOWN 1
> > +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_AVAIL 2
> > +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_UNAFFECTED 3
> > +#define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_ENABLED (1U << 4)
> >
> > /* Device Control API: ARM VGIC */
> > #define KVM_DEV_ARM_VGIC_GRP_ADDR 0
> > diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c
> > index 9b73d3ad918a..97d2d13756f6 100644
> > --- a/virt/kvm/arm/psci.c
> > +++ b/virt/kvm/arm/psci.c
> > @@ -445,42 +445,97 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
> >
> > int kvm_arm_get_fw_num_regs(struct kvm_vcpu *vcpu)
> > {
> > - return 1; /* PSCI version */
> > + return 3; /* PSCI version and two workaround registers */
>
> Meh. But this is no worse than the way we do it elsewhere.
>
> > }
> >
> > int kvm_arm_copy_fw_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
> > {
> > - if (put_user(KVM_REG_ARM_PSCI_VERSION, uindices))
> > + if (put_user(KVM_REG_ARM_PSCI_VERSION, uindices++))
> > return -EFAULT;
> >
> > + if (put_user(KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1, uindices++))
> > + return -EFAULT;
> > +
> > + if (put_user(KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2, uindices++))
> > + return -EFAULT;
> > +
> > + return 0;
> > +}
> > +
> > +#define KVM_REG_FEATURE_LEVEL_WIDTH 4
> > +#define KVM_REG_FEATURE_LEVEL_MASK (BIT(KVM_REG_FEATURE_LEVEL_WIDTH) - 1)
>
> Nit: Maybe use GENMASK?
Yeah, but it would need to be GENMASK(KVM_REG_FEATURE_LEVEL_WIDTH - 1, 0),
which is not really more readable, also breaks 80 columns ;-)
> > +
> > +/*
> > + * Convert the workaround level into an easy-to-compare number, where
> > higher
> > + * values mean better protection.
> > + */
> > +static int get_kernel_wa_level(u64 regid)
> > +{
> > + switch (regid) {
> > + case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1:
> > + if (kvm_arm_harden_branch_predictor())
> > + return
> > KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_AVAIL;
> > + else
> > + return
> > KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_NOT_AVAIL;
> > + case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2:
> > + switch (kvm_arm_have_ssbd()) {
> > + case KVM_SSBD_FORCE_DISABLE:
> > + return
> > KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_AVAIL;
> > + case KVM_SSBD_KERNEL:
> > + return
> > KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_AVAIL;
> > + case KVM_SSBD_FORCE_ENABLE:
> > + case KVM_SSBD_MITIGATED:
> > + return
> > KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_UNAFFECTED;
> > + case KVM_SSBD_UNKNOWN:
> > + default:
> > + return
> > KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_UNKNOWN;
> > + }
> > + }
> > +
> > return 0;
> > }
> >
> > int kvm_arm_get_fw_reg(struct kvm_vcpu *vcpu, const struct
> > kvm_one_reg *reg) {
> > - if (reg->id == KVM_REG_ARM_PSCI_VERSION) {
> > - void __user *uaddr = (void __user *)(long)reg->addr;
> > - u64 val;
> > + void __user *uaddr = (void __user *)(long)reg->addr;
>
> (Why (long), I wonder? Anyway, no bother.)
Just copied from above. You need some cast to a pointer-sized integer for
32-bit. uintptr_t doesn't seem too popular in the kernel.
> > + u64 val;
> >
> > + switch (reg->id) {
> > + case KVM_REG_ARM_PSCI_VERSION:
> > val = kvm_psci_version(vcpu, vcpu->kvm);
> > - if (copy_to_user(uaddr, &val, KVM_REG_SIZE(reg->id)))
> > - return -EFAULT;
> > -
> > - return 0;
> > + break;
> > + case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1:
> > + val = get_kernel_wa_level(reg->id) &
> > KVM_REG_FEATURE_LEVEL_MASK;
> > + break;
> > + case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2:
> > + val = get_kernel_wa_level(reg->id) &
> > KVM_REG_FEATURE_LEVEL_MASK;
> > + if (kvm_arm_have_ssbd() == KVM_SSBD_KERNEL &&
> > + kvm_arm_get_vcpu_workaround_2_flag(vcpu))
> > + val |=
> > KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_ENABLED;
> > + break;
> > + default:
> > + return -EINVAL;
>
> Hmmm, it could be more reasonable to return -ENOENT. But this precedent
> may be established now.
Good point. For non-existent sysregs we return -ENOENT, indeed. So I think
we should follow this here as well. Don't think it's a regression to
switch now.
> Anyway, userspace is unlikely to make any useful distinction between
> these two errors.
>
> > }
> >
> > - return -EINVAL;
>
> You could try a BUILD_BUG_ON(KVM_REG_SIZE(reg->id) == sizeof(val)).
>
> I suspect gcc may not be quite bright enough to spot that condition is
> compiletime-constant, but if it does then so much the better.
>
> Not sure it's worth a runtime check though.
Do we rely on them being 64 bit, actually? Not sure we would be checking
something useful here.
> > + if (copy_to_user(uaddr, &val, KVM_REG_SIZE(reg->id)))
> > + return -EFAULT;
> > +
> > + return 0;
> > }
> >
> > int kvm_arm_set_fw_reg(struct kvm_vcpu *vcpu, const struct
> > kvm_one_reg *reg) {
> > - if (reg->id == KVM_REG_ARM_PSCI_VERSION) {
> > - void __user *uaddr = (void __user *)(long)reg->addr;
> > - bool wants_02;
> > - u64 val;
> > + void __user *uaddr = (void __user *)(long)reg->addr;
> > + u64 val;
> > + int wa_level;
> > +
> > + if (copy_from_user(&val, uaddr, KVM_REG_SIZE(reg->id)))
> > + return -EFAULT;
> >
> > - if (copy_from_user(&val, uaddr,
> > KVM_REG_SIZE(reg->id)))
> > - return -EFAULT;
> > + switch (reg->id) {
> > + case KVM_REG_ARM_PSCI_VERSION:
> > + {
> > + bool wants_02;
> >
> > wants_02 = test_bit(KVM_ARM_VCPU_PSCI_0_2,
> > vcpu->arch.features);
> > @@ -497,6 +552,38 @@ int kvm_arm_set_fw_reg(struct kvm_vcpu *vcpu,
> > const struct kvm_one_reg *reg) vcpu->kvm->arch.psci_version = val;
> > return 0;
> > }
> > + break;
> > + }
> > +
> > + case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1:
> > + wa_level = val & KVM_REG_FEATURE_LEVEL_MASK;
>
> Should we require the other bits to be zero?
>
> Providing we never add anything else to this register we will get away
> with it, but it would be cleaner to police the extra bits here and in
> similar places.
Mmh, I remember we are strict about this in other places, and I think I
had some check in an earlier version, so might indeed be useful to have.
> > +
> > + /* For now we only accept the very same workaround
> > level. */
> > + if (get_kernel_wa_level(reg->id) != wa_level)
> > + return -EINVAL;
> > +
> > + return 0;
> > +
> > + case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2:
> > + wa_level = val & KVM_REG_FEATURE_LEVEL_MASK;
>
> Ditto...
>
> > +
> > + if (get_kernel_wa_level(reg->id) < wa_level)
> > + return -EINVAL;
>
> Is the SMCCC_ARCH_WORKAROUND_2 interface definitely always available to
> the guest even when in the get_kernel_wa_level() == _UNAFFECTED case?
>
> With wa_level == _AVAIL, the guest assumes that the interface is there.
>
> (This may be fine; I'm just not so aware of the history.)
If I understand arch/arm64/kvm/hyp/hyp-entry.S:el1_hvc_guest correctly, we
do.
> > +
> > + if (kvm_arm_have_ssbd() != KVM_SSBD_KERNEL)
> > + return 0;
>
> (val & KVM_REG_FEATURE_LEVEL_MASK) could still be _NOT_AVAIL or _UNKNOWN
> here, yet we still attempt to call kvm_arm_set_vcpu_workaround_2_flag()
> based on the user's KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_ENABLED bit.
Not sure I understand. If wa_level is not _AVAIL or _UNAFFECTED, we just
fall through the switch/case below without doing anything. So what do I
miss here?
> Should we? I'm not clear on what the sematics should be for this case.
>
> > +
> > + switch (wa_level) {
> > + case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_AVAIL:
> > + kvm_arm_set_vcpu_workaround_2_flag(vcpu,
> > + val &
> > KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_ENABLED);
> > + break;
> > + case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_UNAFFECTED:
> > + kvm_arm_set_vcpu_workaround_2_flag(vcpu,
> > true);
> > + break;
> > + }
>
> This has the odd effect that userspace can bypass PSCI/SMCCC and twiddle
> this bit directly via KVM_SET_ONE_REG. I'm not sure this matters, and
> it may even be useful. A bit unexpected though. Are there any pitfalls
> here I've not thought of?
I think that userland can generally have more "power" than the guest,
which is fine, since it's under the host's admin control. Is there any
threat scenario would should be worried about? And what could be the
solution?
> > +
> > + return 0;
>
> The logic overall feels a bit fragile and arbitrary, though given what
> it is describing, we may not be able to do a whole lot better.
Given that it was a lot worse before, I am now quite happy with it ;-)
> Do we trust the user-supplied val more than we should?
I think we allow QEMU to treat its guest badly ...
> [...]
>
> Overall, the code looks reasonable; my comments are pretty much nits.
Thanks,
Andre.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 2/2] KVM: doc: add API documentation on the KVM_REG_ARM_WORKAROUNDS register
2019-02-22 17:27 ` Dave Martin
@ 2019-02-22 18:44 ` Andre Przywara
-1 siblings, 0 replies; 20+ messages in thread
From: Andre Przywara @ 2019-02-22 18:44 UTC (permalink / raw)
To: Dave Martin; +Cc: kvm, Marc Zyngier, Steven Price, kvmarm, linux-arm-kernel
On Fri, 22 Feb 2019 17:27:44 +0000
Dave Martin <Dave.Martin@arm.com> wrote:
> On Fri, Feb 22, 2019 at 12:18:18PM +0000, Andre Przywara wrote:
> > Add documentation for the newly defined firmware registers to save and
> > restore any vulnerability migitation status.
> >
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > ---
> > Documentation/virtual/kvm/arm/psci.txt | 24 ++++++++++++++++++++++++
> > 1 file changed, 24 insertions(+)
> >
> > diff --git a/Documentation/virtual/kvm/arm/psci.txt b/Documentation/virtual/kvm/arm/psci.txt
> > index aafdab887b04..fe8930765cc7 100644
> > --- a/Documentation/virtual/kvm/arm/psci.txt
> > +++ b/Documentation/virtual/kvm/arm/psci.txt
> > @@ -28,3 +28,27 @@ The following register is defined:
> > - Allows any PSCI version implemented by KVM and compatible with
> > v0.2 to be set with SET_ONE_REG
> > - Affects the whole VM (even if the register view is per-vcpu)
>
> Doh, ignore my comments about undescribed semantics in the last patch.
>
> > +
> > +* KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1:
> > + Holds the state of the firmware controlled workaround to mitigate
> > + CVE-2017-5715, as described under SMCCC_ARCH_WORKAROUND_1 in [1].
> > + Accepted values are:
> > + KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_NOT_AVAIL: Workaround not available.
> > + The mitigation status for the guest is unknown.
> > + KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_AVAIL: Workaround active for the guest.
> > + KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_UNAFFECTED: The workaround is not
> > + available and not needed.
> > +
> > +* KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2:
> > + Holds the state of the firmware controlled workaround to mitigate
> > + CVE-2018-3639, as described under SMCCC_ARCH_WORKAROUND_2 in [1].
> > + Accepted values are:
> > + KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_AVAIL: Workaround not available.
> > + KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_UNKNOWN: Workaround state unknown.
> > + KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_AVAIL: Workaround available, and can
> > + be disabled by a vCPU. If KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_ENABLED is
> > + set, it is active for this vCPU.
>
> Ah, so this is really not supposed to be set except for _AVAIL?
Yes, it's only used if the kernel offers the switch. The reason is to
allow the guest to turn it *off*, actually.
> > + KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_UNAFFECTED: Workaround always active
> > + or not needed.
>
> But is the workaround available (i.e., SMCCC interface implemented?)
Yes, that mirrors return value 1 of the actual firmware interface
permanently enable or not require, but safe to call the firmware).
> The code assumes it is, IIUC -- i.e., it will happily upgrade _AVAIL
> to _UNAFFECTED. That's bad if it means the SMCCC call disappears under
> the guest's feet.
I think migrating to a "better" system is an important use case, so we
should make it possible.
> (Not saying it does, just that I'm not sure.)
>
>
> For KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1, we apply an == check, avoiding
> this subtlety.
Because we only have two cases there at the moment, and NOT_AVAIL could
mean not protected. Since we require guest interaction for WORKAROUND_1,
we can't allow any change at the moment. This will change with the
introduction of UNAFFECTED.
Cheers,
Andre
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 2/2] KVM: doc: add API documentation on the KVM_REG_ARM_WORKAROUNDS register
@ 2019-02-22 18:44 ` Andre Przywara
0 siblings, 0 replies; 20+ messages in thread
From: Andre Przywara @ 2019-02-22 18:44 UTC (permalink / raw)
To: Dave Martin
Cc: kvm, Marc Zyngier, Christoffer Dall, Steven Price, kvmarm,
linux-arm-kernel
On Fri, 22 Feb 2019 17:27:44 +0000
Dave Martin <Dave.Martin@arm.com> wrote:
> On Fri, Feb 22, 2019 at 12:18:18PM +0000, Andre Przywara wrote:
> > Add documentation for the newly defined firmware registers to save and
> > restore any vulnerability migitation status.
> >
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > ---
> > Documentation/virtual/kvm/arm/psci.txt | 24 ++++++++++++++++++++++++
> > 1 file changed, 24 insertions(+)
> >
> > diff --git a/Documentation/virtual/kvm/arm/psci.txt b/Documentation/virtual/kvm/arm/psci.txt
> > index aafdab887b04..fe8930765cc7 100644
> > --- a/Documentation/virtual/kvm/arm/psci.txt
> > +++ b/Documentation/virtual/kvm/arm/psci.txt
> > @@ -28,3 +28,27 @@ The following register is defined:
> > - Allows any PSCI version implemented by KVM and compatible with
> > v0.2 to be set with SET_ONE_REG
> > - Affects the whole VM (even if the register view is per-vcpu)
>
> Doh, ignore my comments about undescribed semantics in the last patch.
>
> > +
> > +* KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1:
> > + Holds the state of the firmware controlled workaround to mitigate
> > + CVE-2017-5715, as described under SMCCC_ARCH_WORKAROUND_1 in [1].
> > + Accepted values are:
> > + KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_NOT_AVAIL: Workaround not available.
> > + The mitigation status for the guest is unknown.
> > + KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_AVAIL: Workaround active for the guest.
> > + KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1_UNAFFECTED: The workaround is not
> > + available and not needed.
> > +
> > +* KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2:
> > + Holds the state of the firmware controlled workaround to mitigate
> > + CVE-2018-3639, as described under SMCCC_ARCH_WORKAROUND_2 in [1].
> > + Accepted values are:
> > + KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_AVAIL: Workaround not available.
> > + KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_UNKNOWN: Workaround state unknown.
> > + KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_AVAIL: Workaround available, and can
> > + be disabled by a vCPU. If KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_ENABLED is
> > + set, it is active for this vCPU.
>
> Ah, so this is really not supposed to be set except for _AVAIL?
Yes, it's only used if the kernel offers the switch. The reason is to
allow the guest to turn it *off*, actually.
> > + KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_UNAFFECTED: Workaround always active
> > + or not needed.
>
> But is the workaround available (i.e., SMCCC interface implemented?)
Yes, that mirrors return value 1 of the actual firmware interface
permanently enable or not require, but safe to call the firmware).
> The code assumes it is, IIUC -- i.e., it will happily upgrade _AVAIL
> to _UNAFFECTED. That's bad if it means the SMCCC call disappears under
> the guest's feet.
I think migrating to a "better" system is an important use case, so we
should make it possible.
> (Not saying it does, just that I'm not sure.)
>
>
> For KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1, we apply an == check, avoiding
> this subtlety.
Because we only have two cases there at the moment, and NOT_AVAIL could
mean not protected. Since we require guest interaction for WORKAROUND_1,
we can't allow any change at the moment. This will change with the
introduction of UNAFFECTED.
Cheers,
Andre
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2019-02-22 18:44 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-22 12:18 [PATCH v3 0/2] KVM: arm/arm64: Add VCPU workarounds firmware register Andre Przywara
2019-02-22 12:18 ` Andre Przywara
2019-02-22 12:18 ` [PATCH v3 1/2] KVM: arm/arm64: Add save/restore support for firmware workaround state Andre Przywara
2019-02-22 12:18 ` Andre Przywara
2019-02-22 17:16 ` Steven Price
2019-02-22 17:16 ` Steven Price
2019-02-22 17:36 ` Andre Przywara
2019-02-22 17:36 ` Andre Przywara
2019-02-22 17:22 ` Dave Martin
2019-02-22 17:22 ` Dave Martin
2019-02-22 18:32 ` Andre Przywara
2019-02-22 18:32 ` Andre Przywara
2019-02-22 12:18 ` [PATCH v3 2/2] KVM: doc: add API documentation on the KVM_REG_ARM_WORKAROUNDS register Andre Przywara
2019-02-22 12:18 ` Andre Przywara
2019-02-22 17:16 ` Steven Price
2019-02-22 17:16 ` Steven Price
2019-02-22 17:27 ` Dave Martin
2019-02-22 17:27 ` Dave Martin
2019-02-22 18:44 ` Andre Przywara
2019-02-22 18:44 ` Andre Przywara
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.