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 > 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 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 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 ~{PmV>HI~} 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 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 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 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 ~{PmV>HI~} 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
HMAC regression
Hi, 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. The appended patch adds a silly hmac(sha1) test vector larger than a 4KB page and fails on current crypto-2.6. It runs successful when reverting sha1 to the old hash API (54ccb367). Data on the first page gets hashed correctly, but walking to the next page fails. The new page does not get mapped and the remaining bytes are read from the beginning of the first page, resulting in wrong MACs. I did not fully understand the hash_walk code in ahash.c, but either the length/offset calculation is wrong (when using compat functions) or the scatterlist is not set up correctly. Martin --- crypto/testmgr.h | 252 +- 1 files changed, 250 insertions(+), 2 deletions(-) diff --git a/crypto/testmgr.h b/crypto/testmgr.h index 526f00a..ab90006 100644 --- a/crypto/testmgr.h +++ b/crypto/testmgr.h @@ -32,7 +32,7 @@ struct hash_testvec { char *plaintext; char *digest; unsigned char tap[MAX_TAP]; - unsigned char psize; + unsigned int psize; unsigned char np; unsigned char ksize; }; @@ -1238,7 +1238,7 @@ static struct hash_testvec hmac_rmd160_tv_template[] = { /* * HMAC-SHA1 test vectors from RFC2202 */ -#define HMAC_SHA1_TEST_VECTORS 7 +#define HMAC_SHA1_TEST_VECTORS 8 static struct hash_testvec hmac_sha1_tv_template[] = { { @@ -1314,6 +1314,254 @@ static struct hash_testvec hmac_sha1_tv_template[] = { .psize = 73, .digest = "\xe8\xe9\x9d\x0f\x45\x23\x7d\x78\x6d\x6b" "\xba\xa7\x96\x5c\x78\x08\xbb\xff\x1a\x91", + }, { + .key= "\x0c\x0c\x0c\x0c\x0c\x0c\x0c\x0c\x0c\x0c\x0c\x0c\x0c\x0c\x0c\x0c\x0c\x0c\x0c\x0c", + .ksize = 20, + .plaintext = +"\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90" +"\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90" +"\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90" +"\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90" +"\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90" + +"\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90" +"\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90" +"\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90" +"\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90" +"\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90" + +"\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90" +"\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90" +"\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90" +"\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90" +"\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90" + +"\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90" +"\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90" +"\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90" +"\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90" +"\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90" + +"\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90" +"\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90" +"\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90" +"\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90" +"\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90" + +"\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90" +"\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90" +"\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90" +"\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90" +"\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90" + +"\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90" +"\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90" +"\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90" +"\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90" +"\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90" + +"\x12\x34\x56\x78\x90\x12\x34\x56\x78\x90\x12\x34\x56\x7