linux-kernel - Re: [PATCH v2 05/11] KVM: s390: Add optional storage key checking to MEMOP IOCTL

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <71f07914-d0b2-e98b-22b2-bc05f04df2da@linux.ibm.com>
Date:   Wed, 9 Feb 2022 09:49:21 +0100
From:   Janis Schoetterl-Glausch <scgl@...ux.ibm.com>
To:     Christian Borntraeger <borntraeger@...ux.ibm.com>,
        Heiko Carstens <hca@...ux.ibm.com>,
        Janosch Frank <frankja@...ux.ibm.com>
Cc:     Alexander Gordeev <agordeev@...ux.ibm.com>,
        Claudio Imbrenda <imbrenda@...ux.ibm.com>,
        David Hildenbrand <david@...hat.com>,
        Jonathan Corbet <corbet@....net>, kvm@...r.kernel.org,
        linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-s390@...r.kernel.org, Paolo Bonzini <pbonzini@...hat.com>,
        Sven Schnelle <svens@...ux.ibm.com>,
        Vasily Gorbik <gor@...ux.ibm.com>
Subject: Re: [PATCH v2 05/11] KVM: s390: Add optional storage key checking to
 MEMOP IOCTL

On 2/9/22 08:34, Christian Borntraeger wrote:
> 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@...ux.ibm.com>
>> Acked-by: Janosch Frank <frankja@...ux.ibm.com>
>> Reviewed-by: Claudio Imbrenda <imbrenda@...ux.ibm.com>
>> ---
>>   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?

It's so the check of reserved below works like that, they're all part of the anonymous union.
> 
>> +    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.

I can move the allocation down, goto out or get rid of the reserved check and keep everything as before.
First is simpler, but second makes handling that case more explicit and might help in the future.
Patch 6 has the same issue in the vm ioctl handler.
> 
> 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.

[...]