Re: [PATCH v3 1/1] s390/zcrypt: Improve zcrypt reply message verification checks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Low] The `check_add_overflow()` error paths are mathematically unreachable because 32-bit operands cannot overflow a 64-bit `size_t` destination.

Pre-existing issues:
- [High] Unvalidated `offset1` and `count1` fields in `zcrypt_msgtype6_receive()` lead to a kernel memory leak in `convert_type86_xcrb()` when `count2 != 0`.
--

--- Patch [1]: [PATCH v3 1/1] s390/zcrypt: Improve zcrypt reply message verification checks ---
commit b96e9311c01803c5c1f3023f735aa7268d688c81
Author: Harald Freudenberger <freude@linux.ibm.com>

s390/zcrypt: Improve zcrypt reply message verification checks

This commit adds and improves checks related to buffer sizes and reply sizes
in the handling of replies from crypto cards for CCA and EP11 messages. It
changes length variables from int to size_t and adds overflow checks to
inspect length fields more closely.

Cc: stable@vger.kernel.org

> diff --git a/drivers/s390/crypto/zcrypt_msgtype6.c b/drivers/s390/crypto/zcrypt_msgtype6.c
> index 40f72cdf284d3..5deef2e38f065 100644
> --- a/drivers/s390/crypto/zcrypt_msgtype6.c
> +++ b/drivers/s390/crypto/zcrypt_msgtype6.c

[ ... ]

> @@ -874,10 +881,23 @@ static void zcrypt_msgtype6_receive(struct ap_queue *aq,
>  			msg->len = len;
>  			break;
>  		case CEXXC_RESPONSE_TYPE_XCRB:
> -			if (t86r->fmt2.count2)
> -				len = t86r->fmt2.offset2 + t86r->fmt2.count2;
> -			else
> -				len = t86r->fmt2.offset1 + t86r->fmt2.count1;
> +			if (t86r->fmt2.count2) {
> +				if (check_add_overflow(t86r->fmt2.offset2,
> +						       t86r->fmt2.count2,
> +						       &len)) {

[Severity: High]
This is a pre-existing issue, but does this conditional validation leave
offset1 and count1 unchecked when count2 is non-zero?

If the hardware or firmware returns a malformed XCRB reply with a non-zero
count2, only offset2 and count2 are validated here against the buffer bounds.

However, convert_type86_xcrb() appears to unconditionally copy count1 bytes
from offset1 to userspace without validating if they remain within the bounds
of the reply message:

drivers/s390/crypto/zcrypt_msgtype6.c:convert_type86_xcrb() {
    ...
    if (z_copy_to_user(userspace, xcrb->reply_control_blk_addr,
               data + msg->fmt2.offset1, msg->fmt2.count1))
        return -EFAULT;
    ...
}

Can this lead to an out-of-bounds memory read and an information leak to
userspace?

[Severity: Low]
Is this check_add_overflow() mathematically reachable?

Since len was changed to a size_t, which is strictly 64-bit on the s390x
architecture, and the offset and count variables are 32-bit unsigned integers,
their sum can never overflow the 64-bit destination.

Does this make the overflow check always evaluate to false and the error
branch dead code?

> +					pr_debug("len overflow => EMSGSIZE\n");
> +					msg->rc = -EMSGSIZE;
> +					goto out;
> +				}
> +			} else {
> +				if (check_add_overflow(t86r->fmt2.offset1,
> +						       t86r->fmt2.count1,
> +						       &len)) {
> +					pr_debug("len overflow => EMSGSIZE\n");
> +					msg->rc = -EMSGSIZE;
> +					goto out;
> +				}
> +			}
>  			if (len > reply->bufsize || len > msg->bufsize ||
>  			    len != reply->len) {
>  				pr_debug("len mismatch => EMSGSIZE\n");

[ ... ]

> @@ -927,7 +947,12 @@ static void zcrypt_msgtype6_receive_ep11(struct ap_queue *aq,
>  	    t86r->cprbx.cprb_ver_id == 0x04) {
>  		switch (resp_type->type) {
>  		case CEXXC_RESPONSE_TYPE_EP11:
> -			len = t86r->fmt2.offset1 + t86r->fmt2.count1;
> +			if (check_add_overflow(t86r->fmt2.offset1,
> +					       t86r->fmt2.count1, &len)) {

[Severity: Low]
Is this check_add_overflow() also mathematically unreachable for the same
reasons mentioned above?

> +				pr_debug("len overflow => EMSGSIZE\n");
> +				msg->rc = -EMSGSIZE;
> +				goto out;
> +			}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709092219.57356-2-freude@linux.ibm.com?part=1