Re: HMAC regression
On Thu, May 28, 2009 at 05:09:08PM +0200, Martin Willi wrote:
Switching the hash implementations to the new shash API introduced a
regression. HMACs are created incorrectly if the data is scattered over
multiple pages, resulting in very unreliable IPsec tunnels.
What are the symptoms?
+ .psize = 4100,
+ .digest =
\x4F\x3B\x6B\xD1\x1A\x2E\xD6\x12\x3D\x5A\xC8\x39\x91\xE3\xC3\x0E\xB6\x51\x85\xA5,
This test vector is wrong. We don't support vectors longer than
a page without scattering it. You must set np and tap. I tried
it using tap = { 4064, 36 } and it worked under cryptodev-2.6.
Here's a patch to detect this for future reference.
commit dfddf5dbe683cfdeb84bd218a1f819c09f5ea44a
Author: Herbert Xu herb...@gondor.apana.org.au
Date: Fri May 29 16:05:42 2009 +1000
crypto: testmgr - Check all test vector lengths
As we cannot guarantee the availability of contiguous pages at
run-time, all test vectors must either fit within a page, or use
scatter lists. In some cases vectors were not checked as to
whether they fit inside a page. This patch adds all the missing
checks.
Signed-off-by: Herbert Xu herb...@gondor.apana.org.au
diff --git a/crypto/testmgr.c b/crypto/testmgr.c
index 376ea88..8fcea70 100644
--- a/crypto/testmgr.c
+++ b/crypto/testmgr.c
@@ -185,6 +185,10 @@ static int test_hash(struct crypto_ahash *tfm, struct
hash_testvec *template,
hash_buff = xbuf[0];
+ ret = -EINVAL;
+ if (WARN_ON(template[i].psize PAGE_SIZE))
+ goto out;
+
memcpy(hash_buff, template[i].plaintext, template[i].psize);
sg_init_one(sg[0], hash_buff, template[i].psize);
@@ -238,7 +242,11 @@ static int test_hash(struct crypto_ahash *tfm, struct
hash_testvec *template,
temp = 0;
sg_init_table(sg, template[i].np);
+ ret = -EINVAL;
for (k = 0; k template[i].np; k++) {
+ if (WARN_ON(offset_in_page(IDX[k]) +
+ template[i].tap[k] PAGE_SIZE))
+ goto out;
sg_set_buf(sg[k],
memcpy(xbuf[IDX[k] PAGE_SHIFT] +
offset_in_page(IDX[k]),
@@ -357,6 +365,11 @@ static int test_aead(struct crypto_aead *tfm, int enc,
input = xbuf[0];
assoc = axbuf[0];
+ ret = -EINVAL;
+ if (WARN_ON(template[i].ilen PAGE_SIZE ||
+ template[i].alen PAGE_SIZE))
+ goto out;
+
memcpy(input, template[i].input, template[i].ilen);
memcpy(assoc, template[i].assoc, template[i].alen);
if (template[i].iv)
@@ -516,7 +529,11 @@ static int test_aead(struct crypto_aead *tfm, int enc,
}
sg_init_table(asg, template[i].anp);
+ ret = -EINVAL;
for (k = 0, temp = 0; k template[i].anp; k++) {
+ if (WARN_ON(offset_in_page(IDX[k]) +
+ template[i].atap[k] PAGE_SIZE))
+ goto out;
sg_set_buf(asg[k],
memcpy(axbuf[IDX[k] PAGE_SHIFT] +
offset_in_page(IDX[k]),
@@ -650,6 +667,10 @@ static int test_cipher(struct crypto_cipher *tfm, int enc,
j++;
+ ret = -EINVAL;
+ if (WARN_ON(template[i].ilen PAGE_SIZE))
+ goto out;
+
data = xbuf[0];
memcpy(data, template[i].input, template[i].ilen);
@@ -741,6 +762,10 @@ static int test_skcipher(struct crypto_ablkcipher *tfm,
int enc,
if (!(template[i].np)) {
j++;
+ ret = -EINVAL;
+ if (WARN_ON(template[i].ilen PAGE_SIZE))
+ goto out;
+
data = xbuf[0];
memcpy(data, template[i].input, template[i].ilen);
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmVHI~} herb...@gondor.apana.org.au
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: HMAC regression
On Fri, May 29, 2009 at 04:19:31PM +1000, Herbert Xu wrote:
Here's a patch to detect this for future reference.
commit dfddf5dbe683cfdeb84bd218a1f819c09f5ea44a
Author: Herbert Xu herb...@gondor.apana.org.au
Date: Fri May 29 16:05:42 2009 +1000
crypto: testmgr - Check all test vector lengths
I also needed this to actually test your vector.
BTW, please reformat your test vector so that it doesn't exceed
80 columns when you resubmit.
commit 35a20d2814525826700e25529cdbe361d685caf7
Author: Herbert Xu herb...@gondor.apana.org.au
Date: Fri May 29 16:23:12 2009 +1000
crypto: testmgr - Allow hash test vectors longer than a page
As it stands we will each test hash vector both linearly and as
a scatter list if applicable. This means that we cannot have
vectors longer than a page, even with scatter lists.
This patch fixes this by skipping test vectors with np != 0 when
testing linearly.
Signed-off-by: Herbert Xu herb...@gondor.apana.org.au
diff --git a/crypto/testmgr.c b/crypto/testmgr.c
index 8fcea70..e9e9d84 100644
--- a/crypto/testmgr.c
+++ b/crypto/testmgr.c
@@ -180,7 +180,12 @@ static int test_hash(struct crypto_ahash *tfm, struct
hash_testvec *template,
ahash_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG,
tcrypt_complete, tresult);
+ j = 0;
for (i = 0; i tcount; i++) {
+ if (template[i].np)
+ continue;
+
+ j++;
memset(result, 0, 64);
hash_buff = xbuf[0];
@@ -198,7 +203,7 @@ static int test_hash(struct crypto_ahash *tfm, struct
hash_testvec *template,
template[i].ksize);
if (ret) {
printk(KERN_ERR alg: hash: setkey failed on
- test %d for %s: ret=%d\n, i + 1, algo,
+ test %d for %s: ret=%d\n, j, algo,
-ret);
goto out;
}
@@ -220,14 +225,14 @@ static int test_hash(struct crypto_ahash *tfm, struct
hash_testvec *template,
/* fall through */
default:
printk(KERN_ERR alg: hash: digest failed on test %d
- for %s: ret=%d\n, i + 1, algo, -ret);
+ for %s: ret=%d\n, j, algo, -ret);
goto out;
}
if (memcmp(result, template[i].digest,
crypto_ahash_digestsize(tfm))) {
printk(KERN_ERR alg: hash: Test %d failed for %s\n,
- i + 1, algo);
+ j, algo);
hexdump(result, crypto_ahash_digestsize(tfm));
ret = -EINVAL;
goto out;
Thanks,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmVHI~} herb...@gondor.apana.org.au
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
sha384 self-test failure oddity
While doing a bit of testing of some other crypto code, I've repeatedly noticed a sha384 self-test failure. If you 'modprobe tcrypt', the sha384 self-test fails, then immediately after it, sha384-generic self-tests succeed. Something is awry w/sha384 initialization, as can be more plainly seen by the following after a reboot: # modprobe tcrypt mode=11 (run sha384 self-test) dmesg - alg: hash: Failed to load transform for sha384: -2 # modprobe tcrypt mode=12 (run sha512 self-test) dmesg - alg: self-tests for sha384-generic (sha384) passed alg: self-tests for sha512-generic (sha512) passed alg: self-tests for sha512 (sha512) passed # modprobe tcrypt mode=11 (run sha384 self-test again) dmesg - alg: self-tests for sha384 (sha384) passed Not sure offhand what's going awry, and don't have time to look deeper right now, but will do so if nobody else gets around to it first... -- Jarod Wilson ja...@redhat.com -- To unsubscribe from this list: send the line unsubscribe linux-crypto in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH] crypto: add buffer overflow checks to testmgr
On Fri, May 29, 2009 at 11:32:54AM -0400, Jarod Wilson wrote:
At present, its entirely possible to add a test vector to testmgr with
an input longer than a page in length w/o specifying a .np option, and
overflow the page of memory allocated to {a,}xbuf[0], silently
corrupting memory. I know, because I've accidentally done it. :)
While this doesn't currently happen in practice w/the existing code,
due to all !np vectors being less than a 4k page in length (and the
page allocation loop often returns contiguous pages anyway), explicit
checks or a way to remove the 4k limit would be a good idea.
A few ways to fix and/or work around this:
1) allocate some larger guaranteed contiguous buffers using
__get_free_pages() or kmalloc and use them in the !np case
2) catch the PAGE_SIZE !np case and then do things similar to how
they are done in the np case
3) catch the PAGE_SIZE !np case and simply exit with an error
Since there currently aren't any test vectors that are actually larger
than a page and not tagged np, option 1 seems like a waste of memory
and option 2 sounds like unnecessary complexity, so I'd offer up
option 3 as the most viable alternative right now.
Signed-off-by: Jarod Wilson ja...@redhat.com
Given the rate at which test vectors are added to the testmgr, and the
likelyhood one will be over a page in size, I think this is probably the best
way forward. Its saves memory/complexity until for now, and catches anyone
trying to exceed the current 1 page size, at which point we can spend the time
to modify the testmanager to make use of scatter/gather chains to handle the
longer vectors.
Neil
Acked-by: Neil Horman nhor...@tuxdriver.com
---
crypto/testmgr.c | 34 ++
1 files changed, 34 insertions(+), 0 deletions(-)
diff --git a/crypto/testmgr.c b/crypto/testmgr.c
index 376ea88..9483a2b 100644
--- a/crypto/testmgr.c
+++ b/crypto/testmgr.c
@@ -185,6 +185,13 @@ static int test_hash(struct crypto_ahash *tfm, struct
hash_testvec *template,
hash_buff = xbuf[0];
+ if (template[i].psize PAGE_SIZE) {
+ printk(KERN_ERR alg: hash: psize %u larger than
+contiguous buffer space\n, template[i].psize);
+ ret = -EOVERFLOW;
+ goto out;
+ }
+
memcpy(hash_buff, template[i].plaintext, template[i].psize);
sg_init_one(sg[0], hash_buff, template[i].psize);
@@ -357,6 +364,16 @@ static int test_aead(struct crypto_aead *tfm, int enc,
input = xbuf[0];
assoc = axbuf[0];
+ if (template[i].ilen PAGE_SIZE ||
+ template[i].alen PAGE_SIZE) {
+ printk(KERN_ERR alg: aead: input larger than
+contiguous buffer space (ilen: %u,
+alen: %u)\n,
+template[i].ilen, template[i].alen);
+ ret = -EOVERFLOW;
+ goto out;
+ }
+
memcpy(input, template[i].input, template[i].ilen);
memcpy(assoc, template[i].assoc, template[i].alen);
if (template[i].iv)
@@ -651,6 +668,14 @@ static int test_cipher(struct crypto_cipher *tfm, int
enc,
j++;
data = xbuf[0];
+
+ if (template[i].ilen PAGE_SIZE) {
+ printk(KERN_ERR alg: cipher: ilen %u larger than
+contiguous buffer space\n, template[i].ilen);
+ ret = -EOVERFLOW;
+ goto out;
+ }
+
memcpy(data, template[i].input, template[i].ilen);
crypto_cipher_clear_flags(tfm, ~0);
@@ -742,6 +767,15 @@ static int test_skcipher(struct crypto_ablkcipher *tfm,
int enc,
j++;
data = xbuf[0];
+
+ if (template[i].ilen PAGE_SIZE) {
+ printk(KERN_ERR alg: skcipher: ilen %u larger
+than contiguous buffer space\n,
+template[i].ilen);
+ ret = -EOVERFLOW;
+ goto out;
+ }
+
memcpy(data, template[i].input, template[i].ilen);
crypto_ablkcipher_clear_flags(tfm, ~0);
--
Jarod Wilson
ja...@redhat.com
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH] crypto: add buffer overflow checks to testmgr
On Fri, May 29, 2009 at 11:32:54AM -0400, Jarod Wilson wrote:
At present, its entirely possible to add a test vector to testmgr with
an input longer than a page in length w/o specifying a .np option, and
overflow the page of memory allocated to {a,}xbuf[0], silently
corrupting memory. I know, because I've accidentally done it. :)
While this doesn't currently happen in practice w/the existing code,
due to all !np vectors being less than a 4k page in length (and the
page allocation loop often returns contiguous pages anyway), explicit
checks or a way to remove the 4k limit would be a good idea.
A few ways to fix and/or work around this:
1) allocate some larger guaranteed contiguous buffers using
__get_free_pages() or kmalloc and use them in the !np case
2) catch the PAGE_SIZE !np case and then do things similar to how
they are done in the np case
3) catch the PAGE_SIZE !np case and simply exit with an error
Since there currently aren't any test vectors that are actually larger
than a page and not tagged np, option 1 seems like a waste of memory
and option 2 sounds like unnecessary complexity, so I'd offer up
option 3 as the most viable alternative right now.
Signed-off-by: Jarod Wilson ja...@redhat.com
I just posted exactly the same thing yesterday :)
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmVHI~} herb...@gondor.apana.org.au
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: HMAC regression
On Fri, May 29, 2009 at 12:04:32PM +0200, Martin Willi wrote:
After doing further tests, it seems that this is additionally related to
User-Mode-Linux and/or it's TUN/TAP network driver. I couldn't reproduce
the issue on a x64 with e1000.
I think the bug is actually in the UML network code, but changing the
scatterwalk logic by using the hash_walk functions triggered the issue.
Ah, I think I see the problem. You must getting an sg entry that
crosses a page boundary, rather than two sg entries that both stay
within a page. These things are very rare, and usually occurs as
a result of SLAB debugging causing kmalloc to return memory that
crosses page boundaries.
Can you see if this patch fixes the problem?
diff --git a/crypto/ahash.c b/crypto/ahash.c
index b2d1ee3..f347637 100644
--- a/crypto/ahash.c
+++ b/crypto/ahash.c
@@ -82,10 +82,11 @@ int crypto_hash_walk_done(struct crypto_hash_walk *walk,
int err)
if (err)
return err;
- walk-offset = 0;
-
- if (nbytes)
+ if (nbytes) {
+ walk-offset = 0;
+ walk-pg++;
return hash_walk_next(walk);
+ }
if (!walk-total)
return 0;
Thanks,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmVHI~} herb...@gondor.apana.org.au
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH] crypto: add buffer overflow checks to testmgr
On 05/29/2009 06:10 PM, Herbert Xu wrote:
On Fri, May 29, 2009 at 11:32:54AM -0400, Jarod Wilson wrote:
At present, its entirely possible to add a test vector to testmgr with
an input longer than a page in length w/o specifying a .np option, and
overflow the page of memory allocated to {a,}xbuf[0], silently
corrupting memory. I know, because I've accidentally done it. :)
While this doesn't currently happen in practice w/the existing code,
due to all !np vectors being less than a 4k page in length (and the
page allocation loop often returns contiguous pages anyway), explicit
checks or a way to remove the 4k limit would be a good idea.
A few ways to fix and/or work around this:
1) allocate some larger guaranteed contiguous buffers using
__get_free_pages() or kmalloc and use them in the !np case
2) catch the PAGE_SIZE !np case and then do things similar to how
they are done in the np case
3) catch the PAGE_SIZE !np case and simply exit with an error
Since there currently aren't any test vectors that are actually larger
than a page and not tagged np, option 1 seems like a waste of memory
and option 2 sounds like unnecessary complexity, so I'd offer up
option 3 as the most viable alternative right now.
Signed-off-by: Jarod Wilson ja...@redhat.com
I just posted exactly the same thing yesterday :)
Oh, haha, serves me right for not looking first... Your variant seems to
be a bit more complete too, as I didn't look at any of the possible cases
where there might be overflows when using scatterlists. Cool, worksforme!
Thanks much,
--
Jarod Wilson
ja...@redhat.com
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
