Re: [PATCH 0/9] crypto: fix alignmask handling

[Ard Biesheuvel <ardb@kernel.org>]

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.