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

[Ard Biesheuvel <ardb@kernel.org>]

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)