Re: [v2 PATCH 0/3] crypto: skcipher - Add support for no chaining and partial chaining

From: Eric Biggers <ebiggers@kernel.org>
To: Ard Biesheuvel <ardb@kernel.org>
Cc: Herbert Xu <herbert@gondor.apana.org.au>,
	Stephan Mueller <smueller@chronox.de>,
	Linux Crypto Mailing List <linux-crypto@vger.kernel.org>
Subject: Re: [v2 PATCH 0/3] crypto: skcipher - Add support for no chaining and partial chaining
Date: Mon, 15 Jun 2020 11:50:28 -0700	[thread overview]
Message-ID: <20200615185028.GB85413@gmail.com> (raw)
In-Reply-To: <CAMj1kXHQNHh4PTLmGKaL+sSyuU1AS4u5F=OyjV6XuAaD21e6yg@mail.gmail.com>

On Mon, Jun 15, 2020 at 09:50:50AM +0200, Ard Biesheuvel wrote:
> On Mon, 15 Jun 2020 at 09:30, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> >
> > On Fri, Jun 12, 2020 at 06:10:57PM +0200, Ard Biesheuvel wrote:
> > >
> > > First of all, the default fcsize for all existing XTS implementations
> > > should be -1 as well, given that chaining is currently not supported
> > > at all at the sckipher interface layer for any of them (due to the
> > > fact that the IV gets encrypted with a different key at the start of
> >
> > Sure.  I was just too lazy to actually set the -1 everywhere.  I'll
> > try to do that before I repost again.
> >
> 
> Fair enough
> 
> > > the operation). This also means it is going to be rather tricky to
> > > implement for h/w accelerated XTS implementations, and it seems to me
> > > that the only way to deal with this is to decrypt the IV in software
> > > before chaining the next operation, which is rather horrid and needs
> > > to be implemented by all of them.
> >
> > I don't think we should support chaining for XTS at all so I don't
> > see why we need to worry about the hardware accelerated XTS code.
> >
> 
> I would prefer that. But if it is fine to disallow chaining altogether
> for XTS, why can't we do the same for cbc-cts? In both cases, user
> space cannot be relying on it today, since the output is incorrect,
> even for inputs that are a round multiple of the block size but are
> broken up and chained.
> 
> > > Given that
> > >
> > > a) this is wholly an AF_ALG issue, as there are no in-kernel users
> > > currently suffering from this afaik,
> > > b) using AF_ALG to get access to software implementations is rather
> > > pointless in general, given that userspace can simply issue the same
> > > instructions directly
> > > c) fixing all XTS and CTS implementation on all arches and all
> > > accelerators is not a small task
> > >
> > > wouldn't it be better to special case XTS and CBC-CTS in
> > > algif_skcipher instead, rather than polluting the skipcher API this
> > > way?
> >
> > As I said we need to be able to differentiate between the ones
> > that can chain vs. the ones that can't.  Putting this knowledge
> > directly into algif_skcipher is just too horrid.
> >
> 
> No disagreement on the horrid. But polluting the API for an issue that
> only affects AF_ALG, which can't possibly be working as expected right
> now is not a great thing either.
> 
> > The alternative is to add this marker into the algorithms.  My
> > point was that if you're going to do that you might as well go
> > a step further and allow cts to chain as it is so straightforward.
> >
> 
> Given the fact that algos that require chaining are broken today and
> nobody noticed until Stephan started relying on the skcipher request
> object's IV field magically retaining its value on subsequent reuse, I
> would prefer it if we could simply mark all of them as non-chainable
> and be done with it. (Note that Stephan's case was invalid to begin
> with)

Wouldn't it make a lot more sense to make skcipher algorithms non-chainable by
default, and only opt-in the ones where chaining is actually working?  At the
moment we only test iv_out for CBC and CTR, so we can expect that all the others
are broken.

Note that wide-block modes such as Adiantum don't support chaining either.

Also, please use a better name than "fcsize".

- Eric