Re: [PATCH] crypto: caam - Remove broken arc4 support

[Ard Biesheuvel <ardb@kernel.org>]

On Thu, 2 Jul 2020 at 09:32, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Thu, 2 Jul 2020 at 09:27, Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Thu, 2 Jul 2020 at 06:36, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> > >
> > > The arc4 algorithm requires storing state in the request context
> > > in order to allow more than one encrypt/decrypt operation.  As this
> > > driver does not seem to do that, it means that using it for more
> > > than one operation is broken.
> > >
> > > Fixes: eaed71a44ad9 ("crypto: caam - add ecb(*) support")
> > > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> > >
> >
> > Acked-by: Ard Biesheuvel <ardb@kernel.org>
> >
> > All internal users of ecb(arc4) use sync skciphers, so this should
> > only affect user space.
> >
> > I do wonder if the others are doing any better - n2 and bcm iproc also
> > appear to keep the state in the TFM object, while I'd expect the
> > setkey() to be a simple memcpy(), and the initial state derivation to
> > be part of the encrypt flow, right?
> >
> > Maybe we should add a test for this to tcrypt, i.e., do setkey() once
> > and do two encryptions of the same input, and check whether we get
> > back the original data.
> >
>
> Actually, it seems the generic ecb(arc4) is broken as well in this regard.

This may be strictly true, but perhaps reusing the key is not such a
great idea to begin with, given the lack of an IV, so the fact that
skcipher::setkey() operates on the TFM and not the request simply does
not match the ARC4 model.

I suppose you are looking into this for chaining algif_skipcher
requests, right? So in that case, the ARC4 state should really be
treated as an IV, which is owned by the caller, and not stored in
either the TFM or the skcipher request object.