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]>
---
diff --git a/crypto/blkcipher.c b/crypto/blkcipher.c
index 6e93004..3b6734e 100644
--- a/crypto/blkcipher.c
+++ b/crypto/blkcipher.c
@@ -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;
}
On Fri, Sep 07, 2007 at 05:09:17PM -0700, Bob Gilligan wrote:
>
> 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,
--
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
--
diff --git a/crypto/blkcipher.c b/crypto/blkcipher.c
index 7755834..469cb7f 100644
--- a/crypto/blkcipher.c
+++ b/crypto/blkcipher.c
@@ -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);
On Sat, Sep 08, 2007 at 12:14:23PM +0800, Herbert Xu wrote:
>
> [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,
--
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
--
diff --git a/crypto/blkcipher.c b/crypto/blkcipher.c
--- a/crypto/blkcipher.c
+++ b/crypto/blkcipher.c
@@ -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,
Hi Herbert,
On Monday 10 September 2007, Herbert Xu wrote:
> On Sat, Sep 08, 2007 at 12:14:23PM +0800, Herbert Xu wrote:
> >
> > [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
On Mon, Sep 10, 2007 at 09:46:46PM +0200, Ingo Oeser wrote:
>
> 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,
--
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
[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(-)
diff --git a/crypto/blkcipher.c b/crypto/blkcipher.c
index d8f8ec3..1c99d92 100644
--- a/crypto/blkcipher.c
+++ b/crypto/blkcipher.c
@@ -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,
--
1.5.2.5
On Tue, Sep 11, 2007 at 09:53:24PM +0200, Ingo Oeser wrote:
> [PATCH] crypto: cleanup: Use max() in blkcipher_get_spot() to state the intention.
>
> Signed-off-by: Ingo Oeser <[email protected]>
Patch applied. Thanks!
--
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