On 3/23/22 09:52, Janis Schoetterl-Glausch wrote:
On 3/23/22 08:58, Janosch Frank wrote:
On 3/22/22 16:32, Janis Schoetterl-Glausch wrote:
Issuing a memop on a protected vm does not make sense,
Issuing a vm memop on a protected vm...
The cpu memop still makes sense, no?
The vcpu memop does hold the vcpu->lock, so no lockdep issue.
If you issue a vcpu memop while enabling protected virtualization,
the memop might find that the vcpu is not protected, while other vcpus
might already be, but I don't think there's a way to create secure memory
concurrent with the memop.
I just wanted you to make this a bit more specific since we now have vm
and vcpu memops. vm memops don't make sense for pv guests but vcpu ones
are needed to access the sida.
neither is the memory readable/writable, nor does it make sense to check
storage keys. This is why the ioctl will return -EINVAL when it detects
the vm to be protected. However, in order to ensure that the vm cannot
become protected during the memop, the kvm->lock would need to be taken
for the duration of the ioctl. This is also required because
kvm_s390_pv_is_protected asserts that the lock must be held.
Instead, don't try to prevent this. If user space enables secure
execution concurrently with a memop it must accecpt the possibility of
the memop failing.
Still check if the vm is currently protected, but without locking and
consider it a heuristic.
Fixes: ef11c9463ae0 ("KVM: s390: Add vm IOCTL for key checked guest absolute memory access")
Signed-off-by: Janis Schoetterl-Glausch <scgl@xxxxxxxxxxxxx>
Makes sense to me.
Reviewed-by: Janosch Frank <frankja@xxxxxxxxxxxxx>
---
arch/s390/kvm/kvm-s390.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index ca96f84db2cc..53adbe86a68f 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -2385,7 +2385,16 @@ static int kvm_s390_vm_mem_op(struct kvm *kvm, struct kvm_s390_mem_op *mop)
return -EINVAL;
if (mop->size > MEM_OP_MAX_SIZE)
return -E2BIG;
- if (kvm_s390_pv_is_protected(kvm))
+ /*
+ * This is technically a heuristic only, if the kvm->lock is not
+ * taken, it is not guaranteed that the vm is/remains non-protected.
+ * This is ok from a kernel perspective, wrongdoing is detected
+ * on the access, -EFAULT is returned and the vm may crash the
+ * next time it accesses the memory in question.
+ * There is no sane usecase to do switching and a memop on two
+ * different CPUs at the same time.
+ */
+ if (kvm_s390_pv_get_handle(kvm))
return -EINVAL;
if (mop->flags & KVM_S390_MEMOP_F_SKEY_PROTECTION) {
if (access_key_invalid(mop->key))
base-commit: c9b8fecddb5bb4b67e351bbaeaa648a6f7456912
