Hello Herbert,
this fixes the bug reported by Stefan Hellermann which breaks his
padlock-aes.
Sebastian
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 <[email protected]>
Signed-off-by: Sebastian Siewior <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
Tested-by: Stefan Hellermann <[email protected]>
> ---
> 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;
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 <[email protected]>
>
> Tested-by: Stefan Hellermann <[email protected]>
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~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
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 <[email protected]>
> Signed-off-by: Sebastian Siewior <[email protected]>
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~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
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 <[email protected]>
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~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
* 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
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~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
* 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
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
* 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
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 <[email protected]>
---
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
> 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 <[email protected]>
> ---
> 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 <[email protected]
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);
* 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
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
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 <[email protected]>
> > ---
> > 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 <[email protected]
Patch applied. Thanks everyeone!
I'll push this to stable too.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt