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

On 2026-07-13 15:21, Holger Dengler wrote:
> 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 <[email protected]>
>> Cc: [email protected]
> 
> 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;
> 

Yes - removed.

>>   			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?

Well this calculation is tricky. So let me summarize what I think should 
be checked:
1) offset1 should lie in the buffer
    ==> offset1 < reply->len should be true
2) the block should fit into the buffer
    ==> count1 <= reply->len should be true
with that it is clear and no need to check that the end (offset1 + 
count1)
is also covered:
    ==> offset1 + count1 <= reply->len should then be implicitly true

Maybe have a look at v6 of this code. I reworked this again and now it 
clearly
distinguishes between validations of the message fields (count, offset) 
and
checks about length of messages and buffer sizes.

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