diff mbox series

[v2] crypto: xts - add support for ciphertext stealing

Message ID 20190809171457.12400-1-ard.biesheuvel@linaro.org
State Accepted
Commit 8083b1bf8163e7ae7d8c90f221106d96450b8aa8
Headers show
Series [v2] crypto: xts - add support for ciphertext stealing | expand

Commit Message

Ard Biesheuvel Aug. 9, 2019, 5:14 p.m. UTC
Add support for the missing ciphertext stealing part of the XTS-AES
specification, which permits inputs of any size >= the block size.

Cc: Pascal van Leeuwen <pvanleeuwen@verimatrix.com>
Cc: Ondrej Mosnacek <omosnace@redhat.com>
Tested-by: Milan Broz <gmazyland@gmail.com>

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

---
v2: fix scatterlist issue in async handling
    remove stale comment

 crypto/xts.c | 152 +++++++++++++++++---
 1 file changed, 132 insertions(+), 20 deletions(-)

-- 
2.17.1

Comments

Herbert Xu Aug. 15, 2019, 12:08 p.m. UTC | #1
On Fri, Aug 09, 2019 at 08:14:57PM +0300, Ard Biesheuvel wrote:
> Add support for the missing ciphertext stealing part of the XTS-AES

> specification, which permits inputs of any size >= the block size.

> 

> Cc: Pascal van Leeuwen <pvanleeuwen@verimatrix.com>

> Cc: Ondrej Mosnacek <omosnace@redhat.com>

> Tested-by: Milan Broz <gmazyland@gmail.com>

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---

> v2: fix scatterlist issue in async handling

>     remove stale comment

> 

>  crypto/xts.c | 152 +++++++++++++++++---

>  1 file changed, 132 insertions(+), 20 deletions(-)


Patch 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
Eric Biggers Aug. 16, 2019, 1:02 a.m. UTC | #2
On Thu, Aug 15, 2019 at 10:08:00PM +1000, Herbert Xu wrote:
> On Fri, Aug 09, 2019 at 08:14:57PM +0300, Ard Biesheuvel wrote:

> > Add support for the missing ciphertext stealing part of the XTS-AES

> > specification, which permits inputs of any size >= the block size.

> > 

> > Cc: Pascal van Leeuwen <pvanleeuwen@verimatrix.com>

> > Cc: Ondrej Mosnacek <omosnace@redhat.com>

> > Tested-by: Milan Broz <gmazyland@gmail.com>

> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> > ---

> > v2: fix scatterlist issue in async handling

> >     remove stale comment

> > 

> >  crypto/xts.c | 152 +++++++++++++++++---

> >  1 file changed, 132 insertions(+), 20 deletions(-)

> 

> Patch applied.  Thanks.

> -- 


I'm confused why this was applied as-is, since there are no test vectors for
this added yet.  Nor were any other XTS implementations updated yet, so now
users see inconsistent behavior, and all the XTS comparison fuzz tests fail.
What is the plan for addressing these?  I had assumed that as much as possible
would be fixed up at once.

- Eric
Ard Biesheuvel Aug. 16, 2019, 5:21 a.m. UTC | #3
On Fri, 16 Aug 2019 at 04:02, Eric Biggers <ebiggers@kernel.org> wrote:
>

> On Thu, Aug 15, 2019 at 10:08:00PM +1000, Herbert Xu wrote:

> > On Fri, Aug 09, 2019 at 08:14:57PM +0300, Ard Biesheuvel wrote:

> > > Add support for the missing ciphertext stealing part of the XTS-AES

> > > specification, which permits inputs of any size >= the block size.

> > >

> > > Cc: Pascal van Leeuwen <pvanleeuwen@verimatrix.com>

> > > Cc: Ondrej Mosnacek <omosnace@redhat.com>

> > > Tested-by: Milan Broz <gmazyland@gmail.com>

> > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> > > ---

> > > v2: fix scatterlist issue in async handling

> > >     remove stale comment

> > >

> > >  crypto/xts.c | 152 +++++++++++++++++---

> > >  1 file changed, 132 insertions(+), 20 deletions(-)

> >

> > Patch applied.  Thanks.

> > --

>

> I'm confused why this was applied as-is, since there are no test vectors for

> this added yet.  Nor were any other XTS implementations updated yet, so now

> users see inconsistent behavior, and all the XTS comparison fuzz tests fail.

> What is the plan for addressing these?  I had assumed that as much as possible

> would be fixed up at once.

>


I have the ARM/arm64 changes mostly ready to go [0], but I haven't had
the opportunity to test them on actual hardware yet (nor will I until
the end of next month). This branch contains the test vectors as well,
which check out against these implementations and the generic one (and
Pascal's safexcel one), but obviously, we cannot merge those until all
drivers are fixed.

The fuzz tests failing transiently is not a huge deal, IMO, but we do
need a deadline when we apply the test vectors.

We'll need volunteers to fix the x86, powerpc and s390 code. My branch
adds some helpers that could be useful here, but it really needs the
attention of people who can understand the code and are able to test
it.


[0] https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/log/?h=xts-cts
Herbert Xu Aug. 19, 2019, 6:33 a.m. UTC | #4
On Thu, Aug 15, 2019 at 06:02:33PM -0700, Eric Biggers wrote:
> 

> I'm confused why this was applied as-is, since there are no test vectors for

> this added yet.  Nor were any other XTS implementations updated yet, so now

> users see inconsistent behavior, and all the XTS comparison fuzz tests fail.

> What is the plan for addressing these?  I had assumed that as much as possible

> would be fixed up at once.


Yes we should get all the arch xts code fixed up but as far as
the fuzz testing status is concerned we were already broken in
that some drivers supported this while the generic code didn't
so I don't think this makes it that much worse.

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
diff mbox series

Patch

diff --git a/crypto/xts.c b/crypto/xts.c
index 11211003db7e..78c19e4dde7d 100644
--- a/crypto/xts.c
+++ b/crypto/xts.c
@@ -1,8 +1,6 @@ 
 // SPDX-License-Identifier: GPL-2.0-or-later
 /* XTS: as defined in IEEE1619/D16
  *	http://grouper.ieee.org/groups/1619/email/pdf00086.pdf
- *	(sector sizes which are not a multiple of 16 bytes are,
- *	however currently unsupported)
  *
  * Copyright (c) 2007 Rik Snel <rsnel@cube.dyndns.org>
  *
@@ -34,6 +32,8 @@  struct xts_instance_ctx {
 
 struct rctx {
 	le128 t;
+	struct scatterlist *tail;
+	struct scatterlist sg[2];
 	struct skcipher_request subreq;
 };
 
@@ -84,10 +84,11 @@  static int setkey(struct crypto_skcipher *parent, const u8 *key,
  * mutliple calls to the 'ecb(..)' instance, which usually would be slower than
  * just doing the gf128mul_x_ble() calls again.
  */
-static int xor_tweak(struct skcipher_request *req, bool second_pass)
+static int xor_tweak(struct skcipher_request *req, bool second_pass, bool enc)
 {
 	struct rctx *rctx = skcipher_request_ctx(req);
 	struct crypto_skcipher *tfm = crypto_skcipher_reqtfm(req);
+	const bool cts = (req->cryptlen % XTS_BLOCK_SIZE);
 	const int bs = XTS_BLOCK_SIZE;
 	struct skcipher_walk w;
 	le128 t = rctx->t;
@@ -109,6 +110,20 @@  static int xor_tweak(struct skcipher_request *req, bool second_pass)
 		wdst = w.dst.virt.addr;
 
 		do {
+			if (unlikely(cts) &&
+			    w.total - w.nbytes + avail < 2 * XTS_BLOCK_SIZE) {
+				if (!enc) {
+					if (second_pass)
+						rctx->t = t;
+					gf128mul_x_ble(&t, &t);
+				}
+				le128_xor(wdst, &t, wsrc);
+				if (enc && second_pass)
+					gf128mul_x_ble(&rctx->t, &t);
+				skcipher_walk_done(&w, avail - bs);
+				return 0;
+			}
+
 			le128_xor(wdst++, &t, wsrc++);
 			gf128mul_x_ble(&t, &t);
 		} while ((avail -= bs) >= bs);
@@ -119,17 +134,71 @@  static int xor_tweak(struct skcipher_request *req, bool second_pass)
 	return err;
 }
 
-static int xor_tweak_pre(struct skcipher_request *req)
+static int xor_tweak_pre(struct skcipher_request *req, bool enc)
 {
-	return xor_tweak(req, false);
+	return xor_tweak(req, false, enc);
 }
 
-static int xor_tweak_post(struct skcipher_request *req)
+static int xor_tweak_post(struct skcipher_request *req, bool enc)
 {
-	return xor_tweak(req, true);
+	return xor_tweak(req, true, enc);
 }
 
-static void crypt_done(struct crypto_async_request *areq, int err)
+static void cts_done(struct crypto_async_request *areq, int err)
+{
+	struct skcipher_request *req = areq->data;
+	le128 b;
+
+	if (!err) {
+		struct rctx *rctx = skcipher_request_ctx(req);
+
+		scatterwalk_map_and_copy(&b, rctx->tail, 0, XTS_BLOCK_SIZE, 0);
+		le128_xor(&b, &rctx->t, &b);
+		scatterwalk_map_and_copy(&b, rctx->tail, 0, XTS_BLOCK_SIZE, 1);
+	}
+
+	skcipher_request_complete(req, err);
+}
+
+static int cts_final(struct skcipher_request *req,
+		     int (*crypt)(struct skcipher_request *req))
+{
+	struct priv *ctx = crypto_skcipher_ctx(crypto_skcipher_reqtfm(req));
+	int offset = req->cryptlen & ~(XTS_BLOCK_SIZE - 1);
+	struct rctx *rctx = skcipher_request_ctx(req);
+	struct skcipher_request *subreq = &rctx->subreq;
+	int tail = req->cryptlen % XTS_BLOCK_SIZE;
+	le128 b[2];
+	int err;
+
+	rctx->tail = scatterwalk_ffwd(rctx->sg, req->dst,
+				      offset - XTS_BLOCK_SIZE);
+
+	scatterwalk_map_and_copy(b, rctx->tail, 0, XTS_BLOCK_SIZE, 0);
+	memcpy(b + 1, b, tail);
+	scatterwalk_map_and_copy(b, req->src, offset, tail, 0);
+
+	le128_xor(b, &rctx->t, b);
+
+	scatterwalk_map_and_copy(b, rctx->tail, 0, XTS_BLOCK_SIZE + tail, 1);
+
+	skcipher_request_set_tfm(subreq, ctx->child);
+	skcipher_request_set_callback(subreq, req->base.flags, cts_done, req);
+	skcipher_request_set_crypt(subreq, rctx->tail, rctx->tail,
+				   XTS_BLOCK_SIZE, NULL);
+
+	err = crypt(subreq);
+	if (err)
+		return err;
+
+	scatterwalk_map_and_copy(b, rctx->tail, 0, XTS_BLOCK_SIZE, 0);
+	le128_xor(b, &rctx->t, b);
+	scatterwalk_map_and_copy(b, rctx->tail, 0, XTS_BLOCK_SIZE, 1);
+
+	return 0;
+}
+
+static void encrypt_done(struct crypto_async_request *areq, int err)
 {
 	struct skcipher_request *req = areq->data;
 
@@ -137,47 +206,90 @@  static void crypt_done(struct crypto_async_request *areq, int err)
 		struct rctx *rctx = skcipher_request_ctx(req);
 
 		rctx->subreq.base.flags &= ~CRYPTO_TFM_REQ_MAY_SLEEP;
-		err = xor_tweak_post(req);
+		err = xor_tweak_post(req, true);
+
+		if (!err && unlikely(req->cryptlen % XTS_BLOCK_SIZE)) {
+			err = cts_final(req, crypto_skcipher_encrypt);
+			if (err == -EINPROGRESS)
+				return;
+		}
 	}
 
 	skcipher_request_complete(req, err);
 }
 
-static void init_crypt(struct skcipher_request *req)
+static void decrypt_done(struct crypto_async_request *areq, int err)
+{
+	struct skcipher_request *req = areq->data;
+
+	if (!err) {
+		struct rctx *rctx = skcipher_request_ctx(req);
+
+		rctx->subreq.base.flags &= ~CRYPTO_TFM_REQ_MAY_SLEEP;
+		err = xor_tweak_post(req, false);
+
+		if (!err && unlikely(req->cryptlen % XTS_BLOCK_SIZE)) {
+			err = cts_final(req, crypto_skcipher_decrypt);
+			if (err == -EINPROGRESS)
+				return;
+		}
+	}
+
+	skcipher_request_complete(req, err);
+}
+
+static int init_crypt(struct skcipher_request *req, crypto_completion_t compl)
 {
 	struct priv *ctx = crypto_skcipher_ctx(crypto_skcipher_reqtfm(req));
 	struct rctx *rctx = skcipher_request_ctx(req);
 	struct skcipher_request *subreq = &rctx->subreq;
 
+	if (req->cryptlen < XTS_BLOCK_SIZE)
+		return -EINVAL;
+
 	skcipher_request_set_tfm(subreq, ctx->child);
-	skcipher_request_set_callback(subreq, req->base.flags, crypt_done, req);
+	skcipher_request_set_callback(subreq, req->base.flags, compl, req);
 	skcipher_request_set_crypt(subreq, req->dst, req->dst,
-				   req->cryptlen, NULL);
+				   req->cryptlen & ~(XTS_BLOCK_SIZE - 1), NULL);
 
 	/* calculate first value of T */
 	crypto_cipher_encrypt_one(ctx->tweak, (u8 *)&rctx->t, req->iv);
+
+	return 0;
 }
 
 static int encrypt(struct skcipher_request *req)
 {
 	struct rctx *rctx = skcipher_request_ctx(req);
 	struct skcipher_request *subreq = &rctx->subreq;
+	int err;
 
-	init_crypt(req);
-	return xor_tweak_pre(req) ?:
-		crypto_skcipher_encrypt(subreq) ?:
-		xor_tweak_post(req);
+	err = init_crypt(req, encrypt_done) ?:
+	      xor_tweak_pre(req, true) ?:
+	      crypto_skcipher_encrypt(subreq) ?:
+	      xor_tweak_post(req, true);
+
+	if (err || likely((req->cryptlen % XTS_BLOCK_SIZE) == 0))
+		return err;
+
+	return cts_final(req, crypto_skcipher_encrypt);
 }
 
 static int decrypt(struct skcipher_request *req)
 {
 	struct rctx *rctx = skcipher_request_ctx(req);
 	struct skcipher_request *subreq = &rctx->subreq;
+	int err;
+
+	err = init_crypt(req, decrypt_done) ?:
+	      xor_tweak_pre(req, false) ?:
+	      crypto_skcipher_decrypt(subreq) ?:
+	      xor_tweak_post(req, false);
+
+	if (err || likely((req->cryptlen % XTS_BLOCK_SIZE) == 0))
+		return err;
 
-	init_crypt(req);
-	return xor_tweak_pre(req) ?:
-		crypto_skcipher_decrypt(subreq) ?:
-		xor_tweak_post(req);
+	return cts_final(req, crypto_skcipher_decrypt);
 }
 
 static int init_tfm(struct crypto_skcipher *tfm)