[PATCH] [crypto] XTS: use proper alignment.

[PATCH] [crypto] XTS: use proper alignment.

[Sebastian Siewior]

The XTS blockmode uses a copy of the IV which is saved on the stack
and may or may not be properly aligned. If it is not, it will break
hardware cipher like the geode or padlock.
This patch moves the copy of IV to the private structre which has the
same aligment as the underlying cipher.

Tested-by: Stefan Hellermann <stefan@the2masters.de>
Signed-off-by: Sebastian Siewior <sebastian@breakpoint.cc>
---
 crypto/xts.c |   32 +++++++++++++++++---------------
 1 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/crypto/xts.c b/crypto/xts.c
index 8eb08bf..4457022 100644
--- a/crypto/xts.c
+++ b/crypto/xts.c
@@ -24,7 +24,17 @@
 #include <crypto/b128ops.h>
 #include <crypto/gf128mul.h>
 
+struct sinfo {
+	be128 t;
+	struct crypto_tfm *tfm;
+	void (*fn)(struct crypto_tfm *, u8 *, const u8 *);
+};
+
 struct priv {
+	/* s.t being the first member in this struct enforces proper alignment
+	 * required by the underlying cipher without explicit knowing the it.
+	 */
+	struct sinfo s;
 	struct crypto_cipher *child;
 	struct crypto_cipher *tweak;
 };
@@ -76,12 +86,6 @@ static int setkey(struct crypto_tfm *parent, const u8 *key,
 	return 0;
 }
 
-struct sinfo {
-	be128 t;
-	struct crypto_tfm *tfm;
-	void (*fn)(struct crypto_tfm *, u8 *, const u8 *);
-};
-
 static inline void xts_round(struct sinfo *s, void *dst, const void *src)
 {
 	be128_xor(dst, &s->t, src);		/* PP <- T xor P */
@@ -97,13 +101,12 @@ static int crypt(struct blkcipher_desc *d,
 	int err;
 	unsigned int avail;
 	const int bs = crypto_cipher_blocksize(ctx->child);
-	struct sinfo s = {
-		.tfm = crypto_cipher_tfm(ctx->child),
-		.fn = fn
-	};
-	be128 *iv;
 	u8 *wsrc;
 	u8 *wdst;
+	struct sinfo *s = &ctx->s;
+
+	s->tfm = crypto_cipher_tfm(ctx->child);
+	s->fn = fn;
 
 	err = blkcipher_walk_virt(d, w);
 	if (!w->nbytes)
@@ -115,17 +118,16 @@ static int crypt(struct blkcipher_desc *d,
 	wdst = w->dst.virt.addr;
 
 	/* calculate first value of T */
-	iv = (be128 *)w->iv;
-	tw(crypto_cipher_tfm(ctx->tweak), (void *)&s.t, w->iv);
+	tw(crypto_cipher_tfm(ctx->tweak), (void *)&s->t, w->iv);
 
 	goto first;
 
 	for (;;) {
 		do {
-			gf128mul_x_ble(&s.t, &s.t);
+			gf128mul_x_ble(&s->t, &s->t);
 
 first:
-			xts_round(&s, wdst, wsrc);
+			xts_round(s, wdst, wsrc);
 
 			wsrc += bs;
 			wdst += bs;
-- 
1.5.3.4

[PATCH] [crypto] XTS: use proper alignment.

[Sebastian Siewior]

The XTS blockmode uses a copy of the IV which is saved on the stack
and may or may not be properly aligned. If it is not, it will break
hardware cipher like the geode or padlock.
This patch moves the copy of IV to the private structre which has the
same aligment as the underlying cipher.

Signed-off-by: Sebastian Siewior <sebastian@breakpoint.cc>
---
Stefan, please try the following patch, it should fix your xts problem.

 crypto/xts.c |   32 +++++++++++++++++---------------
 1 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/crypto/xts.c b/crypto/xts.c
index 8eb08bf..4457022 100644
--- a/crypto/xts.c
+++ b/crypto/xts.c
@@ -24,7 +24,17 @@
 #include <crypto/b128ops.h>
 #include <crypto/gf128mul.h>
 
+struct sinfo {
+	be128 t;
+	struct crypto_tfm *tfm;
+	void (*fn)(struct crypto_tfm *, u8 *, const u8 *);
+};
+
 struct priv {
+	/* s.t being the first member in this struct enforces proper alignment
+	 * required by the underlying cipher without explicit knowing the it.
+	 */
+	struct sinfo s;
 	struct crypto_cipher *child;
 	struct crypto_cipher *tweak;
 };
@@ -76,12 +86,6 @@ static int setkey(struct crypto_tfm *parent, const u8 *key,
 	return 0;
 }
 
-struct sinfo {
-	be128 t;
-	struct crypto_tfm *tfm;
-	void (*fn)(struct crypto_tfm *, u8 *, const u8 *);
-};
-
 static inline void xts_round(struct sinfo *s, void *dst, const void *src)
 {
 	be128_xor(dst, &s->t, src);		/* PP <- T xor P */
@@ -97,13 +101,12 @@ static int crypt(struct blkcipher_desc *d,
 	int err;
 	unsigned int avail;
 	const int bs = crypto_cipher_blocksize(ctx->child);
-	struct sinfo s = {
-		.tfm = crypto_cipher_tfm(ctx->child),
-		.fn = fn
-	};
-	be128 *iv;
 	u8 *wsrc;
 	u8 *wdst;
+	struct sinfo *s = &ctx->s;
+
+	s->tfm = crypto_cipher_tfm(ctx->child);
+	s->fn = fn;
 
 	err = blkcipher_walk_virt(d, w);
 	if (!w->nbytes)
@@ -115,17 +118,16 @@ static int crypt(struct blkcipher_desc *d,
 	wdst = w->dst.virt.addr;
 
 	/* calculate first value of T */
-	iv = (be128 *)w->iv;
-	tw(crypto_cipher_tfm(ctx->tweak), (void *)&s.t, w->iv);
+	tw(crypto_cipher_tfm(ctx->tweak), (void *)&s->t, w->iv);
 
 	goto first;
 
 	for (;;) {
 		do {
-			gf128mul_x_ble(&s.t, &s.t);
+			gf128mul_x_ble(&s->t, &s->t);
 
 first:
-			xts_round(&s, wdst, wsrc);
+			xts_round(s, wdst, wsrc);
 
 			wsrc += bs;
 			wdst += bs;
-- 
1.5.3.4

Re: [PATCH] [crypto] XTS: use proper alignment.

[Stefan Hellermann]

Sebastian Siewior schrieb:
> The XTS blockmode uses a copy of the IV which is saved on the stack
> and may or may not be properly aligned. If it is not, it will break
> hardware cipher like the geode or padlock.
> This patch moves the copy of IV to the private structre which has the
> same aligment as the underlying cipher.
> 
> Signed-off-by: Sebastian Siewior <sebastian@breakpoint.cc>

It works now! Thanks!
But I get much lower speed than with aes-cbc-essiv:sha256.
With xts I get 57MB/s while reading the cryptodev with dd, and >90% sys in top, 0% wait
With cbc-essiv I get about 75MB/s while reading it with dd, 60% sys int top, 30% wait
without cryptodev I get 75MB/s while reading the raw lvm-volume with dd, 40% sys, 50% wait
I do a blockdev --flushbufs beetween each read.


Tested-by: Stefan Hellermann <stefan@the2masters.de>

> ---
> Stefan, please try the following patch, it should fix your xts problem.
> 
>  crypto/xts.c |   32 +++++++++++++++++---------------
>  1 files changed, 17 insertions(+), 15 deletions(-)
> 
> diff --git a/crypto/xts.c b/crypto/xts.c
> index 8eb08bf..4457022 100644
> --- a/crypto/xts.c
> +++ b/crypto/xts.c
> @@ -24,7 +24,17 @@
>  #include <crypto/b128ops.h>
>  #include <crypto/gf128mul.h>
>  
> +struct sinfo {
> +	be128 t;
> +	struct crypto_tfm *tfm;
> +	void (*fn)(struct crypto_tfm *, u8 *, const u8 *);
> +};
> +
>  struct priv {
> +	/* s.t being the first member in this struct enforces proper alignment
> +	 * required by the underlying cipher without explicit knowing the it.
> +	 */
> +	struct sinfo s;
>  	struct crypto_cipher *child;
>  	struct crypto_cipher *tweak;
>  };
> @@ -76,12 +86,6 @@ static int setkey(struct crypto_tfm *parent, const u8 *key,
>  	return 0;
>  }
>  
> -struct sinfo {
> -	be128 t;
> -	struct crypto_tfm *tfm;
> -	void (*fn)(struct crypto_tfm *, u8 *, const u8 *);
> -};
> -
>  static inline void xts_round(struct sinfo *s, void *dst, const void *src)
>  {
>  	be128_xor(dst, &s->t, src);		/* PP <- T xor P */
> @@ -97,13 +101,12 @@ static int crypt(struct blkcipher_desc *d,
>  	int err;
>  	unsigned int avail;
>  	const int bs = crypto_cipher_blocksize(ctx->child);
> -	struct sinfo s = {
> -		.tfm = crypto_cipher_tfm(ctx->child),
> -		.fn = fn
> -	};
> -	be128 *iv;
>  	u8 *wsrc;
>  	u8 *wdst;
> +	struct sinfo *s = &ctx->s;
> +
> +	s->tfm = crypto_cipher_tfm(ctx->child);
> +	s->fn = fn;
>  
>  	err = blkcipher_walk_virt(d, w);
>  	if (!w->nbytes)
> @@ -115,17 +118,16 @@ static int crypt(struct blkcipher_desc *d,
>  	wdst = w->dst.virt.addr;
>  
>  	/* calculate first value of T */
> -	iv = (be128 *)w->iv;
> -	tw(crypto_cipher_tfm(ctx->tweak), (void *)&s.t, w->iv);
> +	tw(crypto_cipher_tfm(ctx->tweak), (void *)&s->t, w->iv);
>  
>  	goto first;
>  
>  	for (;;) {
>  		do {
> -			gf128mul_x_ble(&s.t, &s.t);
> +			gf128mul_x_ble(&s->t, &s->t);
>  
>  first:
> -			xts_round(&s, wdst, wsrc);
> +			xts_round(s, wdst, wsrc);
>  
>  			wsrc += bs;
>  			wdst += bs;

Re: [PATCH] [crypto] XTS: use proper alignment.

[Sebastian Siewior]

* Stefan Hellermann | 2008-03-02 13:04:37 [+0100]:

>But I get much lower speed than with aes-cbc-essiv:sha256.
Yes, I expected this :)
The aes-cbc operation is supported directly in HW. So the
driver just says here is the key, source, destination, length and now do
it. So the HW fetches the key once and is going to process the whole
request (lets say 4 KiB) in one go.

The XTS blockmode on the other hand encrypts encrypts only 16 bytes in
one go and performs some GF operations in between. This is
repeated until we encrypt the whole request. So for a 4 KiB we need here
257 calls to the HW instead of one (the one extra is to encrypt the IV).
For every encryption call we have to reset the HW key. According to the
spec fetching the key from memory takes more time than the whole
encryption process as it (in case of a 16 byte block). This might still
be faster than the pure software solution.
Anyway, XTS is way more complex than CBC and part of it is done in
software what we can't change.

>With xts I get 57MB/s while reading the cryptodev with dd, and >90% sys in top, 0% wait
>With cbc-essiv I get about 75MB/s while reading it with dd, 60% sys int top, 30% wait
>without cryptodev I get 75MB/s while reading the raw lvm-volume with dd, 40% sys, 50% wait
>I do a blockdev --flushbufs beetween each read.
According to this numbers I would say in CBC mode the HD is breaking in
XTS the CPU is.
I could try to tune it a little but don't expect much.
Could you please compare xts with and without padlock?

Sebastian

[PATCH] [PATCH] [crypto] LRW: use proper alignment.

[Sebastian Siewior]

The LRW blockmode uses a copy of the IV which is saved on the stack
and may or may not be properly aligned. If it is not, it will break
hardware cipher like the geode or padlock.
This patch moves the copy of IV to the private structre which has the
same aligment as the underlying cipher.
Signed-off-by: Sebastian Siewior <sebastian@breakpoint.cc>
---
 crypto/lrw.c |   32 ++++++++++++++++++--------------
 1 files changed, 18 insertions(+), 14 deletions(-)

diff --git a/crypto/lrw.c b/crypto/lrw.c
index 9d52e58..0c3ce3e 100644
--- a/crypto/lrw.c
+++ b/crypto/lrw.c
@@ -27,7 +27,17 @@
 #include <crypto/b128ops.h>
 #include <crypto/gf128mul.h>
 
+struct sinfo {
+	be128 t;
+	struct crypto_tfm *tfm;
+	void (*fn)(struct crypto_tfm *, u8 *, const u8 *);
+};
+
 struct priv {
+	/* s.t being the first member in this struct enforces proper alignment
+	 * required by the underlying cipher without explicit knowing the it.
+	 */
+	struct sinfo s;
 	struct crypto_cipher *child;
 	/* optimizes multiplying a random (non incrementing, as at the
 	 * start of a new sector) value with key2, we could also have
@@ -83,12 +93,6 @@ static int setkey(struct crypto_tfm *parent, const u8 *key,
 	return 0;
 }
 
-struct sinfo {
-	be128 t;
-	struct crypto_tfm *tfm;
-	void (*fn)(struct crypto_tfm *, u8 *, const u8 *);
-};
-
 static inline void inc(be128 *iv)
 {
 	if (!(iv->b = cpu_to_be64(be64_to_cpu(iv->b) + 1)))
@@ -128,14 +132,14 @@ static int crypt(struct blkcipher_desc *d,
 	int err;
 	unsigned int avail;
 	const int bs = crypto_cipher_blocksize(ctx->child);
-	struct sinfo s = {
-		.tfm = crypto_cipher_tfm(ctx->child),
-		.fn = fn
-	};
+	struct sinfo *s = &ctx->s;
 	be128 *iv;
 	u8 *wsrc;
 	u8 *wdst;
 
+	s->tfm = crypto_cipher_tfm(ctx->child);
+	s->fn = fn;
+
 	err = blkcipher_walk_virt(d, w);
 	if (!(avail = w->nbytes))
 		return err;
@@ -145,10 +149,10 @@ static int crypt(struct blkcipher_desc *d,
 
 	/* calculate first value of T */
 	iv = (be128 *)w->iv;
-	s.t = *iv;
+	s->t = *iv;
 
 	/* T <- I*Key2 */
-	gf128mul_64k_bbe(&s.t, ctx->table);
+	gf128mul_64k_bbe(&s->t, ctx->table);
 
 	goto first;
 
@@ -156,11 +160,11 @@ static int crypt(struct blkcipher_desc *d,
 		do {
 			/* T <- I*Key2, using the optimization
 			 * discussed in the specification */
-			be128_xor(&s.t, &s.t, &ctx->mulinc[get_index128(iv)]);
+			be128_xor(&s->t, &s->t, &ctx->mulinc[get_index128(iv)]);
 			inc(iv);
 
 first:
-			lrw_round(&s, wdst, wsrc);
+			lrw_round(s, wdst, wsrc);
 
 			wsrc += bs;
 			wdst += bs;
-- 
1.5.3.4

Re: [PATCH] [crypto] XTS: use proper alignment.

[Stefan Hellermann]

Sebastian Siewior schrieb:
> * Stefan Hellermann | 2008-03-02 13:04:37 [+0100]:
> 
>> But I get much lower speed than with aes-cbc-essiv:sha256.
> Yes, I expected this :)
> The aes-cbc operation is supported directly in HW. So the
> driver just says here is the key, source, destination, length and now do
> it. So the HW fetches the key once and is going to process the whole
> request (lets say 4 KiB) in one go.
> 
> The XTS blockmode on the other hand encrypts encrypts only 16 bytes in
> one go and performs some GF operations in between. This is
> repeated until we encrypt the whole request. So for a 4 KiB we need here
> 257 calls to the HW instead of one (the one extra is to encrypt the IV).
> For every encryption call we have to reset the HW key. According to the
> spec fetching the key from memory takes more time than the whole
> encryption process as it (in case of a 16 byte block). This might still
> be faster than the pure software solution.
> Anyway, XTS is way more complex than CBC and part of it is done in
> software what we can't change.

Ah, good to know! Could this information be placed in the Kconfig-help for padlock_aes?

> 
>> With xts I get 57MB/s while reading the cryptodev with dd, and >90% sys in top, 0% wait
>> With cbc-essiv I get about 75MB/s while reading it with dd, 60% sys int top, 30% wait
>> without cryptodev I get 75MB/s while reading the raw lvm-volume with dd, 40% sys, 50% wait
>> I do a blockdev --flushbufs beetween each read.
> According to this numbers I would say in CBC mode the HD is breaking in
> XTS the CPU is.
> I could try to tune it a little but don't expect much.
> Could you please compare xts with and without padlock?

Yes, xts with padlock is almost 3 times faster.
20-21MB/s read in dd without padlock_aes, >90% CPU sys, 0% wait
57-58MB/s read in dd with padlock_aes, >90% CPU sys, 0% wait

I tried lrw-benbi/lrw-plain this time, but it doesn't work, with or without padlock_aes.
dmesg logs:
device-mapper: table: 252:6: crypt: Error allocating crypto tfm
device-mapper: ioctl: error adding target to table
device-mapper: ioctl: device doesn't appear to be in the dev hash table

... but I will use cbc-essiv, if I ever need better encryption I can take xts-plain. (no
need for lrw-benbi)

Thanks
Stefan

[PATCH] fix alignment problem in XTS and LRW blockmode

[Sebastian Siewior]

Hello Herbert,

this fixes the bug reported by Stefan Hellermann which breaks his
padlock-aes.

Sebastian

Re: [PATCH] [PATCH] [crypto] LRW: use proper alignment.

[Stefan Hellermann]

Sebastian Siewior schrieb:
> The LRW blockmode uses a copy of the IV which is saved on the stack
> and may or may not be properly aligned. If it is not, it will break
> hardware cipher like the geode or padlock.
> This patch moves the copy of IV to the private structre which has the
> same aligment as the underlying cipher.
> Signed-off-by: Sebastian Siewior <sebastian@breakpoint.cc>

Tested-by: Stefan Hellermann <stefan@the2masters.de>

> ---
>  crypto/lrw.c |   32 ++++++++++++++++++--------------
>  1 files changed, 18 insertions(+), 14 deletions(-)
> 
> diff --git a/crypto/lrw.c b/crypto/lrw.c
> index 9d52e58..0c3ce3e 100644
> --- a/crypto/lrw.c
> +++ b/crypto/lrw.c
> @@ -27,7 +27,17 @@
>  #include <crypto/b128ops.h>
>  #include <crypto/gf128mul.h>
>  
> +struct sinfo {
> +	be128 t;
> +	struct crypto_tfm *tfm;
> +	void (*fn)(struct crypto_tfm *, u8 *, const u8 *);
> +};
> +
>  struct priv {
> +	/* s.t being the first member in this struct enforces proper alignment
> +	 * required by the underlying cipher without explicit knowing the it.
> +	 */
> +	struct sinfo s;
>  	struct crypto_cipher *child;
>  	/* optimizes multiplying a random (non incrementing, as at the
>  	 * start of a new sector) value with key2, we could also have
> @@ -83,12 +93,6 @@ static int setkey(struct crypto_tfm *parent, const u8 *key,
>  	return 0;
>  }
>  
> -struct sinfo {
> -	be128 t;
> -	struct crypto_tfm *tfm;
> -	void (*fn)(struct crypto_tfm *, u8 *, const u8 *);
> -};
> -
>  static inline void inc(be128 *iv)
>  {
>  	if (!(iv->b = cpu_to_be64(be64_to_cpu(iv->b) + 1)))
> @@ -128,14 +132,14 @@ static int crypt(struct blkcipher_desc *d,
>  	int err;
>  	unsigned int avail;
>  	const int bs = crypto_cipher_blocksize(ctx->child);
> -	struct sinfo s = {
> -		.tfm = crypto_cipher_tfm(ctx->child),
> -		.fn = fn
> -	};
> +	struct sinfo *s = &ctx->s;
>  	be128 *iv;
>  	u8 *wsrc;
>  	u8 *wdst;
>  
> +	s->tfm = crypto_cipher_tfm(ctx->child);
> +	s->fn = fn;
> +
>  	err = blkcipher_walk_virt(d, w);
>  	if (!(avail = w->nbytes))
>  		return err;
> @@ -145,10 +149,10 @@ static int crypt(struct blkcipher_desc *d,
>  
>  	/* calculate first value of T */
>  	iv = (be128 *)w->iv;
> -	s.t = *iv;
> +	s->t = *iv;
>  
>  	/* T <- I*Key2 */
> -	gf128mul_64k_bbe(&s.t, ctx->table);
> +	gf128mul_64k_bbe(&s->t, ctx->table);
>  
>  	goto first;
>  
> @@ -156,11 +160,11 @@ static int crypt(struct blkcipher_desc *d,
>  		do {
>  			/* T <- I*Key2, using the optimization
>  			 * discussed in the specification */
> -			be128_xor(&s.t, &s.t, &ctx->mulinc[get_index128(iv)]);
> +			be128_xor(&s->t, &s->t, &ctx->mulinc[get_index128(iv)]);
>  			inc(iv);
>  
>  first:
> -			lrw_round(&s, wdst, wsrc);
> +			lrw_round(s, wdst, wsrc);
>  
>  			wsrc += bs;
>  			wdst += bs;

Re: [PATCH] [crypto] XTS: use proper alignment.

[Stefan Hellermann]

> 
> I tried lrw-benbi/lrw-plain this time, but it doesn't work, with or without padlock_aes.
> dmesg logs:
> device-mapper: table: 252:6: crypt: Error allocating crypto tfm
> device-mapper: ioctl: error adding target to table
> device-mapper: ioctl: device doesn't appear to be in the dev hash table
> 

forget this ... with CONFIG_CRYPTO_LRW unset it can't work. But with your new patch it
works even with padlock_aes.


Thanks
Stefan

Re: [PATCH] [PATCH] [crypto] LRW: use proper alignment.

[Herbert Xu]

On Sun, Mar 02, 2008 at 03:01:34PM +0100, Stefan Hellermann wrote:
> Sebastian Siewior schrieb:
> > The LRW blockmode uses a copy of the IV which is saved on the stack
> > and may or may not be properly aligned. If it is not, it will break
> > hardware cipher like the geode or padlock.
> > This patch moves the copy of IV to the private structre which has the
> > same aligment as the underlying cipher.
> > Signed-off-by: Sebastian Siewior <sebastian@breakpoint.cc>
> 
> Tested-by: Stefan Hellermann <stefan@the2masters.de>

Thanks Sebastian and Stefan!  I'll apply this for 2.6.25 and push
it to stable as well.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] [crypto] XTS: use proper alignment.

[Herbert Xu]

On Sun, Mar 02, 2008 at 11:09:10AM +0000, Sebastian Siewior wrote:
> The XTS blockmode uses a copy of the IV which is saved on the stack
> and may or may not be properly aligned. If it is not, it will break
> hardware cipher like the geode or padlock.
> This patch moves the copy of IV to the private structre which has the
> same aligment as the underlying cipher.
> 
> Tested-by: Stefan Hellermann <stefan@the2masters.de>
> Signed-off-by: Sebastian Siewior <sebastian@breakpoint.cc>

Sorry but this patch isn't right.

> +struct sinfo {
> +	be128 t;
> +	struct crypto_tfm *tfm;
> +	void (*fn)(struct crypto_tfm *, u8 *, const u8 *);
> +};
> +
>  struct priv {
> +	/* s.t being the first member in this struct enforces proper alignment
> +	 * required by the underlying cipher without explicit knowing the it.
> +	 */
> +	struct sinfo s;

tfm objects should be reentrant so you can't store any per-op
info in the context structure.

> -	tw(crypto_cipher_tfm(ctx->tweak), (void *)&s.t, w->iv);
> +	tw(crypto_cipher_tfm(ctx->tweak), (void *)&s->t, w->iv);

However, the real question is why do we need this at all? The
tw argument should be using the proper entry points that do
copying for you if necessary.

OK, I see that the issue is that we're using cia_encrypt instead
of cit_encrypt_one.  So if we just change that then it should work
correctly.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] [PATCH] [crypto] LRW: use proper alignment.

[Herbert Xu]

On Sun, Mar 02, 2008 at 01:35:17PM +0000, Sebastian Siewior wrote:
> The LRW blockmode uses a copy of the IV which is saved on the stack
> and may or may not be properly aligned. If it is not, it will break
> hardware cipher like the geode or padlock.
> This patch moves the copy of IV to the private structre which has the
> same aligment as the underlying cipher.
> Signed-off-by: Sebastian Siewior <sebastian@breakpoint.cc>

The same comment applies to this patch.  Please change it to use
cit_* instead of cia_*.

Thanks,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] [crypto] XTS: use proper alignment.

[Sebastian Siewior]

* Herbert Xu | 2008-03-05 19:16:02 [+0800]:

>> +struct sinfo {
>> +	be128 t;
>> +	struct crypto_tfm *tfm;
>> +	void (*fn)(struct crypto_tfm *, u8 *, const u8 *);
>> +};
>> +
>>  struct priv {
>> +	/* s.t being the first member in this struct enforces proper alignment
>> +	 * required by the underlying cipher without explicit knowing the it.
>> +	 */
>> +	struct sinfo s;
>
>tfm objects should be reentrant so you can't store any per-op
>info in the context structure.
I could also do kmalloc(), align() and kfree() after encrypt but this
was faster.

>
>> -	tw(crypto_cipher_tfm(ctx->tweak), (void *)&s.t, w->iv);
>> +	tw(crypto_cipher_tfm(ctx->tweak), (void *)&s->t, w->iv);
>
>However, the real question is why do we need this at all? The
>tw argument should be using the proper entry points that do
>copying for you if necessary.
>
>OK, I see that the issue is that we're using cia_encrypt instead
>of cit_encrypt_one.  So if we just change that then it should work
>correctly.
I'm not sure if we are allowed to modify the IV or if it should
remain untouched. If it is possible to modify it, I could encrypt it
inplace and save two memcpy().
I will check this tonight.

>
>Cheers,
Sebastian

Re: [PATCH] [crypto] XTS: use proper alignment.

[Herbert Xu]

On Wed, Mar 05, 2008 at 12:46:52PM +0100, Sebastian Siewior wrote:
>
> I'm not sure if we are allowed to modify the IV or if it should
> remain untouched. If it is possible to modify it, I could encrypt it
> inplace and save two memcpy().
> I will check this tonight.

I just had a quick look and it seems that you should be able to
store the result in the IV.

However this won't work for LRW since we need the IV to increment
it.  But then again LRW seems to be fine as it is since its
arguments are already aligned by the blkcipher walker.

Thanks,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] [crypto] XTS: use proper alignment.

[Sebastian Siewior]

* Herbert Xu | 2008-03-05 19:52:03 [+0800]:

>On Wed, Mar 05, 2008 at 12:46:52PM +0100, Sebastian Siewior wrote:
>>
>> I'm not sure if we are allowed to modify the IV or if it should
>> remain untouched. If it is possible to modify it, I could encrypt it
>> inplace and save two memcpy().
>> I will check this tonight.
>
>I just had a quick look and it seems that you should be able to
>store the result in the IV.
Okey,

>However this won't work for LRW since we need the IV to increment
>it.  But then again LRW seems to be fine as it is since its
>arguments are already aligned by the blkcipher walker.
I just browsed LRW and it seems that it does not encrypt the IV at all.

Stefan: Didn't you report that both, XTS and LRW are broken on your
padlock? If so, could you please post the backtrace?

>
>Thanks,

Sebastian

Re: [PATCH] [crypto] XTS: use proper alignment.

[Stefan Hellermann]

Sebastian Siewior schrieb:
> * Herbert Xu | 2008-03-05 19:52:03 [+0800]:
> 
>> On Wed, Mar 05, 2008 at 12:46:52PM +0100, Sebastian Siewior wrote:
>>> I'm not sure if we are allowed to modify the IV or if it should
>>> remain untouched. If it is possible to modify it, I could encrypt it
>>> inplace and save two memcpy().
>>> I will check this tonight.
>> I just had a quick look and it seems that you should be able to
>> store the result in the IV.
> Okey,
> 
>> However this won't work for LRW since we need the IV to increment
>> it.  But then again LRW seems to be fine as it is since its
>> arguments are already aligned by the blkcipher walker.
> I just browsed LRW and it seems that it does not encrypt the IV at all.
> 
> Stefan: Didn't you report that both, XTS and LRW are broken on your
> padlock? If so, could you please post the backtrace?

I think it crashed one time, but I haven't really tried using LRW since XTS is said to
provide better security. Now I'm not able to reproduce the crash, it works with vanilla
2.6.25-rc4.

I have other problems in 2.6.25-rc[1-3], I get segfaults every here and then. I tried
compiling gcc several time, 90% of the time it crashed somewhere. I have the feeling it
segfaults faster when I do the compile in a tmpfs-mounted directory. 2.6.24 works fine, I
haven't tested 2.6.25-rc4. I have to check my RAM, if it's good I will report this to LKML.
I think the LRW-crash I reported could be related to this.

Thanks
Stefan

PS: I'm away from Thursday 12:00 UTC till Tuesday.


> 
>> Thanks,
> 
> Sebastian

Re: [PATCH] [crypto] XTS: use proper alignment.

[Sebastian Siewior]

* Stefan Hellermann | 2008-03-05 15:02:29 [+0100]:

>I think it crashed one time, but I haven't really tried using LRW since XTS is said to
>provide better security. Now I'm not able to reproduce the crash, it works with vanilla
>2.6.25-rc4.
Okey, I drop the LRW patch and provide only a XTS fix.

Sebastian

[PATCH] [crypto] XTS: use proper alignment v2

[Sebastian Siewior]

The XTS blockmode uses a copy of the IV which is saved on the stack
and may or may not be properly aligned. If it is not, it will break
hardware cipher like the geode or padlock.
This patch encrypts the IV in place so we don't have to worry about
alignment.

Signed-off-by: Sebastian Siewior <sebastian@breakpoint.cc>
---
Herbert, I tried the small patch thing :)
It passed tcrypt on my geode, dunno about dm-crypt & friends.
Stefan if you could test it with dm-crypt than we have a small fix :)

 crypto/xts.c |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/crypto/xts.c b/crypto/xts.c
index 8eb08bf..d87b0f3 100644
--- a/crypto/xts.c
+++ b/crypto/xts.c
@@ -77,16 +77,16 @@ static int setkey(struct crypto_tfm *parent, const u8 *key,
 }
 
 struct sinfo {
-	be128 t;
+	be128 *t;
 	struct crypto_tfm *tfm;
 	void (*fn)(struct crypto_tfm *, u8 *, const u8 *);
 };
 
 static inline void xts_round(struct sinfo *s, void *dst, const void *src)
 {
-	be128_xor(dst, &s->t, src);		/* PP <- T xor P */
+	be128_xor(dst, s->t, src);		/* PP <- T xor P */
 	s->fn(s->tfm, dst, dst);		/* CC <- E(Key1,PP) */
-	be128_xor(dst, dst, &s->t);		/* C <- T xor CC */
+	be128_xor(dst, dst, s->t);		/* C <- T xor CC */
 }
 
 static int crypt(struct blkcipher_desc *d,
@@ -101,7 +101,6 @@ static int crypt(struct blkcipher_desc *d,
 		.tfm = crypto_cipher_tfm(ctx->child),
 		.fn = fn
 	};
-	be128 *iv;
 	u8 *wsrc;
 	u8 *wdst;
 
@@ -109,20 +108,20 @@ static int crypt(struct blkcipher_desc *d,
 	if (!w->nbytes)
 		return err;
 
+	s.t = (be128 *)w->iv;
 	avail = w->nbytes;
 
 	wsrc = w->src.virt.addr;
 	wdst = w->dst.virt.addr;
 
 	/* calculate first value of T */
-	iv = (be128 *)w->iv;
-	tw(crypto_cipher_tfm(ctx->tweak), (void *)&s.t, w->iv);
+	tw(crypto_cipher_tfm(ctx->tweak), w->iv, w->iv);
 
 	goto first;
 
 	for (;;) {
 		do {
-			gf128mul_x_ble(&s.t, &s.t);
+			gf128mul_x_ble(s.t, s.t);
 
 first:
 			xts_round(&s, wdst, wsrc);
-- 
1.5.3.4

Re: [PATCH] [crypto] XTS: use proper alignment v2

[Stefan Hellermann]

> The XTS blockmode uses a copy of the IV which is saved on the stack
> and may or may not be properly aligned. If it is not, it will break
> hardware cipher like the geode or padlock.
> This patch encrypts the IV in place so we don't have to worry about
> alignment.
> 
> Signed-off-by: Sebastian Siewior <sebastian@breakpoint.cc>
> ---
> Herbert, I tried the small patch thing :)
> It passed tcrypt on my geode, dunno about dm-crypt & friends.
> Stefan if you could test it with dm-crypt than we have a small fix :)

Yes, this passwd my tests, too! Nice :)

Tested-by: Stefan Hellermann <stefan@the2masters.de


PS: The segfaults I got with 2.6.25-rc[1-3] are gone ... LRW is stable here.

>  crypto/xts.c |   13 ++++++-------
>  1 files changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/crypto/xts.c b/crypto/xts.c
> index 8eb08bf..d87b0f3 100644
> --- a/crypto/xts.c
> +++ b/crypto/xts.c
> @@ -77,16 +77,16 @@ static int setkey(struct crypto_tfm *parent, const u8 *key,
>  }
>  
>  struct sinfo {
> -	be128 t;
> +	be128 *t;
>  	struct crypto_tfm *tfm;
>  	void (*fn)(struct crypto_tfm *, u8 *, const u8 *);
>  };
>  
>  static inline void xts_round(struct sinfo *s, void *dst, const void *src)
>  {
> -	be128_xor(dst, &s->t, src);		/* PP <- T xor P */
> +	be128_xor(dst, s->t, src);		/* PP <- T xor P */
>  	s->fn(s->tfm, dst, dst);		/* CC <- E(Key1,PP) */
> -	be128_xor(dst, dst, &s->t);		/* C <- T xor CC */
> +	be128_xor(dst, dst, s->t);		/* C <- T xor CC */
>  }
>  
>  static int crypt(struct blkcipher_desc *d,
> @@ -101,7 +101,6 @@ static int crypt(struct blkcipher_desc *d,
>  		.tfm = crypto_cipher_tfm(ctx->child),
>  		.fn = fn
>  	};
> -	be128 *iv;
>  	u8 *wsrc;
>  	u8 *wdst;
>  
> @@ -109,20 +108,20 @@ static int crypt(struct blkcipher_desc *d,
>  	if (!w->nbytes)
>  		return err;
>  
> +	s.t = (be128 *)w->iv;
>  	avail = w->nbytes;
>  
>  	wsrc = w->src.virt.addr;
>  	wdst = w->dst.virt.addr;
>  
>  	/* calculate first value of T */
> -	iv = (be128 *)w->iv;
> -	tw(crypto_cipher_tfm(ctx->tweak), (void *)&s.t, w->iv);
> +	tw(crypto_cipher_tfm(ctx->tweak), w->iv, w->iv);
>  
>  	goto first;
>  
>  	for (;;) {
>  		do {
> -			gf128mul_x_ble(&s.t, &s.t);
> +			gf128mul_x_ble(s.t, s.t);
>  
>  first:
>  			xts_round(&s, wdst, wsrc);

Re: [PATCH] [crypto] XTS: use proper alignment v2

[Sebastian Siewior]

* Stefan Hellermann | 2008-03-05 23:48:01 [+0100]:

>PS: The segfaults I got with 2.6.25-rc[1-3] are gone ... LRW is stable here.
Good to hear. Now that everything seems to run, do you thing that you
could test my key unification patches when you have some spare time?

Sebastian

Re: [PATCH] [crypto] XTS: use proper alignment v2

[Stefan Hellermann]

Sebastian Siewior schrieb:
> * Stefan Hellermann | 2008-03-05 23:48:01 [+0100]:
> 
>> PS: The segfaults I got with 2.6.25-rc[1-3] are gone ... LRW is stable here.
> Good to hear. Now that everything seems to run, do you thing that you
> could test my key unification patches when you have some spare time?

I'm away till Tuesday, but then I can test everything :)

Stefan

> 
> Sebastian

Re: [PATCH] [crypto] XTS: use proper alignment v2

[Herbert Xu]

On Wed, Mar 05, 2008 at 11:48:01PM +0100, Stefan Hellermann wrote:
> > The XTS blockmode uses a copy of the IV which is saved on the stack
> > and may or may not be properly aligned. If it is not, it will break
> > hardware cipher like the geode or padlock.
> > This patch encrypts the IV in place so we don't have to worry about
> > alignment.
> > 
> > Signed-off-by: Sebastian Siewior <sebastian@breakpoint.cc>
> > ---
> > Herbert, I tried the small patch thing :)
> > It passed tcrypt on my geode, dunno about dm-crypt & friends.
> > Stefan if you could test it with dm-crypt than we have a small fix :)
> 
> Yes, this passwd my tests, too! Nice :)
> 
> Tested-by: Stefan Hellermann <stefan@the2masters.de

Patch applied.  Thanks everyeone!

I'll push this to stable too.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt