Re: [PATCH v9 2/5] s390/zcrypt: Improve CCA CPRB length and overflow checks

[Harald Freudenberger <freude@linux.ibm.com>]

On 2026-07-29 18:19, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider:
> 
> New issues:
> - [High] Uninitialized memory read due to incorrect bounds check when
> copying subfunction code.
> - [Low] Redundant and mathematically impossible integer overflow
> checks on 64-bit sum calculations.
> 
> Pre-existing issues:
> - [High] Missing minimum size validation for EP11 CPRB messages allows
> reading uninitialized memory.
> --
> 
> --- Patch [2]: [PATCH v9 2/5] s390/zcrypt: Improve CCA CPRB length and
> overflow checks ---
> commit 93cf9dfadb28a27fc37f0252a7721661ccec467d
> Author: Harald Freudenberger <freude@linux.ibm.com>
> s390/zcrypt: Improve CCA CPRB length and overflow checks
> 
> This commit addresses integer overflow and missing bounds checks in the
> xcrb_msg_to_type6cprb_msgx function during CCA CPRB processing. It 
> introduces
> 64-bit size_t variables for length arithmetic and explicitly bounds the
> calculated lengths to U32_MAX. Minimum control block sizes are also 
> validated
> prior to copying data from userspace.
> 
>> --- a/drivers/s390/crypto/zcrypt_msgtype6.c
>> +++ b/drivers/s390/crypto/zcrypt_msgtype6.c
> 
> [ ... ]
> 
>> @@ -342,49 +342,40 @@ static int xcrb_msg_to_type6cprb_msgx(bool 
>> userspace, struct ap_message *ap_msg,
>> +	req_sumlen = req_cblen + xcrb->request_data_length;
>> +	if (req_sumlen > U32_MAX)
>> +		return -EINVAL;
>> +	if (req_cblen <= xcrb->request_data_length ?
>>  	    req_sumlen < xcrb->request_data_length :
>> -	    req_sumlen < CEIL4(xcrb->request_control_blk_length)) {
>> +	    req_sumlen < req_cblen) {
>>  		return -EINVAL;
>>  	}
> 
> [Severity: Low]
> Does this ternary check provide any value now that the lengths are 
> using
> 64-bit size_t?
> 
> Since req_cblen and req_sumlen are bounded to U32_MAX, their sum cannot
> overflow a 64-bit variable, making this condition impossible to reach.
> 
> [ ... ]
> 
>> @@ -404,8 +395,8 @@ static int xcrb_msg_to_type6cprb_msgx(bool 
>> userspace, struct ap_message *ap_msg,
>>  			     xcrb->request_control_blk_addr,
>>  			     xcrb->request_control_blk_length))
>>  		return -EFAULT;
>> -	if (msg->cprbx.cprb_len + sizeof(msg->hdr.function_code) >
>> -	    xcrb->request_control_blk_length)
>> +	/* copy subfunction code into AP msg type 6 function code field */
>> +	if (msg->cprbx.cprb_len > req_cblen - 
>> sizeof(msg->hdr.function_code))
>>  		return -EINVAL;
>>  	function_code = ((unsigned char *)&msg->cprbx) + 
>> msg->cprbx.cprb_len;
>>  	memcpy(msg->hdr.function_code, function_code,
> 
> [Severity: High]
> Can this modification lead to an uninitialized memory read?
> 
> If an attacker passes a CPRB request where 
> xcrb->request_control_blk_length
> is 13 bytes, the aligned req_cblen becomes 16. If they also set
> cprbx.cprb_len to 14, z_copy_from_user() will copy only 13 bytes, 
> leaving
> the padding uninitialized.
> 
> The new bounds check allows cprb_len = 14 (since 14 is not greater than
> 16 - 2), and the code then copies 2 uninitialized bytes starting at 
> offset 14
> into msg->hdr.function_code. This could leak kernel heap memory to the
> crypto device and influence routing decisions.

already addressed with patch #5 of this series.

> 
> [Severity: High]
> This isn't a bug introduced by this patch, but I noticed a similar 
> missing
> minimum size validation in xcrb_msg_to_type6_ep11cprb_msgx().
> 
> drivers/s390/crypto/zcrypt_msgtype6.c:xcrb_msg_to_type6_ep11cprb_msgx() 
> {
> ...
> 	/* Import CPRB data from the ioctl input parameter */
> 	if (z_copy_from_user(userspace, msg->userdata,
> 			     (char __force __user *)xcrb->req, xcrb->req_len)) {
> 		return -EFAULT;
> 	}
> 
> 	if ((msg->pld_lenfmt & 0x80) == 0x80) {
> ...
> }
> 
> Does this function also need a minimum size validation?
> 
> If a user issues the ZSENDEP11CPRB ioctl with an EP11 user request 
> block
> where req_len is extremely small (e.g., 0), z_copy_from_user() will 
> succeed
> by copying 0 bytes. The kernel then evaluates fields like 
> msg->pld_lenfmt,
> msg->cprbx.flags, and msg->cprbx.target_id from uninitialized kernel 
> heap
> memory, potentially allowing bypasses of authorization boundaries.

already addressed with patch #3 of this series.