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
|