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

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

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] |


> +	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)

The rest looks good to me.

-- 
Mit freundlichen Grüßen / Kind regards
Holger Dengler