[PATCH 1/3] ligceph: fix overflow in __decode_pool_names()

[PATCH 1/3] ligceph: fix overflow in __decode_pool_names()

[Xi Wang]

`len' is read from network and thus needs validation.  Otherwise a
large `len' would cause out-of-bounds access via the memcpy() call.
In addition, len = 0xffffffff would overflow the kmalloc() size,
leading to out-of-bounds write.

This patch adds a check of `len' via ceph_decode_need().  Also use
kstrndup rather than kmalloc/memcpy.

Signed-off-by: Xi Wang <xi.wang@gmail.com>
---
 net/ceph/osdmap.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/net/ceph/osdmap.c b/net/ceph/osdmap.c
index 29ad46e..f80afc3 100644
--- a/net/ceph/osdmap.c
+++ b/net/ceph/osdmap.c
@@ -495,15 +495,12 @@ static int __decode_pool_names(void **p, void *end, struct ceph_osdmap *map)
 		ceph_decode_32_safe(p, end, pool, bad);
 		ceph_decode_32_safe(p, end, len, bad);
 		dout("  pool %d len %d\n", pool, len);
+		ceph_decode_need(p, end, len, bad);
 		pi = __lookup_pg_pool(&map->pg_pools, pool);
 		if (pi) {
 			kfree(pi->name);
-			pi->name = kmalloc(len + 1, GFP_NOFS);
-			if (pi->name) {
-				memcpy(pi->name, *p, len);
-				pi->name[len] = '\0';
-				dout("  name is %s\n", pi->name);
-			}
+			pi->name = kstrndup(*p, len, GFP_NOFS);
+			dout("  name is %s\n", pi->name);
 		}
 		*p += len;
 	}
-- 
1.7.5.4

[PATCH 2/3] libceph: fix overflow in osdmap_decode()

[Xi Wang]

On 32-bit systems, a large `n' would overflow `n * sizeof(u32)' and bypass
the check ceph_decode_need(p, end, n * sizeof(u32), bad).  It would also
overflow the subsequent kmalloc() size, leading to out-of-bounds write.

Signed-off-by: Xi Wang <xi.wang@gmail.com>
---
 net/ceph/osdmap.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/net/ceph/osdmap.c b/net/ceph/osdmap.c
index f80afc3..774eac6 100644
--- a/net/ceph/osdmap.c
+++ b/net/ceph/osdmap.c
@@ -670,6 +670,9 @@ struct ceph_osdmap *osdmap_decode(void **p, void *end)
 		ceph_decode_need(p, end, sizeof(u32) + sizeof(u64), bad);
 		ceph_decode_copy(p, &pgid, sizeof(pgid));
 		n = ceph_decode_32(p);
+		err = -EINVAL;
+		if (n > (UINT_MAX - sizeof(*pg)) / sizeof(u32))
+			goto bad;
 		ceph_decode_need(p, end, n * sizeof(u32), bad);
 		err = -ENOMEM;
 		pg = kmalloc(sizeof(*pg) + n*sizeof(u32), GFP_NOFS);
-- 
1.7.5.4

Re: [PATCH 2/3] libceph: fix overflow in osdmap_decode()

[Alex Elder]

On 04/29/2012 01:59 AM, Xi Wang wrote:
> On 32-bit systems, a large `n' would overflow `n * sizeof(u32)' and bypass
> the check ceph_decode_need(p, end, n * sizeof(u32), bad).  It would also
> overflow the subsequent kmalloc() size, leading to out-of-bounds write.

This looks good.

Your previous patch made me look at something else though.  If
you can think of a good solution would you be willing to send a
patch to implement it?  (See below.)  I won't hold up committing
this for it, but I'd like your opinion.

Reviewed-by: Alex Elder <elder@inktank.com>

> Signed-off-by: Xi Wang <xi.wang@gmail.com>
> ---
>  net/ceph/osdmap.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/net/ceph/osdmap.c b/net/ceph/osdmap.c
> index f80afc3..774eac6 100644
> --- a/net/ceph/osdmap.c
> +++ b/net/ceph/osdmap.c
> @@ -670,6 +670,9 @@ struct ceph_osdmap *osdmap_decode(void **p, void *end)

Just above here we see:
        /* pg_temp */
        ceph_decode_32_safe(p, end, len, bad);
        for (i = 0; i < len; i++) {

We haven't validated "len" here either.  Looking at it I'm not sure
we can do much, but I think we do know a few things should be true:
- (len & (sizeof (u32) - 1)) == 0
- len <= (UINT_MAX / (sizeof (struct ceph_pg) + sizeof (u32)))
    and further, if it's invalid to have a value for pg->len of
    zero, then we can instead assert:
- len <= (UINT_MAX / (sizeof (struct ceph_pg) + 2 * sizeof (u32)))

I don't know if it's that important do do a check like this though.

I appreciate these detail-oriented fixes that you've been sending.

>  		ceph_decode_need(p, end, sizeof(u32) + sizeof(u64), bad);
>  		ceph_decode_copy(p, &pgid, sizeof(pgid));
>  		n = ceph_decode_32(p);
> +		err = -EINVAL;
> +		if (n > (UINT_MAX - sizeof(*pg)) / sizeof(u32))
> +			goto bad;
>  		ceph_decode_need(p, end, n * sizeof(u32), bad);
>  		err = -ENOMEM;
>  		pg = kmalloc(sizeof(*pg) + n*sizeof(u32), GFP_NOFS);

Re: [PATCH 2/3] libceph: fix overflow in osdmap_decode()

[Xi Wang]

On Jun 6, 2012, at 12:26 PM, Alex Elder wrote:
> 
> Just above here we see:
>        /* pg_temp */
>        ceph_decode_32_safe(p, end, len, bad);
>        for (i = 0; i < len; i++) {
> 
> We haven't validated "len" here either.  Looking at it I'm not sure
> we can do much, but I think we do know a few things should be true:
> - (len & (sizeof (u32) - 1)) == 0
> - len <= (UINT_MAX / (sizeof (struct ceph_pg) + sizeof (u32)))
>    and further, if it's invalid to have a value for pg->len of
>    zero, then we can instead assert:
> - len <= (UINT_MAX / (sizeof (struct ceph_pg) + 2 * sizeof (u32)))
> 
> I don't know if it's that important do do a check like this though.

I don't see any overflow issue here.  Are you worried about the loop
running for a while given a large n?  How about this check?

	/* pg_temp */
	ceph_decode_32_safe(p, end, len, bad);
+	if (len > UINT_MAX / (sizeof(u32) + sizeof(u64)))
+		goto bad;
+	ceph_decode_need(p, end, len * (sizeof(u32) + sizeof(u64)), bad);

- xi

Re: [PATCH 2/3] libceph: fix overflow in osdmap_decode()

[Alex Elder]

On 06/06/2012 12:56 PM, Xi Wang wrote:
> On Jun 6, 2012, at 12:26 PM, Alex Elder wrote:
>>
>> Just above here we see:
>>        /* pg_temp */
>>        ceph_decode_32_safe(p, end, len, bad);
>>        for (i = 0; i < len; i++) {
>>
>> We haven't validated "len" here either.  Looking at it I'm not sure
>> we can do much, but I think we do know a few things should be true:
>> - (len & (sizeof (u32) - 1)) == 0
>> - len <= (UINT_MAX / (sizeof (struct ceph_pg) + sizeof (u32)))
>>    and further, if it's invalid to have a value for pg->len of
>>    zero, then we can instead assert:
>> - len <= (UINT_MAX / (sizeof (struct ceph_pg) + 2 * sizeof (u32)))
>>
>> I don't know if it's that important do do a check like this though.
> 
> I don't see any overflow issue here.  Are you worried about the loop
> running for a while given a large n?  How about this check?

Not an overflow check, but a validity check nevertheless.

> 	/* pg_temp */
> 	ceph_decode_32_safe(p, end, len, bad);
> +	if (len > UINT_MAX / (sizeof(u32) + sizeof(u64)))
> +		goto bad;

That part is sufficient, though I'd prefer sizeof (ceph_pg) both
here and in the line that follows, rather than sizeof (u64).

> +	ceph_decode_need(p, end, len * (sizeof(u32) + sizeof(u64)), bad);

This isn't necessary--I was not looking for overflow, just for
some sanity checking on the value that came in from the wire.

It probably won't matter because in time if the value is too large
then one of the checks inside the loop might bail out.  But catching
it as early as possible is always better.

I'm not too concerned about it.  I may get around to implement
fixes like this myself, but would probably do it comprehensively
if I do.

					-Alex

Re: [PATCH 2/3] libceph: fix overflow in osdmap_decode()

[Xi Wang]

On Jun 6, 2012, at 3:14 PM, Alex Elder wrote:
> It probably won't matter because in time if the value is too large
> then one of the checks inside the loop might bail out.  But catching
> it as early as possible is always better.

Yeah I Agree. :-)

- xi

[PATCH 3/3] libceph: fix overflow in osdmap_apply_incremental()

[Xi Wang]

On 32-bit systems, a large `pglen' would overflow `pglen*sizeof(u32)'
and bypass the check ceph_decode_need(p, end, pglen*sizeof(u32), bad).
It would also overflow the subsequent kmalloc() size, leading to
out-of-bounds write.

Signed-off-by: Xi Wang <xi.wang@gmail.com>
---
 net/ceph/osdmap.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/net/ceph/osdmap.c b/net/ceph/osdmap.c
index 774eac6..b1ea6d1 100644
--- a/net/ceph/osdmap.c
+++ b/net/ceph/osdmap.c
@@ -891,6 +891,10 @@ struct ceph_osdmap *osdmap_apply_incremental(void **p, void *end,
 
 		if (pglen) {
 			/* insert */
+			if (pglen > (UINT_MAX - sizeof(*pg)) / sizeof(u32)) {
+				err = -EINVAL;
+				goto bad;
+			}
 			ceph_decode_need(p, end, pglen*sizeof(u32), bad);
 			pg = kmalloc(sizeof(*pg) + sizeof(u32)*pglen, GFP_NOFS);
 			if (!pg) {
-- 
1.7.5.4

Re: [PATCH 3/3] libceph: fix overflow in osdmap_apply_incremental()

[Alex Elder]

On 04/29/2012 01:59 AM, Xi Wang wrote:
> On 32-bit systems, a large `pglen' would overflow `pglen*sizeof(u32)'
> and bypass the check ceph_decode_need(p, end, pglen*sizeof(u32), bad).
> It would also overflow the subsequent kmalloc() size, leading to
> out-of-bounds write.
> 
> Signed-off-by: Xi Wang <xi.wang@gmail.com>

This looks good.  I'll wait to hear back on my comments on
your earlier patches before committing these.

Reviewed-by: Alex Elder <elder@inktank.com>

> ---
>  net/ceph/osdmap.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/net/ceph/osdmap.c b/net/ceph/osdmap.c
> index 774eac6..b1ea6d1 100644
> --- a/net/ceph/osdmap.c
> +++ b/net/ceph/osdmap.c
> @@ -891,6 +891,10 @@ struct ceph_osdmap *osdmap_apply_incremental(void **p, void *end,
>  
>  		if (pglen) {
>  			/* insert */
> +			if (pglen > (UINT_MAX - sizeof(*pg)) / sizeof(u32)) {
> +				err = -EINVAL;
> +				goto bad;
> +			}
>  			ceph_decode_need(p, end, pglen*sizeof(u32), bad);
>  			pg = kmalloc(sizeof(*pg) + sizeof(u32)*pglen, GFP_NOFS);
>  			if (!pg) {

[PATCH v2 1/3] libceph: fix overflow in __decode_pool_names()

[Xi Wang]

`len' is read from network and thus needs validation.  Otherwise a
large `len' would cause out-of-bounds access via the memcpy() call.
In addition, len = 0xffffffff would overflow the kmalloc() size,
leading to out-of-bounds write.

This patch adds a check of `len' via ceph_decode_need().  Also use
kstrndup rather than kmalloc/memcpy.

Signed-off-by: Xi Wang <xi.wang@gmail.com>
---
Subject corrected.  Sorry, my bad.
---
 net/ceph/osdmap.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/net/ceph/osdmap.c b/net/ceph/osdmap.c
index 29ad46e..f80afc3 100644
--- a/net/ceph/osdmap.c
+++ b/net/ceph/osdmap.c
@@ -495,15 +495,12 @@ static int __decode_pool_names(void **p, void *end, struct ceph_osdmap *map)
 		ceph_decode_32_safe(p, end, pool, bad);
 		ceph_decode_32_safe(p, end, len, bad);
 		dout("  pool %d len %d\n", pool, len);
+		ceph_decode_need(p, end, len, bad);
 		pi = __lookup_pg_pool(&map->pg_pools, pool);
 		if (pi) {
 			kfree(pi->name);
-			pi->name = kmalloc(len + 1, GFP_NOFS);
-			if (pi->name) {
-				memcpy(pi->name, *p, len);
-				pi->name[len] = '\0';
-				dout("  name is %s\n", pi->name);
-			}
+			pi->name = kstrndup(*p, len, GFP_NOFS);
+			dout("  name is %s\n", pi->name);
 		}
 		*p += len;
 	}
-- 
1.7.5.4

Re: [PATCH v2 1/3] libceph: fix overflow in __decode_pool_names()

[Alex Elder]

On 04/29/2012 02:07 AM, Xi Wang wrote:
> `len' is read from network and thus needs validation.  Otherwise a
> large `len' would cause out-of-bounds access via the memcpy() call.
> In addition, len = 0xffffffff would overflow the kmalloc() size,
> leading to out-of-bounds write.
> 
> This patch adds a check of `len' via ceph_decode_need().  Also use
> kstrndup rather than kmalloc/memcpy.

This looks good, however I'd like to correct one thing, and fix
another (both noted below) before committing.  Please confirm/ack my
suggested change; I'll still credit you with the original patch.
Thanks.

					-Alex

> Signed-off-by: Xi Wang <xi.wang@gmail.com>
> ---
> Subject corrected.  Sorry, my bad.
> ---
>  net/ceph/osdmap.c |    9 +++------
>  1 files changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/net/ceph/osdmap.c b/net/ceph/osdmap.c
> index 29ad46e..f80afc3 100644
> --- a/net/ceph/osdmap.c
> +++ b/net/ceph/osdmap.c
> @@ -495,15 +495,12 @@ static int __decode_pool_names(void **p, void *end, struct ceph_osdmap *map)
>  		ceph_decode_32_safe(p, end, pool, bad);
>  		ceph_decode_32_safe(p, end, len, bad);
>  		dout("  pool %d len %d\n", pool, len);
> +		ceph_decode_need(p, end, len, bad);
>  		pi = __lookup_pg_pool(&map->pg_pools, pool);
>  		if (pi) {
>  			kfree(pi->name);
> -			pi->name = kmalloc(len + 1, GFP_NOFS);
> -			if (pi->name) {
> -				memcpy(pi->name, *p, len);
> -				pi->name[len] = '\0';
> -				dout("  name is %s\n", pi->name);
> -			}
> +			pi->name = kstrndup(*p, len, GFP_NOFS);
> +			dout("  name is %s\n", pi->name);

Instead:
		if (pi) {
			char *name = kstrndup(*p, len, GFP_NOFS);

			if (!name)
				return -ENOMEM;
			kfree(pi->name);
			pi->name = name;
			dout("  name is %s\n", pi->name);
		}

>  		}
>  		*p += len;
>  	}

Re: [PATCH v2 1/3] libceph: fix overflow in __decode_pool_names()

[Xi Wang]

On Jun 6, 2012, at 12:26 PM, Alex Elder wrote:
>> diff --git a/net/ceph/osdmap.c b/net/ceph/osdmap.c
>> index 29ad46e..f80afc3 100644
>> --- a/net/ceph/osdmap.c
>> +++ b/net/ceph/osdmap.c
>> @@ -495,15 +495,12 @@ static int __decode_pool_names(void **p, void *end, struct ceph_osdmap *map)
>> 		ceph_decode_32_safe(p, end, pool, bad);
>> 		ceph_decode_32_safe(p, end, len, bad);
>> 		dout("  pool %d len %d\n", pool, len);
>> +		ceph_decode_need(p, end, len, bad);
>> 		pi = __lookup_pg_pool(&map->pg_pools, pool);
>> 		if (pi) {
>> 			kfree(pi->name);
>> -			pi->name = kmalloc(len + 1, GFP_NOFS);
>> -			if (pi->name) {
>> -				memcpy(pi->name, *p, len);
>> -				pi->name[len] = '\0';
>> -				dout("  name is %s\n", pi->name);
>> -			}
>> +			pi->name = kstrndup(*p, len, GFP_NOFS);
>> +			dout("  name is %s\n", pi->name);
> 
> Instead:
> 		if (pi) {
> 			char *name = kstrndup(*p, len, GFP_NOFS);
> 
> 			if (!name)
> 				return -ENOMEM;
> 			kfree(pi->name);
> 			pi->name = name;
> 			dout("  name is %s\n", pi->name);
> 		}

Looks good to me.  Thanks!

Do you want me to send another patch?

- xi

Re: [PATCH v2 1/3] libceph: fix overflow in __decode_pool_names()

[Alex Elder]

On 06/06/2012 12:54 PM, Xi Wang wrote:
> On Jun 6, 2012, at 12:26 PM, Alex Elder wrote:
>>> diff --git a/net/ceph/osdmap.c b/net/ceph/osdmap.c
>>> index 29ad46e..f80afc3 100644
>>> --- a/net/ceph/osdmap.c
>>> +++ b/net/ceph/osdmap.c
>>> @@ -495,15 +495,12 @@ static int __decode_pool_names(void **p, void *end, struct ceph_osdmap *map)
>>> 		ceph_decode_32_safe(p, end, pool, bad);
>>> 		ceph_decode_32_safe(p, end, len, bad);
>>> 		dout("  pool %d len %d\n", pool, len);
>>> +		ceph_decode_need(p, end, len, bad);
>>> 		pi = __lookup_pg_pool(&map->pg_pools, pool);
>>> 		if (pi) {
>>> 			kfree(pi->name);
>>> -			pi->name = kmalloc(len + 1, GFP_NOFS);
>>> -			if (pi->name) {
>>> -				memcpy(pi->name, *p, len);
>>> -				pi->name[len] = '\0';
>>> -				dout("  name is %s\n", pi->name);
>>> -			}
>>> +			pi->name = kstrndup(*p, len, GFP_NOFS);
>>> +			dout("  name is %s\n", pi->name);
>>
>> Instead:
>> 		if (pi) {
>> 			char *name = kstrndup(*p, len, GFP_NOFS);
>>
>> 			if (!name)
>> 				return -ENOMEM;
>> 			kfree(pi->name);
>> 			pi->name = name;
>> 			dout("  name is %s\n", pi->name);
>> 		}
> 
> Looks good to me.  Thanks!
> 
> Do you want me to send another patch?

Nope.  It's already done, I just wasn't going to commit it without
hearing back from you.

Thanks.

					-Alex