diff mbox series

[v3,05/10] KVM: s390: Add optional storage key checking to MEMOP IOCTL

Message ID 20220209170422.1910690-6-scgl@linux.ibm.com (mailing list archive)
State New, archived
Headers show
Series KVM: s390: Do storage key checking | expand

Commit Message

Janis Schoetterl-Glausch Feb. 9, 2022, 5:04 p.m. UTC 
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@linux.ibm.com>
Acked-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 arch/s390/kvm/kvm-s390.c | 30 ++++++++++++++++++++----------
 include/uapi/linux/kvm.h |  6 +++++-
 2 files changed, 25 insertions(+), 11 deletions(-)

Comments

Christian Borntraeger Feb. 10, 2022, 9:29 a.m. UTC | #1 
Am 09.02.22 um 18:04 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@linux.ibm.com>
> Acked-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

Claudio, Janosch, can you confirm that this is still valid?


Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>

minor thing below
> ---
>   arch/s390/kvm/kvm-s390.c | 30 ++++++++++++++++++++----------
>   include/uapi/linux/kvm.h |  6 +++++-
>   2 files changed, 25 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index cf347e1a4f17..85763ec7bc60 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>

do we still need that after the changes?

>   
>   #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)
>   {
> @@ -4690,17 +4696,19 @@ static long kvm_s390_guest_mem_op(struct kvm_vcpu *vcpu,
>   	void *tmpbuf = NULL;
>   	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)
>   		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_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)
> @@ -4710,11 +4718,12 @@ static long kvm_s390_guest_mem_op(struct kvm_vcpu *vcpu,
>   	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, mop->ar, mop->size,
> +					    GACC_FETCH, mop->key);
>   			break;
>   		}
> -		r = read_guest(vcpu, mop->gaddr, mop->ar, tmpbuf, mop->size);
> +		r = read_guest_with_key(vcpu, mop->gaddr, mop->ar, tmpbuf,
> +					mop->size, mop->key);
>   		if (r == 0) {
>   			if (copy_to_user(uaddr, tmpbuf, mop->size))
>   				r = -EFAULT;
> @@ -4722,15 +4731,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, mop->ar, mop->size,
> +					    GACC_STORE, mop->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, mop->ar, tmpbuf,
> +					 mop->size, mop->key);
>   		break;
>   	}
>   
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index b46bcdb0cab1..44558cf4c52e 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -562,7 +562,10 @@ 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, ignored if flag unset */
> +		};
>   		__u32 sida_offset; /* offset into the sida */
>   		__u8 reserved[32]; /* should be set to 0 */
>   	};
> @@ -575,6 +578,7 @@ struct kvm_s390_mem_op {
>   /* 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)
> +#define KVM_S390_MEMOP_F_SKEY_PROTECTION	(1ULL << 2)
>   
>   /* for KVM_INTERRUPT */
>   struct kvm_interrupt {
Janis Schoetterl-Glausch Feb. 10, 2022, 9:40 a.m. UTC | #2 
On 2/10/22 10:29, Christian Borntraeger wrote:
> Am 09.02.22 um 18:04 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@linux.ibm.com>
>> Acked-by: Janosch Frank <frankja@linux.ibm.com>
>> Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> 
> Claudio, Janosch, can you confirm that this is still valid?
> 
> 
> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>

Not @linux.ibm.com?
> 
> minor thing below
>> ---
>>   arch/s390/kvm/kvm-s390.c | 30 ++++++++++++++++++++----------
>>   include/uapi/linux/kvm.h |  6 +++++-
>>   2 files changed, 25 insertions(+), 11 deletions(-)
>>
>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>> index cf347e1a4f17..85763ec7bc60 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>
> 
> do we still need that after the changes?

No, not since we moved the key out of the flags.
> 
>>     #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)
>>   {
>> @@ -4690,17 +4696,19 @@ static long kvm_s390_guest_mem_op(struct kvm_vcpu *vcpu,
>>       void *tmpbuf = NULL;
>>       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)
>>           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_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)
>> @@ -4710,11 +4718,12 @@ static long kvm_s390_guest_mem_op(struct kvm_vcpu *vcpu,
>>       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, mop->ar, mop->size,
>> +                        GACC_FETCH, mop->key);
>>               break;
>>           }
>> -        r = read_guest(vcpu, mop->gaddr, mop->ar, tmpbuf, mop->size);
>> +        r = read_guest_with_key(vcpu, mop->gaddr, mop->ar, tmpbuf,
>> +                    mop->size, mop->key);
>>           if (r == 0) {
>>               if (copy_to_user(uaddr, tmpbuf, mop->size))
>>                   r = -EFAULT;
>> @@ -4722,15 +4731,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, mop->ar, mop->size,
>> +                        GACC_STORE, mop->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, mop->ar, tmpbuf,
>> +                     mop->size, mop->key);
>>           break;
>>       }
>>   diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
>> index b46bcdb0cab1..44558cf4c52e 100644
>> --- a/include/uapi/linux/kvm.h
>> +++ b/include/uapi/linux/kvm.h
>> @@ -562,7 +562,10 @@ 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, ignored if flag unset */
>> +        };
>>           __u32 sida_offset; /* offset into the sida */
>>           __u8 reserved[32]; /* should be set to 0 */
>>       };
>> @@ -575,6 +578,7 @@ struct kvm_s390_mem_op {
>>   /* 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)
>> +#define KVM_S390_MEMOP_F_SKEY_PROTECTION    (1ULL << 2)
>>     /* for KVM_INTERRUPT */
>>   struct kvm_interrupt {
Christian Borntraeger Feb. 10, 2022, 9:43 a.m. UTC | #3 
Am 10.02.22 um 10:40 schrieb Janis Schoetterl-Glausch:
> On 2/10/22 10:29, Christian Borntraeger wrote:
>> Am 09.02.22 um 18:04 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@linux.ibm.com>
>>> Acked-by: Janosch Frank <frankja@linux.ibm.com>
>>> Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
>>
>> Claudio, Janosch, can you confirm that this is still valid?
>>
>>
>> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> 
> Not @linux.ibm.com?

Yes, of course. Old habits...
Reviewed-by: Christian Borntraeger <borntraeger@linux.ibm.com>

>>
>> minor thing below
>>> ---
>>>    arch/s390/kvm/kvm-s390.c | 30 ++++++++++++++++++++----------
>>>    include/uapi/linux/kvm.h |  6 +++++-
>>>    2 files changed, 25 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>>> index cf347e1a4f17..85763ec7bc60 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>
>>
>> do we still need that after the changes?
> 
> No, not since we moved the key out of the flags.
>>
>>>      #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)
>>>    {
>>> @@ -4690,17 +4696,19 @@ static long kvm_s390_guest_mem_op(struct kvm_vcpu *vcpu,
>>>        void *tmpbuf = NULL;
>>>        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)
>>>            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_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)
>>> @@ -4710,11 +4718,12 @@ static long kvm_s390_guest_mem_op(struct kvm_vcpu *vcpu,
>>>        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, mop->ar, mop->size,
>>> +                        GACC_FETCH, mop->key);
>>>                break;
>>>            }
>>> -        r = read_guest(vcpu, mop->gaddr, mop->ar, tmpbuf, mop->size);
>>> +        r = read_guest_with_key(vcpu, mop->gaddr, mop->ar, tmpbuf,
>>> +                    mop->size, mop->key);
>>>            if (r == 0) {
>>>                if (copy_to_user(uaddr, tmpbuf, mop->size))
>>>                    r = -EFAULT;
>>> @@ -4722,15 +4731,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, mop->ar, mop->size,
>>> +                        GACC_STORE, mop->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, mop->ar, tmpbuf,
>>> +                     mop->size, mop->key);
>>>            break;
>>>        }
>>>    diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
>>> index b46bcdb0cab1..44558cf4c52e 100644
>>> --- a/include/uapi/linux/kvm.h
>>> +++ b/include/uapi/linux/kvm.h
>>> @@ -562,7 +562,10 @@ 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, ignored if flag unset */
>>> +        };
>>>            __u32 sida_offset; /* offset into the sida */
>>>            __u8 reserved[32]; /* should be set to 0 */
>>>        };
>>> @@ -575,6 +578,7 @@ struct kvm_s390_mem_op {
>>>    /* 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)
>>> +#define KVM_S390_MEMOP_F_SKEY_PROTECTION    (1ULL << 2)
>>>      /* for KVM_INTERRUPT */
>>>    struct kvm_interrupt {
>
Janis Schoetterl-Glausch Feb. 10, 2022, 11:59 a.m. UTC | #4 
On 2/9/22 18:04, Janis Schoetterl-Glausch wrote:
> 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@linux.ibm.com>
> Acked-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> ---
>  arch/s390/kvm/kvm-s390.c | 30 ++++++++++++++++++++----------
>  include/uapi/linux/kvm.h |  6 +++++-
>  2 files changed, 25 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index cf347e1a4f17..85763ec7bc60 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)
>  {
> @@ -4690,17 +4696,19 @@ static long kvm_s390_guest_mem_op(struct kvm_vcpu *vcpu,
>  	void *tmpbuf = NULL;
>  	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)
>  		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_SKEY_PROTECTION) {
> +		if (access_key_invalid(mop->key))
> +			return -EINVAL;

I got this wrong unfortunately, we need to explicitly default to key 0, i.e.
+       } else {
+               mop->key = 0;
Same for the vm memop.
Didn't have a test case for this, yet.
> +	}>  	if (!(mop->flags & KVM_S390_MEMOP_F_CHECK_ONLY)) {
>  		tmpbuf = vmalloc(mop->size);
>  		if (!tmpbuf)
> @@ -4710,11 +4718,12 @@ static long kvm_s390_guest_mem_op(struct kvm_vcpu *vcpu,
>  	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, mop->ar, mop->size,
> +					    GACC_FETCH, mop->key);
>  			break;
>  		}
> -		r = read_guest(vcpu, mop->gaddr, mop->ar, tmpbuf, mop->size);
> +		r = read_guest_with_key(vcpu, mop->gaddr, mop->ar, tmpbuf,
> +					mop->size, mop->key);
>  		if (r == 0) {
>  			if (copy_to_user(uaddr, tmpbuf, mop->size))
>  				r = -EFAULT;
> @@ -4722,15 +4731,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, mop->ar, mop->size,
> +					    GACC_STORE, mop->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, mop->ar, tmpbuf,
> +					 mop->size, mop->key);
>  		break;
>  	}
>  

[...]
Claudio Imbrenda Feb. 10, 2022, 4:33 p.m. UTC | #5 
On Thu, 10 Feb 2022 12:59:03 +0100
Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:

> On 2/9/22 18:04, Janis Schoetterl-Glausch wrote:
> > 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@linux.ibm.com>
> > Acked-by: Janosch Frank <frankja@linux.ibm.com>
> > Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> > ---
> >  arch/s390/kvm/kvm-s390.c | 30 ++++++++++++++++++++----------
> >  include/uapi/linux/kvm.h |  6 +++++-
> >  2 files changed, 25 insertions(+), 11 deletions(-)
> > 
> > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> > index cf347e1a4f17..85763ec7bc60 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)
> >  {
> > @@ -4690,17 +4696,19 @@ static long kvm_s390_guest_mem_op(struct kvm_vcpu *vcpu,
> >  	void *tmpbuf = NULL;
> >  	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)
> >  		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_SKEY_PROTECTION) {
> > +		if (access_key_invalid(mop->key))
> > +			return -EINVAL;  
> 
> I got this wrong unfortunately, we need to explicitly default to key 0, i.e.
> +       } else {
> +               mop->key = 0;
> Same for the vm memop.
> Didn't have a test case for this, yet.

you can keep my r-b once you fix this (and the spurious include)

> > +	}>  	if (!(mop->flags & KVM_S390_MEMOP_F_CHECK_ONLY)) {
> >  		tmpbuf = vmalloc(mop->size);
> >  		if (!tmpbuf)
> > @@ -4710,11 +4718,12 @@ static long kvm_s390_guest_mem_op(struct kvm_vcpu *vcpu,
> >  	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, mop->ar, mop->size,
> > +					    GACC_FETCH, mop->key);
> >  			break;
> >  		}
> > -		r = read_guest(vcpu, mop->gaddr, mop->ar, tmpbuf, mop->size);
> > +		r = read_guest_with_key(vcpu, mop->gaddr, mop->ar, tmpbuf,
> > +					mop->size, mop->key);
> >  		if (r == 0) {
> >  			if (copy_to_user(uaddr, tmpbuf, mop->size))
> >  				r = -EFAULT;
> > @@ -4722,15 +4731,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, mop->ar, mop->size,
> > +					    GACC_STORE, mop->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, mop->ar, tmpbuf,
> > +					 mop->size, mop->key);
> >  		break;
> >  	}
> >    
> 
> [...]
Janosch Frank Feb. 11, 2022, 8:53 a.m. UTC | #6 
On 2/10/22 10:29, Christian Borntraeger wrote:
> Am 09.02.22 um 18:04 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@linux.ibm.com>
>> Acked-by: Janosch Frank <frankja@linux.ibm.com>
>> Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> 
> Claudio, Janosch, can you confirm that this is still valid?

Reviewed-by: Janosch Frank <frankja@linux.ibm.com>

> 
> 
> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> 
> minor thing below
>> ---
>>    arch/s390/kvm/kvm-s390.c | 30 ++++++++++++++++++++----------
>>    include/uapi/linux/kvm.h |  6 +++++-
>>    2 files changed, 25 insertions(+), 11 deletions(-)
>>
>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>> index cf347e1a4f17..85763ec7bc60 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>
> 
> do we still need that after the changes?
> 
>>    
>>    #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)
>>    {
>> @@ -4690,17 +4696,19 @@ static long kvm_s390_guest_mem_op(struct kvm_vcpu *vcpu,
>>    	void *tmpbuf = NULL;
>>    	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)
>>    		return -EINVAL;
>> -
>>    	if (mop->size > MEM_OP_MAX_SIZE)
>>    		return -E2BIG;
>> -
>>    	if (kvm_s390_pv_cpu_is_protected(vcpu))
>>    		return -EINVAL;
>> -

You want to make it look like the sida op?
Personally I'd leave it as is.

>> +	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)
>> @@ -4710,11 +4718,12 @@ static long kvm_s390_guest_mem_op(struct kvm_vcpu *vcpu,
>>    	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, mop->ar, mop->size,
>> +					    GACC_FETCH, mop->key);
>>    			break;
>>    		}
>> -		r = read_guest(vcpu, mop->gaddr, mop->ar, tmpbuf, mop->size);
>> +		r = read_guest_with_key(vcpu, mop->gaddr, mop->ar, tmpbuf,
>> +					mop->size, mop->key);
>>    		if (r == 0) {
>>    			if (copy_to_user(uaddr, tmpbuf, mop->size))
>>    				r = -EFAULT;
>> @@ -4722,15 +4731,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, mop->ar, mop->size,
>> +					    GACC_STORE, mop->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, mop->ar, tmpbuf,
>> +					 mop->size, mop->key);
>>    		break;
>>    	}
>>    
>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
>> index b46bcdb0cab1..44558cf4c52e 100644
>> --- a/include/uapi/linux/kvm.h
>> +++ b/include/uapi/linux/kvm.h
>> @@ -562,7 +562,10 @@ 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, ignored if flag unset */
>> +		};
>>    		__u32 sida_offset; /* offset into the sida */
>>    		__u8 reserved[32]; /* should be set to 0 */
>>    	};
>> @@ -575,6 +578,7 @@ struct kvm_s390_mem_op {
>>    /* 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)
>> +#define KVM_S390_MEMOP_F_SKEY_PROTECTION	(1ULL << 2)
>>    
>>    /* for KVM_INTERRUPT */
>>    struct kvm_interrupt {
diff mbox series

Patch

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index cf347e1a4f17..85763ec7bc60 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)
 {
@@ -4690,17 +4696,19 @@  static long kvm_s390_guest_mem_op(struct kvm_vcpu *vcpu,
 	void *tmpbuf = NULL;
 	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)
 		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_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)
@@ -4710,11 +4718,12 @@  static long kvm_s390_guest_mem_op(struct kvm_vcpu *vcpu,
 	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, mop->ar, mop->size,
+					    GACC_FETCH, mop->key);
 			break;
 		}
-		r = read_guest(vcpu, mop->gaddr, mop->ar, tmpbuf, mop->size);
+		r = read_guest_with_key(vcpu, mop->gaddr, mop->ar, tmpbuf,
+					mop->size, mop->key);
 		if (r == 0) {
 			if (copy_to_user(uaddr, tmpbuf, mop->size))
 				r = -EFAULT;
@@ -4722,15 +4731,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, mop->ar, mop->size,
+					    GACC_STORE, mop->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, mop->ar, tmpbuf,
+					 mop->size, mop->key);
 		break;
 	}
 
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index b46bcdb0cab1..44558cf4c52e 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -562,7 +562,10 @@  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, ignored if flag unset */
+		};
 		__u32 sida_offset; /* offset into the sida */
 		__u8 reserved[32]; /* should be set to 0 */
 	};
@@ -575,6 +578,7 @@  struct kvm_s390_mem_op {
 /* 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)
+#define KVM_S390_MEMOP_F_SKEY_PROTECTION	(1ULL << 2)
 
 /* for KVM_INTERRUPT */
 struct kvm_interrupt {