Am 07.02.22 um 17:59 schrieb Janis Schoetterl-Glausch:
User space needs a mechanism to perform key checked accesses when
emulating instructions.
The key can be passed as an additional argument.
Having an additional argument is flexible, as user space can
pass the guest PSW's key, in order to make an access the same way the
CPU would, or pass another key if necessary.
Signed-off-by: Janis Schoetterl-Glausch <scgl@xxxxxxxxxxxxx>
Acked-by: Janosch Frank <frankja@xxxxxxxxxxxxx>
Reviewed-by: Claudio Imbrenda <imbrenda@xxxxxxxxxxxxx>
---
arch/s390/kvm/kvm-s390.c | 49 +++++++++++++++++++++++++++++++---------
include/uapi/linux/kvm.h | 8 +++++--
2 files changed, 44 insertions(+), 13 deletions(-)
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index cf347e1a4f17..71e61fb3f0d9 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -32,6 +32,7 @@
#include <linux/sched/signal.h>
#include <linux/string.h>
#include <linux/pgtable.h>
+#include <linux/bitfield.h>
#include <asm/asm-offsets.h>
#include <asm/lowcore.h>
@@ -2359,6 +2360,11 @@ static int kvm_s390_handle_pv(struct kvm *kvm, struct kvm_pv_cmd *cmd)
return r;
}
+static bool access_key_invalid(u8 access_key)
+{
+ return access_key > 0xf;
+}
+
long kvm_arch_vm_ioctl(struct file *filp,
unsigned int ioctl, unsigned long arg)
{
@@ -4687,34 +4693,54 @@ static long kvm_s390_guest_mem_op(struct kvm_vcpu *vcpu,
struct kvm_s390_mem_op *mop)
{
void __user *uaddr = (void __user *)mop->buf;
+ u8 access_key = 0, ar = 0;
void *tmpbuf = NULL;
+ bool check_reserved;
int r = 0;
const u64 supported_flags = KVM_S390_MEMOP_F_INJECT_EXCEPTION
- | KVM_S390_MEMOP_F_CHECK_ONLY;
+ | KVM_S390_MEMOP_F_CHECK_ONLY
+ | KVM_S390_MEMOP_F_SKEY_PROTECTION;
- if (mop->flags & ~supported_flags || mop->ar >= NUM_ACRS || !mop->size)
+ if (mop->flags & ~supported_flags || !mop->size)
return -EINVAL;
-
if (mop->size > MEM_OP_MAX_SIZE)
return -E2BIG;
-
if (kvm_s390_pv_cpu_is_protected(vcpu))
return -EINVAL;
-
if (!(mop->flags & KVM_S390_MEMOP_F_CHECK_ONLY)) {
tmpbuf = vmalloc(mop->size);
if (!tmpbuf)
return -ENOMEM;
}
+ ar = mop->ar;
+ mop->ar = 0;
Why this assignment to 0?
+ if (ar >= NUM_ACRS)
+ return -EINVAL;
+ if (mop->flags & KVM_S390_MEMOP_F_SKEY_PROTECTION) {
+ access_key = mop->key;
+ mop->key = 0;
and this? I think we can leave mop unchanged.
In fact, why do we add the ar and access_key variable?
This breaks the check from above (if (mop->flags & ~supported_flags || mop->ar >= NUM_ACRS || !mop->size)) into two checks
and it will create a memleak for tmpbuf.
Simply use mop->key and mop->ar below and get rid of the local variables.
The structure has no concurrency and gcc will handle that just as the local variable.
Other than that this looks good.
+ if (access_key_invalid(access_key))
+ return -EINVAL;
+ }
+ /*
+ * Check that reserved/unused == 0, but only for extensions,
+ * so we stay backward compatible.
+ * This gives us more design flexibility for future extensions, i.e.
+ * we can add functionality without adding a flag.
+ */
+ check_reserved = mop->flags & KVM_S390_MEMOP_F_SKEY_PROTECTION;
+ if (check_reserved && memchr_inv(&mop->reserved, 0, sizeof(mop->reserved)))
+ return -EINVAL;
switch (mop->op) {
case KVM_S390_MEMOP_LOGICAL_READ:
if (mop->flags & KVM_S390_MEMOP_F_CHECK_ONLY) {
- r = check_gva_range(vcpu, mop->gaddr, mop->ar,
- mop->size, GACC_FETCH, 0);
+ r = check_gva_range(vcpu, mop->gaddr, ar, mop->size,
+ GACC_FETCH, access_key);
break;
}
- r = read_guest(vcpu, mop->gaddr, mop->ar, tmpbuf, mop->size);
+ r = read_guest_with_key(vcpu, mop->gaddr, ar, tmpbuf,
+ mop->size, access_key);
if (r == 0) {
if (copy_to_user(uaddr, tmpbuf, mop->size))
r = -EFAULT;
@@ -4722,15 +4748,16 @@ static long kvm_s390_guest_mem_op(struct kvm_vcpu *vcpu,
break;
case KVM_S390_MEMOP_LOGICAL_WRITE:
if (mop->flags & KVM_S390_MEMOP_F_CHECK_ONLY) {
- r = check_gva_range(vcpu, mop->gaddr, mop->ar,
- mop->size, GACC_STORE, 0);
+ r = check_gva_range(vcpu, mop->gaddr, ar, mop->size,
+ GACC_STORE, access_key);
break;
}
if (copy_from_user(tmpbuf, uaddr, mop->size)) {
r = -EFAULT;
break;
}
- r = write_guest(vcpu, mop->gaddr, mop->ar, tmpbuf, mop->size);
+ r = write_guest_with_key(vcpu, mop->gaddr, ar, tmpbuf,
+ mop->size, access_key);
break;
}
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index b46bcdb0cab1..5771b026fbc0 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -562,9 +562,12 @@ struct kvm_s390_mem_op {
__u32 op; /* type of operation */
__u64 buf; /* buffer in userspace */
union {
- __u8 ar; /* the access register number */
+ struct {
+ __u8 ar; /* the access register number */
+ __u8 key; /* access key to use for storage key protection */
+ };
__u32 sida_offset; /* offset into the sida */
- __u8 reserved[32]; /* should be set to 0 */
+ __u8 reserved[32]; /* must be set to 0 */
};
};
