[PATCH] xfs_metadump: agcount*agblocks overflow

[PATCH] xfs_metadump: agcount*agblocks overflow

[Eric Sandeen]

Found another potential overflow in xfs_metadump,
similar to those just fixed in repair.

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

diff --git a/db/metadump.c b/db/metadump.c
index 19aed4f..ef6e571 100644
--- a/db/metadump.c
+++ b/db/metadump.c
@@ -222,7 +222,8 @@ valid_bno(
 		return 1;
 	if (agno == (mp->m_sb.sb_agcount - 1) && agbno > 0 &&
 			agbno <= (mp->m_sb.sb_dblocks -
-			 (mp->m_sb.sb_agcount - 1) * mp->m_sb.sb_agblocks))
+			 (xfs_drfsbno_t)(mp->m_sb.sb_agcount - 1) *
+			 mp->m_sb.sb_agblocks))
 		return 1;
 
 	return 0;

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

Re: [PATCH] xfs_metadump: agcount*agblocks overflow

[Christoph Hellwig]

On Thu, Jul 02, 2009 at 12:03:23PM -0500, Eric Sandeen wrote:
> Found another potential overflow in xfs_metadump,
> similar to those just fixed in repair.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> --
> 
> diff --git a/db/metadump.c b/db/metadump.c
> index 19aed4f..ef6e571 100644
> --- a/db/metadump.c
> +++ b/db/metadump.c
> @@ -222,7 +222,8 @@ valid_bno(
>  		return 1;
>  	if (agno == (mp->m_sb.sb_agcount - 1) && agbno > 0 &&
>  			agbno <= (mp->m_sb.sb_dblocks -
> -			 (mp->m_sb.sb_agcount - 1) * mp->m_sb.sb_agblocks))
> +			 (xfs_drfsbno_t)(mp->m_sb.sb_agcount - 1) *
> +			 mp->m_sb.sb_agblocks))
>  		return 1;
>  
>  	return 0;

I have a really hard time reading the function (both before and after
your patch).  It's a real mess and no wonder we have these overflow
problems here.  What about the following instead:

static int
valid_bno(
	xfs_agnumber_t		agno,
	xfs_agblock_t		agbno)
{
	xfs_agnumber_t		last_agno = mp->m_sb.sb_agcount - 1;
	xfs_drfsbno_t		nblocks;

	/*
	 * The first block in every AG contains a backups superblock,
	 * and is copied separately, and we can skip it early as an
	 * optimization.
	 */
	if (agbno == 0)
		return 0;

	/*
	 * An invalid AG number is never okay.
	 */
	if (agno > last_agno)
		return 0;

	if (agno == last_agno) {
		nblocks = mp->m_sb.sb_dblocks -
			((xfs_drfsbno_t)mp->m_sb.sb_agblocks *
			 (mp->m_sb.sb_agcount - 1));
	} else {
		nblocks = mp->m_sb.sb_agblocks);
	}
	
	if (agbno > nblocks)
		return 0;
	return 1;
}

and with that form I wonder if we don't still have an off-by-one
in the last if clause - shouldn't the agblocks be the count of blocks
while agbno is an indes and thus 0-based?

Btw, do you have a testcase for this?

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

Re: [PATCH] xfs_metadump: agcount*agblocks overflow

[Eric Sandeen]

Christoph Hellwig wrote:
> On Thu, Jul 02, 2009 at 12:03:23PM -0500, Eric Sandeen wrote:
>> Found another potential overflow in xfs_metadump,
>> similar to those just fixed in repair.
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> --
>>
>> diff --git a/db/metadump.c b/db/metadump.c
>> index 19aed4f..ef6e571 100644
>> --- a/db/metadump.c
>> +++ b/db/metadump.c
>> @@ -222,7 +222,8 @@ valid_bno(
>>  		return 1;
>>  	if (agno == (mp->m_sb.sb_agcount - 1) && agbno > 0 &&
>>  			agbno <= (mp->m_sb.sb_dblocks -
>> -			 (mp->m_sb.sb_agcount - 1) * mp->m_sb.sb_agblocks))
>> +			 (xfs_drfsbno_t)(mp->m_sb.sb_agcount - 1) *
>> +			 mp->m_sb.sb_agblocks))
>>  		return 1;
>>  
>>  	return 0;
> 
> I have a really hard time reading the function (both before and after
> your patch).  It's a real mess and no wonder we have these overflow
> problems here.  What about the following instead:

well, I think the original goal was to make it efficient for the common
case.  How muchthis matters, not really sure.  (at least that's the
comment in the xfs_repair counterpart)

> static int
> valid_bno(
> 	xfs_agnumber_t		agno,
> 	xfs_agblock_t		agbno)
> {
> 	xfs_agnumber_t		last_agno = mp->m_sb.sb_agcount - 1;
> 	xfs_drfsbno_t		nblocks;
> 
> 	/*
> 	 * The first block in every AG contains a backups superblock,
> 	 * and is copied separately, and we can skip it early as an
> 	 * optimization.
> 	 */
> 	if (agbno == 0)
> 		return 0;
> 
> 	/*
> 	 * An invalid AG number is never okay.
> 	 */
> 	if (agno > last_agno)
> 		return 0;
> 
> 	if (agno == last_agno) {
> 		nblocks = mp->m_sb.sb_dblocks -
> 			((xfs_drfsbno_t)mp->m_sb.sb_agblocks *
> 			 (mp->m_sb.sb_agcount - 1));
> 	} else {
> 		nblocks = mp->m_sb.sb_agblocks);
> 	}
> 	
> 	if (agbno > nblocks)
> 		return 0;
> 	return 1;
> }
> 
> and with that form I wonder if we don't still have an off-by-one
> in the last if clause - shouldn't the agblocks be the count of blocks
> while agbno is an indes and thus 0-based?
> 
> Btw, do you have a testcase for this?

no testcase here, though could craft one.  I discovered it on Jesse's
corruptd fs (very big metadump image)

-Eric

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