[PATCH] crypto: ahash - Fix handling of unaligned buffers

[PATCH] crypto: ahash - Fix handling of unaligned buffers

[Szilveszter Ordog]

The correct way to calculate the start of the aligned part of an
unaligned buffer is:

  offset = ALIGN(offset, alignmask + 1);

However, crypto_hash_walk_done() has:

  offset += alignmask - 1;
  offset = ALIGN(offset, alignmask + 1);

which actually skips a whole block unless offset % (alignmask + 1) == 1.

This patch fixes the problem.

Signed-off-by: Szilveszter Ördög <slipszi@gmail.com>
---
 crypto/ahash.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/crypto/ahash.c b/crypto/ahash.c
index f347637..db42202 100644
--- a/crypto/ahash.c
+++ b/crypto/ahash.c
@@ -65,7 +65,6 @@ int crypto_hash_walk_done(struct crypto_hash_walk
*walk, int err)
 	walk->data -= walk->offset;

 	if (nbytes && walk->offset & alignmask && !err) {
-		walk->offset += alignmask - 1;
 		walk->offset = ALIGN(walk->offset, alignmask + 1);
 		walk->data += walk->offset;

-- 
1.5.5.6
--
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

Re: [PATCH] crypto: ahash - Fix handling of unaligned buffers

[Herbert Xu]

On Tue, Feb 23, 2010 at 11:50:01AM +0000, Szilveszter Ordog wrote:
> The correct way to calculate the start of the aligned part of an
> unaligned buffer is:
> 
>   offset = ALIGN(offset, alignmask + 1);
> 
> However, crypto_hash_walk_done() has:
> 
>   offset += alignmask - 1;
>   offset = ALIGN(offset, alignmask + 1);
> 
> which actually skips a whole block unless offset % (alignmask + 1) == 1.
> 
> This patch fixes the problem.
> 
> Signed-off-by: Szilveszter Ördög <slipszi@gmail.com>

I think you did find a bug, but it's not what you think it is :)

When we get an unaligned buffer, we first process the bit from the
start to the first aligned address.  Once we get to the aligned
address everything happens as usual.

So where this code is, we're trying to move to the next aligned
address, and as ALIGN rounds down, we need to add alignmask.
So the bug is the fact that we're adding alignmask - 1.

Were you able to reproduce this? If so please give this patch a
spin.

commit 483b84aa69382d581f272e882158b91387fa2b7a
Author: Herbert Xu <herbert@gondor.apana.org.au>
Date:   Tue Mar 2 22:36:33 2010 +0800

    crypto: hash - Fix SG walk on unaligned addresses
    
    When we do an SG walk on an unaligned address that is exactly 1
    modulo the alignment, we end up hashing some of the data twice.
    
    Reported-by: Szilveszter Ordog <slipszi@gmail.com>
    Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/crypto/ahash.c b/crypto/ahash.c
index 33a4ff4..b52eb6d 100644
--- a/crypto/ahash.c
+++ b/crypto/ahash.c
@@ -78,7 +78,7 @@ int crypto_hash_walk_done(struct crypto_hash_walk *walk, int err)
 	walk->data -= walk->offset;
 
 	if (nbytes && walk->offset & alignmask && !err) {
-		walk->offset += alignmask - 1;
+		walk->offset += alignmask;
 		walk->offset = ALIGN(walk->offset, alignmask + 1);
 		walk->data += walk->offset;
 
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
--
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

Re: [PATCH] crypto: ahash - Fix handling of unaligned buffers

[Szilveszter Ordog]

> When we get an unaligned buffer, we first process the bit from the
> start to the first aligned address.  Once we get to the aligned
> address everything happens as usual.
>
> So where this code is, we're trying to move to the next aligned
> address, and as ALIGN rounds down, we need to add alignmask.
> So the bug is the fact that we're adding alignmask - 1.

No, ALIGN rounds *up* (as it should). See:

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=include/linux/kernel.h#l40

Best regards,
Szilveszter
--
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

Re: [PATCH] crypto: ahash - Fix handling of unaligned buffers

[Herbert Xu]

Szilveszter Ordog <slipszi@gmail.com> wrote:
>> When we get an unaligned buffer, we first process the bit from the
>> start to the first aligned address.  Once we get to the aligned
>> address everything happens as usual.
>>
>> So where this code is, we're trying to move to the next aligned
>> address, and as ALIGN rounds down, we need to add alignmask.
>> So the bug is the fact that we're adding alignmask - 1.
> 
> No, ALIGN rounds *up* (as it should). See:

Ah, of course it does.  I've applied your patch.  Thanks a lot!
-- 
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
--
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

Re: [PATCH] crypto: ahash - Fix handling of unaligned buffers

[Szilveszter Ordog]

> I've applied your patch.

Great! Thank you!

Best regards,
Szilveszter