On Mon, Jun 15, 2020 at 09:50:50AM +0200, Ard Biesheuvel wrote: > On Mon, 15 Jun 2020 at 09:30, Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx> 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
