2019-08-06 07:58:41

by Pascal van Leeuwen

[permalink] [raw]
Subject: [PATCH] crypto: xts - Add support for Cipher Text Stealing

This adds support for Cipher Text Stealing for data blocks that are not an
integer multiple of the cipher block size in size, bringing it fully in
line with the IEEE P1619/D16 standard.

This has been tested with the AES-XTS test vectors from the IEEE P1619/D16
specification as well as some additional test vectors supplied to the
linux_crypto mailing list previously. It has also been fuzzed against
Inside Secure AES-XTS hardware which has been actively used in the field
for more than a decade already.

Signed-off-by: Pascal van Leeuwen <[email protected]>
---
crypto/xts.c | 230 +++++++++++++++++++++++++++++++++++++++++++++++++++++------
1 file changed, 210 insertions(+), 20 deletions(-)

diff --git a/crypto/xts.c b/crypto/xts.c
index 33cf726..9e48876 100644
--- a/crypto/xts.c
+++ b/crypto/xts.c
@@ -1,7 +1,5 @@
/* 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 <[email protected]>
*
@@ -28,6 +26,7 @@

struct priv {
struct crypto_skcipher *child;
+ struct crypto_cipher *base;
struct crypto_cipher *tweak;
};

@@ -37,8 +36,9 @@ struct xts_instance_ctx {
};

struct rctx {
- le128 t;
+ le128 t, tcur;
struct skcipher_request subreq;
+ int rem_bytes, is_encrypt;
};

static int setkey(struct crypto_skcipher *parent, const u8 *key,
@@ -47,6 +47,7 @@ static int setkey(struct crypto_skcipher *parent, const u8 *key,
struct priv *ctx = crypto_skcipher_ctx(parent);
struct crypto_skcipher *child;
struct crypto_cipher *tweak;
+ struct crypto_cipher *base;
int err;

err = xts_verify_key(parent, key, keylen);
@@ -55,9 +56,11 @@ static int setkey(struct crypto_skcipher *parent, const u8 *key,

keylen /= 2;

- /* we need two cipher instances: one to compute the initial 'tweak'
- * by encrypting the IV (usually the 'plain' iv) and the other
- * one to encrypt and decrypt the data */
+ /* we need three cipher instances: one to compute the initial 'tweak'
+ * by encrypting the IV (usually the 'plain' iv), one to encrypt and
+ * decrypt the data and finally one to encrypt the last block(s) for
+ * cipher text stealing
+ */

/* tweak cipher, uses Key2 i.e. the second half of *key */
tweak = ctx->tweak;
@@ -79,6 +82,13 @@ static int setkey(struct crypto_skcipher *parent, const u8 *key,
crypto_skcipher_set_flags(parent, crypto_skcipher_get_flags(child) &
CRYPTO_TFM_RES_MASK);

+ /* Also data cipher, using Key1, for applying CTS */
+ base = ctx->base;
+ crypto_cipher_clear_flags(base, CRYPTO_TFM_REQ_MASK);
+ crypto_cipher_set_flags(base, crypto_skcipher_get_flags(parent) &
+ CRYPTO_TFM_REQ_MASK);
+ err = crypto_cipher_setkey(base, key, keylen);
+
return err;
}

@@ -88,13 +98,12 @@ 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, le128 *t)
{
struct rctx *rctx = skcipher_request_ctx(req);
struct crypto_skcipher *tfm = crypto_skcipher_reqtfm(req);
const int bs = XTS_BLOCK_SIZE;
struct skcipher_walk w;
- le128 t = rctx->t;
int err;

if (second_pass) {
@@ -104,6 +113,7 @@ static int xor_tweak(struct skcipher_request *req, bool second_pass)
}
err = skcipher_walk_virt(&w, req, false);

+ *t = rctx->t;
while (w.nbytes) {
unsigned int avail = w.nbytes;
le128 *wsrc;
@@ -113,8 +123,8 @@ static int xor_tweak(struct skcipher_request *req, bool second_pass)
wdst = w.dst.virt.addr;

do {
- le128_xor(wdst++, &t, wsrc++);
- gf128mul_x_ble(&t, &t);
+ le128_xor(wdst++, t, wsrc++);
+ gf128mul_x_ble(t, t);
} while ((avail -= bs) >= bs);

err = skcipher_walk_done(&w, avail);
@@ -123,14 +133,102 @@ 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, le128 *t)
+{
+ return xor_tweak(req, false, t);
+}
+
+static int xor_tweak_post(struct skcipher_request *req, le128 *t)
{
- return xor_tweak(req, false);
+ return xor_tweak(req, true, t);
}

-static int xor_tweak_post(struct skcipher_request *req)
+static int encrypt_finish_cts(struct skcipher_request *req)
{
- return xor_tweak(req, true);
+ struct rctx *rctx = skcipher_request_ctx(req);
+ /* Not a multiple of cipher blocksize, need CTS applied */
+ struct priv *ctx = crypto_skcipher_ctx(crypto_skcipher_reqtfm(req));
+ le128 lastblock, lastptext;
+ int err = 0;
+
+ /*
+ * Handle last partial block - apply Cipher Text Stealing
+ */
+ /* Copy last ciphertext block just processed to buffer */
+ sg_pcopy_to_buffer(req->dst, sg_nents(req->dst), &lastblock,
+ XTS_BLOCK_SIZE,
+ req->cryptlen - XTS_BLOCK_SIZE);
+ /* Save last plaintext bytes, next step may overwrite!! */
+ sg_pcopy_to_buffer(req->src, sg_nents(req->src), &lastptext,
+ rctx->rem_bytes, req->cryptlen);
+ /* Copy first rem_bytes of ciphertext behind last full block */
+ sg_pcopy_from_buffer(req->dst, sg_nents(req->dst), &lastblock,
+ rctx->rem_bytes, req->cryptlen);
+ /*
+ * Copy last remaining bytes of plaintext to combine buffer,
+ * replacing part of the ciphertext
+ */
+ memcpy(&lastblock, &lastptext, rctx->rem_bytes);
+ /* XTS encrypt the combined block */
+ le128_xor(&lastblock, &rctx->tcur, &lastblock);
+ crypto_cipher_encrypt_one(ctx->base, (u8 *)&lastblock,
+ (u8 *)&lastblock);
+ le128_xor(&lastblock, &rctx->tcur, &lastblock);
+ /* Write combined block to dst as 2nd last cipherblock */
+ sg_pcopy_from_buffer(req->dst, sg_nents(req->dst), &lastblock,
+ XTS_BLOCK_SIZE,
+ req->cryptlen - XTS_BLOCK_SIZE);
+
+ /* Fix up original request length */
+ req->cryptlen += rctx->rem_bytes;
+ return err;
+}
+
+static int decrypt_finish_cts(struct skcipher_request *req)
+{
+ struct rctx *rctx = skcipher_request_ctx(req);
+ /* Not a multiple of cipher blocksize, need CTS applied */
+ struct priv *ctx = crypto_skcipher_ctx(crypto_skcipher_reqtfm(req));
+ le128 tnext, lastblock, lastctext;
+ int err = 0;
+
+ /*
+ * Handle last 2 (partial) blocks - apply Cipher Text Stealing
+ */
+
+ /* Copy last full ciphertext block to buffer */
+ sg_pcopy_to_buffer(req->src, sg_nents(req->src), &lastblock,
+ XTS_BLOCK_SIZE, req->cryptlen);
+ /* Decrypt last full block using *next* tweak */
+ gf128mul_x_ble(&tnext, &rctx->tcur);
+ le128_xor(&lastblock, &tnext, &lastblock);
+ crypto_cipher_decrypt_one(ctx->base, (u8 *)&lastblock,
+ (u8 *)&lastblock);
+ le128_xor(&lastblock, &tnext, &lastblock);
+ /* Save last ciphertext bytes, next step may overwrite!! */
+ sg_pcopy_to_buffer(req->src, sg_nents(req->src), &lastctext,
+ rctx->rem_bytes, req->cryptlen + XTS_BLOCK_SIZE);
+ /* Copy first rem_bytes of this ptext as last partial block */
+ sg_pcopy_from_buffer(req->dst, sg_nents(req->dst), &lastblock,
+ rctx->rem_bytes,
+ req->cryptlen + XTS_BLOCK_SIZE);
+ /*
+ * Copy last remaining bytes of "plaintext" to combine buffer,
+ * replacing part of the ciphertext
+ */
+ memcpy(&lastblock, &lastctext, rctx->rem_bytes);
+ /* XTS decrypt the combined block */
+ le128_xor(&lastblock, &rctx->tcur, &lastblock);
+ crypto_cipher_decrypt_one(ctx->base, (u8 *)&lastblock,
+ (u8 *)&lastblock);
+ le128_xor(&lastblock, &rctx->tcur, &lastblock);
+ /* Write combined block to dst as 2nd last plaintext block */
+ sg_pcopy_from_buffer(req->dst, sg_nents(req->dst), &lastblock,
+ XTS_BLOCK_SIZE, req->cryptlen);
+
+ /* Fix up original request length */
+ req->cryptlen += rctx->rem_bytes + XTS_BLOCK_SIZE;
+ return err;
}

static void crypt_done(struct crypto_async_request *areq, int err)
@@ -139,9 +237,16 @@ static void crypt_done(struct crypto_async_request *areq, int err)

if (!err) {
struct rctx *rctx = skcipher_request_ctx(req);
+ le128 t;

rctx->subreq.base.flags &= ~CRYPTO_TFM_REQ_MAY_SLEEP;
- err = xor_tweak_post(req);
+ err = xor_tweak_post(req, &t);
+
+ if (unlikely(!err && rctx->rem_bytes)) {
+ err = rctx->is_encrypt ?
+ encrypt_finish_cts(req) :
+ decrypt_finish_cts(req);
+ }
}

skcipher_request_complete(req, err);
@@ -167,10 +272,44 @@ static int encrypt(struct skcipher_request *req)
struct rctx *rctx = skcipher_request_ctx(req);
struct skcipher_request *subreq = &rctx->subreq;

+ /* valid bytes in last crypto block */
+ rctx->rem_bytes = req->cryptlen & (XTS_BLOCK_SIZE - 1);
+
+ /* IEEE P1619 does not allow less data than block cipher blocksize */
+ if (unlikely(req->cryptlen < XTS_BLOCK_SIZE))
+ return -EINVAL;
+
init_crypt(req);
- return xor_tweak_pre(req) ?:
+ if (unlikely(rctx->rem_bytes)) {
+ /* Not a multiple of cipher blocksize, need CTS applied */
+ int err = 0;
+
+ /* First process all *full* cipher blocks */
+ req->cryptlen -= rctx->rem_bytes;
+ subreq->cryptlen -= rctx->rem_bytes;
+ err = xor_tweak_pre(req, &rctx->tcur);
+ if (err)
+ goto encrypt_exit;
+ rctx->is_encrypt = 1;
+ err = crypto_skcipher_encrypt(subreq);
+ if (err)
+ goto encrypt_exit;
+ err = xor_tweak_post(req, &rctx->tcur);
+ if (err)
+ goto encrypt_exit;
+
+ return encrypt_finish_cts(req);
+
+encrypt_exit:
+ /* Fix up original request length */
+ req->cryptlen += rctx->rem_bytes;
+ return err;
+ }
+
+ /* Multiple of cipher blocksize, no CTS required */
+ return xor_tweak_pre(req, &rctx->tcur) ?:
crypto_skcipher_encrypt(subreq) ?:
- xor_tweak_post(req);
+ xor_tweak_post(req, &rctx->tcur);
}

static int decrypt(struct skcipher_request *req)
@@ -178,10 +317,49 @@ static int decrypt(struct skcipher_request *req)
struct rctx *rctx = skcipher_request_ctx(req);
struct skcipher_request *subreq = &rctx->subreq;

+ /* valid bytes in last crypto block */
+ rctx->rem_bytes = req->cryptlen & (XTS_BLOCK_SIZE - 1);
+
+ /* IEEE P1619 does not allow less data than block cipher blocksize */
+ if (unlikely(req->cryptlen < XTS_BLOCK_SIZE))
+ return -EINVAL;
+
init_crypt(req);
- return xor_tweak_pre(req) ?:
+ if (unlikely(rctx->rem_bytes)) {
+ int err = 0;
+
+ /* First process all but the last(!) full cipher blocks */
+ req->cryptlen -= rctx->rem_bytes + XTS_BLOCK_SIZE;
+ subreq->cryptlen -= rctx->rem_bytes + XTS_BLOCK_SIZE;
+ /* May not have any full blocks to process here */
+ if (req->cryptlen) {
+ err = xor_tweak_pre(req, &rctx->tcur);
+ if (err)
+ goto decrypt_exit;
+ rctx->is_encrypt = 0;
+ err = crypto_skcipher_decrypt(subreq);
+ if (err)
+ goto decrypt_exit;
+ err = xor_tweak_post(req, &rctx->tcur);
+ if (err)
+ goto decrypt_exit;
+ } else {
+ /* Start from initial tweak */
+ rctx->tcur = rctx->t;
+ }
+
+ return decrypt_finish_cts(req);
+
+decrypt_exit:
+ /* Fix up original request length */
+ req->cryptlen += rctx->rem_bytes + XTS_BLOCK_SIZE;
+ return err;
+ }
+
+ /* Multiple of cipher blocksize, no CTS required */
+ return xor_tweak_pre(req, &rctx->tcur) ?:
crypto_skcipher_decrypt(subreq) ?:
- xor_tweak_post(req);
+ xor_tweak_post(req, &rctx->tcur);
}

static int init_tfm(struct crypto_skcipher *tfm)
@@ -191,6 +369,7 @@ static int init_tfm(struct crypto_skcipher *tfm)
struct priv *ctx = crypto_skcipher_ctx(tfm);
struct crypto_skcipher *child;
struct crypto_cipher *tweak;
+ struct crypto_cipher *base;

child = crypto_spawn_skcipher(&ictx->spawn);
if (IS_ERR(child))
@@ -206,6 +385,15 @@ static int init_tfm(struct crypto_skcipher *tfm)

ctx->tweak = tweak;

+ base = crypto_alloc_cipher(ictx->name, 0, 0);
+ if (IS_ERR(base)) {
+ crypto_free_skcipher(ctx->child);
+ crypto_free_cipher(ctx->tweak);
+ return PTR_ERR(base);
+ }
+
+ ctx->base = base;
+
crypto_skcipher_set_reqsize(tfm, crypto_skcipher_reqsize(child) +
sizeof(struct rctx));

@@ -218,6 +406,7 @@ static void exit_tfm(struct crypto_skcipher *tfm)

crypto_free_skcipher(ctx->child);
crypto_free_cipher(ctx->tweak);
+ crypto_free_cipher(ctx->base);
}

static void free(struct skcipher_instance *inst)
@@ -314,11 +503,12 @@ static int create(struct crypto_template *tmpl, struct rtattr **tb)

inst->alg.base.cra_flags = alg->base.cra_flags & CRYPTO_ALG_ASYNC;
inst->alg.base.cra_priority = alg->base.cra_priority;
- inst->alg.base.cra_blocksize = XTS_BLOCK_SIZE;
+ inst->alg.base.cra_blocksize = 1;
inst->alg.base.cra_alignmask = alg->base.cra_alignmask |
(__alignof__(u64) - 1);

inst->alg.ivsize = XTS_BLOCK_SIZE;
+ inst->alg.chunksize = XTS_BLOCK_SIZE;
inst->alg.min_keysize = crypto_skcipher_alg_min_keysize(alg) * 2;
inst->alg.max_keysize = crypto_skcipher_alg_max_keysize(alg) * 2;

--
1.8.3.1


2019-08-06 18:36:57

by Milan Broz

[permalink] [raw]
Subject: Re: [PATCH] crypto: xts - Add support for Cipher Text Stealing

On 06/08/2019 08:55, Pascal van Leeuwen wrote:
> This adds support for Cipher Text Stealing for data blocks that are not an
> integer multiple of the cipher block size in size, bringing it fully in
> line with the IEEE P1619/D16 standard.
>
> This has been tested with the AES-XTS test vectors from the IEEE P1619/D16
> specification as well as some additional test vectors supplied to the
> linux_crypto mailing list previously. It has also been fuzzed against
> Inside Secure AES-XTS hardware which has been actively used in the field
> for more than a decade already.
>
> Signed-off-by: Pascal van Leeuwen <[email protected]>

Wow, it was quick... thanks for this! :)

I tried to test with my wrapper through AF_ALG (with the discussed test vectors),
but it crashed my 32bit i686 VM (Linus tree with your patch applied)

To reproduce it, run this "kernel" af_alg branch of extracted cryptsetup vector testsuite:
https://github.com/mbroz/cryptsetup_backend_test/tree/kernel

(output) ...
CIPHER vector 00: [aes-ecb,128bits][serpent-ecb,128bits]
CIPHER vector 01: [aes-cbc,128bits][serpent-cbc,128bits]
CIPHER vector 02: [aes-ecb,256bits][serpent-ecb,256bits]
CIPHER vector 03: [aes-cbc,256bits][serpent-cbc,256bits]
CIPHER vector 04: [aes-xts,256bits][serpent-xts,256bits]
CIPHER vector 05: [aes-xts,512bits][serpent-xts,512bits]
CIPHER vector 06: [xchacha12,aes-adiantum,256bits][xchacha20,aes-adiantum,256bits]
Segmentation fault


(If you cannot reproduce it, I'll check it tomorrow. It is quite possible
I have a bug in wrapper, but it should definitely not OOPS the kernel...
moreover, this crash is possible from a user context)

Thanks,
Milan


OOPS log here:

kernel: detected buffer overflow in memcpy
kernel: ------------[ cut here ]------------
kernel: kernel BUG at lib/string.c:1115!
kernel: invalid opcode: 0000 [#1] PREEMPT SMP
kernel: CPU: 1 PID: 2303 Comm: tst Not tainted 5.3.0-rc3+ #572
kernel: Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 04/13/2018
kernel: EIP: fortify_panic+0xe/0x19
kernel: Code: b6 00 00 00 00 8b 45 e4 83 c4 10 5b 5e 29 f8 5f 5d c3 0f b6 c2 0f b6 f3 29 f0 eb ce 55 89 e5 50 68 70 03 8d c1 e8 cc 27 a6 ff <0f> 0b 90 90 90 90 90 90 90 90 90 55 89 e5 57 56 53 89 d3 83 ec 04
kernel: EAX: 00000022 EBX: f1418dc8 ECX: f40ab784 EDX: 00000001
kernel: ESI: f317a318 EDI: 00000020 EBP: f0c2bdf4 ESP: f0c2bdec
kernel: DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068 EFLAGS: 00010286
kernel: CR0: 80050033 CR2: b7f543a0 CR3: 30e2a000 CR4: 00140690
kernel: Call Trace:
kernel: encrypt_finish_cts.cold+0xa/0xa
kernel: encrypt+0xe7/0x100
kernel: crypto_skcipher_encrypt+0xe/0x20
kernel: skcipher_recvmsg+0x2f5/0x390 [algif_skcipher]
kernel: sock_read_iter+0x86/0xd0
kernel: __vfs_read+0x140/0x1f0
kernel: vfs_read+0x8b/0x150
kernel: ksys_read+0x5c/0xd0
kernel: sys_read+0x11/0x20
kernel: do_int80_syscall_32+0x4b/0x1a0
kernel: entry_INT80_32+0xfb/0xfb
kernel: EIP: 0xb7f64092
kernel: Code: 00 00 00 e9 90 ff ff ff ff a3 24 00 00 00 68 30 00 00 00 e9 80 ff ff ff ff a3 2c 00 00 00 66 90 00 00 00 00 00 00 00 00 cd 80 <c3> 8d b4 26 00 00 00 00 8d b6 00 00 00 00 8b 1c 24 c3 8d b4 26 00
kernel: EAX: ffffffda EBX: 00000004 ECX: bfde1d3c EDX: 00000025
kernel: ESI: b7f57000 EDI: 00000000 EBP: bfde1cb8 ESP: bfde1bcc
kernel: DS: 007b ES: 007b FS: 0000 GS: 0033 SS: 007b EFLAGS: 00000282
kernel: Modules linked in: nhpoly1305 chacha_generic adiantum poly1305_generic serpent_sse2_i586 serpent_generic glue_helper algif_skcipher rmd160 wp512 sha512_generic sha1_generic algif_hash af_alg loop dm_mod pktcdvd crc32_pclmul crc32c_intel aesni_intel aes_i586 crypto_simd cryptd ata_piix
kernel: ---[ end trace 29d18b04feffc139 ]---

2019-08-06 19:37:22

by Pascal Van Leeuwen

[permalink] [raw]
Subject: RE: [PATCH] crypto: xts - Add support for Cipher Text Stealing

Milan,

Thanks for trying :-)

> -----Original Message-----
> From: Milan Broz <[email protected]>
> Sent: Tuesday, August 6, 2019 8:35 PM
> To: Pascal van Leeuwen <[email protected]>; [email protected]
> Cc: [email protected]; [email protected]; [email protected]; Pascal Van
> Leeuwen <[email protected]>
> Subject: Re: [PATCH] crypto: xts - Add support for Cipher Text Stealing
>
> On 06/08/2019 08:55, Pascal van Leeuwen wrote:
> > This adds support for Cipher Text Stealing for data blocks that are not an
> > integer multiple of the cipher block size in size, bringing it fully in
> > line with the IEEE P1619/D16 standard.
> >
> > This has been tested with the AES-XTS test vectors from the IEEE P1619/D16
> > specification as well as some additional test vectors supplied to the
> > linux_crypto mailing list previously. It has also been fuzzed against
> > Inside Secure AES-XTS hardware which has been actively used in the field
> > for more than a decade already.
> >
> > Signed-off-by: Pascal van Leeuwen <[email protected]>
>
> Wow, it was quick... thanks for this! :)
>
> I tried to test with my wrapper through AF_ALG (with the discussed test vectors),
> but it crashed my 32bit i686 VM (Linus tree with your patch applied)
>
Ok, I have to admit I only tried it on one particular kernel configuration.
I don't really have the time nor the resources (I also have to work on this
machine, so I need it to be stable ...) to really try other configurations.

> To reproduce it, run this "kernel" af_alg branch of extracted cryptsetup vector testsuite:
> https://github.com/mbroz/cryptsetup_backend_test/tree/kernel
>
I'll try that tomorrow if I can get it to work without too much effort.

> (output) ...
> CIPHER vector 00: [aes-ecb,128bits][serpent-ecb,128bits]
> CIPHER vector 01: [aes-cbc,128bits][serpent-cbc,128bits]
> CIPHER vector 02: [aes-ecb,256bits][serpent-ecb,256bits]
> CIPHER vector 03: [aes-cbc,256bits][serpent-cbc,256bits]
> CIPHER vector 04: [aes-xts,256bits][serpent-xts,256bits]
> CIPHER vector 05: [aes-xts,512bits][serpent-xts,512bits]
> CIPHER vector 06: [xchacha12,aes-adiantum,256bits][xchacha20,aes-adiantum,256bits]
> Segmentation fault
>
I also noticed some algorithms that I don't think I have configured into my
kernel at the moment (never occurred to me really, I try not to touch my kernel
config so I don't break it and I wasn't originally interested in these as I can't
think of any use case for them), maybe I should try enabling those with my kernel
first.

I know there's part of the code I can't hit with xts(aes), but I also didn't know
how to otherwise hit it, maybe one of these does hit that and reveals a bug there.

>
> (If you cannot reproduce it, I'll check it tomorrow. It is quite possible
> I have a bug in wrapper, but it should definitely not OOPS the kernel...
> moreover, this crash is possible from a user context)
>
> Thanks,
> Milan
>
>
> OOPS log here:
>
> kernel: detected buffer overflow in memcpy
>
Ok, so I probably messed up some length somewhere :-)
Let's see if I can spot it by careful review ...

> kernel: ------------[ cut here ]------------
> kernel: kernel BUG at lib/string.c:1115!
> kernel: invalid opcode: 0000 [#1] PREEMPT SMP
> kernel: CPU: 1 PID: 2303 Comm: tst Not tainted 5.3.0-rc3+ #572
> kernel: Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform,
> BIOS 6.00 04/13/2018
> kernel: EIP: fortify_panic+0xe/0x19
> kernel: Code: b6 00 00 00 00 8b 45 e4 83 c4 10 5b 5e 29 f8 5f 5d c3 0f b6 c2 0f b6 f3 29 f0
> eb ce 55 89 e5 50 68 70 03 8d c1 e8 cc 27 a6 ff <0f> 0b 90 90 90 90 90 90 90 90 90 55 89 e5
> 57 56 53 89 d3 83 ec 04
> kernel: EAX: 00000022 EBX: f1418dc8 ECX: f40ab784 EDX: 00000001
> kernel: ESI: f317a318 EDI: 00000020 EBP: f0c2bdf4 ESP: f0c2bdec
> kernel: DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068 EFLAGS: 00010286
> kernel: CR0: 80050033 CR2: b7f543a0 CR3: 30e2a000 CR4: 00140690
> kernel: Call Trace:
> kernel: encrypt_finish_cts.cold+0xa/0xa
> kernel: encrypt+0xe7/0x100
> kernel: crypto_skcipher_encrypt+0xe/0x20
> kernel: skcipher_recvmsg+0x2f5/0x390 [algif_skcipher]
> kernel: sock_read_iter+0x86/0xd0
> kernel: __vfs_read+0x140/0x1f0
> kernel: vfs_read+0x8b/0x150
> kernel: ksys_read+0x5c/0xd0
> kernel: sys_read+0x11/0x20
> kernel: do_int80_syscall_32+0x4b/0x1a0
> kernel: entry_INT80_32+0xfb/0xfb
> kernel: EIP: 0xb7f64092
> kernel: Code: 00 00 00 e9 90 ff ff ff ff a3 24 00 00 00 68 30 00 00 00 e9 80 ff ff ff ff a3
> 2c 00 00 00 66 90 00 00 00 00 00 00 00 00 cd 80 <c3> 8d b4 26 00 00 00 00 8d b6 00 00 00 00
> 8b 1c 24 c3 8d b4 26 00
> kernel: EAX: ffffffda EBX: 00000004 ECX: bfde1d3c EDX: 00000025
> kernel: ESI: b7f57000 EDI: 00000000 EBP: bfde1cb8 ESP: bfde1bcc
> kernel: DS: 007b ES: 007b FS: 0000 GS: 0033 SS: 007b EFLAGS: 00000282
> kernel: Modules linked in: nhpoly1305 chacha_generic adiantum poly1305_generic
> serpent_sse2_i586 serpent_generic glue_helper algif_skcipher rmd160 wp512 sha512_generic
> sha1_generic algif_hash af_alg loop dm_mod pktcdvd crc32_pclmul crc32c_intel aesni_intel
> aes_i586 crypto_simd cryptd ata_piix
> kernel: ---[ end trace 29d18b04feffc139 ]---

Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
http://www.insidesecure.com

2019-08-07 08:19:17

by Pascal Van Leeuwen

[permalink] [raw]
Subject: RE: [PATCH] crypto: xts - Add support for Cipher Text Stealing

Milan,


> -----Original Message-----
> From: [email protected] <[email protected]> On Behalf Of
> Pascal Van Leeuwen
> Sent: Tuesday, August 6, 2019 9:37 PM
> To: Milan Broz <[email protected]>; Pascal van Leeuwen <[email protected]>; linux-
> [email protected]
> Cc: [email protected]; [email protected]; [email protected]
> Subject: RE: [PATCH] crypto: xts - Add support for Cipher Text Stealing
>
> Milan,
>
> Thanks for trying :-)
>
> > -----Original Message-----
> > From: Milan Broz <[email protected]>
> > Sent: Tuesday, August 6, 2019 8:35 PM
> > To: Pascal van Leeuwen <[email protected]>; [email protected]
> > Cc: [email protected]; [email protected]; [email protected]; Pascal Van
> > Leeuwen <[email protected]>
> > Subject: Re: [PATCH] crypto: xts - Add support for Cipher Text Stealing
> >
> > On 06/08/2019 08:55, Pascal van Leeuwen wrote:
> > > This adds support for Cipher Text Stealing for data blocks that are not an
> > > integer multiple of the cipher block size in size, bringing it fully in
> > > line with the IEEE P1619/D16 standard.
> > >
> > > This has been tested with the AES-XTS test vectors from the IEEE P1619/D16
> > > specification as well as some additional test vectors supplied to the
> > > linux_crypto mailing list previously. It has also been fuzzed against
> > > Inside Secure AES-XTS hardware which has been actively used in the field
> > > for more than a decade already.
> > >
> > > Signed-off-by: Pascal van Leeuwen <[email protected]>
> >
> > Wow, it was quick... thanks for this! :)
> >
> > I tried to test with my wrapper through AF_ALG (with the discussed test vectors),
> > but it crashed my 32bit i686 VM (Linus tree with your patch applied)
> >
> Ok, I have to admit I only tried it on one particular kernel configuration.
> I don't really have the time nor the resources (I also have to work on this
> machine, so I need it to be stable ...) to really try other configurations.
>
> > To reproduce it, run this "kernel" af_alg branch of extracted cryptsetup vector
> testsuite:
> > https://github.com/mbroz/cryptsetup_backend_test/tree/kernel
> >
> I'll try that tomorrow if I can get it to work without too much effort.
>
Ok, I tried this on my kernel but it works just fine.
But if I understand correctly, you run a 32 bit kernel under some VM or so?
Perhaps that does some stricter buffer overflow checks? In any case, I
wouldn't know how to do that ...

> > (output) ...
> > CIPHER vector 00: [aes-ecb,128bits][serpent-ecb,128bits]
> > CIPHER vector 01: [aes-cbc,128bits][serpent-cbc,128bits]
> > CIPHER vector 02: [aes-ecb,256bits][serpent-ecb,256bits]
> > CIPHER vector 03: [aes-cbc,256bits][serpent-cbc,256bits]
> > CIPHER vector 04: [aes-xts,256bits][serpent-xts,256bits]
> > CIPHER vector 05: [aes-xts,512bits][serpent-xts,512bits]
> > CIPHER vector 06: [xchacha12,aes-adiantum,256bits][xchacha20,aes-adiantum,256bits]
> > Segmentation fault
> >
> I also noticed some algorithms that I don't think I have configured into my
> kernel at the moment (never occurred to me really, I try not to touch my kernel
> config so I don't break it and I wasn't originally interested in these as I can't
> think of any use case for them), maybe I should try enabling those with my kernel
> first.
>
Actually, it turns out I had everything enabled already except adiantum, which
should not be relevant for XTS anyway. But it actually doesn't matter much,
since (my) testmgr only has CTS vectors for AES, so the CTS code is not hit for
anything else unless you run some application that does so.

> I know there's part of the code I can't hit with xts(aes), but I also didn't know
> how to otherwise hit it, maybe one of these does hit that and reveals a bug there.
>
> >
> > (If you cannot reproduce it, I'll check it tomorrow. It is quite possible
> > I have a bug in wrapper, but it should definitely not OOPS the kernel...
> > moreover, this crash is possible from a user context)
> >
> > Thanks,
> > Milan
> >
> >
> > OOPS log here:
> >
> > kernel: detected buffer overflow in memcpy
> >
> Ok, so I probably messed up some length somewhere :-)
> Let's see if I can spot it by careful review ...
>
I went through the code a couple of times, but I cannot spot any mistakes in
the lengths I'm using. Is it possible that your application is supplying a
buffer that is just not large enough?

> > kernel: ------------[ cut here ]------------
> > kernel: kernel BUG at lib/string.c:1115!
> > kernel: invalid opcode: 0000 [#1] PREEMPT SMP
> > kernel: CPU: 1 PID: 2303 Comm: tst Not tainted 5.3.0-rc3+ #572
> > kernel: Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference
> Platform,
> > BIOS 6.00 04/13/2018
> > kernel: EIP: fortify_panic+0xe/0x19
> > kernel: Code: b6 00 00 00 00 8b 45 e4 83 c4 10 5b 5e 29 f8 5f 5d c3 0f b6 c2 0f b6 f3 29
> f0
> > eb ce 55 89 e5 50 68 70 03 8d c1 e8 cc 27 a6 ff <0f> 0b 90 90 90 90 90 90 90 90 90 55 89
> e5
> > 57 56 53 89 d3 83 ec 04
> > kernel: EAX: 00000022 EBX: f1418dc8 ECX: f40ab784 EDX: 00000001
> > kernel: ESI: f317a318 EDI: 00000020 EBP: f0c2bdf4 ESP: f0c2bdec
> > kernel: DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068 EFLAGS: 00010286
> > kernel: CR0: 80050033 CR2: b7f543a0 CR3: 30e2a000 CR4: 00140690
> > kernel: Call Trace:
> > kernel: encrypt_finish_cts.cold+0xa/0xa
> > kernel: encrypt+0xe7/0x100
> > kernel: crypto_skcipher_encrypt+0xe/0x20
> > kernel: skcipher_recvmsg+0x2f5/0x390 [algif_skcipher]
> > kernel: sock_read_iter+0x86/0xd0
> > kernel: __vfs_read+0x140/0x1f0
> > kernel: vfs_read+0x8b/0x150
> > kernel: ksys_read+0x5c/0xd0
> > kernel: sys_read+0x11/0x20
> > kernel: do_int80_syscall_32+0x4b/0x1a0
> > kernel: entry_INT80_32+0xfb/0xfb
> > kernel: EIP: 0xb7f64092
> > kernel: Code: 00 00 00 e9 90 ff ff ff ff a3 24 00 00 00 68 30 00 00 00 e9 80 ff ff ff ff
> a3
> > 2c 00 00 00 66 90 00 00 00 00 00 00 00 00 cd 80 <c3> 8d b4 26 00 00 00 00 8d b6 00 00 00
> 00
> > 8b 1c 24 c3 8d b4 26 00
> > kernel: EAX: ffffffda EBX: 00000004 ECX: bfde1d3c EDX: 00000025
> > kernel: ESI: b7f57000 EDI: 00000000 EBP: bfde1cb8 ESP: bfde1bcc
> > kernel: DS: 007b ES: 007b FS: 0000 GS: 0033 SS: 007b EFLAGS: 00000282
> > kernel: Modules linked in: nhpoly1305 chacha_generic adiantum poly1305_generic
> > serpent_sse2_i586 serpent_generic glue_helper algif_skcipher rmd160 wp512 sha512_generic
> > sha1_generic algif_hash af_alg loop dm_mod pktcdvd crc32_pclmul crc32c_intel aesni_intel
> > aes_i586 crypto_simd cryptd ata_piix
> > kernel: ---[ end trace 29d18b04feffc139 ]---
>
> Regards,
> Pascal van Leeuwen
> Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
> http://www.insidesecure.com

Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
http://www.insidesecure.com

2019-08-07 11:23:37

by Milan Broz

[permalink] [raw]
Subject: Re: [PATCH] crypto: xts - Add support for Cipher Text Stealing

Hi,

On 07/08/2019 10:15, Pascal Van Leeuwen wrote:
> I went through the code a couple of times, but I cannot spot any mistakes in
> the lengths I'm using. Is it possible that your application is supplying a
> buffer that is just not large enough?

Seems there is no mistake in your code, it is some bug in aesni_intel implementation.
If I disable this module, it works as expected (with aes generic and aes_i586).

Seems something is rewritten in call
crypto_skcipher_encrypt(subreq);

(after that call, I see rctx->rem_bytes set to 32, that does not make sense...)

I'll check that, but not sure that understand that optimized code :)

Milan

2019-08-07 11:28:49

by Pascal Van Leeuwen

[permalink] [raw]
Subject: RE: [PATCH] crypto: xts - Add support for Cipher Text Stealing

Milan,


> -----Original Message-----
> From: Milan Broz <[email protected]>
> Sent: Wednesday, August 7, 2019 1:20 PM
> To: Pascal Van Leeuwen <[email protected]>; Pascal van Leeuwen
> <[email protected]>; [email protected]
> Cc: [email protected]; [email protected]; [email protected]
> Subject: Re: [PATCH] crypto: xts - Add support for Cipher Text Stealing
>
> Hi,
>
> On 07/08/2019 10:15, Pascal Van Leeuwen wrote:
> > I went through the code a couple of times, but I cannot spot any mistakes in
> > the lengths I'm using. Is it possible that your application is supplying a
> > buffer that is just not large enough?
>
> Seems there is no mistake in your code, it is some bug in aesni_intel implementation.
> If I disable this module, it works as expected (with aes generic and aes_i586).
>
That's odd though, considering there is a dedicated xts-aes-ni implementation,
i.e. I would not expect that to end up at the generic xts wrapper at all?

> Seems something is rewritten in call
> crypto_skcipher_encrypt(subreq);
>
> (after that call, I see rctx->rem_bytes set to 32, that does not make sense...)
>
Eh ... no, it should never become > 15 ... if it gets set to 32 somehow,
then I can at least explain why that would result in a buffer overflow :-)

> I'll check that, but not sure that understand that optimized code :)
>
> Milan

Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
http://www.insidesecure.com

2019-08-07 11:42:51

by Milan Broz

[permalink] [raw]
Subject: Re: [PATCH] crypto: xts - Add support for Cipher Text Stealing

On 07/08/2019 13:27, Pascal Van Leeuwen wrote:
>> On 07/08/2019 10:15, Pascal Van Leeuwen wrote:
>>> I went through the code a couple of times, but I cannot spot any mistakes in
>>> the lengths I'm using. Is it possible that your application is supplying a
>>> buffer that is just not large enough?
>>
>> Seems there is no mistake in your code, it is some bug in aesni_intel implementation.
>> If I disable this module, it works as expected (with aes generic and aes_i586).
>>
> That's odd though, considering there is a dedicated xts-aes-ni implementation,
> i.e. I would not expect that to end up at the generic xts wrapper at all?

Note it is 32bit system, AESNI XTS is under #ifdef CONFIG_X86_64 so it is not used.

I guess it only ECB part ...

>> Seems something is rewritten in call
>> crypto_skcipher_encrypt(subreq);
>>
>> (after that call, I see rctx->rem_bytes set to 32, that does not make sense...)
>>
> Eh ... no, it should never become > 15 ... if it gets set to 32 somehow,
> then I can at least explain why that would result in a buffer overflow :-)

Yes, that explains it.

(And rewriting this value back does not help, I got different test vector output, but no crash.)

Milan

2019-08-07 15:16:32

by Pascal Van Leeuwen

[permalink] [raw]
Subject: RE: [PATCH] crypto: xts - Add support for Cipher Text Stealing

> -----Original Message-----
> From: Milan Broz <[email protected]>
> Sent: Wednesday, August 7, 2019 1:42 PM
> To: Pascal Van Leeuwen <[email protected]>; Pascal van Leeuwen
> <[email protected]>; [email protected]
> Cc: [email protected]; [email protected]; [email protected]
> Subject: Re: [PATCH] crypto: xts - Add support for Cipher Text Stealing
>
> On 07/08/2019 13:27, Pascal Van Leeuwen wrote:
> >> On 07/08/2019 10:15, Pascal Van Leeuwen wrote:
> >>> I went through the code a couple of times, but I cannot spot any mistakes in
> >>> the lengths I'm using. Is it possible that your application is supplying a
> >>> buffer that is just not large enough?
> >>
> >> Seems there is no mistake in your code, it is some bug in aesni_intel implementation.
> >> If I disable this module, it works as expected (with aes generic and aes_i586).
> >>
> > That's odd though, considering there is a dedicated xts-aes-ni implementation,
> > i.e. I would not expect that to end up at the generic xts wrapper at all?
>
> Note it is 32bit system, AESNI XTS is under #ifdef CONFIG_X86_64 so it is not used.
>
Ok, so I guess no one bothered to make an optimized XTS version for i386.
I quickly browsed through the code - took me a while to realise the assembly is
"backwards" compared to the original Intel definition :-) - but I did not spot
anything obvious :-(

> I guess it only ECB part ...
>
> >> Seems something is rewritten in call
> >> crypto_skcipher_encrypt(subreq);
> >>
> >> (after that call, I see rctx->rem_bytes set to 32, that does not make sense...)
> >>
Depending on what you have observed so far, it could also be corrupted earlier,
i.e. during one of the skcipher_request* calls inside init_crypt?

Notably, the rem_bytes variable is immediately behind the subreq struct inside
rctx, but obviously these calls should not write beyond the end of that struct
(as without those added variables, those writes would end up outside of the
allocated rctx struct)

> > Eh ... no, it should never become > 15 ... if it gets set to 32 somehow,
> > then I can at least explain why that would result in a buffer overflow :-)
>
> Yes, that explains it.
>
> (And rewriting this value back does not help, I got different test vector output, but no
> crash.)
>
Well, there's also an is_encrypt flag immediately behind rem_bytes that
likely also gets corrupted?

> Milan

Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
http://www.insidesecure.com

2019-08-07 17:44:44

by Milan Broz

[permalink] [raw]
Subject: Re: [PATCH] crypto: xts - Add support for Cipher Text Stealing

On 07/08/2019 17:13, Pascal Van Leeuwen wrote:
>>>> Seems there is no mistake in your code, it is some bug in aesni_intel implementation.
>>>> If I disable this module, it works as expected (with aes generic and aes_i586).
>>>>
>>> That's odd though, considering there is a dedicated xts-aes-ni implementation,
>>> i.e. I would not expect that to end up at the generic xts wrapper at all?
>>
>> Note it is 32bit system, AESNI XTS is under #ifdef CONFIG_X86_64 so it is not used.
>>
> Ok, so I guess no one bothered to make an optimized XTS version for i386.
> I quickly browsed through the code - took me a while to realise the assembly is
> "backwards" compared to the original Intel definition :-) - but I did not spot
> anything obvious :-(
>
>> I guess it only ECB part ...

Mystery solved, the skcipher subreq must be te last member in the struct.
(Some comments in Adiantum code mentions it too, so I do not think it
just hides the corruption after the struct. Seems like another magic requirement
in crypto API :-)

This chunk is enough to fix it for me:

--- a/crypto/xts.c
+++ b/crypto/xts.c
@@ -33,8 +33,8 @@ struct xts_instance_ctx {

struct rctx {
le128 t, tcur;
- struct skcipher_request subreq;
int rem_bytes, is_encrypt;
+ struct skcipher_request subreq;
};

While at it, shouldn't be is_encrypt bool?

Thanks,
Milan

2019-08-07 20:26:36

by Pascal Van Leeuwen

[permalink] [raw]
Subject: RE: [PATCH] crypto: xts - Add support for Cipher Text Stealing

> -----Original Message-----
> From: Milan Broz <[email protected]>
> Sent: Wednesday, August 7, 2019 7:24 PM
> To: Pascal Van Leeuwen <[email protected]>; Pascal van Leeuwen
> <[email protected]>; [email protected]
> Cc: [email protected]; [email protected]; [email protected]
> Subject: Re: [PATCH] crypto: xts - Add support for Cipher Text Stealing
>
> On 07/08/2019 17:13, Pascal Van Leeuwen wrote:
> >>>> Seems there is no mistake in your code, it is some bug in aesni_intel implementation.
> >>>> If I disable this module, it works as expected (with aes generic and aes_i586).
> >>>>
> >>> That's odd though, considering there is a dedicated xts-aes-ni implementation,
> >>> i.e. I would not expect that to end up at the generic xts wrapper at all?
> >>
> >> Note it is 32bit system, AESNI XTS is under #ifdef CONFIG_X86_64 so it is not used.
> >>
> > Ok, so I guess no one bothered to make an optimized XTS version for i386.
> > I quickly browsed through the code - took me a while to realise the assembly is
> > "backwards" compared to the original Intel definition :-) - but I did not spot
> > anything obvious :-(
> >
> >> I guess it only ECB part ...
>
> Mystery solved, the skcipher subreq must be te last member in the struct.
> (Some comments in Adiantum code mentions it too, so I do not think it
> just hides the corruption after the struct. Seems like another magic requirement
> in crypto API :-)
>
> This chunk is enough to fix it for me:
>
> --- a/crypto/xts.c
> +++ b/crypto/xts.c
> @@ -33,8 +33,8 @@ struct xts_instance_ctx {
>
> struct rctx {
> le128 t, tcur;
> - struct skcipher_request subreq;
> int rem_bytes, is_encrypt;
> + struct skcipher_request subreq;
> };
>
> While at it, shouldn't be is_encrypt bool?
>
> Thanks,
> Milan
While I do understand how that prevents corruption of rem_bytes and
is_encrypt, doesn't that just *hide* the issue?

The memory beyond the end of the rctx struct is not allocated as far
as I can tell, so how can you legally write there?

I hope someone can explain this to me.

Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
http://www.insidesecure.com

2019-08-07 20:50:34

by Ondrej Mosnáček

[permalink] [raw]
Subject: Re: [PATCH] crypto: xts - Add support for Cipher Text Stealing

st 7. 8. 2019 o 19:44 Milan Broz <[email protected]> napísal(a):
> On 07/08/2019 17:13, Pascal Van Leeuwen wrote:
> >>>> Seems there is no mistake in your code, it is some bug in aesni_intel implementation.
> >>>> If I disable this module, it works as expected (with aes generic and aes_i586).
> >>>>
> >>> That's odd though, considering there is a dedicated xts-aes-ni implementation,
> >>> i.e. I would not expect that to end up at the generic xts wrapper at all?
> >>
> >> Note it is 32bit system, AESNI XTS is under #ifdef CONFIG_X86_64 so it is not used.
> >>
> > Ok, so I guess no one bothered to make an optimized XTS version for i386.
> > I quickly browsed through the code - took me a while to realise the assembly is
> > "backwards" compared to the original Intel definition :-) - but I did not spot
> > anything obvious :-(
> >
> >> I guess it only ECB part ...
>
> Mystery solved, the skcipher subreq must be te last member in the struct.
> (Some comments in Adiantum code mentions it too, so I do not think it
> just hides the corruption after the struct. Seems like another magic requirement
> in crypto API :-)

Oh, yes, this makes sense! I would have noticed this immediately if I
had looked carefully at the struct definition :) The reason is that
the skcipher_request struct is followed by a variable-length request
context. So when you want to nest requests, you need to make the
subrequest the last member and declare your request context size as:
size of your request context struct + size of the sub-algorithm's
request context.

It is a bit confusing, but it is the only reasonable way to support
variably sized context and at the same time keep the whole request in
a single allocation.

>
> This chunk is enough to fix it for me:
>
> --- a/crypto/xts.c
> +++ b/crypto/xts.c
> @@ -33,8 +33,8 @@ struct xts_instance_ctx {
>
> struct rctx {
> le128 t, tcur;
> - struct skcipher_request subreq;
> int rem_bytes, is_encrypt;
> + struct skcipher_request subreq;
> };
>
> While at it, shouldn't be is_encrypt bool?
>
> Thanks,
> Milan

2019-08-07 21:33:46

by Pascal Van Leeuwen

[permalink] [raw]
Subject: RE: [PATCH] crypto: xts - Add support for Cipher Text Stealing

> -----Original Message-----
> From: Ondrej Mosnáček <[email protected]>
> Sent: Wednesday, August 7, 2019 10:33 PM
> To: Milan Broz <[email protected]>
> Cc: Pascal Van Leeuwen <[email protected]>; Pascal van Leeuwen
> <[email protected]>; [email protected]; [email protected];
> [email protected]; [email protected]
> Subject: Re: [PATCH] crypto: xts - Add support for Cipher Text Stealing
>
> st 7. 8. 2019 o 19:44 Milan Broz <[email protected]> napísal(a):
> > On 07/08/2019 17:13, Pascal Van Leeuwen wrote:
> > >>>> Seems there is no mistake in your code, it is some bug in aesni_intel implementation.
> > >>>> If I disable this module, it works as expected (with aes generic and aes_i586).
> > >>>>
> > >>> That's odd though, considering there is a dedicated xts-aes-ni implementation,
> > >>> i.e. I would not expect that to end up at the generic xts wrapper at all?
> > >>
> > >> Note it is 32bit system, AESNI XTS is under #ifdef CONFIG_X86_64 so it is not used.
> > >>
> > > Ok, so I guess no one bothered to make an optimized XTS version for i386.
> > > I quickly browsed through the code - took me a while to realise the assembly is
> > > "backwards" compared to the original Intel definition :-) - but I did not spot
> > > anything obvious :-(
> > >
> > >> I guess it only ECB part ...
> >
> > Mystery solved, the skcipher subreq must be te last member in the struct.
> > (Some comments in Adiantum code mentions it too, so I do not think it
> > just hides the corruption after the struct. Seems like another magic requirement
> > in crypto API :-)
>
> Oh, yes, this makes sense! I would have noticed this immediately if I
> had looked carefully at the struct definition :) The reason is that
> the skcipher_request struct is followed by a variable-length request
> context. So when you want to nest requests, you need to make the
> subrequest the last member and declare your request context size as:
> size of your request context struct + size of the sub-algorithm's
> request context.
>
> It is a bit confusing, but it is the only reasonable way to support
> variably sized context and at the same time keep the whole request in
> a single allocation.
>
Ah, ok, I did not know anything about that ... so there's really no way
I could've done this correctly or to have found the problem myself really.
Good that it's resolved now, though.

I fixed a couple of other minor things already, is it OK if I roll this
into an update to my original patch?

Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
http://www.insidesecure.com

2019-08-08 04:37:21

by Milan Broz

[permalink] [raw]
Subject: Re: [PATCH] crypto: xts - Add support for Cipher Text Stealing

Hi,

On 07/08/2019 23:13, Pascal Van Leeuwen wrote:
>> It is a bit confusing, but it is the only reasonable way to support
>> variably sized context and at the same time keep the whole request in
>> a single allocation.

Yes, and the reason here it was detected only for aesni_intel is that
it is submitted though more layers, these depends on variable context length
(at least it run through crypt_simd layer).

I think all other implementations on this 32bit machine were called directly,
so no corruption seen there.

> Ah, ok, I did not know anything about that ... so there's really no way
> I could've done this correctly or to have found the problem myself really.
> Good that it's resolved now, though.
>
> I fixed a couple of other minor things already, is it OK if I roll this
> into an update to my original patch?

Sure, feel free to fold in my change in v2 patch.
I'll test it and provide Tested-by later.

Maybe it would be good to also include /* must be the last */ comment
to the req in struct, though.

Also, maybe this req should have CRYPTO_MINALIGN_ATTR attribute also?

I expect req can be run through exotic hw accelerators later with some
memory alignment requirements. Ondra will know better here, though ;-)

Thanks!
Milan