On Fri, Feb 1, 2019 at 8:52 AM Eric Biggers <ebiggers@xxxxxxxxxx> wrote:
> From: Eric Biggers <ebiggers@xxxxxxxxxx>
>
> The x86 MORUS implementations all fail the improved AEAD tests because
> they produce the wrong result with some data layouts. The issue is that
> they assume that if the skcipher_walk API gives 'nbytes' not aligned to
> the walksize (a.k.a. walk.stride), then it is the end of the data. In
> fact, this can happen before the end.
>
> Also, when the CRYPTO_TFM_REQ_MAY_SLEEP flag is given, they can
> incorrectly sleep in the skcipher_walk_*() functions while preemption
> has been disabled by kernel_fpu_begin().
>
> Fix these bugs.
>
> Fixes: 56e8e57fc3a7 ("crypto: morus - Add common SIMD glue code for MORUS")
> Cc: <stable@xxxxxxxxxxxxxxx> # v4.18+
> Cc: Ondrej Mosnacek <omosnace@xxxxxxxxxx>
> Signed-off-by: Eric Biggers <ebiggers@xxxxxxxxxx>
Reviewed-by: Ondrej Mosnacek <omosnace@xxxxxxxxxx>
> ---
> arch/x86/crypto/morus1280_glue.c | 40 +++++++++++++-------------------
> arch/x86/crypto/morus640_glue.c | 39 ++++++++++++-------------------
> 2 files changed, 31 insertions(+), 48 deletions(-)
>
> diff --git a/arch/x86/crypto/morus1280_glue.c b/arch/x86/crypto/morus1280_glue.c
> index 0dccdda1eb3a1..7e600f8bcdad8 100644
> --- a/arch/x86/crypto/morus1280_glue.c
> +++ b/arch/x86/crypto/morus1280_glue.c
> @@ -85,31 +85,20 @@ static void crypto_morus1280_glue_process_ad(
>
> static void crypto_morus1280_glue_process_crypt(struct morus1280_state *state,
> struct morus1280_ops ops,
> - struct aead_request *req)
> + struct skcipher_walk *walk)
> {
> - struct skcipher_walk walk;
> - u8 *cursor_src, *cursor_dst;
> - unsigned int chunksize, base;
> -
> - ops.skcipher_walk_init(&walk, req, false);
> -
> - while (walk.nbytes) {
> - cursor_src = walk.src.virt.addr;
> - cursor_dst = walk.dst.virt.addr;
> - chunksize = walk.nbytes;
> -
> - ops.crypt_blocks(state, cursor_src, cursor_dst, chunksize);
> -
> - base = chunksize & ~(MORUS1280_BLOCK_SIZE - 1);
> - cursor_src += base;
> - cursor_dst += base;
> - chunksize &= MORUS1280_BLOCK_SIZE - 1;
> -
> - if (chunksize > 0)
> - ops.crypt_tail(state, cursor_src, cursor_dst,
> - chunksize);
> + while (walk->nbytes >= MORUS1280_BLOCK_SIZE) {
> + ops.crypt_blocks(state, walk->src.virt.addr,
> + walk->dst.virt.addr,
> + round_down(walk->nbytes,
> + MORUS1280_BLOCK_SIZE));
> + skcipher_walk_done(walk, walk->nbytes % MORUS1280_BLOCK_SIZE);
> + }
>
> - skcipher_walk_done(&walk, 0);
> + if (walk->nbytes) {
> + ops.crypt_tail(state, walk->src.virt.addr, walk->dst.virt.addr,
> + walk->nbytes);
> + skcipher_walk_done(walk, 0);
> }
> }
>
> @@ -147,12 +136,15 @@ static void crypto_morus1280_glue_crypt(struct aead_request *req,
> struct crypto_aead *tfm = crypto_aead_reqtfm(req);
> struct morus1280_ctx *ctx = crypto_aead_ctx(tfm);
> struct morus1280_state state;
> + struct skcipher_walk walk;
> +
> + ops.skcipher_walk_init(&walk, req, true);
>
> kernel_fpu_begin();
>
> ctx->ops->init(&state, &ctx->key, req->iv);
> crypto_morus1280_glue_process_ad(&state, ctx->ops, req->src, req->assoclen);
> - crypto_morus1280_glue_process_crypt(&state, ops, req);
> + crypto_morus1280_glue_process_crypt(&state, ops, &walk);
> ctx->ops->final(&state, tag_xor, req->assoclen, cryptlen);
>
> kernel_fpu_end();
> diff --git a/arch/x86/crypto/morus640_glue.c b/arch/x86/crypto/morus640_glue.c
> index 7b58fe4d9bd1a..cb3a817320160 100644
> --- a/arch/x86/crypto/morus640_glue.c
> +++ b/arch/x86/crypto/morus640_glue.c
> @@ -85,31 +85,19 @@ static void crypto_morus640_glue_process_ad(
>
> static void crypto_morus640_glue_process_crypt(struct morus640_state *state,
> struct morus640_ops ops,
> - struct aead_request *req)
> + struct skcipher_walk *walk)
> {
> - struct skcipher_walk walk;
> - u8 *cursor_src, *cursor_dst;
> - unsigned int chunksize, base;
> -
> - ops.skcipher_walk_init(&walk, req, false);
> -
> - while (walk.nbytes) {
> - cursor_src = walk.src.virt.addr;
> - cursor_dst = walk.dst.virt.addr;
> - chunksize = walk.nbytes;
> -
> - ops.crypt_blocks(state, cursor_src, cursor_dst, chunksize);
> -
> - base = chunksize & ~(MORUS640_BLOCK_SIZE - 1);
> - cursor_src += base;
> - cursor_dst += base;
> - chunksize &= MORUS640_BLOCK_SIZE - 1;
> -
> - if (chunksize > 0)
> - ops.crypt_tail(state, cursor_src, cursor_dst,
> - chunksize);
> + while (walk->nbytes >= MORUS640_BLOCK_SIZE) {
> + ops.crypt_blocks(state, walk->src.virt.addr,
> + walk->dst.virt.addr,
> + round_down(walk->nbytes, MORUS640_BLOCK_SIZE));
> + skcipher_walk_done(walk, walk->nbytes % MORUS640_BLOCK_SIZE);
> + }
>
> - skcipher_walk_done(&walk, 0);
> + if (walk->nbytes) {
> + ops.crypt_tail(state, walk->src.virt.addr, walk->dst.virt.addr,
> + walk->nbytes);
> + skcipher_walk_done(walk, 0);
> }
> }
>
> @@ -143,12 +131,15 @@ static void crypto_morus640_glue_crypt(struct aead_request *req,
> struct crypto_aead *tfm = crypto_aead_reqtfm(req);
> struct morus640_ctx *ctx = crypto_aead_ctx(tfm);
> struct morus640_state state;
> + struct skcipher_walk walk;
> +
> + ops.skcipher_walk_init(&walk, req, true);
>
> kernel_fpu_begin();
>
> ctx->ops->init(&state, &ctx->key, req->iv);
> crypto_morus640_glue_process_ad(&state, ctx->ops, req->src, req->assoclen);
> - crypto_morus640_glue_process_crypt(&state, ops, req);
> + crypto_morus640_glue_process_crypt(&state, ops, &walk);
> ctx->ops->final(&state, tag_xor, req->assoclen, cryptlen);
>
> kernel_fpu_end();
> --
> 2.20.1
>
--
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.
