linux-kernel - Re: [PATCH] xfs: remove the redundant check in xfs_bmap_first

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 14 Sep 2022 09:42:48 -0700
From:   "Darrick J. Wong" <djwong@...nel.org>
To:     Stephen Zhang <starzhangzsd@...il.com>
Cc:     Dave Chinner <david@...morbit.com>, dchinner@...hat.com,
        chandan.babu@...cle.com, zhangshida@...inos.cn,
        linux-kernel@...r.kernel.org, linux-xfs@...r.kernel.org
Subject: Re: [PATCH] xfs: remove the redundant check in xfs_bmap_first_unused

On Mon, Sep 12, 2022 at 02:39:23PM +0800, Stephen Zhang wrote:
> Dave Chinner <david@...morbit.com> 于2022年9月12日周一 07:12写道：
> > Given that all the types and comparisons involved are 64 bit
> > unsigned:
> >
> > typedef uint64_t        xfs_fileoff_t;  /* block number in a file */
> >
> > #define XFS_FILEOFF_MAX(a,b) max_t(xfs_fileoff_t, (a), (b))
> >
> >         xfs_fileoff_t br_startoff;
> >
> >         xfs_fileoff_t           lastaddr = 0;
> >         xfs_fileoff_t           lowest, max;
> >
> > We end up with the following calculations (in FSBs, not bytes):
> >
> >         lowest + len    = 0x800000ULL + 1
> >                         = 0x800001ULL
> >
> >         got.br_startoff - max   = 0ULL - 0x800000
> >                                 = 0xffffffffff800000ULL
> >
> > and so the existing check is:
> >
> >         if (0 >= 0x800001ULL && 0xffffffffff800000 >= 1)
> >
> > which evaluates as false because the extent that was found is not
> > beyond the initial offset (first_unused) that we need to start
> > searching at.
> >
> > With your modification, this would now evaluate as:
> >
> >         if (0xffffffffff800000 >= 1)
> >
> > Because of the underflow, this would then evaluate as true  and we'd
> > return 0 as the first unused offset. This is incorrect as we do not
> > have a hole at offset 0, nor is it within the correct directory
> > offset segment, nor is it within the search bounds we have
> > specified.
> >
> > If these were all signed types, then your proposed code might be
> > correct. But they are unsigned and hence we have to ensure that we
> > handle overflow/underflow appropriately.
> >
> > Which leads me to ask: did you test this change before you send
> > it to the list?
> >
> 
> I am so sorry about the mistake, and thanks for your elaboration about
> this problem. it indeed teaches me a lesson about the necessity of test
> even for the simplest change.
> 
> By the way, theoretically, in order to solve this, I wonder if we could
> change the code in the following way:
> ====
> xfs_bmap_first_unused(
>                 /*
>                  * See if the hole before this extent will work.
>                  */
> -               if (got.br_startoff >= lowest + len &&
> -                   got.br_startoff - max >= len)
> +               if (got.br_startoff >= max + len)

Er... what problem does this solve?  Is there a defect in this range
checking code?  Why not leave it alone, since that's less retesting for
all of us.

--D

>                         break;
> ====
> 
> Thanks,
> 
> Stephen.