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

[Finn Callies <fcallies@linux.ibm.com>]

On 04.08.26 16:49, Harald Freudenberger wrote:
> Add or improve checks related to buffer sizes and reply sizes to the
> handling of replies from the crypto cards for CCA, EP11 (AP message
> type 6) and ICA (AP type 50) messages. The verification code related
> to reply field length was not designed well and thus firmware
> deficiencies could lead to unexpected behavior in the zcrypt device
> driver. Thus improve the code to more closely inspect especially
> length fields at message replies.
> 
> Rework zcrypt_msgtype6_receive(), zcrypt_msgtype6_receive_ep11() and
> zcrypt_msgtype50_receive() to validate reply lengths more carefully
> before copying data back into the request buffer. Use size_t for
> length calculations, reject inconsistent reply sizes, and add
> defensive handling for short invalid replies. For XCRB replies,
> validate both reply segments and derive the effective message length
> from the covered range instead of trusting only the second segment.
> 
> Signed-off-by: Harald Freudenberger <freude@linux.ibm.com>
> ---
>   drivers/s390/crypto/zcrypt_msgtype50.c |  39 ++++---
>   drivers/s390/crypto/zcrypt_msgtype6.c  | 151 ++++++++++++++++---------
>   2 files changed, 126 insertions(+), 64 deletions(-)
> 
> diff --git a/drivers/s390/crypto/zcrypt_msgtype50.c b/drivers/s390/crypto/zcrypt_msgtype50.c
> index d6fc2d8e7fad..ef925b399806 100644
> --- a/drivers/s390/crypto/zcrypt_msgtype50.c
> +++ b/drivers/s390/crypto/zcrypt_msgtype50.c
> @@ -416,26 +416,39 @@ static void zcrypt_msgtype50_receive(struct ap_queue *aq,
>   		.reply_code = REP82_ERROR_MACHINE_FAILURE,
>   	};
>   	struct type80_hdr *t80h;
> -	int len;
> +	size_t len;
>   
>   	/* Copy the reply message to the request message buffer. */
>   	if (!reply)
>   		goto out;	/* ap_msg->rc indicates the error */
> +
>   	t80h = reply->msg;
> -	if (t80h->type == TYPE80_RSP_CODE) {
> -		len = t80h->len;
> -		if (len > reply->bufsize || len > msg->bufsize ||
> -		    len != reply->len) {
> -			pr_debug("len mismatch => EMSGSIZE\n");

I don't like this debug statement, its very undescriptive. What is len? 
What does it mismatch against? What does the mismatch mean?

The "=> EMSGSIZE" looks very uncommon for me as well, is this a common 
way of logging error paths?

> -			msg->rc = -EMSGSIZE;
> -			goto out;
> -		}
> -		memcpy(msg->msg, reply->msg, len);
> -		msg->len = len;
> -	} else {
> -		memcpy(msg->msg, reply->msg, sizeof(error_reply));
> +
> +	if (t80h->type != TYPE80_RSP_CODE) {
> +		if (reply->len < sizeof(error_reply))
> +			memcpy(msg->msg, &error_reply, sizeof(error_reply));
> +		else
> +			memcpy(msg->msg, reply->msg, sizeof(error_reply));
>   		msg->len = sizeof(error_reply);
> +		goto out;
> +	}
> +
> +	len = t80h->len;
> +	if (len != reply->len) {
> +		pr_warn_ratelimited("len %zu rpl.len %zu mismatch => EMSGSIZE\n",
> +				    len, reply->len);

Same here. What is len? rpl.len is a technical statement, but logs 
should be descriptive right?

> +		msg->rc = -EMSGSIZE;
> +		goto out;
>   	}
> +	if (len > reply->bufsize || len > msg->bufsize) {
> +		pr_warn_ratelimited("len %zu exceeds buf %zu/%zu => EMSGSIZE\n",
> +				    len, reply->bufsize, msg->bufsize);

Why buf instead of buffer? Why safe space here?

> +		msg->rc = -EMSGSIZE;
> +		goto out;
> +	}
> +	memcpy(msg->msg, reply->msg, len);
> +	msg->len = len;
> +
>   out:
>   	complete(&msg->response.work);
>   }
> diff --git a/drivers/s390/crypto/zcrypt_msgtype6.c b/drivers/s390/crypto/zcrypt_msgtype6.c
> index 3df1d676de5d..b98449913e24 100644
> --- a/drivers/s390/crypto/zcrypt_msgtype6.c
> +++ b/drivers/s390/crypto/zcrypt_msgtype6.c
> @@ -766,6 +766,13 @@ static int convert_type86_rng(struct zcrypt_queue *zq,
>   
>   	if (msg->cprbx.ccp_rtcode != 0 || msg->cprbx.ccp_rscode != 0)
>   		return -EINVAL;
> +	/*
> +	 * Note that offset2 and count2 have already been checked in
> +	 * zcrypt_msgtype6_receive(). So only check for not exceeding
> +	 * the hard coded rng buffer size.
> +	 */
> +	if (msg->fmt2.count2 > ZCRYPT_RNG_BUFFER_SIZE)
> +		return -EMSGSIZE;
>   	memcpy(buffer, data + msg->fmt2.offset2, msg->fmt2.count2);
>   	return msg->fmt2.count2;
>   }
> @@ -928,48 +935,75 @@ static void zcrypt_msgtype6_receive(struct ap_queue *aq,
>   	};
>   	struct ap_response_type *resp_type = &msg->response;
>   	struct type86x_reply *t86r;
> -	int len;
> +	size_t len, len1, len2 = 0;
>   
>   	/* Copy the reply message to the request message buffer. */
>   	if (!reply)
>   		goto out;	/* ap_msg->rc indicates the error */
> +
>   	t86r = reply->msg;
> -	if (t86r->hdr.type == TYPE86_RSP_CODE &&
> -	    t86r->cprbx.cprb_ver_id == 0x02) {
> -		switch (resp_type->type) {
> -		case CEXXC_RESPONSE_TYPE_ICA:
> -			len = sizeof(struct type86x_reply) + t86r->length;
> -			if (len > reply->bufsize || len > msg->bufsize ||
> -			    len != reply->len) {
> -				pr_debug("len mismatch => EMSGSIZE\n");
> -				msg->rc = -EMSGSIZE;
> -				goto out;
> -			}
> -			memcpy(msg->msg, reply->msg, len);
> -			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 (len > reply->bufsize || len > msg->bufsize ||
> -			    len != reply->len) {
> -				pr_debug("len mismatch => EMSGSIZE\n");
> +
> +	if (t86r->hdr.type != TYPE86_RSP_CODE ||
> +	    t86r->cprbx.cprb_ver_id != 0x02) {
> +		if (reply->len < sizeof(error_reply))
> +			memcpy(msg->msg, &error_reply, sizeof(error_reply));
> +		else
> +			memcpy(msg->msg, reply->msg, sizeof(error_reply));
> +		msg->len = sizeof(error_reply);
> +		goto out;
> +	}
> +
> +	switch (resp_type->type) {
> +	case CEXXC_RESPONSE_TYPE_ICA:
> +		len = sizeof(struct type86x_reply) + (size_t)t86r->length;
> +		break;
> +	case CEXXC_RESPONSE_TYPE_XCRB:
> +		len1 = (size_t)t86r->fmt2.offset1 + (size_t)t86r->fmt2.count1;
> +		if (t86r->fmt2.offset1 > reply->len ||
> +		    t86r->fmt2.count1 > reply->len) {
> +			pr_warn_ratelimited(
> +				"offset1 %u count1 %u rpl.len %zu mismatch => EMSGSIZE\n",
> +				t86r->fmt2.offset1, t86r->fmt2.count1,
> +				reply->len);

same here

> +			msg->rc = -EMSGSIZE;
> +			goto out;
> +		}
> +		if (t86r->fmt2.count2) {
> +			len2 = (size_t)t86r->fmt2.offset2 +
> +				(size_t)t86r->fmt2.count2;
> +			if (t86r->fmt2.offset2 > reply->len ||
> +			    t86r->fmt2.count2 > reply->len) {
> +				pr_warn_ratelimited(
> +					"offset2 %u count2 %u rpl.len %zu mismatch => EMSGSIZE\n",
> +					t86r->fmt2.offset2, t86r->fmt2.count2,
> +					reply->len);

same here

>   				msg->rc = -EMSGSIZE;
>   				goto out;
>   			}
> -			memcpy(msg->msg, reply->msg, len);
> -			msg->len = len;
> -			break;
> -		default:
> -			memcpy(msg->msg, &error_reply, sizeof(error_reply));
> -			msg->len = sizeof(error_reply);
>   		}
> -	} else {
> -		memcpy(msg->msg, reply->msg, sizeof(error_reply));
> +		len = max_t(size_t, len1, len2);
> +		break;
> +	default:
> +		memcpy(msg->msg, &error_reply, sizeof(error_reply));
>   		msg->len = sizeof(error_reply);
> +		goto out;
> +	}
> +
> +	if (len != reply->len) {
> +		pr_warn_ratelimited("len %zu rpl.len %zu mismatch => EMSGSIZE\n",
> +				    len, reply->len);

same here

> +		msg->rc = -EMSGSIZE;
> +		goto out;
>   	}
> +	if (len > reply->bufsize || len > msg->bufsize) {
> +		pr_warn_ratelimited("len %zu exceeds buf %zu/%zu => EMSGSIZE\n",
> +				    len, reply->bufsize, msg->bufsize);

same here

> +		msg->rc = -EMSGSIZE;
> +		goto out;
> +	}
> +	memcpy(msg->msg, reply->msg, len);
> +	msg->len = len;
> +
>   out:
>   	complete(&resp_type->work);
>   }
> @@ -992,34 +1026,49 @@ static void zcrypt_msgtype6_receive_ep11(struct ap_queue *aq,
>   	};
>   	struct ap_response_type *resp_type = &msg->response;
>   	struct type86_ep11_reply *t86r;
> -	int len;
> +	size_t len;
>   
>   	/* Copy the reply message to the request message buffer. */
>   	if (!reply)
>   		goto out;	/* ap_msg->rc indicates the error */
> +
>   	t86r = reply->msg;
> -	if (t86r->hdr.type == TYPE86_RSP_CODE &&
> -	    t86r->cprbx.cprb_ver_id == 0x04) {
> -		switch (resp_type->type) {
> -		case CEXXC_RESPONSE_TYPE_EP11:
> -			len = t86r->fmt2.offset1 + t86r->fmt2.count1;
> -			if (len > reply->bufsize || len > msg->bufsize ||
> -			    len != reply->len) {
> -				pr_debug("len mismatch => EMSGSIZE\n");
> -				msg->rc = -EMSGSIZE;
> -				goto out;
> -			}
> -			memcpy(msg->msg, reply->msg, len);
> -			msg->len = len;
> -			break;
> -		default:
> +
> +	if (t86r->hdr.type != TYPE86_RSP_CODE ||
> +	    t86r->cprbx.cprb_ver_id != 0x04) {
> +		if (reply->len < sizeof(error_reply))
>   			memcpy(msg->msg, &error_reply, sizeof(error_reply));
> -			msg->len = sizeof(error_reply);
> -		}
> -	} else {
> -		memcpy(msg->msg, reply->msg, sizeof(error_reply));
> +		else
> +			memcpy(msg->msg, reply->msg, sizeof(error_reply));
>   		msg->len = sizeof(error_reply);
> +		goto out;
>   	}
> +
> +	switch (resp_type->type) {
> +	case CEXXC_RESPONSE_TYPE_EP11:
> +		len = t86r->fmt2.offset1 + (size_t)t86r->fmt2.count1;
> +		break;
> +	default:
> +		memcpy(msg->msg, &error_reply, sizeof(error_reply));
> +		msg->len = sizeof(error_reply);
> +		goto out;
> +	}
> +
> +	if (len != reply->len) {
> +		pr_warn_ratelimited("len %zu rpl.len %zu mismatch => EMSGSIZE\n",
> +				    len, reply->len);

same here

> +		msg->rc = -EMSGSIZE;
> +		goto out;
> +	}
> +	if (len > reply->bufsize || len > msg->bufsize) {
> +		pr_warn_ratelimited("len %zu exceeds buf %zu/%zu => EMSGSIZE\n",
> +				    len, reply->bufsize, msg->bufsize);

same here

> +		msg->rc = -EMSGSIZE;
> +		goto out;
> +	}
> +	memcpy(msg->msg, reply->msg, len);
> +	msg->len = len;
> +
>   out:
>   	complete(&resp_type->work);
>   }