On Mon, 15 Jun 2020 at 20:50, Eric Biggers <ebiggers@xxxxxxxxxx> wrote: > > 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. > Agreed. But there is a difference, though. XTS and CBC-CTS are guaranteed not to be used in a chaining manner today, given that there is no possible way you could get the right output for a AF_ALG request that has been split into several skcipher requests: XTS has the IV encryption that occurs only once, and CBC-CTS has the unconditional swapping of the last two blocks, which occurs even if the output is a whole multiple of the block size. Doing either of these more than once will necessarily result in corrupt output. For cases where chaining is more straight-forward, we may have users that we are unaware of, so it is trickier. But that only means that we may need to require iv_out support for other modes than CBC and CTR, not that we need to add complexity like this series is doing. For now, I would prefer to simply introduce a 'permits chaining' flag that only gets set for CBC and CTR, and without any special handling to support chaining for modes where doing so is non-trivial and known to be broken and thus unused anyway. algif_skcipher can then attempt to allocate the skcipher with the 'permits chaining' flag, and fall back to allocating without, and deal with the difference accordingly. Then, we can start adding support for this where necessary, i.e., start with any generic skcipher templates that are needed for chaining support, and add support to other implementations gradually. We should also start adding iv_out test cases and conditionalize the test on whether the skcipher has the 'permits chaining' flag set. > Note that wide-block modes such as Adiantum don't support chaining either. > > Also, please use a better name than "fcsize". > I don't think we need this at all.
