crypto: blkcipher_get_spot() handling of buffer at end of page

PATCH In review Bob Gilligan 3 participants 7 messages Sep 8, 2007 – Sep 19, 2007 Crypto API

Share & tools

Bob Gilligan

author

2007-09-08 00:33 UTC #0 lore raw

PATCH 1/1 crypto: blkcipher_get_spot() handling of buffer at end of page

Hi -- There appears to be a bug in the function blkcipher_get_spot(), which resides in crypto/blkcipher.c. This small function reads:

static inline u8 *blkcipher_get_spot(u8 *start, unsigned int len)

{
        if (offset_in_page(start + len) < len)
                return (u8 *)((unsigned long)(start + len) & PAGE_MASK);
        return start;
}

This function is apparently attempting to detect the case where the buffer pointed to by "start", of length "len" bytes, straddles a page boundary. In that case, it will return the address of the start of the last page that the buffer resides on. It works correctly in all cases except when the buffer resides entirely within one page but is located at the end of that page (i.e. the last byte of the buffer is at address 0x.....fff). In that one case, this function will return the address of the start of the next page (i.e. one byte beyond the end of the buffer), when it should return "start".

For example, say blkcipher_get_spot() is called with start=0xf7e71ff0, and len=16. The function returns 0xf7e72000. The correct return value should be 0xf7e71ff0.

This bug appears to be the cause of occasional crashes within the slab allocator that we have seen testing ipsec with AES encryption. Tracking the crashes down, we see occasions when the kmalloc() call in blkcipher_next_slow() is being called with n=32, which is satisfied out of the size-32 slab. The kmalloc'ed buffer is passed to blkcipher_get_spot() twice to generate two pointers into that buffer, then scatterwalk_copychunks() copies into the second pointer. In the case when the buffer returned by kmalloc() occupies the last 32-byte buffer on the page, the second call to blkcipher_get_spot() returns the address of the start of the NEXT page, and scaterwalk_copychunks() over-writes 16 bytes at that address. Since the next page often holds another slab, this stomps on the list_head in the slab struct, and the system crashes when the slab allocator dereferences those pointers a later point in time. We've seen several crashes in free_block().

Proposed patch is below. We found the problem and tested this fix in 2.6.20, but it looks like the relevant code in blkcipher.c is the same in the latest tree.

Bob.

Correctly handle the case where the buffer passed into blkcipher_get_spot() resides at the end of a page.

Signed-off-by Bob Gilligan <[email protected]>

---

crypto/blkcipher.c +4 -2

index 6e93004..3b6734e 100644
@@ -60,8 +60,10 @@ static inline void blkcipher_unmap_dst(struct blkcipher_walk\
 *walk)
 
 static inline u8 *blkcipher_get_spot(u8 *start, unsigned int len)
 {
-       if (offset_in_page(start + len) < len)
-               return (u8 *)((unsigned long)(start + len) & PAGE_MASK);
+       u8 *end = (start + len - 1);
+
+       if (offset_in_page(end) < (len - 1))
+               return (u8 *)((unsigned long)(end) & PAGE_MASK);
        return start;
 }

Herbert Xu

maintainer

2007-09-08 04:14 UTC #1 lore raw

Quoted 4 lines · Bob Gilligan

Proposed patch is below. We found the problem and tested this fix in 2.6.20, but it looks like the relevant code in blkcipher.c is the same in the latest tree.

Good catch!

I've fixed this slightly differently. Also, the kmalloc size in the case where it does straddle a page isn't enough either.

[CRYPTO] blkcipher: Fix handling of kmalloc page straddling

The function blkcipher_get_spot tries to return a buffer of the specified length that does not straddle a page. It has an off-by-one bug so it may advance a page unnecessarily.

What's worse, one of its callers doesn't provide a buffer that's sufficiently long for this operation.

This patch fixes both problems. Thanks to Bob Gilligan for diagnosing this problem and providing a fix.

Signed-off-by Herbert Xu <[email protected]>

Cheers,

Signature · 5 lines

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

crypto/blkcipher.c +7 -4

index 7755834..469cb7f 100644
@@ -59,11 +59,13 @@ static inline void blkcipher_unmap_dst(struct blkcipher_walk *walk)
 	scatterwalk_unmap(walk->dst.virt.addr, 1);
 }
 
+/* Get a spot of the specified length that does not straddle a page.
+ * The caller needs to ensure that there is enough space for this operation.
+ */
 static inline u8 *blkcipher_get_spot(u8 *start, unsigned int len)
 {
-	if (offset_in_page(start + len) < len)
-		return (u8 *)((unsigned long)(start + len) & PAGE_MASK);
-	return start;
+	u8 *end_page = (u8 *)(((unsigned long)(start + len - 1)) & PAGE_MASK);
+	return start < end_page ? start : end_page;
 }
 
 static inline unsigned int blkcipher_done_slow(struct crypto_blkcipher *tfm,

@@ -155,7 +157,8 @@ static inline int blkcipher_next_slow(struct blkcipher_desc *desc,
 	if (walk->buffer)
 		goto ok;
 
-	n = bsize * 2 + (alignmask & ~(crypto_tfm_ctx_alignment() - 1));
+	n = bsize * 3 - (alignmask + 1) +
+	    (alignmask & ~(crypto_tfm_ctx_alignment() - 1));
 	walk->buffer = kmalloc(n, GFP_ATOMIC);
 	if (!walk->buffer)
 		return blkcipher_walk_done(desc, walk, -ENOMEM);

Herbert Xu

maintainer

2007-09-10 07:58 UTC #2 lore raw

Quoted 2 lines · Herbert Xu

[CRYPTO] blkcipher: Fix handling of kmalloc page straddling

As Bob correctly noted, I had the boolean test inverted. Here is the correction:

[CRYPTO] blkcipher: Fix inverted test in blkcipher_get_spot

The previous patch had the conditional inverted. This patch fixes it so that we return the original position if it does not straddle a page.

Thanks to Bob Gilligan for spotting this.

Signed-off-by Herbert Xu <[email protected]>

Cheers,

Signature · 5 lines

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

crypto/blkcipher.c +1 -1

@@ -65,7 +65,7 @@ static inline void blkcipher_unmap_dst(s
 static inline u8 *blkcipher_get_spot(u8 *start, unsigned int len)
 {
 	u8 *end_page = (u8 *)(((unsigned long)(start + len - 1)) & PAGE_MASK);
-	return start < end_page ? start : end_page;
+	return start > end_page ? start : end_page;
 }
 
 static inline unsigned int blkcipher_done_slow(struct crypto_blkcipher *tfm,

Ingo Oeser

2007-09-10 19:46 UTC #3 lore raw

Hi Herbert,

Quoted 11 lines · Herbert Xu

Quoted 2 lines · Herbert Xu
[CRYPTO] blkcipher: Fix handling of kmalloc page straddling
As Bob correctly noted, I had the boolean test inverted. Here is the correction:
[CRYPTO] blkcipher: Fix inverted test in blkcipher_get_spot
The previous patch had the conditional inverted. This patch fixes it so that we return the original position if it does not straddle a page.

What about using max() for this to make your intention obvious?

static inline u8 *blkcipher_get_spot(u8 *start, unsigned int len)

{
	u8 *end_page = (u8 *)(((unsigned long)(start + len - 1)) & PAGE_MASK); 
	return max(start, end_page);
}

Best Regards

Ingo Oeser

Herbert Xu

maintainer

2007-09-11 08:41 UTC #4 lore raw

Quoted 8 lines · Ingo Oeser

What about using max() for this to make your intention obvious?
static inline u8 *blkcipher_get_spot(u8 *start, unsigned int len)
{
	u8 *end_page = (u8 *)(((unsigned long)(start + len - 1)) & PAGE_MASK); 
	return max(start, end_page);
}

Yes that looks sane. Could you please send a patch?

Cheers,

Signature · 4 lines

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

Ingo Oeser

2007-09-11 19:52 UTC #5 lore raw

PATCH 1/1 crypto: cleanup: Use max() in blkcipher_get_spot() to state the intention.

[PATCH] crypto: cleanup: Use max() in blkcipher_get_spot() to state the intention.

Signed-off-by Ingo Oeser <[email protected]>

---
Hi Herbert,

here is the requested patch against Linus' latest tree. It at least compiles.

Best Regards

Ingo Oeser

 crypto/blkcipher.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

crypto/blkcipher.c +1 -1

index d8f8ec3..1c99d92 100644
@@ -65,7 +65,7 @@ static inline void blkcipher_unmap_dst(struct blkcipher_walk *walk)
 static inline u8 *blkcipher_get_spot(u8 *start, unsigned int len)
 {
 	u8 *end_page = (u8 *)(((unsigned long)(start + len - 1)) & PAGE_MASK);
-	return start > end_page ? start : end_page;
+	return max(start, end_page);
 }
 
 static inline unsigned int blkcipher_done_slow(struct crypto_blkcipher *tfm,

Signature · 1 line

1.5.2.5

Herbert Xu

maintainer

2007-09-19 11:16 UTC #6 lore raw

Re: [PATCH] crypto: cleanup: Use max() in blkcipher_get_spot() to state the intention.

Quoted 3 lines · Ingo Oeser

[PATCH] crypto: cleanup: Use max() in blkcipher_get_spot() to state the intention.
Signed-off-by Ingo Oeser <[email protected]>

Patch applied. Thanks!

Signature · 4 lines

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