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

Finn Callies <[email protected]>
Newsgroups org.kernel.vger.linux-s390
Message-ID <[email protected]> 

On 06.08.26 07:22, Finn Callies wrote:
> 
> 
> On 04.08.26 16:49, 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, EP11 (AP message
>> type 6) and ICA (AP type 50) 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.
>>
>> Rework zcrypt_msgtype6_receive(), zcrypt_msgtype6_receive_ep11() and
>> zcrypt_msgtype50_receive() to validate reply lengths more carefully
>> before copying data back into the request buffer. Use size_t for
>> length calculations, reject inconsistent reply sizes, and add
>> defensive handling for short invalid replies. For XCRB replies,
>> validate both reply segments and derive the effective message length
>> from the covered range instead of trusting only the second segment.
>>
>> Signed-off-by: Harald Freudenberger <[email protected]>
>> ---
>>   drivers/s390/crypto/zcrypt_msgtype50.c |  39 ++++---
>>   drivers/s390/crypto/zcrypt_msgtype6.c  | 151 ++++++++++++++++---------
>>   2 files changed, 126 insertions(+), 64 deletions(-)
>>
>> diff --git a/drivers/s390/crypto/zcrypt_msgtype50.c b/drivers/s390/ 
>> crypto/zcrypt_msgtype50.c
>> index d6fc2d8e7fad..ef925b399806 100644
>> --- a/drivers/s390/crypto/zcrypt_msgtype50.c
>> +++ b/drivers/s390/crypto/zcrypt_msgtype50.c
>> @@ -416,26 +416,39 @@ static void zcrypt_msgtype50_receive(struct 
>> ap_queue *aq,
>>           .reply_code = REP82_ERROR_MACHINE_FAILURE,
>>       };
>>       struct type80_hdr *t80h;
>> -    int len;
>> +    size_t len;
>>       /* Copy the reply message to the request message buffer. */
>>       if (!reply)
>>           goto out;    /* ap_msg->rc indicates the error */
>> +
>>       t80h = reply->msg;
>> -    if (t80h->type == TYPE80_RSP_CODE) {
>> -        len = t80h->len;
>> -        if (len > reply->bufsize || len > msg->bufsize ||
>> -            len != reply->len) {
>> -            pr_debug("len mismatch => EMSGSIZE\n");
> 
> I don't like this debug statement, its very undescriptive. What is len? 
> What does it mismatch against? What does the mismatch mean?
> 
> The "=> EMSGSIZE" looks very uncommon for me as well, is this a common 
> way of logging error paths?
> 
>> -            msg->rc = -EMSGSIZE;
>> -            goto out;
>> -        }
>> -        memcpy(msg->msg, reply->msg, len);
>> -        msg->len = len;
>> -    } else {
>> -        memcpy(msg->msg, reply->msg, sizeof(error_reply));
>> +
>> +    if (t80h->type != TYPE80_RSP_CODE) {
>> +        if (reply->len < sizeof(error_reply))
>> +            memcpy(msg->msg, &error_reply, sizeof(error_reply));
>> +        else
>> +            memcpy(msg->msg, reply->msg, sizeof(error_reply));
>>           msg->len = sizeof(error_reply);
>> +        goto out;
>> +    }
>> +
>> +    len = t80h->len;
>> +    if (len != reply->len) {
>> +        pr_warn_ratelimited("len %zu rpl.len %zu mismatch => 
>> EMSGSIZE\n",
>> +                    len, reply->len);
> 
> Same here. What is len? rpl.len is a technical statement, but logs 
> should be descriptive right?

I suggest something like "... mismatch: EMSGSIZE"

Additionally do not hardcode return code strings. Either use a function 
which converts rc to string or Just use %d and print the rc itself.

> 
>> +        msg->rc = -EMSGSIZE;
>> +        goto out;
>>       }
>> +    if (len > reply->bufsize || len > msg->bufsize) {
>> +        pr_warn_ratelimited("len %zu exceeds buf %zu/%zu => EMSGSIZE\n",
>> +                    len, reply->bufsize, msg->bufsize);
> 
> Why buf instead of buffer? Why safe space here?
> 
>> +        msg->rc = -EMSGSIZE;
>> +        goto out;
>> +    }
>> +    memcpy(msg->msg, reply->msg, len);
>> +    msg->len = len;
>> +
>>   out:
>>       complete(&msg->response.work);
>>   }
>> diff --git a/drivers/s390/crypto/zcrypt_msgtype6.c b/drivers/s390/ 
>> crypto/zcrypt_msgtype6.c
>> index 3df1d676de5d..b98449913e24 100644
>> --- a/drivers/s390/crypto/zcrypt_msgtype6.c
>> +++ b/drivers/s390/crypto/zcrypt_msgtype6.c
>> @@ -766,6 +766,13 @@ static int convert_type86_rng(struct zcrypt_queue 
>> *zq,
>>       if (msg->cprbx.ccp_rtcode != 0 || msg->cprbx.ccp_rscode != 0)
>>           return -EINVAL;
>> +    /*
>> +     * Note that offset2 and count2 have already been checked in
>> +     * zcrypt_msgtype6_receive(). So only check for not exceeding
>> +     * the hard coded rng buffer size.
>> +     */
>> +    if (msg->fmt2.count2 > ZCRYPT_RNG_BUFFER_SIZE)
>> +        return -EMSGSIZE;
>>       memcpy(buffer, data + msg->fmt2.offset2, msg->fmt2.count2);
>>       return msg->fmt2.count2;
>>   }
>> @@ -928,48 +935,75 @@ static void zcrypt_msgtype6_receive(struct 
>> ap_queue *aq,
>>       };
>>       struct ap_response_type *resp_type = &msg->response;
>>       struct type86x_reply *t86r;
>> -    int len;
>> +    size_t len, len1, len2 = 0;
>>       /* Copy the reply message to the request message buffer. */
>>       if (!reply)
>>           goto out;    /* ap_msg->rc indicates the error */
>> +
>>       t86r = reply->msg;
>> -    if (t86r->hdr.type == TYPE86_RSP_CODE &&
>> -        t86r->cprbx.cprb_ver_id == 0x02) {
>> -        switch (resp_type->type) {
>> -        case CEXXC_RESPONSE_TYPE_ICA:
>> -            len = sizeof(struct type86x_reply) + t86r->length;
>> -            if (len > reply->bufsize || len > msg->bufsize ||
>> -                len != reply->len) {
>> -                pr_debug("len mismatch => EMSGSIZE\n");
>> -                msg->rc = -EMSGSIZE;
>> -                goto out;
>> -            }
>> -            memcpy(msg->msg, reply->msg, len);
>> -            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;
>> -            if (len > reply->bufsize || len > msg->bufsize ||
>> -                len != reply->len) {
>> -                pr_debug("len mismatch => EMSGSIZE\n");
>> +
>> +    if (t86r->hdr.type != TYPE86_RSP_CODE ||
>> +        t86r->cprbx.cprb_ver_id != 0x02) {
>> +        if (reply->len < sizeof(error_reply))
>> +            memcpy(msg->msg, &error_reply, sizeof(error_reply));
>> +        else
>> +            memcpy(msg->msg, reply->msg, sizeof(error_reply));
>> +        msg->len = sizeof(error_reply);
>> +        goto out;
>> +    }
>> +
>> +    switch (resp_type->type) {
>> +    case CEXXC_RESPONSE_TYPE_ICA:
>> +        len = sizeof(struct type86x_reply) + (size_t)t86r->length;
>> +        break;
>> +    case CEXXC_RESPONSE_TYPE_XCRB:
>> +        len1 = (size_t)t86r->fmt2.offset1 + (size_t)t86r->fmt2.count1;
>> +        if (t86r->fmt2.offset1 > reply->len ||
>> +            t86r->fmt2.count1 > reply->len) {
>> +            pr_warn_ratelimited(
>> +                "offset1 %u count1 %u rpl.len %zu mismatch => 
>> EMSGSIZE\n",
>> +                t86r->fmt2.offset1, t86r->fmt2.count1,
>> +                reply->len);
> 
> same here
> 
>> +            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) {
>> +                pr_warn_ratelimited(
>> +                    "offset2 %u count2 %u rpl.len %zu mismatch => 
>> EMSGSIZE\n",
>> +                    t86r->fmt2.offset2, t86r->fmt2.count2,
>> +                    reply->len);
> 
> same here
> 
>>                   msg->rc = -EMSGSIZE;
>>                   goto out;
>>               }
>> -            memcpy(msg->msg, reply->msg, len);
>> -            msg->len = len;
>> -            break;
>> -        default:
>> -            memcpy(msg->msg, &error_reply, sizeof(error_reply));
>> -            msg->len = sizeof(error_reply);
>>           }
>> -    } else {
>> -        memcpy(msg->msg, reply->msg, sizeof(error_reply));
>> +        len = max_t(size_t, len1, len2);
>> +        break;
>> +    default:
>> +        memcpy(msg->msg, &error_reply, sizeof(error_reply));
>>           msg->len = sizeof(error_reply);
>> +        goto out;
>> +    }
>> +
>> +    if (len != reply->len) {
>> +        pr_warn_ratelimited("len %zu rpl.len %zu mismatch => 
>> EMSGSIZE\n",
>> +                    len, reply->len);
> 
> same here
> 
>> +        msg->rc = -EMSGSIZE;
>> +        goto out;
>>       }
>> +    if (len > reply->bufsize || len > msg->bufsize) {
>> +        pr_warn_ratelimited("len %zu exceeds buf %zu/%zu => EMSGSIZE\n",
>> +                    len, reply->bufsize, msg->bufsize);
> 
> same here
> 
>> +        msg->rc = -EMSGSIZE;
>> +        goto out;
>> +    }
>> +    memcpy(msg->msg, reply->msg, len);
>> +    msg->len = len;
>> +
>>   out:
>>       complete(&resp_type->work);
>>   }
>> @@ -992,34 +1026,49 @@ static void zcrypt_msgtype6_receive_ep11(struct 
>> ap_queue *aq,
>>       };
>>       struct ap_response_type *resp_type = &msg->response;
>>       struct type86_ep11_reply *t86r;
>> -    int len;
>> +    size_t len;
>>       /* Copy the reply message to the request message buffer. */
>>       if (!reply)
>>           goto out;    /* ap_msg->rc indicates the error */
>> +
>>       t86r = reply->msg;
>> -    if (t86r->hdr.type == TYPE86_RSP_CODE &&
>> -        t86r->cprbx.cprb_ver_id == 0x04) {
>> -        switch (resp_type->type) {
>> -        case CEXXC_RESPONSE_TYPE_EP11:
>> -            len = t86r->fmt2.offset1 + t86r->fmt2.count1;
>> -            if (len > reply->bufsize || len > msg->bufsize ||
>> -                len != reply->len) {
>> -                pr_debug("len mismatch => EMSGSIZE\n");
>> -                msg->rc = -EMSGSIZE;
>> -                goto out;
>> -            }
>> -            memcpy(msg->msg, reply->msg, len);
>> -            msg->len = len;
>> -            break;
>> -        default:
>> +
>> +    if (t86r->hdr.type != TYPE86_RSP_CODE ||
>> +        t86r->cprbx.cprb_ver_id != 0x04) {
>> +        if (reply->len < sizeof(error_reply))
>>               memcpy(msg->msg, &error_reply, sizeof(error_reply));
>> -            msg->len = sizeof(error_reply);
>> -        }
>> -    } else {
>> -        memcpy(msg->msg, reply->msg, sizeof(error_reply));
>> +        else
>> +            memcpy(msg->msg, reply->msg, sizeof(error_reply));
>>           msg->len = sizeof(error_reply);
>> +        goto out;
>>       }
>> +
>> +    switch (resp_type->type) {
>> +    case CEXXC_RESPONSE_TYPE_EP11:
>> +        len = t86r->fmt2.offset1 + (size_t)t86r->fmt2.count1;
>> +        break;
>> +    default:
>> +        memcpy(msg->msg, &error_reply, sizeof(error_reply));
>> +        msg->len = sizeof(error_reply);
>> +        goto out;
>> +    }
>> +
>> +    if (len != reply->len) {
>> +        pr_warn_ratelimited("len %zu rpl.len %zu mismatch => 
>> EMSGSIZE\n",
>> +                    len, reply->len);
> 
> same here
> 
>> +        msg->rc = -EMSGSIZE;
>> +        goto out;
>> +    }
>> +    if (len > reply->bufsize || len > msg->bufsize) {
>> +        pr_warn_ratelimited("len %zu exceeds buf %zu/%zu => EMSGSIZE\n",
>> +                    len, reply->bufsize, msg->bufsize);
> 
> same here
> 
>> +        msg->rc = -EMSGSIZE;
>> +        goto out;
>> +    }
>> +    memcpy(msg->msg, reply->msg, len);
>> +    msg->len = len;
>> +
>>   out:
>>       complete(&resp_type->work);
>>   }
>
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.