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

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

[Harald Freudenberger]

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
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
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
---
 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;
+
 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);
 	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 */
+	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 */
+	delta = msg->hdr.fromcardlen1 + msg->hdr.fromcardlen2
+		- reply_bufsize_minus_headers;
+	if (delta > 0) {
+		/*
+		 * Sum exceeds AP bus limit - headers, prune fromcardlen1
+		 * (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);
-- 
2.34.1

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

[Holger Dengler]

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

> 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

> 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.).

>  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 */

> +	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 */

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

> +		 * (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);

-- 
Mit freundlichen Grüßen / Kind regards
Holger Dengler
--
IBM Systems, Linux on IBM Z Development
dengler@linux.ibm.com

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

[Harald Freudenberger]

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);