| Message ID | 20210201180237.3171-1-ardb@kernel.org |
|---|---|
| Headers | show |
| Series | crypto: fix alignmask handling | expand |
On Mon, Feb 01, 2021 at 07:02:28PM +0100, Ard Biesheuvel wrote: > Some generic implementations of vintage ciphers rely on alignmasks to > ensure that the input is presented with the right alignment. Given that > these are all C implementations, which may execute on architectures that > don't care about alignment in the first place, it is better to use the > unaligned accessors, which will deal with the misalignment in a way that > is appropriate for the architecture in question (and in many cases, this > means simply ignoring the misalignment, as the hardware doesn't care either) > > So fix this across a number of implementations. Patch #1 stands out because > michael_mic.c was broken in spite of the alignmask. Patch #2 removes tnepres > instead of updating it, given that there is no point in keeping it. > > The remaining patches all update generic ciphers that are outdated but still > used, and which are the only implementations available on most architectures > other than x86. > > > > Ard Biesheuvel (9): > crypto: michael_mic - fix broken misalignment handling > crypto: serpent - get rid of obsolete tnepres variant > crypto: serpent - use unaligned accessors instead of alignmask > crypto: blowfish - use unaligned accessors instead of alignmask > crypto: camellia - use unaligned accessors instead of alignmask > crypto: cast5 - use unaligned accessors instead of alignmask > crypto: cast6 - use unaligned accessors instead of alignmask > crypto: fcrypt - drop unneeded alignmask > crypto: twofish - use unaligned accessors instead of alignmask > > crypto/Kconfig | 3 +- > crypto/blowfish_generic.c | 23 ++-- > crypto/camellia_generic.c | 45 +++---- > crypto/cast5_generic.c | 23 ++-- > crypto/cast6_generic.c | 39 +++--- > crypto/fcrypt.c | 1 - > crypto/michael_mic.c | 31 ++--- > crypto/serpent_generic.c | 126 ++++---------------- > crypto/tcrypt.c | 6 +- > crypto/testmgr.c | 6 - > crypto/testmgr.h | 79 ------------ > crypto/twofish_generic.c | 11 +- > 12 files changed, 90 insertions(+), 303 deletions(-) Thanks for fixing this up! These patches all look good to me, and all the self-tests still pass. You can add: Reviewed-by: Eric Biggers <ebiggers@google.com> - Eric
On Tue, 2 Feb 2021 at 23:20, Eric Biggers <ebiggers@kernel.org> wrote: > > On Mon, Feb 01, 2021 at 07:02:28PM +0100, Ard Biesheuvel wrote: > > Some generic implementations of vintage ciphers rely on alignmasks to > > ensure that the input is presented with the right alignment. Given that > > these are all C implementations, which may execute on architectures that > > don't care about alignment in the first place, it is better to use the > > unaligned accessors, which will deal with the misalignment in a way that > > is appropriate for the architecture in question (and in many cases, this > > means simply ignoring the misalignment, as the hardware doesn't care either) > > > > So fix this across a number of implementations. Patch #1 stands out because > > michael_mic.c was broken in spite of the alignmask. Patch #2 removes tnepres > > instead of updating it, given that there is no point in keeping it. > > > > The remaining patches all update generic ciphers that are outdated but still > > used, and which are the only implementations available on most architectures > > other than x86. > > > > > > > > Ard Biesheuvel (9): > > crypto: michael_mic - fix broken misalignment handling > > crypto: serpent - get rid of obsolete tnepres variant > > crypto: serpent - use unaligned accessors instead of alignmask > > crypto: blowfish - use unaligned accessors instead of alignmask > > crypto: camellia - use unaligned accessors instead of alignmask > > crypto: cast5 - use unaligned accessors instead of alignmask > > crypto: cast6 - use unaligned accessors instead of alignmask > > crypto: fcrypt - drop unneeded alignmask > > crypto: twofish - use unaligned accessors instead of alignmask > > > > crypto/Kconfig | 3 +- > > crypto/blowfish_generic.c | 23 ++-- > > crypto/camellia_generic.c | 45 +++---- > > crypto/cast5_generic.c | 23 ++-- > > crypto/cast6_generic.c | 39 +++--- > > crypto/fcrypt.c | 1 - > > crypto/michael_mic.c | 31 ++--- > > crypto/serpent_generic.c | 126 ++++---------------- > > crypto/tcrypt.c | 6 +- > > crypto/testmgr.c | 6 - > > crypto/testmgr.h | 79 ------------ > > crypto/twofish_generic.c | 11 +- > > 12 files changed, 90 insertions(+), 303 deletions(-) > > Thanks for fixing this up! These patches all look good to me, and all the > self-tests still pass. You can add: > > Reviewed-by: Eric Biggers <ebiggers@google.com> > Thanks Eric One thing that became apparent to me while looking into this stuff is that the skcipher encrypt/decrypt API ignores alignmasks altogether, so this is something we should probably look into at some point, i.e., whether the alignmask handling in the core API is still worth it, and if it is, make skcipher calls honour them. In the ablkcipher->skcipher conversion I did, I was not aware of this, but I don't remember seeing any issues being reported in this area either, so I wonder how many cases actually exist where alignmasks actually matter.
On Wed, Feb 03, 2021 at 10:37:10AM +0100, Ard Biesheuvel wrote: > > One thing that became apparent to me while looking into this stuff is > that the skcipher encrypt/decrypt API ignores alignmasks altogether, > so this is something we should probably look into at some point, i.e., > whether the alignmask handling in the core API is still worth it, and > if it is, make skcipher calls honour them. > > In the ablkcipher->skcipher conversion I did, I was not aware of this, > but I don't remember seeing any issues being reported in this area > either, so I wonder how many cases actually exist where alignmasks > actually matter. What do you mean? With both ablkcipher/skcipher the alignmask was usually enforced through the walker mechanism. Cheers, -- Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On Wed, 3 Feb 2021 at 12:19, Herbert Xu <herbert@gondor.apana.org.au> wrote: > > On Wed, Feb 03, 2021 at 10:37:10AM +0100, Ard Biesheuvel wrote: > > > > One thing that became apparent to me while looking into this stuff is > > that the skcipher encrypt/decrypt API ignores alignmasks altogether, > > so this is something we should probably look into at some point, i.e., > > whether the alignmask handling in the core API is still worth it, and > > if it is, make skcipher calls honour them. > > > > In the ablkcipher->skcipher conversion I did, I was not aware of this, > > but I don't remember seeing any issues being reported in this area > > either, so I wonder how many cases actually exist where alignmasks > > actually matter. > > What do you mean? With both ablkcipher/skcipher the alignmask was > usually enforced through the walker mechanism. > Oops, I missed that completely. Apologies for the noise.
On Mon, Feb 01, 2021 at 07:02:28PM +0100, Ard Biesheuvel wrote: > Some generic implementations of vintage ciphers rely on alignmasks to > ensure that the input is presented with the right alignment. Given that > these are all C implementations, which may execute on architectures that > don't care about alignment in the first place, it is better to use the > unaligned accessors, which will deal with the misalignment in a way that > is appropriate for the architecture in question (and in many cases, this > means simply ignoring the misalignment, as the hardware doesn't care either) > > So fix this across a number of implementations. Patch #1 stands out because > michael_mic.c was broken in spite of the alignmask. Patch #2 removes tnepres > instead of updating it, given that there is no point in keeping it. > > The remaining patches all update generic ciphers that are outdated but still > used, and which are the only implementations available on most architectures > other than x86. > > > > Ard Biesheuvel (9): > crypto: michael_mic - fix broken misalignment handling > crypto: serpent - get rid of obsolete tnepres variant > crypto: serpent - use unaligned accessors instead of alignmask > crypto: blowfish - use unaligned accessors instead of alignmask > crypto: camellia - use unaligned accessors instead of alignmask > crypto: cast5 - use unaligned accessors instead of alignmask > crypto: cast6 - use unaligned accessors instead of alignmask > crypto: fcrypt - drop unneeded alignmask > crypto: twofish - use unaligned accessors instead of alignmask > > crypto/Kconfig | 3 +- > crypto/blowfish_generic.c | 23 ++-- > crypto/camellia_generic.c | 45 +++---- > crypto/cast5_generic.c | 23 ++-- > crypto/cast6_generic.c | 39 +++--- > crypto/fcrypt.c | 1 - > crypto/michael_mic.c | 31 ++--- > crypto/serpent_generic.c | 126 ++++---------------- > crypto/tcrypt.c | 6 +- > crypto/testmgr.c | 6 - > crypto/testmgr.h | 79 ------------ > crypto/twofish_generic.c | 11 +- > 12 files changed, 90 insertions(+), 303 deletions(-) All applied. Thanks. -- Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt