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

On 2026-07-31 12:35, Holger Dengler wrote:
> On 7/30/26 16:15, Harald Freudenberger wrote:
>> The xcrb_msg_to_type6cprb_msgx() function lacks proper input
>> validation, creating security vulnerabilities:
>> 1. Integer overflow after CEIL4 alignment: Signed int variables could
>>    overflow during 4-byte boundary alignment, causing undersized
>>    buffer allocations or incorrect bounds checking.
>> 2. Missing minimum size validation: The CPRBX structure is copied from
>>    userspace without verifying sufficient buffer length. Undersized
>>    buffers cause uninitialized memory access when reading structure
>>    fields like cprbx.cprb_len and cprbx.domain.
>> 3. Arithmetic overflow in sum calculations: Adding control block and
>>    data block sizes could overflow, bypassing size checks and enabling
>>    buffer overflows.
>> 
>> Fix by using size_t for length calculations, adding U32_MAX boundary
>> checks after alignment, validating minimum control block size before
>> copying from userspace, and detecting sum calculation overflows.
>> 
>> Fixes: e2c6d91eb8b1 ("s390/zcrypt: Rework domain processing within 
>> zcrypt device driver")
>> Signed-off-by: Harald Freudenberger <[email protected]>
>> Cc: [email protected] # 7.1+
>> ---
>>  drivers/s390/crypto/zcrypt_msgtype6.c | 78 
>> +++++++++++++--------------
>>  1 file changed, 36 insertions(+), 42 deletions(-)
>> 
>> diff --git a/drivers/s390/crypto/zcrypt_msgtype6.c 
>> b/drivers/s390/crypto/zcrypt_msgtype6.c
>> index 40f72cdf284d..fb37e28c8242 100644
>> --- 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,
>>  		};
>>  	} __packed * msg = ap_msg->msg;
>> 
>> -	int rcblen = CEIL4(xcrb->request_control_blk_length);
>> -	int req_sumlen, resp_sumlen;
>> -	char *req_data = ap_msg->msg + sizeof(struct type6_hdr) + rcblen;
>> -	char *function_code;
>> +	size_t req_cblen, rep_cblen, req_sumlen, rep_sumlen;
>> +	char *function_code, *req_data;
>> 
>> -	if (CEIL4(xcrb->request_control_blk_length) <
>> -			xcrb->request_control_blk_length)
>> -		return -EINVAL; /* overflow after alignment*/
>> -
>> -	/* length checks */
>> +	/* request length and overflow checks */
>> +	if (xcrb->request_control_blk_length < sizeof(struct CPRBX))
>> +		return -EINVAL;
>> +	req_cblen = CEIL4((size_t)xcrb->request_control_blk_length);
>> +	if (req_cblen > U32_MAX)
>> +		return -EINVAL;
>>  	ap_msg->len = sizeof(struct type6_hdr) +
>> -		CEIL4(xcrb->request_control_blk_length) +
>> -		xcrb->request_data_length;
>> +		req_cblen + xcrb->request_data_length;
>>  	if (ap_msg->len > ap_msg->bufsize)
>>  		return -EINVAL;
>> -
>> -	/*
>> -	 * Overflow check
>> -	 * sum must be greater (or equal) than the largest operand
>> -	 */
>> -	req_sumlen = CEIL4(xcrb->request_control_blk_length) +
>> -			xcrb->request_data_length;
>> -	if ((CEIL4(xcrb->request_control_blk_length) <=
>> -	     xcrb->request_data_length) ?
>> +	req_sumlen = req_cblen + xcrb->request_data_length;
> 
> The req_sumlen is also used for the calculation of ap_msg->len, right?
> Why not moving the req_sumlen calculation and checks up and use it 
> there?
> 
> req_sumlen = req_cblen + xcrb->request_data_length;
> if (req_sumlen > U32_MAX)
> [...]
> ap_msg->len = sizeof(struct type6_hdr) + req_sumlen;
> if (ap_msg->len > ap_msg->bufsize)
> [...]
> 

Done

> And another question about the aligned buffer lengths:
> We have the request-control-block, followed by the request-data. Is 
> only
> the request-control-block required to be 4-byte aligned or also the
> request-data, or only both together (request-control-block and -data)?
> 
> Lets assume, request-control-block and -data length are both not 4-byte
> alligned. Do we need the padding between the cprb and the data or at 
> the
> end of both blocks or only after data?
> 
> Example:
> req-ctrl-blk: length 5
> req-data: length 5
> 
> With only cprb padded (--> req_sumlen: 13)
> | req-ctrl-blk[5] | pad[3] | req-data[5] |
> 
> 
> With both padded separately (--> req_sumlen: 16)
> | req-ctrl-blk[5] | pad[3] | req-data[5] | pad[3] |
> 
> With both padded together (--> req_sumlen: 12)
> | req-ctrl-blk[5] | req-data[5] | pad[2] |
> 
> 

I can only deduce this from the existing code.
But my suggestion is:
for a control block only:
   ctrl-block | pad
control + data block:
   ctrl-block | pad | data-block | pad
in a way that the start of each block is aligned to a 4 byte
boundary AND can be processed in hunks of 4 bytes (double word).
So we should make sure that the memory backing up an
ap message is allocated with respect to the padding.

>> +	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;
>>  	}
>> 
>> -	if (CEIL4(xcrb->reply_control_blk_length) <
>> -			xcrb->reply_control_blk_length)
>> -		return -EINVAL; /* overflow after alignment*/
>> -
>> -	/*
>> -	 * Overflow check
>> -	 * sum must be greater (or equal) than the largest operand
>> -	 */
>> -	resp_sumlen = CEIL4(xcrb->reply_control_blk_length) +
>> -			xcrb->reply_data_length;
>> -	if ((CEIL4(xcrb->reply_control_blk_length) <=
>> -	     xcrb->reply_data_length) ?
>> -	    resp_sumlen < xcrb->reply_data_length :
>> -	    resp_sumlen < CEIL4(xcrb->reply_control_blk_length)) {
>> +	/* reply length and overflow checks */
>> +	if (xcrb->reply_control_blk_length < sizeof(struct CPRBX))
>> +		return -EINVAL;
>> +	rep_cblen = CEIL4((size_t)xcrb->reply_control_blk_length);
>> +	if (rep_cblen > U32_MAX)
>> +		return -EINVAL;
>> +	rep_sumlen = rep_cblen + xcrb->reply_data_length;
> [...]
> 
> Before this change, resp_sumlen was 4-bate aligned, now only rep_cblen
> is aligned. Is thsi change intended? (the question is similar to the 
> one
> above)

There is no difference in the way how the reply sizes and limits are
calculated to the way how the request sizes and limits are computed.
And there should not be any difference.

> 
> The rest looks good to me.