Re: [PATCH v7 1/5] s390/zcrypt: Fix CPRB memory allocation in zcrypt misc code

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Medium] Incomplete memory scrubbing in CPRB free functions leaves padding bytes unzeroed.

Pre-existing issues:
- [High] Information leak of uninitialized kernel heap memory to AP hardware due to unpadded EP11 request lengths.
- [High] Incomplete fix in EP11 CPRB memory allocation causes valid 4-byte padded hardware replies to be rejected with -EMSGSIZE.
--

--- Patch [1]: [PATCH v7 1/5] s390/zcrypt: Fix CPRB memory allocation in zcrypt misc code ---
commit cba05c09d2194f550fb91ddbd8234cd389b1848a
Author: Harald Freudenberger <freude@linux.ibm.com>

s390/zcrypt: Fix CPRB memory allocation in zcrypt misc code

This commit rounds up the memory allocation for CPRB structures to a
multiple of 4 bytes. It ensures the base CPRB and parameter block are
aligned to a 4-byte boundary, which is required by the zcrypt layer.

> diff --git a/drivers/s390/crypto/zcrypt_ccamisc.c b/drivers/s390/crypto/zcrypt_ccamisc.c
> index 84936a795b956..0aa72593ae2bd 100644
> --- a/drivers/s390/crypto/zcrypt_ccamisc.c
> +++ b/drivers/s390/crypto/zcrypt_ccamisc.c
> @@ -231,6 +232,10 @@ EXPORT_SYMBOL(cca_check_sececckeytoken);
>   * block, reply CPRB and reply param block and fill in values
>   * for the common fields. Returns 0 on success or errno value
>   * on failure.
> + * It is guaranteed that request and a possible param block
> + * are aligned to a 4 byte boundary. Furthermore if a param
> + * block is used, the memory allocated for this is rounded up to
> + * the next multiple of 4 bytes.
>   */
>  static int alloc_and_prep_cprbmem(size_t paramblen,
>  				  u8 **p_cprb_mem,
> @@ -239,7 +244,8 @@ static int alloc_and_prep_cprbmem(size_t paramblen,
>  				  u32 xflags)
>  {
>  	u8 *cprbmem = NULL;
> -	size_t cprbplusparamblen = sizeof(struct CPRBX) + paramblen;
> +	size_t cprbplusparamblen =
> +		ALIGN(sizeof(struct CPRBX), 4) + ALIGN(paramblen, 4);

[Severity: Medium]
If the allocation size is rounded up here, do the corresponding
free_cprbmem() functions need to be updated to clear the padded size?

For example, in drivers/s390/crypto/zcrypt_ccamisc.c:free_cprbmem():

    memzero_explicit(mem, 2 * (sizeof(struct CPRBX) + paramblen));

And in drivers/s390/crypto/zcrypt_ep11misc.c:free_cprbmem():

    memzero_explicit(mem, sizeof(struct ep11_cprb) + payload_len);

Currently they use the unpadded lengths. Can this leave the padding bytes
unzeroed when freed, potentially leaking sensitive hardware response data
that spills into the padding?

[ ... ]

> diff --git a/drivers/s390/crypto/zcrypt_ep11misc.c b/drivers/s390/crypto/zcrypt_ep11misc.c
> index 3dda9589f2b95..e604c100ccce2 100644
> --- a/drivers/s390/crypto/zcrypt_ep11misc.c
> +++ b/drivers/s390/crypto/zcrypt_ep11misc.c
> @@ -355,21 +356,24 @@ EXPORT_SYMBOL(ep11_check_aes_key);
>  
>  /*
>   * Allocate and prepare ep11 cprb plus additional payload.
> + * It is guaranteed that the memory is aligned to a 4 byte boundary.
> + * Furthermore the memory allocation is rounded up to the next
> + * multiple of 4 bytes (with taking the payload_len into account).
>   */
>  static void *alloc_cprbmem(size_t payload_len, u32 xflags)
>  {
> -	size_t len = sizeof(struct ep11_cprb) + payload_len;
> +	size_t memlen = ALIGN(sizeof(struct ep11_cprb) + payload_len, 4);
>  	struct ep11_cprb *cprb = NULL;
>  
>  	if (xflags & ZCRYPT_XFLAG_NOMEMALLOC) {
> -		if (len <= CPRB_MEMPOOL_ITEM_SIZE)
> +		if (memlen <= CPRB_MEMPOOL_ITEM_SIZE)
>  			cprb = mempool_alloc_preallocated(cprb_mempool);
>  	} else {
> -		cprb = kmalloc(len, GFP_KERNEL);
> +		cprb = kmalloc(memlen, GFP_KERNEL);
>  	}
>  	if (!cprb)
>  		return NULL;
> -	memset(cprb, 0, len);
> +	memset(cprb, 0, memlen);

[Severity: High]
This is a pre-existing issue, but since we are aligning EP11 lengths, does
the AP message creation leak uninitialized kernel memory?

In drivers/s390/crypto/zcrypt_msgtype6.c:xcrb_msg_to_type6_ep11cprb_msgx(),
the message length is rounded up:

    ap_msg->len = sizeof(struct type6_hdr) + CEIL4(xcrb->req_len);

But only the unpadded length is copied into the buffer:

    if (z_copy_from_user(userspace, msg->userdata, (char __force __user *)xcrb->req, xcrb->req_len))

Since drivers/s390/crypto/ap_bus.c:ap_init_apmsg() allocates the buffer
without __GFP_ZERO:

    ap_msg->msg = kmalloc(maxmsgsize, GFP_KERNEL);

Can the padding gap between xcrb->req_len and CEIL4(xcrb->req_len) leak up
to 3 bytes of uninitialized kernel heap memory to the AP hardware?


[Severity: High]
This is a pre-existing issue, but does aligning the memory allocation
resolve the EMSGSIZE failures if prep_urb() still uses the unpadded length?

In drivers/s390/crypto/zcrypt_ep11misc.c:prep_urb(), the unpadded length
is passed:

    u->resp_len = rep_len;

When the hardware replies with a 4-byte padded length, 
convert_type86_ep11_xcrb() in drivers/s390/crypto/zcrypt_msgtype6.c
checks it against the unpadded resp_len:

    if (xcrb->resp_len < msg->fmt2.count1) {
        pr_debug("resp_len %u < required %u => EMSGSIZE\n",
                 (unsigned int)xcrb->resp_len, msg->fmt2.count1);
        return -EMSGSIZE;
    }

Does this cause valid padded hardware replies to be rejected with
-EMSGSIZE for unaligned requests?

>  
>  	cprb->cprb_len = sizeof(struct ep11_cprb);
>  	cprb->cprb_ver_id = 0x04;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729095318.184141-1-freude@linux.ibm.com?part=1