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

On 2026-07-29 12:15, [email protected] wrote:
> 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 <[email protected]>
> 
> 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?

Valid and will be addressed with version 8 of this patch series.

> 
> [ ... ]
> 
>> 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?
> 

Addressed with patch #5 of this series.

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

This may be valid but will eventually be covered with another patch 
outside of this series.