[PATCH v1 0/2] Fix minor LLM findings in zcrypt dd

[PATCH v1 0/2] Fix minor LLM findings in zcrypt dd

[Harald Freudenberger]

Fix some minor LLM findings related to the zcrypt device driver.

Changelog:
v1: Two patches:
    - The first patch fixes a gap related to speculative execution
      with the domain index.
    - The second patch fixes a length/pointer check with parsing the
      FQ reply from a CCA card.

Harald Freudenberger (2):
  s390/zcrypt: Guard domain index uses against speculative bypass
  s390/zcrypt: Fix out-of-bounds ptr advance in
    cca_query_crypto_facility()

 drivers/s390/crypto/zcrypt_api.c     | 10 ++++++++--
 drivers/s390/crypto/zcrypt_ccamisc.c | 22 ++++++++++++++++++++--
 2 files changed, 28 insertions(+), 4 deletions(-)

--
2.43.0

[PATCH v1 1/2] s390/zcrypt: Guard domain index uses against speculative bypass

[Harald Freudenberger]

The previous array_index_nospec() placement only covered the admin
permission check, leaving ap_test_config_usage_domain() and
ap_test_config_ctrl_domain() in the CCA path exposed to speculative
execution.

Move the sanitization to before all permission and config checks in
both the CCA and EP11 paths so a single call covers all downstream
uses of the domain index.

Fixes: e935cd525af4 ("s390/zcrypt: Close speculative mem read possibility")
Signed-off-by: Harald Freudenberger <freude@linux.ibm.com>
Cc: stable@vger.kernel.org
---
 drivers/s390/crypto/zcrypt_api.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/s390/crypto/zcrypt_api.c b/drivers/s390/crypto/zcrypt_api.c
index ec6a4c2f9f04..c9d6f4539235 100644
--- a/drivers/s390/crypto/zcrypt_api.c
+++ b/drivers/s390/crypto/zcrypt_api.c
@@ -877,9 +877,12 @@ static long _zcrypt_send_cprb(u32 xflags, struct ap_perms *perms,
 	print_hex_dump_debug("ccareq: ", DUMP_PREFIX_ADDRESS, 16, 1,
 			     ap_msg.msg, ap_msg.len, false);
 
+	/* Make sure domain is either AUTOSEL_DOM or in range 0...AP_DOMAIN-1 */
+	if (domain != AUTOSEL_DOM)
+		domain = array_index_nospec(domain, AP_DOMAINS);
+
 	if (perms != &ap_perms && domain < AP_DOMAINS) {
 		if (ap_msg.flags & AP_MSG_FLAG_ADMIN) {
-			domain = array_index_nospec(domain, AP_DOMAINS);
 			if (!test_bit_inv(domain, perms->adm)) {
 				rc = -ENODEV;
 				goto out;
@@ -1078,9 +1081,12 @@ static long _zcrypt_send_ep11_cprb(u32 xflags, struct ap_perms *perms,
 	print_hex_dump_debug("ep11req: ", DUMP_PREFIX_ADDRESS, 16, 1,
 			     ap_msg.msg, ap_msg.len, false);
 
+	/* Make sure domain is either AUTOSEL_DOM or in range 0...AP_DOMAIN-1 */
+	if (domain != AUTOSEL_DOM)
+		domain = array_index_nospec(domain, AP_DOMAINS);
+
 	if (perms != &ap_perms && domain < AP_DOMAINS) {
 		if (ap_msg.flags & AP_MSG_FLAG_ADMIN) {
-			domain = array_index_nospec(domain, AP_DOMAINS);
 			if (!test_bit_inv(domain, perms->adm)) {
 				rc = -ENODEV;
 				goto out;
-- 
2.43.0

[PATCH v1 2/2] s390/zcrypt: Fix out-of-bounds ptr advance in cca_query_crypto_facility()

[Harald Freudenberger]

The FQ reply parser code blindly advanced the walk pointer by a length
value read directly from the hardware reply payload without checking
that the advance stayed within the allocated reply buffer. A corrupt
or malicious device response could push ptr beyond the cprbmem region,
causing an out-of-bounds dereference or kernel memory exposure via the
subsequent memcpy().

Fix this by tracking the remaining reply buffer space in a variable
and validating each device-supplied length field against it before
advancing or dereferencing the pointer.

Fixes: 2004b57cde6b ("s390/zcrypt: code cleanup")
Signed-off-by: Harald Freudenberger <freude@linux.ibm.com>
Cc: stable@vger.kernel.org
---
 drivers/s390/crypto/zcrypt_ccamisc.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/s390/crypto/zcrypt_ccamisc.c b/drivers/s390/crypto/zcrypt_ccamisc.c
index 19909bf43dc9..862947431164 100644
--- a/drivers/s390/crypto/zcrypt_ccamisc.c
+++ b/drivers/s390/crypto/zcrypt_ccamisc.c
@@ -1627,6 +1627,7 @@ int cca_query_crypto_facility(u16 cardnr, u16 domain,
 		u8  subfunc_code[2];
 		u8  lvdata[];
 	} __packed * prepparm;
+	size_t datalen;
 
 	/* get already prepared memory for 2 cprbs with param block each */
 	rc = alloc_and_prep_cprbmem(parmbsize, &mem,
@@ -1673,27 +1674,44 @@ int cca_query_crypto_facility(u16 cardnr, u16 domain,
 	prepcblk->rpl_parmb = (u8 __user *)ptr;
 	prepparm = (struct fqrepparm *)ptr;
 	ptr = prepparm->lvdata;
+	datalen = parmbsize - 2 * sizeof(u8);
 
 	/* check and possibly copy reply rule array */
 	len = *((u16 *)ptr);
+	if (len > datalen) {
+		ZCRYPT_DBF_ERR("%s reply rule array len %u exceeds datalen %zu\n",
+			       __func__, len, datalen);
+		rc = -EIO;
+		goto out;
+	}
+	datalen -= sizeof(u16);
+	ptr += sizeof(u16);
 	if (len > sizeof(u16)) {
-		ptr += sizeof(u16);
 		len -= sizeof(u16);
 		if (rarray && rarraylen && *rarraylen > 0) {
 			*rarraylen = (len > *rarraylen ? *rarraylen : len);
 			memcpy(rarray, ptr, *rarraylen);
 		}
+		datalen -= len;
 		ptr += len;
 	}
 	/* check and possible copy reply var array */
 	len = *((u16 *)ptr);
+	if (len > datalen) {
+		ZCRYPT_DBF_ERR("%s reply var array len %u exceeds datalen %zu\n",
+			       __func__, len, datalen);
+		rc = -EIO;
+		goto out;
+	}
+	datalen -= sizeof(u16);
+	ptr += sizeof(u16);
 	if (len > sizeof(u16)) {
-		ptr += sizeof(u16);
 		len -= sizeof(u16);
 		if (varray && varraylen && *varraylen > 0) {
 			*varraylen = (len > *varraylen ? *varraylen : len);
 			memcpy(varray, ptr, *varraylen);
 		}
+		datalen -= len;
 		ptr += len;
 	}
 
-- 
2.43.0

Re: [PATCH v1 2/2] s390/zcrypt: Fix out-of-bounds ptr advance in cca_query_crypto_facility()

[sashiko-bot]

> The FQ reply parser code blindly advanced the walk pointer by a length
> value read directly from the hardware reply payload without checking
> that the advance stayed within the allocated reply buffer. A corrupt
> or malicious device response could push ptr beyond the cprbmem region,
> causing an out-of-bounds dereference or kernel memory exposure via the
> subsequent memcpy().
> 
> Fix this by tracking the remaining reply buffer space in a variable
> and validating each device-supplied length field against it before
> advancing or dereferencing the pointer.
> 
> Fixes: 2004b57cde6b ("s390/zcrypt: code cleanup")
> Signed-off-by: Harald Freudenberger <freude@linux.ibm.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261005114018.62489-1-freude@linux.ibm.com?part=2

Re: [PATCH v1 1/2] s390/zcrypt: Guard domain index uses against speculative bypass

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] s390/zcrypt: invalid domain silently routed to domain 0 via array_index_nospec()
--

commit def23e498ab28cdfe6fb435523fe4c6e49365857
Author: Harald Freudenberger <freude@linux.ibm.com>

s390/zcrypt: Guard domain index uses against speculative bypass

This patch moves the array_index_nospec() sanitization for the domain index
to earlier in the CCA and EP11 paths. This aims to ensure a single call
covers all downstream uses of the index and prevents speculative execution
bypasses.

> diff --git a/drivers/s390/crypto/zcrypt_api.c b/drivers/s390/crypto/zcrypt_api.c
> index ec6a4c2f9f04a..c9d6f4539235b 100644
> --- a/drivers/s390/crypto/zcrypt_api.c
> +++ b/drivers/s390/crypto/zcrypt_api.c
> @@ -877,9 +877,12 @@ static long _zcrypt_send_cprb(u32 xflags, struct ap_perms *perms,
>  	print_hex_dump_debug("ccareq: ", DUMP_PREFIX_ADDRESS, 16, 1,
>  			     ap_msg.msg, ap_msg.len, false);
>  
> +	/* Make sure domain is either AUTOSEL_DOM or in range 0...AP_DOMAIN-1 */
> +	if (domain != AUTOSEL_DOM)
> +		domain = array_index_nospec(domain, AP_DOMAINS);
> +
>  	if (perms != &ap_perms && domain < AP_DOMAINS) {

[Severity: High]
Does calling array_index_nospec() before the architectural bounds check cause
invalid domains to be silently routed to domain 0?

If a user issues an ioctl with a domain >= AP_DOMAINS, array_index_nospec()
will clamp the out-of-bounds index to 0. The subsequent check for 
domain < AP_DOMAINS will then evaluate 0 < AP_DOMAINS, causing the operation
to proceed on domain 0 instead of rejecting the invalid input.

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

[ ... ]

> @@ -1078,9 +1081,12 @@ static long _zcrypt_send_ep11_cprb(u32 xflags, struct ap_perms *perms,
>  	print_hex_dump_debug("ep11req: ", DUMP_PREFIX_ADDRESS, 16, 1,
>  			     ap_msg.msg, ap_msg.len, false);
>  
> +	/* Make sure domain is either AUTOSEL_DOM or in range 0...AP_DOMAIN-1 */
> +	if (domain != AUTOSEL_DOM)
> +		domain = array_index_nospec(domain, AP_DOMAINS);
> +
>  	if (perms != &ap_perms && domain < AP_DOMAINS) {

[Severity: High]
Does this code have the same bounds checking issue here in the EP11 path?

An invalid domain >= AP_DOMAINS could be similarly clamped to 0 and pass the
subsequent bounds check, executing on the wrong domain.

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261005114018.62489-1-freude@linux.ibm.com?part=1