Hi Eric,
On Wed, Jan 23, 2019 at 11:52 PM Eric Biggers ebiggers@kernel.org wrote:
From: Eric Biggers ebiggers@google.com
The generic MORUS implementations all fail the improved AEAD tests because they produce the wrong result with some data layouts. Fix them.
Fixes: 396be41f16fd ("crypto: morus - Add generic MORUS AEAD implementations") Cc: stable@vger.kernel.org # v4.18+ Cc: Ondrej Mosnacek omosnace@redhat.com Signed-off-by: Eric Biggers ebiggers@google.com
crypto/morus1280.c | 13 +++++++------ crypto/morus640.c | 13 +++++++------ 2 files changed, 14 insertions(+), 12 deletions(-)
diff --git a/crypto/morus1280.c b/crypto/morus1280.c index 3889c188f266..b83576b4eb55 100644 --- a/crypto/morus1280.c +++ b/crypto/morus1280.c @@ -366,18 +366,19 @@ static void crypto_morus1280_process_crypt(struct morus1280_state *state, const struct morus1280_ops *ops) { struct skcipher_walk walk;
u8 *dst;const u8 *src; ops->skcipher_walk_init(&walk, req, false); while (walk.nbytes) {src = walk.src.virt.addr;dst = walk.dst.virt.addr;
unsigned int nbytes = walk.nbytes;
ops->crypt_chunk(state, dst, src, walk.nbytes);
if (nbytes < walk.total)nbytes = round_down(nbytes, walk.stride);
skcipher_walk_done(&walk, 0);
ops->crypt_chunk(state, walk.dst.virt.addr, walk.src.virt.addr,nbytes);skcipher_walk_done(&walk, walk.nbytes - nbytes); }
Hm... I assume the problem was that skcipher_walk may give you nbytes that is not rounded to the algorithm's walksize (aka walk.stride) even in the middle of the stream, right? I must have misread the code in skcipher.c and assumed that it automatically makes every step of a round size, with the exception of the very last one, which may include a final partial block. Thinking about it now it makes sense to me that this isn't the case, since for stream ciphers it would mean some unnecessary memcpy'ing in the skcipher_walk code...
Maybe you could explain the problem in one or two sentences in the commit message(s), since the contract of skcipher_walk doesn't seem to be documented anywhere and so it is not obvious why the change is necessary.
Thank you for all your work on this - the improved testmgr tests were long needed!
}
diff --git a/crypto/morus640.c b/crypto/morus640.c index da06ec2f6a80..b6a477444f6d 100644 --- a/crypto/morus640.c +++ b/crypto/morus640.c @@ -365,18 +365,19 @@ static void crypto_morus640_process_crypt(struct morus640_state *state, const struct morus640_ops *ops) { struct skcipher_walk walk;
u8 *dst;const u8 *src; ops->skcipher_walk_init(&walk, req, false); while (walk.nbytes) {src = walk.src.virt.addr;dst = walk.dst.virt.addr;
unsigned int nbytes = walk.nbytes;
ops->crypt_chunk(state, dst, src, walk.nbytes);
if (nbytes < walk.total)nbytes = round_down(nbytes, walk.stride);
skcipher_walk_done(&walk, 0);
ops->crypt_chunk(state, walk.dst.virt.addr, walk.src.virt.addr,nbytes);skcipher_walk_done(&walk, walk.nbytes - nbytes); }}
-- 2.20.1.321.g9e740568ce-goog
-- Ondrej Mosnacek <omosnace at redhat dot com> Associate Software Engineer, Security Technologies Red Hat, Inc.