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

On 8/3/26 09:55, Harald Freudenberger wrote:
> 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.

Ok.

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

Ok, than you can add my R-b to this patch.

Reviewed-by: Holger Dengler <[email protected]>

> 
>>
>> The rest looks good to me.

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