Re: [PATCH v11 5/5] s390/zcrypt: Pad trailing CCA or EP11 message with zeros

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Medium] The function `zcrypt_type6_crt_key()` leaves up to 7 bytes of padding uninitialized before sending the key token to the crypto card, leaking kernel memory.
--

--- Patch [5]: [PATCH v11 5/5] s390/zcrypt: Pad trailing CCA or EP11 message with zeros ---
commit a6bb8fce2d346e386b1306d838ac36f1853d13f8
Author: Harald Freudenberger <[email protected]>

s390/zcrypt: Pad trailing CCA or EP11 message with zeros

This commit pads the gap between the message copied from userspace and
the message buffer sent to the CCA/EP11 hardware. This prevents
uninitialized kernel memory from being forwarded to the crypto card.

> diff --git a/drivers/s390/crypto/zcrypt_msgtype6.c b/drivers/s390/crypto/zcrypt_msgtype6.c
> index e79e68b4dd41d..3df1d676de5d8 100644
> --- a/drivers/s390/crypto/zcrypt_msgtype6.c
> +++ b/drivers/s390/crypto/zcrypt_msgtype6.c
> @@ -398,6 +398,10 @@ static int xcrb_msg_to_type6cprb_msgx(bool userspace, struct ap_message *ap_msg,
>  			     xcrb->request_control_blk_addr,
>  			     xcrb->request_control_blk_length))
>  		return -EFAULT;
> +	/* pad tail with 0 up to req_cblen */
> +	if (xcrb->request_control_blk_length < req_cblen)
> +		memset(msg->userdata + xcrb->request_control_blk_length,
> +		       0, req_cblen - xcrb->request_control_blk_length);

[Severity: Medium]
This is a pre-existing issue, but while reviewing this fix for uninitialized
memory leaks, I noticed a similar issue in zcrypt_type6_crt_key() where
pad_len bytes appear to be left uninitialized:

drivers/s390/crypto/zcrypt_cca_key.h:zcrypt_type6_crt_key() {
    ...
    memset(key, 0, sizeof(*key));
    ...
    if (copy_from_user(key->key_parts, crt->np_prime, long_len) ||
        copy_from_user(key->key_parts + long_len,
                       crt->nq_prime, short_len) ||
        copy_from_user(key->key_parts + long_len + short_len,
                       crt->bp_key, long_len) ||
        copy_from_user(key->key_parts + 2 * long_len + short_len,
                       crt->bq_key, short_len) ||
        copy_from_user(key->key_parts + 2 * long_len + 2 * short_len,
                       crt->u_mult_inv, long_len))
        return -EFAULT;
    memset(key->key_parts + 3 * long_len + 2 * short_len + pad_len,
           0xff, crt->inputdatalength);
    ...
}

Because sizeof(*key) doesn't include the flexible array member key_parts[],
the memory inside key_parts is uninitialized before the copies.

Does the final memset() skip over pad_len bytes by adding it to the offset,
leaving those padding bytes uninitialized before being sent to the firmware?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5