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

Holger Dengler
2025-04-09T16:19:45Z

On 09/04/2025 16:02, 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>

I have a few comments below. The rest looks good to me.

> ---
> 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;

I would prefer to set rc in the kmalloc error handling. See below.

> if (target_num != 0) {
> struct ep11_target_dev __user *uptr;

*uptr is no longer required, as the userspace case is ensured by code.

>
> - 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;

Please set rc here. It makes it easier to read as it correlates with the other error case for copy_from_user().

if (!targets) {
rc = -ENOMEM;
goto out;
}

> + uptr = (struct ep11_target_dev __force __user *)xcrb->targets;
> + if (copy_from_user(targets, uptr,
> + target_num * sizeof(*targets))) {

The cast can be dropped and you can copy directly from the userspace pointer in xcrb.

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

> + rc = -EFAULT;
> + goto out;
> + }
> + } else {
> + targets = (struct ep11_target_dev __force __kernel *)xcrb->targets;
> }
> }
>
> rc = prep_ep11_ap_msg(userspace, xcrb, &ap_msg, &func_code, &domain);
> if (rc)
> - goto out_free;
> + goto out;
> print_hex_dump_debug("ep11req: ", DUMP_PREFIX_ADDRESS, 16, 1,
> ap_msg.msg, ap_msg.len, false);
>
[...]

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