Re: [PATCH v2 1/1] s390/zcrypt: Fix and 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:
- [High] Missing error code assignment (`msg->rc = -EMSGSIZE`) on short replies leads to kernel memory disclosure in multiple message receive functions.

Pre-existing issues:
- [Medium] Incomplete length validation for CEXXC_RESPONSE_TYPE_XCRB allows an out-of-bounds read of CPRBX fields in convert_type86_rng().
--

commit 4aedb5baa8548df5ca01ec9ef8a55fbf780a7ee8
Author: Harald Freudenberger <freude@linux.ibm.com>

s390/zcrypt: Fix and improve zcrypt reply message verification checks

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.

> diff --git a/drivers/s390/crypto/zcrypt_msgtype50.c b/drivers/s390/crypto/zcrypt_msgtype50.c
> index d6fc2d8e7fad9..80c447ea2d839 100644
> --- a/drivers/s390/crypto/zcrypt_msgtype50.c
> +++ b/drivers/s390/crypto/zcrypt_msgtype50.c
> @@ -416,26 +416,40 @@ 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");
> -			msg->rc = -EMSGSIZE;
> -			goto out;
> -		}
> -		memcpy(msg->msg, reply->msg, len);
> -		msg->len = len;
> -	} else {
> -		memcpy(msg->msg, reply->msg, sizeof(error_reply));
> +
> +	if (reply->len < sizeof(*t80h) ||
> +	    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;

[Severity: High]
Does this missing error code assignment on short replies inadvertently
validate them?

If the hardware sends a reply that is shorter than sizeof(*t80h) but has a
valid success header, this early bailout catches it but fails to set msg->rc
to an error code like -EMSGSIZE. Because the caller sees rc == 0, it may
parse the success header and read uninitialized/leftover memory from the
request buffer, potentially leaking kernel memory to userspace.

> +	}
> +
> +	len = t80h->len;

[ ... ]

> diff --git a/drivers/s390/crypto/zcrypt_msgtype6.c b/drivers/s390/crypto/zcrypt_msgtype6.c
> index 3df1d676de5d8..b53801b0a9710 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;

[Severity: Medium]
This isn't a bug introduced by this patch, but could this blindly access
ccp_rtcode and ccp_rscode if the hardware sent a truncated reply?

If a hardware or emulator responds to a CEXXC_RESPONSE_TYPE_XCRB request
with a truncated message that satisfies the 51-byte minlen checked in
zcrypt_msgtype6_receive(), but doesn't include the full CPRBX structure,
these fields may contain leftover data from the request buffer.

> +	/*
> +	 * 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,80 @@ 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 minlen, 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 == 0x02) {
> -		switch (resp_type->type) {
> +	minlen = sizeof(t86r->hdr) + sizeof(t86r->fmt2) +
> +		offsetof(struct CPRBX, cprb_ver_id) +
> +		sizeof(t86r->cprbx.cprb_ver_id);

[Severity: Medium]
This isn't a bug introduced by this patch, but does this minlen check
guarantee the rest of the CPRBX structure is present?

This check ensures the message has a valid cprb_ver_id (51 bytes), but it
might not guarantee the rest of the CPRBX structure is present before allowing
the CEXXC_RESPONSE_TYPE_XCRB branch to succeed, potentially leading to the
issue in convert_type86_rng() mentioned above.

> +
> +	if (reply->len < minlen ||
> +	    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;

[Severity: High]
Does this missing error code assignment (msg->rc = -EMSGSIZE) similarly lead
to unintended memory disclosures as in zcrypt_msgtype50_receive()?

For ICA requests, convert_type86_ica() could use the leftover count1 to copy
up to PAGE_SIZE bytes of the buffer back to userspace if the rc remains 0.

> +	}
> +
> +	switch (resp_type->type) {

[ ... ]

> @@ -992,34 +1031,53 @@ 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 minlen, 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) {
> +	minlen = sizeof(t86r->hdr) + sizeof(t86r->fmt2) +
> +		offsetof(struct ep11_cprb, cprb_ver_id) +
> +		sizeof(t86r->cprbx.cprb_ver_id);
> +
> +	if (reply->len < minlen ||
> +	    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;

[Severity: High]
Could this logic omit setting the error code for short EP11 messages as well?

Bypassing length validation here may similarly allow uninitialized buffer
reads.

>  	}
> +
> +	switch (resp_type->type) {

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