Re: [PATCH 08/17] crypto: skcipher - add the ability to abort a skcipher walk

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On Fri, 30 Aug 2019 at 11:03, Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> On Wed, Aug 21, 2019 at 05:32:44PM +0300, Ard Biesheuvel wrote:
> > After starting a skcipher walk, the only way to ensure that all
> > resources it has tied up are released is to complete it. In some
> > cases, it will be useful to be able to abort a walk cleanly after
> > it has started, so add this ability to the skcipher walk API.
> >
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> >  crypto/skcipher.c                  | 3 +++
> >  include/crypto/internal/skcipher.h | 5 +++++
> >  2 files changed, 8 insertions(+)
> >
> > diff --git a/crypto/skcipher.c b/crypto/skcipher.c
> > index 5d836fc3df3e..973ab1c7dcca 100644
> > --- a/crypto/skcipher.c
> > +++ b/crypto/skcipher.c
> > @@ -140,6 +140,9 @@ int skcipher_walk_done(struct skcipher_walk *walk, int err)
> >               goto already_advanced;
> >       }
> >
> > +     if (unlikely(!n))
> > +             goto finish;
> > +
> >       scatterwalk_advance(&walk->in, n);
> >       scatterwalk_advance(&walk->out, n);
> >  already_advanced:
> > diff --git a/include/crypto/internal/skcipher.h b/include/crypto/internal/skcipher.h
> > index d68faa5759ad..bc488173531f 100644
> > --- a/include/crypto/internal/skcipher.h
> > +++ b/include/crypto/internal/skcipher.h
> > @@ -148,6 +148,11 @@ int skcipher_walk_aead_decrypt(struct skcipher_walk *walk,
> >                              struct aead_request *req, bool atomic);
> >  void skcipher_walk_complete(struct skcipher_walk *walk, int err);
> >
> > +static inline void skcipher_walk_abort(struct skcipher_walk *walk)
> > +{
> > +     skcipher_walk_done(walk, walk->nbytes);
> > +}
>
> Couldn't you just abort it by supplying an error in place of
> walk->bytes? IOW I'm fine with this helper but you don't need
> to touch skcipher_walk_done as just giving it an negative err
> value should do the trick.
>

This might be a problem with the implementation of
skcipher_walk_done() in general rather than a limitation in this
particular case, but when calling skcipher_walk_done() with a negative
err value, we never kunmap the src and dst pages. So should I propose
a fix for that instead? Or are the internal callers dealing with this
correctly? (and is it forbidden for external callers to pass negative
values?)