xfs
[Top] [All Lists]

Re: [PATCH] xfs_metadump: agcount*agblocks overflow

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH] xfs_metadump: agcount*agblocks overflow
From: Eric Sandeen <sandeen@xxxxxxxxxxx>
Date: Thu, 02 Jul 2009 14:56:29 -0500
Cc: xfs mailing list <xfs@xxxxxxxxxxx>
In-reply-to: <20090702192903.GA13919@xxxxxxxxxxxxx>
References: <4A4CE85B.1030102@xxxxxxxxxxx> <20090702192903.GA13919@xxxxxxxxxxxxx>
User-agent: Thunderbird 2.0.0.21 (X11/20090320)
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@xxxxxxxxxx>
>> --
>>
>> 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

<Prev in Thread] Current Thread [Next in Thread>