Re: [PATCH v4 1/7] s390/crypto: Fix return code handling at skcipher_walk_done in PAES algorithms

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

Pre-existing issues:
- [Critical] Silent data corruption and broken serialization when hardware queue is full and backlog is enabled.
--

--- Patch [1]: [PATCH v4 1/7] s390/crypto: Fix return code handling at skcipher_walk_done in PAES algorithms ---
commit 521a2dcdf7aa2bf204154c4172f4175cd3764345
Author: Harald Freudenberger <[email protected]>
Subject: s390/crypto: Fix return code handling at skcipher_walk_done in PAES algorithms

This patch adds immediate error checks after skcipher_walk_done() in all
main PAES cipher processing loops. It also checks if the walk has already
been finalized before calling skcipher_walk_done() to avoid double
de-allocation of resources.

> diff --git a/arch/s390/crypto/paes_s390.c b/arch/s390/crypto/paes_s390.c
> index 8cfe6166c193c..ccc7da106c3a0 100644
> --- a/arch/s390/crypto/paes_s390.c
> +++ b/arch/s390/crypto/paes_s390.c

[ ... ]

> @@ -495,7 +498,7 @@ static int ecb_paes_crypt(struct skcipher_request *req, unsigned long modifier)
>  			atomic_dec(&ctx->via_engine_ctr);
>  	}
>  
> -	if (rc != -EINPROGRESS)
> +	if (rc != -EINPROGRESS && walk->nbytes)
>  		skcipher_walk_done(walk, rc);

[Severity: Critical]
This isn't a bug introduced by this patch, but does this logic safely handle
requests when the hardware queue is full?

If crypto_transfer_skcipher_request_to_engine() returns -EBUSY, the request
is successfully backlogged.

However, because rc is not -EINPROGRESS, it appears the driver decrements
the engine counter and calls skcipher_walk_done() here to tear down the
walk.

When the engine later processes this backlogged request, will it operate on
an empty walk and zeroed parameters, silently returning success without
processing the data? Could this lead to data corruption when under heavy I/O
load?

[ ... ]

> @@ -761,7 +766,7 @@ static int cbc_paes_crypt(struct skcipher_request *req, unsigned long modifier)
>  			atomic_dec(&ctx->via_engine_ctr);
>  	}
>  
> -	if (rc != -EINPROGRESS)
> +	if (rc != -EINPROGRESS && walk->nbytes)
>  		skcipher_walk_done(walk, rc);

[Severity: Critical]
This is a pre-existing issue, but does this cbc_paes_crypt() implementation
share the same -EBUSY fallback issue described above, where backlogged
requests are prematurely torn down?

[ ... ]

> @@ -1079,7 +1089,7 @@ static int ctr_paes_crypt(struct skcipher_request *req)
>  			atomic_dec(&ctx->via_engine_ctr);
>  	}
>  
> -	if (rc != -EINPROGRESS)
> +	if (rc != -EINPROGRESS && walk->nbytes)
>  		skcipher_walk_done(walk, rc);

[Severity: Critical]
This is a pre-existing issue, but does ctr_paes_crypt() also suffer from the
same backlogged request cleanup problem when rc is -EBUSY?

[ ... ]

> @@ -1512,7 +1528,7 @@ static inline int xts_paes_crypt(struct skcipher_request *req, unsigned long mod
>  			atomic_dec(&ctx->via_engine_ctr);
>  	}
>  
> -	if (rc != -EINPROGRESS)
> +	if (rc != -EINPROGRESS && walk->nbytes)
>  		skcipher_walk_done(walk, rc);

[Severity: Critical]
This is a pre-existing issue, but does xts_paes_crypt() similarly mask
-EBUSY errors, potentially leading to plaintext being written to disk under
heavy load?

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