[PATCH] xfsdump: prevent segfault in cb_add_inogrp

[PATCH] xfsdump: prevent segfault in cb_add_inogrp

[rjohnston]

[-- Attachment #1: prevent-segfault-in-cb_add_inogrp.patch --]
[-- Type: text/plain, Size: 1744 bytes --]

The call to memset will segfault because the offset for the first
parameter is done twice. We are using pointer math to do the
calculation.
The first time is when calculating oldsize, the size of i2gseg_t
is accounted for.
	oldsize = (numsegs - SEGPERHNK) * sizeof(i2gseg_t);
Then in the call to memset, oldsize is again multiplied by the size
of i2gmap_t.
	memset(inomap.i2gmap + oldsize, ...)
	
i2gmap holds the used inodes in each chunk. When there are 2^31 chunk
entries, it could describe 2^31 (1 inode/chunk)- 2^40 (64 inodes/chunk).

With 100s of millions of inodes there are enough entries to wrap the
32 bit variable oldsize.

Switching to use array index notation instead of calculating the
pointer address twice ;) would resolve this issue. The unneeded
local variable oldsize can be removed as well.

---
 dump/inomap.c |    8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

Index: b/dump/inomap.c
===================================================================
--- a/dump/inomap.c
+++ b/dump/inomap.c
@@ -1125,8 +1125,7 @@ cb_add_inogrp( void *arg1, intgen_t fsfd
 		lastsegp->segoff = 0;
 
 		if (lastsegp->hnkoff == inomap.hnkmaplen) {
-			intgen_t numsegs;
-			intgen_t oldsize;
+			int64_t numsegs;
 
 			inomap.hnkmaplen++;
 			inomap.hnkmap = (hnk_t *)
@@ -1140,10 +1139,7 @@ cb_add_inogrp( void *arg1, intgen_t fsfd
 				return -1;
 
 			/* zero the new portion of the i2gmap */
-			oldsize = (numsegs - SEGPERHNK) * sizeof(i2gseg_t);
-
-			memset(inomap.i2gmap + oldsize,
-			       0,
+			memset(&inomap.i2gmap[numsegs - SEGPERHNK], 0,
 			       SEGPERHNK * sizeof(i2gseg_t));
 		}
 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfsdump: prevent segfault in cb_add_inogrp

[Eric Sandeen]

On 8/26/15 9:33 AM, rjohnston@sgi.com wrote:
> The call to memset will segfault because the offset for the first
> parameter is done twice. We are using pointer math to do the
> calculation.
> The first time is when calculating oldsize, the size of i2gseg_t
> is accounted for.
> 	oldsize = (numsegs - SEGPERHNK) * sizeof(i2gseg_t);
> Then in the call to memset, oldsize is again multiplied by the size
> of i2gmap_t.
> 	memset(inomap.i2gmap + oldsize, ...)
> 	
> i2gmap holds the used inodes in each chunk. When there are 2^31 chunk
> entries, it could describe 2^31 (1 inode/chunk)- 2^40 (64 inodes/chunk).
> 
> With 100s of millions of inodes there are enough entries to wrap the
> 32 bit variable oldsize.
> 
> Switching to use array index notation instead of calculating the
> pointer address twice ;) would resolve this issue. The unneeded
> local variable oldsize can be removed as well.

Ok, this doesn't explain the type change for numsegs, does it?

Can you walk me through why that's needed?  At what point would
numsegs = inomap.hnkmaplen * SEGPERHNK; overflow?

Thanks,
-Eric

> ---
>  dump/inomap.c |    8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> Index: b/dump/inomap.c
> ===================================================================
> --- a/dump/inomap.c
> +++ b/dump/inomap.c
> @@ -1125,8 +1125,7 @@ cb_add_inogrp( void *arg1, intgen_t fsfd
>  		lastsegp->segoff = 0;
>  
>  		if (lastsegp->hnkoff == inomap.hnkmaplen) {
> -			intgen_t numsegs;
> -			intgen_t oldsize;
> +			int64_t numsegs;
>  
>  			inomap.hnkmaplen++;
>  			inomap.hnkmap = (hnk_t *)
> @@ -1140,10 +1139,7 @@ cb_add_inogrp( void *arg1, intgen_t fsfd
>  				return -1;
>  
>  			/* zero the new portion of the i2gmap */
> -			oldsize = (numsegs - SEGPERHNK) * sizeof(i2gseg_t);
> -
> -			memset(inomap.i2gmap + oldsize,
> -			       0,
> +			memset(&inomap.i2gmap[numsegs - SEGPERHNK], 0,
>  			       SEGPERHNK * sizeof(i2gseg_t));
>  		}
>  
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfsdump: prevent segfault in cb_add_inogrp

[Rich Johnston]

On 08/26/2015 11:42 AM, Eric Sandeen wrote:
> On 8/26/15 9:33 AM, rjohnston@sgi.com wrote:
>> The call to memset will segfault because the offset for the first
>> parameter is done twice. We are using pointer math to do the
>> calculation.
>> The first time is when calculating oldsize, the size of i2gseg_t
>> is accounted for.
>> 	oldsize = (numsegs - SEGPERHNK) * sizeof(i2gseg_t);
>> Then in the call to memset, oldsize is again multiplied by the size
>> of i2gmap_t.
>> 	memset(inomap.i2gmap + oldsize, ...)
>> 	
>> i2gmap holds the used inodes in each chunk. When there are 2^31 chunk
>> entries, it could describe 2^31 (1 inode/chunk)- 2^40 (64 inodes/chunk).
>>
>> With 100s of millions of inodes there are enough entries to wrap the
>> 32 bit variable oldsize.
>>
>> Switching to use array index notation instead of calculating the
>> pointer address twice ;) would resolve this issue. The unneeded
>> local variable oldsize can be removed as well.
>
> Ok, this doesn't explain the type change for numsegs, does it?
Nope that was a typo, as it is an array index I meant to change it from
a signed int (intgen_t) to an unsigned (uint32_t).
>
> Can you walk me through why that's needed?  At what point would
> numsegs = inomap.hnkmaplen * SEGPERHNK; overflow?

>
> Thanks,
> -Eric
>
>> ---
>>   dump/inomap.c |    8 ++------
>>   1 file changed, 2 insertions(+), 6 deletions(-)
>>
>> Index: b/dump/inomap.c
>> ===================================================================
>> --- a/dump/inomap.c
>> +++ b/dump/inomap.c
>> @@ -1125,8 +1125,7 @@ cb_add_inogrp( void *arg1, intgen_t fsfd
>>   		lastsegp->segoff = 0;
>>
>>   		if (lastsegp->hnkoff == inomap.hnkmaplen) {
>> -			intgen_t numsegs;
>> -			intgen_t oldsize;
>> +			int64_t numsegs;

>>
>>   			inomap.hnkmaplen++;
>>   			inomap.hnkmap = (hnk_t *)
>> @@ -1140,10 +1139,7 @@ cb_add_inogrp( void *arg1, intgen_t fsfd
>>   				return -1;
>>
>>   			/* zero the new portion of the i2gmap */
>> -			oldsize = (numsegs - SEGPERHNK) * sizeof(i2gseg_t);
>> -
>> -			memset(inomap.i2gmap + oldsize,
>> -			       0,
>> +			memset(&inomap.i2gmap[numsegs - SEGPERHNK], 0,
>>   			       SEGPERHNK * sizeof(i2gseg_t));
>>   		}
>>
>>
>> _______________________________________________
>> xfs mailing list
>> xfs@oss.sgi.com
>> http://oss.sgi.com/mailman/listinfo/xfs
>>
>

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfsdump: prevent segfault in cb_add_inogrp

[Dave Chinner]

On Wed, Aug 26, 2015 at 12:10:42PM -0500, Rich Johnston wrote:
> On 08/26/2015 11:42 AM, Eric Sandeen wrote:
> >On 8/26/15 9:33 AM, rjohnston@sgi.com wrote:
> >>The call to memset will segfault because the offset for the first
> >>parameter is done twice. We are using pointer math to do the
> >>calculation.
> >>The first time is when calculating oldsize, the size of i2gseg_t
> >>is accounted for.
> >>	oldsize = (numsegs - SEGPERHNK) * sizeof(i2gseg_t);
> >>Then in the call to memset, oldsize is again multiplied by the size
> >>of i2gmap_t.
> >>	memset(inomap.i2gmap + oldsize, ...)
> >>	
> >>i2gmap holds the used inodes in each chunk. When there are 2^31 chunk
> >>entries, it could describe 2^31 (1 inode/chunk)- 2^40 (64 inodes/chunk).
> >>
> >>With 100s of millions of inodes there are enough entries to wrap the
> >>32 bit variable oldsize.
> >>
> >>Switching to use array index notation instead of calculating the
> >>pointer address twice ;) would resolve this issue. The unneeded
> >>local variable oldsize can be removed as well.
> >
> >Ok, this doesn't explain the type change for numsegs, does it?
> Nope that was a typo, as it is an array index I meant to change it from
> a signed int (intgen_t) to an unsigned (uint32_t).

That doesn't fix anything. If you push the index down through zero
by decrementing it too much, it will still result in an array bound
overrun. i.e. It will index array element UINT_MAX rather than -1.
And the compiler still won't catch it because it can't bounds check
runtime calculated index values...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH V2] xfsdump: prevent segfault in cb_add_inogrp

[rjohnston]

[-- Attachment #1: prevent-segfault-in-cb_add_inogrp-V2.patch --]
[-- Type: text/plain, Size: 1855 bytes --]

The call to memset will segfault because the offset for the first
parameter is done twice. We are using pointer math to do the
calculation.
The first time is when calculating oldsize, the size of i2gseg_t
is accounted for.
	oldsize = (numsegs - SEGPERHNK) * sizeof(i2gseg_t);
Then in the call to memset, oldsize is again multiplied by the size
of i2gmap_t.
	memset(inomap.i2gmap + oldsize, ...)
	
i2gmap holds the used inodes in each chunk. When there are 2^31 chunk
entries, it could describe 2^31 (1 inode/chunk)- 2^40 (64 inodes/chunk).

With 100s of millions of inodes there are enough entries to wrap the
32 bit variable oldsize.

Switching to use array index notation instead of calculating the
pointer address twice ;) would resolve this issue. The unneeded
local variable oldsize can be removed as well.

numsegs is used to calculate an array index, change it from a
signed int (intgen_t) to an unsigned (uint32_t).
---
 dump/inomap.c |    8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

Index: b/dump/inomap.c
===================================================================
--- a/dump/inomap.c
+++ b/dump/inomap.c
@@ -1125,8 +1125,7 @@ cb_add_inogrp( void *arg1, intgen_t fsfd
 		lastsegp->segoff = 0;
 
 		if (lastsegp->hnkoff == inomap.hnkmaplen) {
-			intgen_t numsegs;
-			intgen_t oldsize;
+			int32_t numsegs;
 
 			inomap.hnkmaplen++;
 			inomap.hnkmap = (hnk_t *)
@@ -1140,10 +1139,7 @@ cb_add_inogrp( void *arg1, intgen_t fsfd
 				return -1;
 
 			/* zero the new portion of the i2gmap */
-			oldsize = (numsegs - SEGPERHNK) * sizeof(i2gseg_t);
-
-			memset(inomap.i2gmap + oldsize,
-			       0,
+			memset(&inomap.i2gmap[numsegs - SEGPERHNK], 0,
 			       SEGPERHNK * sizeof(i2gseg_t));
 		}
 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH V2] xfsdump: prevent segfault in cb_add_inogrp

[Dave Chinner]

On Wed, Aug 26, 2015 at 12:36:42PM -0500, rjohnston@sgi.com wrote:
> The call to memset will segfault because the offset for the first
> parameter is done twice. We are using pointer math to do the
> calculation.
> The first time is when calculating oldsize, the size of i2gseg_t
> is accounted for.
> 	oldsize = (numsegs - SEGPERHNK) * sizeof(i2gseg_t);
> Then in the call to memset, oldsize is again multiplied by the size
> of i2gmap_t.
> 	memset(inomap.i2gmap + oldsize, ...)
> 	
> i2gmap holds the used inodes in each chunk. When there are 2^31 chunk
> entries, it could describe 2^31 (1 inode/chunk)- 2^40 (64 inodes/chunk).
> 
> With 100s of millions of inodes there are enough entries to wrap the
> 32 bit variable oldsize.
> 
> Switching to use array index notation instead of calculating the
> pointer address twice ;) would resolve this issue. The unneeded
> local variable oldsize can be removed as well.
> 
> numsegs is used to calculate an array index, change it from a
> signed int (intgen_t) to an unsigned (uint32_t).

Description does not match code:

> -			intgen_t numsegs;
> -			intgen_t oldsize;
> +			int32_t numsegs;

It's still a signed int here. And, really, just a plain old "int" is
fine here.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH V2] xfsdump: prevent segfault in cb_add_inogrp

[Rich Johnston]

Dave,

On 08/26/2015 04:37 PM, Dave Chinner wrote:
> On Wed, Aug 26, 2015 at 12:36:42PM -0500, rjohnston@sgi.com wrote:
>> The call to memset will segfault because the offset for the first
>> parameter is done twice. We are using pointer math to do the
>> calculation.
>> The first time is when calculating oldsize, the size of i2gseg_t
>> is accounted for.
>> 	oldsize = (numsegs - SEGPERHNK) * sizeof(i2gseg_t);
>> Then in the call to memset, oldsize is again multiplied by the size
>> of i2gmap_t.
>> 	memset(inomap.i2gmap + oldsize, ...)
>> 	
>> i2gmap holds the used inodes in each chunk. When there are 2^31 chunk
>> entries, it could describe 2^31 (1 inode/chunk)- 2^40 (64 inodes/chunk).
>>
>> With 100s of millions of inodes there are enough entries to wrap the
>> 32 bit variable oldsize.
>>
>> Switching to use array index notation instead of calculating the
>> pointer address twice ;) would resolve this issue. The unneeded
>> local variable oldsize can be removed as well.
>>
Per your other comment I will add a bounds check after calculating numsegs:
	if (numsegs < 0)
		return -1;

The description above will change to:

Adding a bounds check (numsegs < 0) and switching to use array
index notation instead of calculating the pointer address twice ;)
would resolve this issue. The unneeded local variable oldsize
can be removed as well.

>> numsegs is used to calculate an array index, change it from a
>> signed int (intgen_t) to an unsigned (uint32_t).
>
I will remove the above description and leave it as is (intgen_t)
> Description does not match code:
>
>> -			intgen_t numsegs;
>> -			intgen_t oldsize;
>> +			int32_t numsegs;
>
> It's still a signed int here. And, really, just a plain old "int" is
> fine here.
>
> Cheers,
>
> Dave.
>

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH V2] xfsdump: prevent segfault in cb_add_inogrp

[Eric Sandeen]

On 8/26/15 4:57 PM, Rich Johnston wrote:
> Dave,
> 
> On 08/26/2015 04:37 PM, Dave Chinner wrote:
>> On Wed, Aug 26, 2015 at 12:36:42PM -0500, rjohnston@sgi.com wrote:
>>> The call to memset will segfault because the offset for the first
>>> parameter is done twice. We are using pointer math to do the
>>> calculation.
>>> The first time is when calculating oldsize, the size of i2gseg_t
>>> is accounted for.
>>>     oldsize = (numsegs - SEGPERHNK) * sizeof(i2gseg_t);
>>> Then in the call to memset, oldsize is again multiplied by the size
>>> of i2gmap_t.
>>>     memset(inomap.i2gmap + oldsize, ...)
>>>     
>>> i2gmap holds the used inodes in each chunk. When there are 2^31 chunk
>>> entries, it could describe 2^31 (1 inode/chunk)- 2^40 (64 inodes/chunk).
>>>
>>> With 100s of millions of inodes there are enough entries to wrap the
>>> 32 bit variable oldsize.
>>>
>>> Switching to use array index notation instead of calculating the
>>> pointer address twice ;) would resolve this issue. The unneeded
>>> local variable oldsize can be removed as well.
>>>
> Per your other comment I will add a bounds check after calculating numsegs:
>     if (numsegs < 0)
>         return -1;

probably fine, but probably not really necessary.

numsegs = inomap.hnkmaplen * SEGPERHNK;

hnkmaplen initializes as:

inomap.hnkmaplen = (igrpcnt + SEGPERHNK - 1) / SEGPERHNK;

>From my prior email, if I was right,

> so I guess that means that if we have more than 2^31 inode groups,
> i.e. 2^31 * 256 = 500 billion inodes, (int) igrpcnt could overflow.

so unless you have > 500 billion inodes, it's not going to be a problem...

I'd just focus on the single problem at hand (extending a pointer
to an array by number of bytes, not number of elements) and leave it
at that.

-Eric

> The description above will change to:
> 
> Adding a bounds check (numsegs < 0) and switching to use array
> index notation instead of calculating the pointer address twice ;)
> would resolve this issue. The unneeded local variable oldsize
> can be removed as well.
> 
>>> numsegs is used to calculate an array index, change it from a
>>> signed int (intgen_t) to an unsigned (uint32_t).
>>
> I will remove the above description and leave it as is (intgen_t)
>> Description does not match code:
>>
>>> -            intgen_t numsegs;
>>> -            intgen_t oldsize;
>>> +            int32_t numsegs;
>>
>> It's still a signed int here. And, really, just a plain old "int" is
>> fine here.
>>
>> Cheers,
>>
>> Dave.
>>
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH V2] xfsdump: prevent segfault in cb_add_inogrp

[Eric Sandeen]

On 8/26/15 5:19 PM, Eric Sandeen wrote:
> On 8/26/15 4:57 PM, Rich Johnston wrote:
>> Dave,
>>
>> On 08/26/2015 04:37 PM, Dave Chinner wrote:
>>> On Wed, Aug 26, 2015 at 12:36:42PM -0500, rjohnston@sgi.com wrote:
>>>> The call to memset will segfault because the offset for the first
>>>> parameter is done twice. We are using pointer math to do the
>>>> calculation.
>>>> The first time is when calculating oldsize, the size of i2gseg_t
>>>> is accounted for.
>>>>     oldsize = (numsegs - SEGPERHNK) * sizeof(i2gseg_t);
>>>> Then in the call to memset, oldsize is again multiplied by the size
>>>> of i2gmap_t.
>>>>     memset(inomap.i2gmap + oldsize, ...)
>>>>     
>>>> i2gmap holds the used inodes in each chunk. When there are 2^31 chunk
>>>> entries, it could describe 2^31 (1 inode/chunk)- 2^40 (64 inodes/chunk).
>>>>
>>>> With 100s of millions of inodes there are enough entries to wrap the
>>>> 32 bit variable oldsize.
>>>>
>>>> Switching to use array index notation instead of calculating the
>>>> pointer address twice ;) would resolve this issue. The unneeded
>>>> local variable oldsize can be removed as well.
>>>>
>> Per your other comment I will add a bounds check after calculating numsegs:
>>     if (numsegs < 0)
>>         return -1;
> 
> probably fine, but probably not really necessary.
> 
> numsegs = inomap.hnkmaplen * SEGPERHNK;
> 
> hnkmaplen initializes as:
> 
> inomap.hnkmaplen = (igrpcnt + SEGPERHNK - 1) / SEGPERHNK;
> 
> From my prior email, if I was right,
> 
>> so I guess that means that if we have more than 2^31 inode groups,
>> i.e. 2^31 * 256 = 500 billion inodes, (int) igrpcnt could overflow.
> 
> so unless you have > 500 billion inodes, it's not going to be a problem...

sorry, then * SEGPERHNK (511), so overflow around 1 billion inodes.

Yeah, ok, closer to a possibility, maybe worth being defensive after
all.  :)

thanks,
-Eric

> I'd just focus on the single problem at hand (extending a pointer
> to an array by number of bytes, not number of elements) and leave it
> at that.
> 
> -Eric
> 
>> The description above will change to:
>>
>> Adding a bounds check (numsegs < 0) and switching to use array
>> index notation instead of calculating the pointer address twice ;)
>> would resolve this issue. The unneeded local variable oldsize
>> can be removed as well.
>>
>>>> numsegs is used to calculate an array index, change it from a
>>>> signed int (intgen_t) to an unsigned (uint32_t).
>>>
>> I will remove the above description and leave it as is (intgen_t)
>>> Description does not match code:
>>>
>>>> -            intgen_t numsegs;
>>>> -            intgen_t oldsize;
>>>> +            int32_t numsegs;
>>>
>>> It's still a signed int here. And, really, just a plain old "int" is
>>> fine here.
>>>
>>> Cheers,
>>>
>>> Dave.
>>>
>>
>> _______________________________________________
>> xfs mailing list
>> xfs@oss.sgi.com
>> http://oss.sgi.com/mailman/listinfo/xfs
>>
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH V2] xfsdump: prevent segfault in cb_add_inogrp

[Dave Chinner]

On Wed, Aug 26, 2015 at 04:57:37PM -0500, Rich Johnston wrote:
> Dave,
> 
> On 08/26/2015 04:37 PM, Dave Chinner wrote:
> >On Wed, Aug 26, 2015 at 12:36:42PM -0500, rjohnston@sgi.com wrote:
> >>The call to memset will segfault because the offset for the first
> >>parameter is done twice. We are using pointer math to do the
> >>calculation.
> >>The first time is when calculating oldsize, the size of i2gseg_t
> >>is accounted for.
> >>	oldsize = (numsegs - SEGPERHNK) * sizeof(i2gseg_t);
> >>Then in the call to memset, oldsize is again multiplied by the size
> >>of i2gmap_t.
> >>	memset(inomap.i2gmap + oldsize, ...)
> >>	
> >>i2gmap holds the used inodes in each chunk. When there are 2^31 chunk
> >>entries, it could describe 2^31 (1 inode/chunk)- 2^40 (64 inodes/chunk).
> >>
> >>With 100s of millions of inodes there are enough entries to wrap the
> >>32 bit variable oldsize.
> >>
> >>Switching to use array index notation instead of calculating the
> >>pointer address twice ;) would resolve this issue. The unneeded
> >>local variable oldsize can be removed as well.
> >>
> Per your other comment I will add a bounds check after calculating numsegs:
> 	if (numsegs < 0)
> 		return -1;
> 
> The description above will change to:
> 
> Adding a bounds check (numsegs < 0) and switching to use array
> index notation instead of calculating the pointer address twice ;)
> would resolve this issue. The unneeded local variable oldsize
> can be removed as well.

Sounds good. Thanks, Rich.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH V3] xfsdump: prevent segfault in cb_add_inogrp

[rjohnston]

[-- Attachment #1: prevent-segfault-in-cb_add_inogrp-V3.patch --]
[-- Type: text/plain, Size: 2165 bytes --]


The call to memset will segfault because the offset for the first
parameter is done twice. We are using pointer math to do the
calculation.

The first time is when calculating oldsize, the size of i2gseg_t
is accounted for.
	oldsize = (numsegs - SEGPERHNK) * sizeof(i2gseg_t);
Then in the call to memset, oldsize is again multiplied by the size
of i2gmap_t.
	memset(inomap.i2gmap + oldsize, ...)
	
i2gmap holds the used inodes in each chunk. When there are 2^31 chunk
entries, it could describe 2^31 (1 inode/chunk)- 2^40 (64 inodes/chunk).

With 100s of millions of inodes there are enough entries to wrap the
32 bit variable oldsize.

Adding a bounds check (numsegs < 0) and switching to use array
index notation instead of calculating the pointer address twice
would resolve this issue. The unneeded local variable oldsize
can be removed as well.
---
V3:
Per review comments:
	add a bounds check after calculating numsegs:
		if (numsegs < 0)
			return -1;

	Remove the description that does not match the code and
	leave numsegs as is (intgen_t) 

 dump/inomap.c |    8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

Index: b/dump/inomap.c
===================================================================
--- a/dump/inomap.c
+++ b/dump/inomap.c
@@ -1126,13 +1126,14 @@ cb_add_inogrp( void *arg1, intgen_t fsfd
 
 		if (lastsegp->hnkoff == inomap.hnkmaplen) {
 			intgen_t numsegs;
-			intgen_t oldsize;
 
 			inomap.hnkmaplen++;
 			inomap.hnkmap = (hnk_t *)
 				realloc(inomap.hnkmap, inomap.hnkmaplen * HNKSZ);
 
 			numsegs = inomap.hnkmaplen * SEGPERHNK;
+			if (numsegs < 0)
+				return -1;
 			inomap.i2gmap = (i2gseg_t *)
 				realloc(inomap.i2gmap, numsegs * sizeof(i2gseg_t));
 
@@ -1140,10 +1141,7 @@ cb_add_inogrp( void *arg1, intgen_t fsfd
 				return -1;
 
 			/* zero the new portion of the i2gmap */
-			oldsize = (numsegs - SEGPERHNK) * sizeof(i2gseg_t);
-
-			memset(inomap.i2gmap + oldsize,
-			       0,
+			memset(&inomap.i2gmap[numsegs - SEGPERHNK], 0,
 			       SEGPERHNK * sizeof(i2gseg_t));
 		}
 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH V3] xfsdump: prevent segfault in cb_add_inogrp

[Dave Chinner]

On Wed, Aug 26, 2015 at 05:29:55PM -0500, rjohnston@sgi.com wrote:
> 
> The call to memset will segfault because the offset for the first
> parameter is done twice. We are using pointer math to do the
> calculation.
> 
> The first time is when calculating oldsize, the size of i2gseg_t
> is accounted for.
> 	oldsize = (numsegs - SEGPERHNK) * sizeof(i2gseg_t);
> Then in the call to memset, oldsize is again multiplied by the size
> of i2gmap_t.
> 	memset(inomap.i2gmap + oldsize, ...)
> 	
> i2gmap holds the used inodes in each chunk. When there are 2^31 chunk
> entries, it could describe 2^31 (1 inode/chunk)- 2^40 (64 inodes/chunk).
> 
> With 100s of millions of inodes there are enough entries to wrap the
> 32 bit variable oldsize.
> 
> Adding a bounds check (numsegs < 0) and switching to use array
> index notation instead of calculating the pointer address twice
> would resolve this issue. The unneeded local variable oldsize
> can be removed as well.

Can't believe I missed this first two times time through - the patch
is missing your signed-off-by. Just reply with it, and I'll fix it
up on commit. :)

Thanks, Rich!

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH V3] xfsdump: prevent segfault in cb_add_inogrp

[Rich Johnston]

On 08/26/2015 05:56 PM, Dave Chinner wrote:
> On Wed, Aug 26, 2015 at 05:29:55PM -0500, rjohnston@sgi.com wrote:
>>
>> The call to memset will segfault because the offset for the first
>> parameter is done twice. We are using pointer math to do the
>> calculation.
>>
>> The first time is when calculating oldsize, the size of i2gseg_t
>> is accounted for.
>> 	oldsize = (numsegs - SEGPERHNK) * sizeof(i2gseg_t);
>> Then in the call to memset, oldsize is again multiplied by the size
>> of i2gmap_t.
>> 	memset(inomap.i2gmap + oldsize, ...)
>> 	
>> i2gmap holds the used inodes in each chunk. When there are 2^31 chunk
>> entries, it could describe 2^31 (1 inode/chunk)- 2^40 (64 inodes/chunk).
>>
>> With 100s of millions of inodes there are enough entries to wrap the
>> 32 bit variable oldsize.
>>
>> Adding a bounds check (numsegs < 0) and switching to use array
>> index notation instead of calculating the pointer address twice
>> would resolve this issue. The unneeded local variable oldsize
>> can be removed as well.
>
> Can't believe I missed this first two times time through - the patch
> is missing your signed-off-by. Just reply with it, and I'll fix it
> up on commit. :)
Dooh my bad

Signed-off-by: Rich Johnston <rjohnston@sgi.com>
>
> Thanks, Rich!
>
> Cheers,
>
> Dave.
>

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH V3] xfsdump: prevent segfault in cb_add_inogrp

[Rich Johnston]

On 08/26/2015 05:58 PM, Rich Johnston wrote:
> On 08/26/2015 05:56 PM, Dave Chinner wrote:
...
>> Can't believe I missed this first two times time through - the patch
>> is missing your signed-off-by. Just reply with it, and I'll fix it
>> up on commit. :)
> Dooh my bad
>
> Signed-off-by: Rich Johnston <rjohnston@sgi.com>
>>
>> Thanks, Rich!
>>
>> Cheers,
>>
>> Dave.
>>
>
Hey Dave,

Just curious when this patch are going to be committed?

Thanks
--Rich

These are also ready to go.

[PATCH V4] xfsrestore: fix fs uuid order check for incremental restores
http://oss.sgi.com/archives/xfs/2015-09/msg00185.html

[PATCH V2] xfsrestore: fix 2GB directory dump limitation for multi-stream
http://oss.sgi.com/archives/xfs/2015-09/msg00033.html

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH V3] xfsdump: prevent segfault in cb_add_inogrp

[Eric Sandeen]

On 8/26/15 5:29 PM, rjohnston@sgi.com wrote:
> The call to memset will segfault because the offset for the first
> parameter is done twice. We are using pointer math to do the
> calculation.
> 
> The first time is when calculating oldsize, the size of i2gseg_t
> is accounted for.
> 	oldsize = (numsegs - SEGPERHNK) * sizeof(i2gseg_t);
> Then in the call to memset, oldsize is again multiplied by the size
> of i2gmap_t.
> 	memset(inomap.i2gmap + oldsize, ...)
> 	
> i2gmap holds the used inodes in each chunk. When there are 2^31 chunk
> entries, it could describe 2^31 (1 inode/chunk)- 2^40 (64 inodes/chunk).
> 
> With 100s of millions of inodes there are enough entries to wrap the
> 32 bit variable oldsize.
> 
> Adding a bounds check (numsegs < 0) and switching to use array
> index notation instead of calculating the pointer address twice
> would resolve this issue. The unneeded local variable oldsize
> can be removed as well.

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

thanks,
-Eric

> ---
> V3:
> Per review comments:
> 	add a bounds check after calculating numsegs:
> 		if (numsegs < 0)
> 			return -1;
> 
> 	Remove the description that does not match the code and
> 	leave numsegs as is (intgen_t) 
> 
>  dump/inomap.c |    8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> Index: b/dump/inomap.c
> ===================================================================
> --- a/dump/inomap.c
> +++ b/dump/inomap.c
> @@ -1126,13 +1126,14 @@ cb_add_inogrp( void *arg1, intgen_t fsfd
>  
>  		if (lastsegp->hnkoff == inomap.hnkmaplen) {
>  			intgen_t numsegs;
> -			intgen_t oldsize;
>  
>  			inomap.hnkmaplen++;
>  			inomap.hnkmap = (hnk_t *)
>  				realloc(inomap.hnkmap, inomap.hnkmaplen * HNKSZ);
>  
>  			numsegs = inomap.hnkmaplen * SEGPERHNK;
> +			if (numsegs < 0)
> +				return -1;
>  			inomap.i2gmap = (i2gseg_t *)
>  				realloc(inomap.i2gmap, numsegs * sizeof(i2gseg_t));
>  
> @@ -1140,10 +1141,7 @@ cb_add_inogrp( void *arg1, intgen_t fsfd
>  				return -1;
>  
>  			/* zero the new portion of the i2gmap */
> -			oldsize = (numsegs - SEGPERHNK) * sizeof(i2gseg_t);
> -
> -			memset(inomap.i2gmap + oldsize,
> -			       0,
> +			memset(&inomap.i2gmap[numsegs - SEGPERHNK], 0,
>  			       SEGPERHNK * sizeof(i2gseg_t));
>  		}
>  
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs