linux-kernel - Re: s390/pkey: Use memdup_user() rather than duplicating its implementation

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <0f90b278-7b3e-6509-1633-301d16513c5d@web.de>
Date:   Wed, 6 Nov 2019 14:00:33 +0100
From:   Markus Elfring <Markus.Elfring@....de>
To:     Joe Perches <joe@...ches.com>, linux-s390@...r.kernel.org,
        kernel-janitors@...r.kernel.org
Cc:     Christian Bornträger <borntraeger@...ibm.com>,
        Harald Freudenberger <freude@...ux.ibm.com>,
        Heiko Carstens <heiko.carstens@...ibm.com>,
        Ingo Franzki <ifranzki@...ux.ibm.com>,
        Vasily Gorbik <gor@...ux.ibm.com>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: s390/pkey: Use memdup_user() rather than duplicating its
 implementation

>> Reuse existing functionality from memdup_user() instead of keeping
>> duplicate source code.
>>
>> Generated by: scripts/coccinelle/api/memdup_user.cocci
…
>> Fixes: f2bbc96e7cfad3891b7bf9bd3e566b9b7ab4553d ("s390/pkey: add CCA AES cipher key support")
>
> This doesn't fix anything

How would you categorise the proposed source code reduction and software reuse?


> and the Fixes: line is not appropriate.

Will the development opinions vary between contributors?


>> +	return !ukey || keylen < MINKEYBLOBSIZE || keylen > KEYBLOBBUFSIZE
>> +	       ? ERR_PTR(-EINVAL)
>> +	       : memdup_user(ukey, keylen);
>
> This is a very poor use of ternary ?: code.

The conditional operator is applied once more in the intended way,
isn't it?


> This is much more readable for humans.

Readability preferences can vary also for this code structure.


>> +	return uapqns && nr_apqns > 0
>> +	       ? memdup_user(uapqns, nr_apqns * sizeof(struct pkey_apqn))
>> +	       : NULL;
>
> And here you reverse the form of the earlier block.

I kept the previous condition specification.


> Please be consistent and use this style:

Would further developers like to get a more verbose variant for this
software transformation?

Regards,
Markus