Re: [PATCH v2] s390/zcrypt: fix reply buffer calculations for CCA replies

[Harald Freudenberger <freude@linux.ibm.com>]

On 2023-07-17 10:45, Holger Dengler wrote:
> On 14/07/2023 16:36, Harald Freudenberger wrote:
>> The length information for available buffer space for CCA
>> replies is covered with two fields in the T6 header prepended
>> on each CCA reply: fromcardlen1 and fromcardlen2. The sum of
>> these both values must not exceed the AP bus limit for this
>> card (24KB for CEX8, 12KB CEX7 and older) minus the always
>> present headers.
>> 
>> The current code adjusted the fromcardlen2 value in case
>> of exceeding the AP bus limit when there was a non-zero
>> value given from userspace. Some tests now showed that this
>> was the wrong assumption. Instead the userspace value given for
>> this field should always be trusted and if the sum of the
>> wo fields exceeds the AP bus limit for this card the first
> 
> typo: two

fixed

> 
>> field fromcardlen1 should be adjusted instead.
>> 
>> So now the calculation is done with this new insight in mind.
>> Also some additional checks for overflow have been introduced
>> and some comments to provide some documentation for future
>> maintainers of this complicated calculation code.
>> 
>> Furthermore the 128 bytes of fix overhead which is used
>> in the current code is not correct. Investications showed
> 
> typo: Investigations

fixed ... did I forget to run the spell checker ??? ...

> 
>> that for a reply always the same two header structs are
>> prepended before a possible payload. So this is also fixed
>> with this patch.
>> 
>> Signed-off-by: Harald Freudenberger <freude@linux.ibm.com>
>> Cc: stable@vger.kernel.org
> 
> With the typos fixed and the changes below
> Reviewed-by: Holger Dengler <dengler@linux.ibm.com>
> 
>> ---
>>  drivers/s390/crypto/zcrypt_msgtype6.c | 45 
>> ++++++++++++++++++++-------
>>  1 file changed, 33 insertions(+), 12 deletions(-)
>> 
>> diff --git a/drivers/s390/crypto/zcrypt_msgtype6.c 
>> b/drivers/s390/crypto/zcrypt_msgtype6.c
>> index 247f0ad38362..5ac110669327 100644
>> --- a/drivers/s390/crypto/zcrypt_msgtype6.c
>> +++ b/drivers/s390/crypto/zcrypt_msgtype6.c
>> @@ -551,6 +551,12 @@ static int xcrb_msg_to_type6_ep11cprb_msgx(bool 
>> userspace, struct ap_message *ap
>>   *
>>   * Returns 0 on success or -EINVAL, -EFAULT, -EAGAIN in case of an 
>> error.
>>   */
>> +struct type86_reply_hdrs {
>> +	struct type86_hdr hdr;
>> +	struct type86_fmt2_ext fmt2;
>> +	/* ... payload may follow ... */
>> +} __packed;
>> +
> 
> There is already a `struct type86_fmt2_msg` in this file (line 329 
> ff.).

Yes, I saw that. This will be consolidated with a cleanup patch series I 
am about
to develop just now.

> 
>>  struct type86x_reply {
>>  	struct type86_hdr hdr;
>>  	struct type86_fmt2_ext fmt2;
>> @@ -1101,23 +1107,38 @@ static long zcrypt_msgtype6_send_cprb(bool 
>> userspace, struct zcrypt_queue *zq,
>>  				      struct ica_xcRB *xcrb,
>>  				      struct ap_message *ap_msg)
>>  {
>> -	int rc;
>> +	unsigned int reply_bufsize_minus_headers =
>> +		zq->reply.bufsize - sizeof(struct type86_reply_hdrs);
> 
> I don't like this variable name. What about `max_payload_size`?
> 
>>  	struct response_type *rtype = ap_msg->private;
>>  	struct {
>>  		struct type6_hdr hdr;
>>  		struct CPRBX cprbx;
>>  		/* ... more data blocks ... */
>>  	} __packed * msg = ap_msg->msg;
>> -
>> -	/*
>> -	 * Set the queue's reply buffer length minus 128 byte padding
>> -	 * as reply limit for the card firmware.
>> -	 */
>> -	msg->hdr.fromcardlen1 = min_t(unsigned int, msg->hdr.fromcardlen1,
>> -				      zq->reply.bufsize - 128);
>> -	if (msg->hdr.fromcardlen2)
>> -		msg->hdr.fromcardlen2 =
>> -			zq->reply.bufsize - msg->hdr.fromcardlen1 - 128;
>> +	int rc, delta;
>> +
>> +	/* limit each of the two from fields to AP bus limit - headers */
> 
> I would also use "maximal payload size" here.
> /* limit each of the two from fields to the maximum payload size */

will do

> 
>> +	msg->hdr.fromcardlen1 = min_t(unsigned int,
>> +				      msg->hdr.fromcardlen1,
>> +				      reply_bufsize_minus_headers);
>> +	msg->hdr.fromcardlen2 = min_t(unsigned int,
>> +				      msg->hdr.fromcardlen2,
>> +				      reply_bufsize_minus_headers);
>> +
>> +	/* calculate delta if the sum of both exceeds AP bus limit - headers 
>> */
> 
> dito:
> /* calculate delta if the sum of both exceeds the maximum payload size 
> */
> 

jaaaa...

>> +	delta = msg->hdr.fromcardlen1 + msg->hdr.fromcardlen2
>> +		- reply_bufsize_minus_headers;
>> +	if (delta > 0) {
>> +		/*
>> +		 * Sum exceeds AP bus limit - headers, prune fromcardlen1
> 
> dito:
>  * Sum exceeds the maximum payload size, prune fromcardlen1
> 

ok

>> +		 * (always trust fromcardlen2)
>> +		 */
>> +		if (delta > msg->hdr.fromcardlen1) {
>> +			rc = -EINVAL;
>> +			goto out;
>> +		}
>> +		msg->hdr.fromcardlen1 -= delta;
>> +	}
>> 
>>  	init_completion(&rtype->work);
>>  	rc = ap_queue_message(zq->queue, ap_msg);
>> @@ -1240,7 +1261,7 @@ static long zcrypt_msgtype6_send_ep11_cprb(bool 
>> userspace, struct zcrypt_queue *
>>  	 * as reply limit for the card firmware.
>>  	 */
>>  	msg->hdr.fromcardlen1 = zq->reply.bufsize -
>> -		sizeof(struct type86_hdr) - sizeof(struct type86_fmt2_ext);
>> +		sizeof(struct type86_reply_hdrs);
>> 
>>  	init_completion(&rtype->work);
>>  	rc = ap_queue_message(zq->queue, ap_msg);