Messages in this thread | | | Subject | Re: [PATCH v2] s390/pkey: Use memdup_user() rather than duplicating its implementation | From | Markus Elfring <> | Date | Thu, 7 Nov 2019 14:45:54 +0100 |
| |
>> Reuse existing functionality from memdup_user() instead of keeping >> duplicate source code. >> >> Generated by: scripts/coccinelle/api/memdup_user.cocci >> >> Delete local variables which became unnecessary with this refactoring >> in two function implementations. >> >> Fixes: f2bbc96e7cfad3891b7bf9bd3e566b9b7ab4553d ("s390/pkey: add CCA AES cipher key support") > > With that patch description, the Fixes tag is wrong...but (see below)
I wonder about such a conclusion together with your subsequent feedback.
>> static void *_copy_apqns_from_user(void __user *uapqns, size_t nr_apqns) >> { > > This part below is not an equivalent replacement.
The shown refactoring provides also different run time characteristics, doesn't it?
> In fact you are fixing a bug here...
Thanks for your acknowledgement.
>> - void *kapqns = NULL; >> - size_t nbytes; >> - >> - if (uapqns && nr_apqns > 0) { >> - nbytes = nr_apqns * sizeof(struct pkey_apqn); >> - kapqns = kmalloc(nbytes, GFP_KERNEL); >> - if (!kapqns) >> - return ERR_PTR(-ENOMEM); >> - if (copy_from_user(kapqns, uapqns, nbytes)) > > .... here we would need to kfree kapqns, but we do not. So this is > a memory leak. Isnt it?
This is another undesirable software weakness because of incomplete exception handling in the previous copy approach.
> So indeed this is fixing something. But please rework your the patch > description accordingly.
Can the final committer pick the opportunity up to extend the change description another bit?
>> + if (!uapqns || nr_apqns <= 0) >> + return NULL; >> >> - return kapqns; >> + return memdup_user(uapqns, nr_apqns * sizeof(struct pkey_apqn)); >> }
Would you like to add any tags for the presented software improvement?
Regards, Markus
| |