| Message ID | 890f4bdb28a1cf72f6b802b220b35ebaf0f76bb9.1484090585.git.luto@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Herbert Xu |
| Headers | show |
From: Andy Lutomirski > Sent: 10 January 2017 23:25 > There are some hashes (e.g. sha224) that have some internal trickery > to make sure that only the correct number of output bytes are > generated. If something goes wrong, they could potentially overrun > the output buffer. > > Make the test more robust by allocating only enough space for the > correct output size so that memory debugging will catch the error if > the output is overrun. Might be better to test this by allocating an overlong buffer and then explicitly checking that the output hasn't overrun the allowed space. If nothing else the error message will be clearer. David -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jan 11, 2017 at 7:13 AM, David Laight <David.Laight@aculab.com> wrote: > From: Andy Lutomirski >> Sent: 10 January 2017 23:25 >> There are some hashes (e.g. sha224) that have some internal trickery >> to make sure that only the correct number of output bytes are >> generated. If something goes wrong, they could potentially overrun >> the output buffer. >> >> Make the test more robust by allocating only enough space for the >> correct output size so that memory debugging will catch the error if >> the output is overrun. > > Might be better to test this by allocating an overlong buffer > and then explicitly checking that the output hasn't overrun > the allowed space. > > If nothing else the error message will be clearer. I thought about that, but then I'd have to figure out what poison value to use. Both KASAN and the usual slab debugging are quite good at this kind of checking, and KASAN will even point you to the problematic code directly. --Andy
Andy Lutomirski <luto@kernel.org> wrote: > There are some hashes (e.g. sha224) that have some internal trickery > to make sure that only the correct number of output bytes are > generated. If something goes wrong, they could potentially overrun > the output buffer. > > Make the test more robust by allocating only enough space for the > correct output size so that memory debugging will catch the error if > the output is overrun. > > Tested by intentionally breaking sha224 to output all 256 > internally-generated bits while running on KASAN. > > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Cc: Herbert Xu <herbert@gondor.apana.org.au> > Signed-off-by: Andy Lutomirski <luto@kernel.org> This patch doesn't seem to depend on anything else in the series. Do you want me to take it separately? Cheers,
On Wed, Jan 11, 2017 at 11:47 PM, Herbert Xu <herbert@gondor.apana.org.au> wrote: > Andy Lutomirski <luto@kernel.org> wrote: >> There are some hashes (e.g. sha224) that have some internal trickery >> to make sure that only the correct number of output bytes are >> generated. If something goes wrong, they could potentially overrun >> the output buffer. >> >> Make the test more robust by allocating only enough space for the >> correct output size so that memory debugging will catch the error if >> the output is overrun. >> >> Tested by intentionally breaking sha224 to output all 256 >> internally-generated bits while running on KASAN. >> >> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> Cc: Herbert Xu <herbert@gondor.apana.org.au> >> Signed-off-by: Andy Lutomirski <luto@kernel.org> > > This patch doesn't seem to depend on anything else in the series. > Do you want me to take it separately? Yes, please. Its only relation to the rest of the series is that I wanted to make sure that I didn't mess up sha224's finalization code. --Andy -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jan 10, 2017 at 03:24:46PM -0800, Andy Lutomirski wrote: > There are some hashes (e.g. sha224) that have some internal trickery > to make sure that only the correct number of output bytes are > generated. If something goes wrong, they could potentially overrun > the output buffer. > > Make the test more robust by allocating only enough space for the > correct output size so that memory debugging will catch the error if > the output is overrun. > > Tested by intentionally breaking sha224 to output all 256 > internally-generated bits while running on KASAN. > > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Cc: Herbert Xu <herbert@gondor.apana.org.au> > Signed-off-by: Andy Lutomirski <luto@kernel.org> Patch applied. Thanks.
diff --git a/crypto/testmgr.c b/crypto/testmgr.c index f616ad74cce7..575fc28a9ab2 100644 --- a/crypto/testmgr.c +++ b/crypto/testmgr.c @@ -265,6 +265,7 @@ static int __test_hash(struct crypto_ahash *tfm, struct hash_testvec *template, const int align_offset) { const char *algo = crypto_tfm_alg_driver_name(crypto_ahash_tfm(tfm)); + size_t digest_size = crypto_ahash_digestsize(tfm); unsigned int i, j, k, temp; struct scatterlist sg[8]; char *result; @@ -275,7 +276,7 @@ static int __test_hash(struct crypto_ahash *tfm, struct hash_testvec *template, char *xbuf[XBUFSIZE]; int ret = -ENOMEM; - result = kmalloc(MAX_DIGEST_SIZE, GFP_KERNEL); + result = kmalloc(digest_size, GFP_KERNEL); if (!result) return ret; key = kmalloc(MAX_KEYLEN, GFP_KERNEL); @@ -305,7 +306,7 @@ static int __test_hash(struct crypto_ahash *tfm, struct hash_testvec *template, goto out; j++; - memset(result, 0, MAX_DIGEST_SIZE); + memset(result, 0, digest_size); hash_buff = xbuf[0]; hash_buff += align_offset; @@ -380,7 +381,7 @@ static int __test_hash(struct crypto_ahash *tfm, struct hash_testvec *template, continue; j++; - memset(result, 0, MAX_DIGEST_SIZE); + memset(result, 0, digest_size); temp = 0; sg_init_table(sg, template[i].np); @@ -458,7 +459,7 @@ static int __test_hash(struct crypto_ahash *tfm, struct hash_testvec *template, continue; j++; - memset(result, 0, MAX_DIGEST_SIZE); + memset(result, 0, digest_size); ret = -EINVAL; hash_buff = xbuf[0];
There are some hashes (e.g. sha224) that have some internal trickery to make sure that only the correct number of output bytes are generated. If something goes wrong, they could potentially overrun the output buffer. Make the test more robust by allocating only enough space for the correct output size so that memory debugging will catch the error if the output is overrun. Tested by intentionally breaking sha224 to output all 256 internally-generated bits while running on KASAN. Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> Cc: Herbert Xu <herbert@gondor.apana.org.au> Signed-off-by: Andy Lutomirski <luto@kernel.org> --- crypto/testmgr.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)