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

[sashiko-bot@kernel.org]

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