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

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