by Christian Borntraeger

[permalink] [raw]

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

Am 09.02.22 um 18:04 schrieb Janis Schoetterl-Glausch:
> 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]>
> Acked-by: Janosch Frank <[email protected]>

same comment that you did for the vcpu part (default key)
with that.

Reviewed-by: Christian Borntraeger <[email protected]>
> ---
> arch/s390/kvm/gaccess.c | 72 ++++++++++++++++++++++++++++++++++++
> arch/s390/kvm/gaccess.h | 6 +++
> arch/s390/kvm/kvm-s390.c | 79 ++++++++++++++++++++++++++++++++++++++++
> include/uapi/linux/kvm.h | 2 +
> 4 files changed, 159 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 85763ec7bc60..7ee3d2e8ecf2 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -2365,6 +2365,76 @@ 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;
> + }
> + 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)
> {
> @@ -2489,6 +2559,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 44558cf4c52e..b83c9286e017 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-14 09:52:40

by Janosch Frank

[permalink] [raw]

Subject: Re: [PATCH v3 09/10] KVM: s390: Update api documentation for memop ioctl

On 2/9/22 18:04, Janis Schoetterl-Glausch wrote:
> Document all currently existing operations, flags and explain under
> which circumstances they are available. Document the recently
> introduced absolute operations and the storage key protection flag,
> as well as the existing SIDA operations.
>
> Signed-off-by: Janis Schoetterl-Glausch <[email protected]>

Reviewed-by: Janosch Frank <[email protected]>

Minor nits below

> ---
> Documentation/virt/kvm/api.rst | 112 ++++++++++++++++++++++++++-------
> include/uapi/linux/kvm.h | 2 +-
> 2 files changed, 91 insertions(+), 23 deletions(-)
>
> diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
> index a4267104db50..2d131af44576 100644
> --- a/Documentation/virt/kvm/api.rst
> +++ b/Documentation/virt/kvm/api.rst
> @@ -3683,15 +3683,17 @@ The fields in each entry are defined as follows:
> 4.89 KVM_S390_MEM_OP
> --------------------
>
> -:Capability: KVM_CAP_S390_MEM_OP
> +:Capability: KVM_CAP_S390_MEM_OP, KVM_CAP_S390_PROTECTED, KVM_CAP_S390_MEM_OP_EXTENSION
> :Architectures: s390
> -:Type: vcpu ioctl
> +:Type: vm ioctl, vcpu ioctl
> :Parameters: struct kvm_s390_mem_op (in)
> :Returns: = 0 on success,
> < 0 on generic error (e.g. -EFAULT or -ENOMEM),
> > 0 if an exception occurred while walking the page tables
>
> -Read or write data from/to the logical (virtual) memory of a VCPU.
> +Read or write data from/to the VM's memory.
> +The KVM_CAP_S390_MEM_OP_EXTENSION capability specifies what functionality is
> +supported.
>
> Parameters are specified via the following structure::
>
> @@ -3701,33 +3703,99 @@ Parameters are specified via the following structure::
> __u32 size; /* amount of bytes */
> __u32 op; /* type of operation */
> __u64 buf; /* buffer in userspace */
> - __u8 ar; /* the access register number */
> - __u8 reserved[31]; /* should be set to 0 */
> + union {
> + struct {
> + __u8 ar; /* the access register number */
> + __u8 key; /* access key, ignored if flag unset */
> + };
> + __u32 sida_offset; /* offset into the sida */
> + __u8 reserved[32]; /* ignored */
> + };
> };
>
> -The type of operation is specified in the "op" field. It is either
> -KVM_S390_MEMOP_LOGICAL_READ for reading from logical memory space or
> -KVM_S390_MEMOP_LOGICAL_WRITE for writing to logical memory space. The
> -KVM_S390_MEMOP_F_CHECK_ONLY flag can be set in the "flags" field to check
> -whether the corresponding memory access would create an access exception
> -(without touching the data in the memory at the destination). In case an
> -access exception occurred while walking the MMU tables of the guest, the
> -ioctl returns a positive error number to indicate the type of exception.
> -This exception is also raised directly at the corresponding VCPU if the
> -flag KVM_S390_MEMOP_F_INJECT_EXCEPTION is set in the "flags" field.
> -
> The start address of the memory region has to be specified in the "gaddr"
> field, and the length of the region in the "size" field (which must not
> be 0). The maximum value for "size" can be obtained by checking the
> KVM_CAP_S390_MEM_OP capability. "buf" is the buffer supplied by the
> userspace application where the read data should be written to for
> -KVM_S390_MEMOP_LOGICAL_READ, or where the data that should be written is
> -stored for a KVM_S390_MEMOP_LOGICAL_WRITE. When KVM_S390_MEMOP_F_CHECK_ONLY
> -is specified, "buf" is unused and can be NULL. "ar" designates the access
> -register number to be used; the valid range is 0..15.
> +a read access, or where the data that should be written is stored for
> +a write access. The "reserved" field is meant for future extensions.
> +Reserved and unused values are ignored. Future extension that add members must
> +introduce new flags.
> +
> +The type of operation is specified in the "op" field. Flags modifying
> +their behavior can be set in the "flags" field. Undefined flag bits must
> +be set to 0.
> +
> +Possible operations are:
> + * ``KVM_S390_MEMOP_LOGICAL_READ``
> + * ``KVM_S390_MEMOP_LOGICAL_WRITE``
> + * ``KVM_S390_MEMOP_ABSOLUTE_READ``
> + * ``KVM_S390_MEMOP_ABSOLUTE_WRITE``
> + * ``KVM_S390_MEMOP_SIDA_READ``
> + * ``KVM_S390_MEMOP_SIDA_WRITE``
> +
> +Logical read/write:
> +^^^^^^^^^^^^^^^^^^^
> +
> +Access logical memory, i.e. translate the given guest address to an absolute
> +address given the state of the VCPU and use the absolute address as target of
> +the access. "ar" designates the access register number to be used; the valid
> +range is 0..15.
> +Logical accesses are permitted for the VCPU ioctl only.
> +Logical accesses are permitted for non secure guests only.

s/secure/protected/

> +
> +Supported flags:
> + * ``KVM_S390_MEMOP_F_CHECK_ONLY``
> + * ``KVM_S390_MEMOP_F_INJECT_EXCEPTION``
> + * ``KVM_S390_MEMOP_F_SKEY_PROTECTION``
> +
> +The KVM_S390_MEMOP_F_CHECK_ONLY flag can be set to check whether the
> +corresponding memory access would cause an access exception, without touching

I think the comma needs to be removed.

> +the data in memory at the destination.
> +In this case, "buf" is unused and can be NULL.
> +
> +In case an access exception occurred during the access (or would occur
> +in case of KVM_S390_MEMOP_F_CHECK_ONLY), the ioctl returns a positive
> +error number indicating the type of exception. This exception is also
> +raised directly at the corresponding VCPU if the flag
> +KVM_S390_MEMOP_F_INJECT_EXCEPTION is set.
> +
> +If the KVM_S390_MEMOP_F_SKEY_PROTECTION flag is set, storage key
> +protection is also in effect and may cause exceptions if accesses are
> +prohibited given the access key passed in "key".
> +KVM_S390_MEMOP_F_SKEY_PROTECTION is available if KVM_CAP_S390_MEM_OP_EXTENSION
> +is > 0.
> +
> +Absolute read/write:
> +^^^^^^^^^^^^^^^^^^^^
> +
> +Access absolute memory. This operation is intended to be used with the
> +KVM_S390_MEMOP_F_SKEY_PROTECTION flag, to allow accessing memory and performing
> +the checks required for storage key protection as one operation (as opposed to
> +user space getting the storage keys, performing the checks, and accessing
> +memory thereafter, which could lead to a delay between check and access).
> +Absolute accesses are permitted for the VM ioctl if KVM_CAP_S390_MEM_OP_EXTENSION
> +is > 0.
> +Currently absolute accesses are not permitted for VCPU ioctls.
> +Absolute accesses are permitted for non secure guests only.

s/secure/protected/

> +
> +Supported flags:
> + * ``KVM_S390_MEMOP_F_CHECK_ONLY``
> + * ``KVM_S390_MEMOP_F_SKEY_PROTECTION``
> +
> +The semantics of the flags are as for logical accesses.
> +
> +SIDA read/write:
> +^^^^^^^^^^^^^^^^
> +
> +Access the secure instruction data area which contains memory operands necessary
> +for instruction emulation for secure guests.
> +SIDA accesses are available if the KVM_CAP_S390_PROTECTED capability is available.
> +SIDA accesses are permitted for the VCPU ioctl only.
> +SIDA accesses are permitted for secure guests only.

s/secure/protected/

>
> -The "reserved" field is meant for future extensions. It is not used by
> -KVM with the currently defined set of flags.
> +No flags are supported.
>
> 4.90 KVM_S390_GET_SKEYS
> -----------------------
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 26bff414f1a0..fd01fe04a183 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -567,7 +567,7 @@ struct kvm_s390_mem_op {
> __u8 key; /* access key, ignored if flag unset */
> };
> __u32 sida_offset; /* offset into the sida */
> - __u8 reserved[32]; /* should be set to 0 */
> + __u8 reserved[32]; /* ignored */
> };
> };
> /* types for kvm_s390_mem_op->op */