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@xxxxxxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
With the typos fixed and the changes below
Reviewed-by: Holger Dengler <dengler@xxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxx
