Re: [PATCH 2/3] s390/crypto: Rework ctr_paes_do_crypt() to remove conditional locking

On 2026-08-04 14:47, Heiko Carstens wrote:
> Harald, Holger,
> 
>> Pre-existing issues:
>> - [High] Silent data corruption when skcipher_walk_done() mapping 
>> errors are overwritten by paes_convert_key() false success.
>> --
>> 
>> --- Patch [2]: [PATCH 2/3] s390/crypto: Rework ctr_paes_do_crypt() to 
>> remove conditional locking ---
>> commit fd135aacfa5dd1086ab5711e4513f6e0edede1dc
>> Author: Heiko Carstens <[email protected]>
>> 
>> s390/crypto: Rework ctr_paes_do_crypt() to remove conditional locking
>> 
>> This commit extracts the core loop of ctr_paes_do_crypt() into a new
>> helper function, __ctr_paes_do_crypt(), to avoid conditionally taken 
>> locks
>> that trip up Clang's static analysis.
>> 
>> > diff --git a/arch/s390/crypto/paes_s390.c b/arch/s390/crypto/paes_s390.c
>> > index 8cfe6166c193c..d4a9108d39f5f 100644
>> > --- a/arch/s390/crypto/paes_s390.c
>> > +++ b/arch/s390/crypto/paes_s390.c
>> 
>> [ ... ]
>> 
>> > @@ -981,34 +955,76 @@ static int ctr_paes_do_crypt(struct s390_paes_ctx *ctx,
>> >  		k = cpacf_kmctr(ctx->fc, param, walk->dst.virt.addr,
>> >  				walk->src.virt.addr, n, ctrptr);
>> >  		if (k) {
>> > -			if (ctrptr == ctrblk)
>> > +			if (ctrptr == ctrblk) {
>> >  				memcpy(walk->iv, ctrptr + k - AES_BLOCK_SIZE,
>> >  				       AES_BLOCK_SIZE);
>> > +			}
>> >  			crypto_inc(walk->iv, AES_BLOCK_SIZE);
>> >  			rc = skcipher_walk_done(walk, nbytes - k);
>> >  		}
>> >  		if (k < n) {
>> >  			if (!maysleep) {
>> > -				if (locked)
>> > -					mutex_unlock(&ctrblk_lock);
>> >  				rc = -EKEYEXPIRED;
>> >  				goto out;
>> >  			}
>> >  			rc = paes_convert_key(ctx, tested);
>> 
>> [Severity: High]
>> This is a pre-existing issue, but does this sequence overwrite an 
>> error
>> from skcipher_walk_done()?
> 
> What should happen with this? Can this be ignored? Or should this be
> fixed like
> 
>   			rc = skcipher_walk_done(walk, nbytes - k);
> 			if (rc) {
> 				if (locked)
> 					mutex_unlock(&ctrblk_lock);
>  				goto out;
> 			}
> 
> before the conversion?
> 
> Even though I said I don't want to fix pre-existing issues, I broke a
> similar code path with the first patch, so...

I'll take care of this and let you know within 1 day. I opened up 
bugzilla
https://bugzilla.linux.ibm.com/show_bug.cgi?id=220960 for tracking.