Re: [PATCH v7 07/25] s390/zcrypt: Introduce cprb mempool for ep11 misc functions

Heiko Carstens
2025-04-24T06:58:47Z

On Wed, Apr 23, 2025 at 12:15:16PM +0200, Harald Freudenberger wrote:
> Introduce a cprb mempool for the zcrypt ep11 misc functions
> (zcrypt_ep11misc.*) do some preparation rework to support
> a do-not-allocate path through some zcrypt ep11 misc functions.
>
> The mempool is controlled by the zcrypt module parameter
> "mempool_threshold" which shall control the minimal amount
> of memory items for CCA and EP11.
>
> The mempool shall support "mempool_threshold" requests/replies
> in parallel which means for EP11 to hold a send and receive
> buffer memory per request. Each of this cprb space items is
> limited to 8 KB. So by default the mempool consumes
> 5 * 2 * 8KB = 80KB
>
> If the mempool is depleted upon one ep11 misc functions is
> called with the ZCRYPT_XFLAG_NOMEMALLOC xflag set, the function
> will fail with -ENOMEM and the caller is responsible for taking
> further actions.
>
> This is only part of an rework to support a new xflag
> ZCRYPT_XFLAG_NOMEMALLOC but not yet complete.
>
> Signed-off-by: Harald Freudenberger <freude@linux.ibm.com>
> Reviewed-by: Holger Dengler <dengler@linux.ibm.com>
> ---
> drivers/s390/crypto/zcrypt_api.c | 6 ++
> drivers/s390/crypto/zcrypt_ep11misc.c | 149 +++++++++++++++-----------
> drivers/s390/crypto/zcrypt_ep11misc.h | 1 +
> 3 files changed, 95 insertions(+), 61 deletions(-)

clang says:

CC [M] drivers/s390/crypto/zcrypt_ep11misc.o
drivers/s390/crypto/zcrypt_ep11misc.c:1041:6: warning: variable 'rep_pl_size' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
1041 | if (!req)
| ^~~~
drivers/s390/crypto/zcrypt_ep11misc.c:1121:20: note: uninitialized use occurs here
1121 | free_cprbmem(rep, rep_pl_size, true, xflags);
| ^~~~~~~~~~~
drivers/s390/crypto/zcrypt_ep11misc.c:1041:2: note: remove the 'if' if its condition is always false
1041 | if (!req)
| ^~~~~~~~~
1042 | goto out;
| ~~~~~~~~
drivers/s390/crypto/zcrypt_ep11misc.c:1028:33: note: initialize the variable 'rep_pl_size' to silence this warning
1028 | size_t req_pl_size, rep_pl_size;
| ^
| = 0
1 warning generated.

> @@ -1095,9 +1117,8 @@ static int ep11_cryptsingle(u16 card, u16 domain,
> *outbufsize = n;
>
> out:
> - kfree(req);
> - kfree(rep);
> - kfree(urb);
> + free_cprbmem(req, req_pl_size, true, xflags);
> + free_cprbmem(rep, rep_pl_size, true, xflags);
> return rc;
> }

Which is this hunk.

In addition:

> @@ -1161,7 +1183,7 @@ static int _ep11_unwrapkey(u16 card, u16 domain,
> req_pl_size = sizeof(struct uw_req_pl) + (iv ? 16 : 0)
> + ASN1TAGLEN(keksize) + ASN1TAGLEN(0)
> + ASN1TAGLEN(pinblob_size) + ASN1TAGLEN(enckeysize);
> - req = alloc_cprb(req_pl_size);
> + req = alloc_cprbmem(req_pl_size, xflags);
> if (!req)
> goto out;
> req_pl = (struct uw_req_pl *)(((u8 *)req) + sizeof(*req));
> @@ -1197,22 +1219,19 @@ static int _ep11_unwrapkey(u16 card, u16 domain,
> p += asn1tag_write(p, 0x04, enckey, enckeysize);
>
> /* reply cprb and payload */
> - rep = alloc_cprb(sizeof(struct uw_rep_pl));
> + rep = alloc_cprbmem(sizeof(struct uw_rep_pl), xflags);
> if (!rep)
> goto out;
> rep_pl = (struct uw_rep_pl *)(((u8 *)rep) + sizeof(*rep));
>
> /* urb and target */
> - urb = kmalloc(sizeof(*urb), GFP_KERNEL);
> - if (!urb)
> - goto out;
> target.ap_id = card;
> target.dom_id = domain;
> - prep_urb(urb, &target, 1,
> + prep_urb(&urb, &target, 1,
> req, sizeof(*req) + req_pl_size,
> rep, sizeof(*rep) + sizeof(*rep_pl));
>
> - rc = zcrypt_send_ep11_cprb(urb, 0);
> + rc = zcrypt_send_ep11_cprb(&urb, xflags);
> if (rc) {
> ZCRYPT_DBF_ERR("%s zcrypt_send_ep11_cprb(card=%d dom=%d) failed, rc=%d\n",
> __func__, (int)card, (int)domain, rc);
> @@ -1245,9 +1264,8 @@ static int _ep11_unwrapkey(u16 card, u16 domain,
> *keybufsize = rep_pl->data_len;
>
> out:
> - kfree(req);
> - kfree(rep);
> - kfree(urb);
> + free_cprbmem(req, req_pl_size, true, xflags);

This would lead to a memset() with destination address 0 if alloc_cprbmem()
fails.

> @@ -1319,15 +1337,16 @@ static int _ep11_wrapkey(u16 card, u16 domain,
> } __packed * rep_pl;
> struct ep11_cprb *req = NULL, *rep = NULL;
> struct ep11_target_dev target;
> - struct ep11_urb *urb = NULL;
> + struct ep11_urb urb;
> size_t req_pl_size;
> int api, rc = -ENOMEM;
> u8 *p;
> + const u32 xflags = 0;
>
> /* request cprb and payload */
> req_pl_size = sizeof(struct wk_req_pl) + (iv ? 16 : 0)
> + ASN1TAGLEN(keysize) + 4;
> - req = alloc_cprb(req_pl_size);
> + req = alloc_cprbmem(req_pl_size, xflags);
> if (!req)
> goto out;
...
> @@ -1405,9 +1421,8 @@ static int _ep11_wrapkey(u16 card, u16 domain,
> *datasize = rep_pl->data_len;
>
> out:
> - kfree(req);
> - kfree(rep);
> - kfree(urb);
> + free_cprbmem(req, req_pl_size, true, xflags);

Same here.

Please check all error paths in all patches and resubmit.
I'm going to revert the whole series for now. Thanks!