2022-02-13 08:38:10

by Janis Schoetterl-Glausch

[permalink] [raw]
Subject: [PATCH v4 06/10] KVM: s390: Add vm IOCTL for key checked guest absolute memory access

Channel I/O honors storage keys and is performed on absolute memory.
For I/O emulation user space therefore needs to be able to do key
checked accesses.
The vm IOCTL supports read/write accesses, as well as checking
if an access would succeed.
Unlike relying on KVM_S390_GET_SKEYS for key checking would,
the vm IOCTL performs the check in lockstep with the read or write,
by, ultimately, mapping the access to move instructions that
support key protection checking with a supplied key.
Fetch and storage protection override are not applicable to absolute
accesses and so are not applied as they are when using the vcpu memop.

Signed-off-by: Janis Schoetterl-Glausch <[email protected]>
Reviewed-by: Christian Borntraeger <[email protected]>
---
arch/s390/kvm/gaccess.c | 72 +++++++++++++++++++++++++++++++++++
arch/s390/kvm/gaccess.h | 6 +++
arch/s390/kvm/kvm-s390.c | 81 ++++++++++++++++++++++++++++++++++++++++
include/uapi/linux/kvm.h | 2 +
4 files changed, 161 insertions(+)

diff --git a/arch/s390/kvm/gaccess.c b/arch/s390/kvm/gaccess.c
index 37838f637707..d53a183c2005 100644
--- a/arch/s390/kvm/gaccess.c
+++ b/arch/s390/kvm/gaccess.c
@@ -795,6 +795,35 @@ static int low_address_protection_enabled(struct kvm_vcpu *vcpu,
return 1;
}

+static int vm_check_access_key(struct kvm *kvm, u8 access_key,
+ enum gacc_mode mode, gpa_t gpa)
+{
+ u8 storage_key, access_control;
+ bool fetch_protected;
+ unsigned long hva;
+ int r;
+
+ if (access_key == 0)
+ return 0;
+
+ hva = gfn_to_hva(kvm, gpa_to_gfn(gpa));
+ if (kvm_is_error_hva(hva))
+ return PGM_ADDRESSING;
+
+ mmap_read_lock(current->mm);
+ r = get_guest_storage_key(current->mm, hva, &storage_key);
+ mmap_read_unlock(current->mm);
+ if (r)
+ return r;
+ access_control = FIELD_GET(_PAGE_ACC_BITS, storage_key);
+ if (access_control == access_key)
+ return 0;
+ fetch_protected = storage_key & _PAGE_FP_BIT;
+ if ((mode == GACC_FETCH || mode == GACC_IFETCH) && !fetch_protected)
+ return 0;
+ return PGM_PROTECTION;
+}
+
static bool fetch_prot_override_applicable(struct kvm_vcpu *vcpu, enum gacc_mode mode,
union asce asce)
{
@@ -994,6 +1023,26 @@ access_guest_page_with_key(struct kvm *kvm, enum gacc_mode mode, gpa_t gpa,
return 0;
}

+int access_guest_abs_with_key(struct kvm *kvm, gpa_t gpa, void *data,
+ unsigned long len, enum gacc_mode mode, u8 access_key)
+{
+ int offset = offset_in_page(gpa);
+ int fragment_len;
+ int rc;
+
+ while (min(PAGE_SIZE - offset, len) > 0) {
+ fragment_len = min(PAGE_SIZE - offset, len);
+ rc = access_guest_page_with_key(kvm, mode, gpa, data, fragment_len, access_key);
+ if (rc)
+ return rc;
+ offset = 0;
+ len -= fragment_len;
+ data += fragment_len;
+ gpa += fragment_len;
+ }
+ return 0;
+}
+
int access_guest_with_key(struct kvm_vcpu *vcpu, unsigned long ga, u8 ar,
void *data, unsigned long len, enum gacc_mode mode,
u8 access_key)
@@ -1144,6 +1193,29 @@ int check_gva_range(struct kvm_vcpu *vcpu, unsigned long gva, u8 ar,
return rc;
}

+/**
+ * check_gpa_range - test a range of guest physical addresses for accessibility
+ * @kvm: virtual machine instance
+ * @gpa: guest physical address
+ * @length: length of test range
+ * @mode: access mode to test, relevant for storage keys
+ * @access_key: access key to mach the storage keys with
+ */
+int check_gpa_range(struct kvm *kvm, unsigned long gpa, unsigned long length,
+ enum gacc_mode mode, u8 access_key)
+{
+ unsigned int fragment_len;
+ int rc = 0;
+
+ while (length && !rc) {
+ fragment_len = min(PAGE_SIZE - offset_in_page(gpa), length);
+ rc = vm_check_access_key(kvm, access_key, mode, gpa);
+ length -= fragment_len;
+ gpa += fragment_len;
+ }
+ return rc;
+}
+
/**
* kvm_s390_check_low_addr_prot_real - check for low-address protection
* @vcpu: virtual cpu
diff --git a/arch/s390/kvm/gaccess.h b/arch/s390/kvm/gaccess.h
index c5f2e7311b17..1124ff282012 100644
--- a/arch/s390/kvm/gaccess.h
+++ b/arch/s390/kvm/gaccess.h
@@ -193,6 +193,12 @@ int guest_translate_address_with_key(struct kvm_vcpu *vcpu, unsigned long gva, u
int check_gva_range(struct kvm_vcpu *vcpu, unsigned long gva, u8 ar,
unsigned long length, enum gacc_mode mode, u8 access_key);

+int check_gpa_range(struct kvm *kvm, unsigned long gpa, unsigned long length,
+ enum gacc_mode mode, u8 access_key);
+
+int access_guest_abs_with_key(struct kvm *kvm, gpa_t gpa, void *data,
+ unsigned long len, enum gacc_mode mode, u8 access_key);
+
int access_guest_with_key(struct kvm_vcpu *vcpu, unsigned long ga, u8 ar,
void *data, unsigned long len, enum gacc_mode mode,
u8 access_key);
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index c31b40abfa23..36bc73b5f5de 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -2364,6 +2364,78 @@ static bool access_key_invalid(u8 access_key)
return access_key > 0xf;
}

+static int kvm_s390_vm_mem_op(struct kvm *kvm, struct kvm_s390_mem_op *mop)
+{
+ void __user *uaddr = (void __user *)mop->buf;
+ u64 supported_flags;
+ void *tmpbuf = NULL;
+ int r, srcu_idx;
+
+ supported_flags = KVM_S390_MEMOP_F_SKEY_PROTECTION
+ | KVM_S390_MEMOP_F_CHECK_ONLY;
+ if (mop->flags & ~supported_flags)
+ return -EINVAL;
+ if (mop->size > MEM_OP_MAX_SIZE)
+ return -E2BIG;
+ if (kvm_s390_pv_is_protected(kvm))
+ return -EINVAL;
+ if (mop->flags & KVM_S390_MEMOP_F_SKEY_PROTECTION) {
+ if (access_key_invalid(mop->key))
+ return -EINVAL;
+ } else {
+ mop->key = 0;
+ }
+ if (!(mop->flags & KVM_S390_MEMOP_F_CHECK_ONLY)) {
+ tmpbuf = vmalloc(mop->size);
+ if (!tmpbuf)
+ return -ENOMEM;
+ }
+
+ srcu_idx = srcu_read_lock(&kvm->srcu);
+
+ if (kvm_is_error_gpa(kvm, mop->gaddr)) {
+ r = PGM_ADDRESSING;
+ goto out_unlock;
+ }
+
+ switch (mop->op) {
+ case KVM_S390_MEMOP_ABSOLUTE_READ: {
+ if (mop->flags & KVM_S390_MEMOP_F_CHECK_ONLY) {
+ r = check_gpa_range(kvm, mop->gaddr, mop->size, GACC_FETCH, mop->key);
+ } else {
+ r = access_guest_abs_with_key(kvm, mop->gaddr, tmpbuf,
+ mop->size, GACC_FETCH, mop->key);
+ if (r == 0) {
+ if (copy_to_user(uaddr, tmpbuf, mop->size))
+ r = -EFAULT;
+ }
+ }
+ break;
+ }
+ case KVM_S390_MEMOP_ABSOLUTE_WRITE: {
+ if (mop->flags & KVM_S390_MEMOP_F_CHECK_ONLY) {
+ r = check_gpa_range(kvm, mop->gaddr, mop->size, GACC_STORE, mop->key);
+ } else {
+ if (copy_from_user(tmpbuf, uaddr, mop->size)) {
+ r = -EFAULT;
+ break;
+ }
+ r = access_guest_abs_with_key(kvm, mop->gaddr, tmpbuf,
+ mop->size, GACC_STORE, mop->key);
+ }
+ break;
+ }
+ default:
+ r = -EINVAL;
+ }
+
+out_unlock:
+ srcu_read_unlock(&kvm->srcu, srcu_idx);
+
+ vfree(tmpbuf);
+ return r;
+}
+
long kvm_arch_vm_ioctl(struct file *filp,
unsigned int ioctl, unsigned long arg)
{
@@ -2488,6 +2560,15 @@ long kvm_arch_vm_ioctl(struct file *filp,
}
break;
}
+ case KVM_S390_MEM_OP: {
+ struct kvm_s390_mem_op mem_op;
+
+ if (copy_from_user(&mem_op, argp, sizeof(mem_op)) == 0)
+ r = kvm_s390_vm_mem_op(kvm, &mem_op);
+ else
+ r = -EFAULT;
+ break;
+ }
default:
r = -ENOTTY;
}
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 4566f429db2c..4bc7623def87 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -575,6 +575,8 @@ struct kvm_s390_mem_op {
#define KVM_S390_MEMOP_LOGICAL_WRITE 1
#define KVM_S390_MEMOP_SIDA_READ 2
#define KVM_S390_MEMOP_SIDA_WRITE 3
+#define KVM_S390_MEMOP_ABSOLUTE_READ 4
+#define KVM_S390_MEMOP_ABSOLUTE_WRITE 5
/* flags for kvm_s390_mem_op->flags */
#define KVM_S390_MEMOP_F_CHECK_ONLY (1ULL << 0)
#define KVM_S390_MEMOP_F_INJECT_EXCEPTION (1ULL << 1)
--
2.32.0


2022-02-15 06:19:55

by Claudio Imbrenda

[permalink] [raw]
Subject: Re: [PATCH v4 06/10] KVM: s390: Add vm IOCTL for key checked guest absolute memory access

On Fri, 11 Feb 2022 19:22:11 +0100
Janis Schoetterl-Glausch <[email protected]> wrote:

> Channel I/O honors storage keys and is performed on absolute memory.
> For I/O emulation user space therefore needs to be able to do key
> checked accesses.
> The vm IOCTL supports read/write accesses, as well as checking
> if an access would succeed.
> Unlike relying on KVM_S390_GET_SKEYS for key checking would,
> the vm IOCTL performs the check in lockstep with the read or write,
> by, ultimately, mapping the access to move instructions that
> support key protection checking with a supplied key.
> Fetch and storage protection override are not applicable to absolute
> accesses and so are not applied as they are when using the vcpu memop.
>
> Signed-off-by: Janis Schoetterl-Glausch <[email protected]>
> Reviewed-by: Christian Borntraeger <[email protected]>

Reviewed-by: Claudio Imbrenda <[email protected]>

> ---
> arch/s390/kvm/gaccess.c | 72 +++++++++++++++++++++++++++++++++++
> arch/s390/kvm/gaccess.h | 6 +++
> arch/s390/kvm/kvm-s390.c | 81 ++++++++++++++++++++++++++++++++++++++++
> include/uapi/linux/kvm.h | 2 +
> 4 files changed, 161 insertions(+)
>
> diff --git a/arch/s390/kvm/gaccess.c b/arch/s390/kvm/gaccess.c
> index 37838f637707..d53a183c2005 100644
> --- a/arch/s390/kvm/gaccess.c
> +++ b/arch/s390/kvm/gaccess.c
> @@ -795,6 +795,35 @@ static int low_address_protection_enabled(struct kvm_vcpu *vcpu,
> return 1;
> }
>
> +static int vm_check_access_key(struct kvm *kvm, u8 access_key,
> + enum gacc_mode mode, gpa_t gpa)
> +{
> + u8 storage_key, access_control;
> + bool fetch_protected;
> + unsigned long hva;
> + int r;
> +
> + if (access_key == 0)
> + return 0;
> +
> + hva = gfn_to_hva(kvm, gpa_to_gfn(gpa));
> + if (kvm_is_error_hva(hva))
> + return PGM_ADDRESSING;
> +
> + mmap_read_lock(current->mm);
> + r = get_guest_storage_key(current->mm, hva, &storage_key);
> + mmap_read_unlock(current->mm);
> + if (r)
> + return r;
> + access_control = FIELD_GET(_PAGE_ACC_BITS, storage_key);
> + if (access_control == access_key)
> + return 0;
> + fetch_protected = storage_key & _PAGE_FP_BIT;
> + if ((mode == GACC_FETCH || mode == GACC_IFETCH) && !fetch_protected)
> + return 0;
> + return PGM_PROTECTION;
> +}
> +
> static bool fetch_prot_override_applicable(struct kvm_vcpu *vcpu, enum gacc_mode mode,
> union asce asce)
> {
> @@ -994,6 +1023,26 @@ access_guest_page_with_key(struct kvm *kvm, enum gacc_mode mode, gpa_t gpa,
> return 0;
> }
>
> +int access_guest_abs_with_key(struct kvm *kvm, gpa_t gpa, void *data,
> + unsigned long len, enum gacc_mode mode, u8 access_key)
> +{
> + int offset = offset_in_page(gpa);
> + int fragment_len;
> + int rc;
> +
> + while (min(PAGE_SIZE - offset, len) > 0) {
> + fragment_len = min(PAGE_SIZE - offset, len);
> + rc = access_guest_page_with_key(kvm, mode, gpa, data, fragment_len, access_key);
> + if (rc)
> + return rc;
> + offset = 0;
> + len -= fragment_len;
> + data += fragment_len;
> + gpa += fragment_len;
> + }
> + return 0;
> +}
> +
> int access_guest_with_key(struct kvm_vcpu *vcpu, unsigned long ga, u8 ar,
> void *data, unsigned long len, enum gacc_mode mode,
> u8 access_key)
> @@ -1144,6 +1193,29 @@ int check_gva_range(struct kvm_vcpu *vcpu, unsigned long gva, u8 ar,
> return rc;
> }
>
> +/**
> + * check_gpa_range - test a range of guest physical addresses for accessibility
> + * @kvm: virtual machine instance
> + * @gpa: guest physical address
> + * @length: length of test range
> + * @mode: access mode to test, relevant for storage keys
> + * @access_key: access key to mach the storage keys with
> + */
> +int check_gpa_range(struct kvm *kvm, unsigned long gpa, unsigned long length,
> + enum gacc_mode mode, u8 access_key)
> +{
> + unsigned int fragment_len;
> + int rc = 0;
> +
> + while (length && !rc) {
> + fragment_len = min(PAGE_SIZE - offset_in_page(gpa), length);
> + rc = vm_check_access_key(kvm, access_key, mode, gpa);
> + length -= fragment_len;
> + gpa += fragment_len;
> + }
> + return rc;
> +}
> +
> /**
> * kvm_s390_check_low_addr_prot_real - check for low-address protection
> * @vcpu: virtual cpu
> diff --git a/arch/s390/kvm/gaccess.h b/arch/s390/kvm/gaccess.h
> index c5f2e7311b17..1124ff282012 100644
> --- a/arch/s390/kvm/gaccess.h
> +++ b/arch/s390/kvm/gaccess.h
> @@ -193,6 +193,12 @@ int guest_translate_address_with_key(struct kvm_vcpu *vcpu, unsigned long gva, u
> int check_gva_range(struct kvm_vcpu *vcpu, unsigned long gva, u8 ar,
> unsigned long length, enum gacc_mode mode, u8 access_key);
>
> +int check_gpa_range(struct kvm *kvm, unsigned long gpa, unsigned long length,
> + enum gacc_mode mode, u8 access_key);
> +
> +int access_guest_abs_with_key(struct kvm *kvm, gpa_t gpa, void *data,
> + unsigned long len, enum gacc_mode mode, u8 access_key);
> +
> int access_guest_with_key(struct kvm_vcpu *vcpu, unsigned long ga, u8 ar,
> void *data, unsigned long len, enum gacc_mode mode,
> u8 access_key);
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index c31b40abfa23..36bc73b5f5de 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -2364,6 +2364,78 @@ static bool access_key_invalid(u8 access_key)
> return access_key > 0xf;
> }
>
> +static int kvm_s390_vm_mem_op(struct kvm *kvm, struct kvm_s390_mem_op *mop)
> +{
> + void __user *uaddr = (void __user *)mop->buf;
> + u64 supported_flags;
> + void *tmpbuf = NULL;
> + int r, srcu_idx;
> +
> + supported_flags = KVM_S390_MEMOP_F_SKEY_PROTECTION
> + | KVM_S390_MEMOP_F_CHECK_ONLY;
> + if (mop->flags & ~supported_flags)
> + return -EINVAL;
> + if (mop->size > MEM_OP_MAX_SIZE)
> + return -E2BIG;
> + if (kvm_s390_pv_is_protected(kvm))
> + return -EINVAL;
> + if (mop->flags & KVM_S390_MEMOP_F_SKEY_PROTECTION) {
> + if (access_key_invalid(mop->key))
> + return -EINVAL;
> + } else {
> + mop->key = 0;
> + }
> + if (!(mop->flags & KVM_S390_MEMOP_F_CHECK_ONLY)) {
> + tmpbuf = vmalloc(mop->size);
> + if (!tmpbuf)
> + return -ENOMEM;
> + }
> +
> + srcu_idx = srcu_read_lock(&kvm->srcu);
> +
> + if (kvm_is_error_gpa(kvm, mop->gaddr)) {
> + r = PGM_ADDRESSING;
> + goto out_unlock;
> + }
> +
> + switch (mop->op) {
> + case KVM_S390_MEMOP_ABSOLUTE_READ: {
> + if (mop->flags & KVM_S390_MEMOP_F_CHECK_ONLY) {
> + r = check_gpa_range(kvm, mop->gaddr, mop->size, GACC_FETCH, mop->key);
> + } else {
> + r = access_guest_abs_with_key(kvm, mop->gaddr, tmpbuf,
> + mop->size, GACC_FETCH, mop->key);
> + if (r == 0) {
> + if (copy_to_user(uaddr, tmpbuf, mop->size))
> + r = -EFAULT;
> + }
> + }
> + break;
> + }
> + case KVM_S390_MEMOP_ABSOLUTE_WRITE: {
> + if (mop->flags & KVM_S390_MEMOP_F_CHECK_ONLY) {
> + r = check_gpa_range(kvm, mop->gaddr, mop->size, GACC_STORE, mop->key);
> + } else {
> + if (copy_from_user(tmpbuf, uaddr, mop->size)) {
> + r = -EFAULT;
> + break;
> + }
> + r = access_guest_abs_with_key(kvm, mop->gaddr, tmpbuf,
> + mop->size, GACC_STORE, mop->key);
> + }
> + break;
> + }
> + default:
> + r = -EINVAL;
> + }
> +
> +out_unlock:
> + srcu_read_unlock(&kvm->srcu, srcu_idx);
> +
> + vfree(tmpbuf);
> + return r;
> +}
> +
> long kvm_arch_vm_ioctl(struct file *filp,
> unsigned int ioctl, unsigned long arg)
> {
> @@ -2488,6 +2560,15 @@ long kvm_arch_vm_ioctl(struct file *filp,
> }
> break;
> }
> + case KVM_S390_MEM_OP: {
> + struct kvm_s390_mem_op mem_op;
> +
> + if (copy_from_user(&mem_op, argp, sizeof(mem_op)) == 0)
> + r = kvm_s390_vm_mem_op(kvm, &mem_op);
> + else
> + r = -EFAULT;
> + break;
> + }
> default:
> r = -ENOTTY;
> }
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 4566f429db2c..4bc7623def87 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -575,6 +575,8 @@ struct kvm_s390_mem_op {
> #define KVM_S390_MEMOP_LOGICAL_WRITE 1
> #define KVM_S390_MEMOP_SIDA_READ 2
> #define KVM_S390_MEMOP_SIDA_WRITE 3
> +#define KVM_S390_MEMOP_ABSOLUTE_READ 4
> +#define KVM_S390_MEMOP_ABSOLUTE_WRITE 5
> /* flags for kvm_s390_mem_op->flags */
> #define KVM_S390_MEMOP_F_CHECK_ONLY (1ULL << 0)
> #define KVM_S390_MEMOP_F_INJECT_EXCEPTION (1ULL << 1)

2022-02-22 05:53:12

by Janis Schoetterl-Glausch

[permalink] [raw]
Subject: [PATCH] KVM: s390: Add missing vm MEM_OP size check

Check that size is not zero, preventing the following warning:

WARNING: CPU: 0 PID: 9692 at mm/vmalloc.c:3059 __vmalloc_node_range+0x528/0x648
Modules linked in:
CPU: 0 PID: 9692 Comm: memop Not tainted 5.17.0-rc3-e4+ #80
Hardware name: IBM 8561 T01 701 (LPAR)
Krnl PSW : 0704c00180000000 0000000082dc584c (__vmalloc_node_range+0x52c/0x648)
R:0 T:1 IO:1 EX:1 Key:0 M:1 W:0 P:0 AS:3 CC:0 PM:0 RI:0 EA:3
Krnl GPRS: 0000000000000083 ffffffffffffffff 0000000000000000 0000000000000001
0000038000000000 000003ff80000000 0000000000000cc0 000000008ebb8000
0000000087a8a700 000000004040aeb1 000003ffd9f7dec8 000000008ebb8000
000000009d9b8000 000000000102a1b4 00000380035afb68 00000380035afaa8
Krnl Code: 0000000082dc583e: d028a7f4ff80 trtr 2036(41,%r10),3968(%r15)
0000000082dc5844: af000000 mc 0,0
#0000000082dc5848: af000000 mc 0,0
>0000000082dc584c: a7d90000 lghi %r13,0
0000000082dc5850: b904002d lgr %r2,%r13
0000000082dc5854: eb6ff1080004 lmg %r6,%r15,264(%r15)
0000000082dc585a: 07fe bcr 15,%r14
0000000082dc585c: 47000700 bc 0,1792
Call Trace:
[<0000000082dc584c>] __vmalloc_node_range+0x52c/0x648
[<0000000082dc5b62>] vmalloc+0x5a/0x68
[<000003ff8067f4ca>] kvm_arch_vm_ioctl+0x2da/0x2a30 [kvm]
[<000003ff806705bc>] kvm_vm_ioctl+0x4ec/0x978 [kvm]
[<0000000082e562fe>] __s390x_sys_ioctl+0xbe/0x100
[<000000008360a9bc>] __do_syscall+0x1d4/0x200
[<0000000083618bd2>] system_call+0x82/0xb0
Last Breaking-Event-Address:
[<0000000082dc5348>] __vmalloc_node_range+0x28/0x648

Other than the warning, there is no ill effect from the missing check,
the condition is detected by subsequent code and causes a return
with ENOMEM.

Fixes: ef11c9463ae0 (KVM: s390: Add vm IOCTL for key checked guest absolute memory access)
Signed-off-by: Janis Schoetterl-Glausch <[email protected]>
---
arch/s390/kvm/kvm-s390.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index c2c26c2aad64..e056ad86ccd2 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -2374,7 +2374,7 @@ static int kvm_s390_vm_mem_op(struct kvm *kvm, struct kvm_s390_mem_op *mop)

supported_flags = KVM_S390_MEMOP_F_SKEY_PROTECTION
| KVM_S390_MEMOP_F_CHECK_ONLY;
- if (mop->flags & ~supported_flags)
+ if (mop->flags & ~supported_flags || !mop->size)
return -EINVAL;
if (mop->size > MEM_OP_MAX_SIZE)
return -E2BIG;
--
2.32.0

2022-02-22 08:12:38

by Christian Borntraeger

[permalink] [raw]
Subject: Re: [PATCH] KVM: s390: Add missing vm MEM_OP size check



Am 21.02.22 um 17:32 schrieb Janis Schoetterl-Glausch:
> Check that size is not zero, preventing the following warning:
>
> WARNING: CPU: 0 PID: 9692 at mm/vmalloc.c:3059 __vmalloc_node_range+0x528/0x648
> Modules linked in:
> CPU: 0 PID: 9692 Comm: memop Not tainted 5.17.0-rc3-e4+ #80
> Hardware name: IBM 8561 T01 701 (LPAR)
> Krnl PSW : 0704c00180000000 0000000082dc584c (__vmalloc_node_range+0x52c/0x648)
> R:0 T:1 IO:1 EX:1 Key:0 M:1 W:0 P:0 AS:3 CC:0 PM:0 RI:0 EA:3
> Krnl GPRS: 0000000000000083 ffffffffffffffff 0000000000000000 0000000000000001
> 0000038000000000 000003ff80000000 0000000000000cc0 000000008ebb8000
> 0000000087a8a700 000000004040aeb1 000003ffd9f7dec8 000000008ebb8000
> 000000009d9b8000 000000000102a1b4 00000380035afb68 00000380035afaa8
> Krnl Code: 0000000082dc583e: d028a7f4ff80 trtr 2036(41,%r10),3968(%r15)
> 0000000082dc5844: af000000 mc 0,0
> #0000000082dc5848: af000000 mc 0,0
> >0000000082dc584c: a7d90000 lghi %r13,0
> 0000000082dc5850: b904002d lgr %r2,%r13
> 0000000082dc5854: eb6ff1080004 lmg %r6,%r15,264(%r15)
> 0000000082dc585a: 07fe bcr 15,%r14
> 0000000082dc585c: 47000700 bc 0,1792
> Call Trace:
> [<0000000082dc584c>] __vmalloc_node_range+0x52c/0x648
> [<0000000082dc5b62>] vmalloc+0x5a/0x68
> [<000003ff8067f4ca>] kvm_arch_vm_ioctl+0x2da/0x2a30 [kvm]
> [<000003ff806705bc>] kvm_vm_ioctl+0x4ec/0x978 [kvm]
> [<0000000082e562fe>] __s390x_sys_ioctl+0xbe/0x100
> [<000000008360a9bc>] __do_syscall+0x1d4/0x200
> [<0000000083618bd2>] system_call+0x82/0xb0
> Last Breaking-Event-Address:
> [<0000000082dc5348>] __vmalloc_node_range+0x28/0x648
>
> Other than the warning, there is no ill effect from the missing check,
> the condition is detected by subsequent code and causes a return
> with ENOMEM.
>
> Fixes: ef11c9463ae0 (KVM: s390: Add vm IOCTL for key checked guest absolute memory access)
> Signed-off-by: Janis Schoetterl-Glausch <[email protected]>

applied to kvms390/next, Thanks.
> ---
> arch/s390/kvm/kvm-s390.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index c2c26c2aad64..e056ad86ccd2 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -2374,7 +2374,7 @@ static int kvm_s390_vm_mem_op(struct kvm *kvm, struct kvm_s390_mem_op *mop)
>
> supported_flags = KVM_S390_MEMOP_F_SKEY_PROTECTION
> | KVM_S390_MEMOP_F_CHECK_ONLY;
> - if (mop->flags & ~supported_flags)
> + if (mop->flags & ~supported_flags || !mop->size)
> return -EINVAL;
> if (mop->size > MEM_OP_MAX_SIZE)
> return -E2BIG;