Re: [PATCH v10 2/5] s390/zcrypt: Improve CCA CPRB length and overflow checks

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

New issues:
- [Medium] Uninitialized alignment padding bytes may be read during function code extraction in `xcrb_msg_to_type6cprb_msgx`.

Pre-existing issues:
- [Critical] A 64-bit integer overflow in `xcrb_msg_to_type6_ep11cprb_msgx` allows a massive `req_len` to bypass the buffer size check.
- [High] The EP11 request path lacks minimum size validation, leading to uninitialized kernel memory access.
- [High] A user can bypass administrative domain permission checks by setting the target domain to AUTOSEL_DOM (0xFFFF).
--

--- Patch [2]: [PATCH v10 2/5] s390/zcrypt: Improve CCA CPRB length and overflow checks ---
commit 168ea93b9d93d40b4d0625f5a9d932544117496a
Author: Harald Freudenberger <[email protected]>

s390/zcrypt: Improve CCA CPRB length and overflow checks
    
This patch improves input validation in the xcrb_msg_to_type6cprb_msgx
function. It uses size_t for length calculations, adds bounds checks after
alignment, validates the minimum control block size, and detects overflow
in sum calculations.

> diff --git a/drivers/s390/crypto/zcrypt_msgtype6.c b/drivers/s390/crypto/zcrypt_msgtype6.c
> --- a/drivers/s390/crypto/zcrypt_msgtype6.c
> +++ b/drivers/s390/crypto/zcrypt_msgtype6.c

[ ... ]

> @@ -404,8 +395,8 @@ 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;
> -	if (msg->cprbx.cprb_len + sizeof(msg->hdr.function_code) >
> -	    xcrb->request_control_blk_length)
> +	/* copy subfunction code into AP msg type 6 function code field */
> +	if (msg->cprbx.cprb_len > req_cblen - sizeof(msg->hdr.function_code))

[Severity: Medium]
Can this relaxed bounds check result in reading uninitialized alignment
padding?

If a userspace caller sends a CCA ioctl where
xcrb->request_control_blk_length is not a multiple of 4 (for example, 21),
the z_copy_from_user() call copies exactly 21 bytes. Up to 3 bytes of
padding might be left uninitialized.

Because the check now compares against req_cblen instead of
xcrb->request_control_blk_length, could a crafted cprbx.cprb_len (e.g., 22)
pass this validation and point into the uninitialized padding area, allowing
the subsequent memcpy() to read uninitialized kernel heap memory into
msg->hdr.function_code?

>  		return -EINVAL;
>  	function_code = ((unsigned char *)&msg->cprbx) + msg->cprbx.cprb_len;
>  	memcpy(msg->hdr.function_code, function_code,

[ ... ]


While reviewing these length check improvements, I also noticed a few
pre-existing issues in related code:


[Severity: Critical]
This isn't a bug introduced by this patch, but does
xcrb_msg_to_type6_ep11cprb_msgx() suffer from a similar 64-bit integer
overflow?

In drivers/s390/crypto/zcrypt_msgtype6.c:xcrb_msg_to_type6_ep11cprb_msgx():

    ap_msg->len = sizeof(struct type6_hdr) + CEIL4(xcrb->req_len);
    if (ap_msg->len > ap_msg->bufsize)
        return -EINVAL;

If a userspace caller passes an ioctl with req_len equal to U64_MAX - 79,
CEIL4(req_len) evaluates to U64_MAX - 79. Adding sizeof(struct type6_hdr)
wraps the 64-bit value to 8, completely bypassing the buffer bounds check.

Would the subsequent copy_from_user() then zero-fill the kernel heap on
failure and cause memory corruption?


[Severity: High]
This is a pre-existing issue, but the EP11 request path seems to lack the
minimum size validation that this patch adds to the CCA path.

In drivers/s390/crypto/zcrypt_msgtype6.c:xcrb_msg_to_type6_ep11cprb_msgx():

    if (z_copy_from_user(userspace, msg->userdata,
                         (char __force __user *)xcrb->req, xcrb->req_len)) {

If an attacker submits an EP11 ioctl with a small req_len (e.g., 4 bytes),
the kernel copies only 4 bytes. Could the function then read EP11 structure
fields such as msg->pld_lenfmt and msg->cprbx.flags from uninitialized
memory to determine function codes and domains?


[Severity: High]
This isn't a bug introduced by this patch, but looking at the domain
permissions, is there a way to bypass administrative checks?

In drivers/s390/crypto/zcrypt_api.c:_zcrypt_send_cprb():

    if (perms != &ap_perms && domain < AP_DOMAINS) {
        if (ap_msg.flags & AP_MSG_FLAG_ADMIN) {
            if (!test_bit_inv(domain, perms->adm)) {

And similarly in _zcrypt_send_ep11_cprb():

    if (perms != &ap_perms && domain < AUTOSEL_DOM) {
        if (ap_msg.flags & AP_MSG_FLAG_ADMIN) {
            if (!test_bit_inv(domain, perms->adm)) {

If a user submits a CPRBX or EP11 request containing an administrative
command and sets the embedded domain field to 0xFFFF (AUTOSEL_DOM), the
condition domain < AP_DOMAINS evaluates to false.

Does this cause the administrative permission check to be completely skipped,
allowing a user with only usage privileges to execute administrative
commands on that domain?

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