Re: [PATCH v6 04/25] s390/zcrypt: Avoid alloc and copy of ep11 targets if kernelspace cprb

[Holger Dengler <dengler@linux.ibm.com>]

On 16/04/2025 15:57, Harald Freudenberger wrote:
> If there is a target list of APQNs given when an CPRB is
> to be send via zcrypt_send_ep11_cprb() there is always a
> kmalloc() done and the targets are copied via z_copy_from_user.
> 
> As there are callers from kernel space (zcrypt_ep11misc.c)
> which signal this via the userspace parameter improve this
> code to directly use the given target list in case of
> kernelspace thus removing the unnecessary memory alloc
> and mem copy.
> 
> Signed-off-by: Harald Freudenberger <freude@linux.ibm.com>

With the change below
Reviewed-by: Holger Dengler <dengler@linux.ibm.com>

> ---
>  drivers/s390/crypto/zcrypt_api.c | 41 ++++++++++++++++----------------
>  1 file changed, 20 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/s390/crypto/zcrypt_api.c b/drivers/s390/crypto/zcrypt_api.c
> index ad09c5306e28..f96f596e578f 100644
> --- a/drivers/s390/crypto/zcrypt_api.c
> +++ b/drivers/s390/crypto/zcrypt_api.c
[...]
> @@ -1052,29 +1052,28 @@ static long _zcrypt_send_ep11_cprb(bool userspace, struct ap_perms *perms,
>  	target_num = (unsigned short)xcrb->targets_num;
>  
>  	/* empty list indicates autoselect (all available targets) */
> -	targets = NULL;
> +	rc = -ENOMEM;
>  	if (target_num != 0) {
>  		struct ep11_target_dev __user *uptr;
>  
> -		targets = kcalloc(target_num, sizeof(*targets), GFP_KERNEL);
> -		if (!targets) {
> -			func_code = 0;
> -			rc = -ENOMEM;
> -			goto out;
> -		}
> -
> -		uptr = (struct ep11_target_dev __force __user *)xcrb->targets;
> -		if (z_copy_from_user(userspace, targets, uptr,
> -				     target_num * sizeof(*targets))) {
> -			func_code = 0;
> -			rc = -EFAULT;
> -			goto out_free;
> +		if (userspace) {
> +			targets = kcalloc(target_num, sizeof(*targets), GFP_KERNEL);
> +			if (!targets)
> +				goto out;
> +			uptr = (struct ep11_target_dev __force __user *)xcrb->targets;

I mentioned this earlier already, the cast is not required here, because xcrb->targets is always a user pointer in this if clause.

> +			if (copy_from_user(targets, uptr,
> +					   target_num * sizeof(*targets))) {

You can use xcrb->targets directly without any cast (and also drop uptr).

if (copy_from_user(targets, xcrb->targets,
    target_num * sizeof(*targets))) {

[...]

-- 
Mit freundlichen Grüßen / Kind regards
Holger Dengler
--
IBM Systems, Linux on IBM Z Development
dengler@linux.ibm.com