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

[Holger Dengler <dengler@linux.ibm.com>]

On 7/10/26 17:10, 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 and EP11 (AP message
> type 6) 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.
> 
> The 3 hunks of this patch deal with CCA, EP11 and (CCA) RNG replies
> and improve the checking for reply buffer size by using size_t instead
> of int. RNG replies an additional check makes sure the hard coded
> limit of the data buffer is not exceeded. Also there was a condition
> with additional data for an CCA reply where some of the field values
> where unchecked used to invoke memcpy into user
> space. zcrypt_msgtype6_receive() now checks all the relevant fields
> before convert_type86_xcrb() uses them.
> 
> Signed-off-by: Harald Freudenberger <freude@linux.ibm.com>
> Cc: stable@vger.kernel.org

See my comments below.

> ---
>   drivers/s390/crypto/zcrypt_msgtype6.c | 42 ++++++++++++++++++++++-----
>   1 file changed, 34 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/s390/crypto/zcrypt_msgtype6.c b/drivers/s390/crypto/zcrypt_msgtype6.c
> index 40f72cdf284d..8252fd185663 100644
> --- a/drivers/s390/crypto/zcrypt_msgtype6.c
> +++ b/drivers/s390/crypto/zcrypt_msgtype6.c
[...]
> @@ -863,7 +870,8 @@ static void zcrypt_msgtype6_receive(struct ap_queue *aq,
>   	    t86r->cprbx.cprb_ver_id == 0x02) {
>   		switch (resp_type->type) {
>   		case CEXXC_RESPONSE_TYPE_ICA:
> -			len = sizeof(struct type86x_reply) + t86r->length;
> +			len = (size_t)sizeof(struct type86x_reply) +
> +				(size_t)t86r->length;

Is the explicit cast for sizeof() really necessary. I would assume, that 
the following should be sufficient:

	len = sizeof(struct type86x_reply) +
		(size_t)t86r->length;

>   			if (len > reply->bufsize || len > msg->bufsize ||
>   			    len != reply->len) {
>   				pr_debug("len mismatch => EMSGSIZE\n");
> @@ -874,10 +882,27 @@ 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;
> +			len1 = (size_t)t86r->fmt2.offset1 +
> +				(size_t)t86r->fmt2.count1;
> +			if (t86r->fmt2.offset1 > reply->len ||
> +			    t86r->fmt2.count1 > reply->len ||
> +			    len1 > reply->len) {

Wouldn't it be sufficient to check only (len1 > reply->len)? If 
(t86r->fmt2.offset1 > reply->len) is true, than also (len1 > reply->len) 
will be true (and the same for count1).

Or did I miss something?

> +				pr_debug("len mismatch => EMSGSIZE\n");
> +				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 ||
> +				    len2 > reply->len) {

Same here.

[...]

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