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

On 7/30/26 16:48, [email protected] wrote:
> 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`.

Valid finding, but already handled by patch 5/5 in this series.

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

These pre-existing findings will be analyzed and - if valid - covered by
a separate series.

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

-- 
Mit freundlichen Grüßen / Kind regards
Holger Dengler