linux-kernel - Re: [kbuild] [djwong-xfs:djwong-wtf 349/351] fs/xfs/xfs_bmap_util.c:1372 xfs_map_free_extent() warn: missing error code 'error'

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:   Tue, 22 Mar 2022 08:47:26 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     "Darrick J. Wong" <djwong@...nel.org>
Cc:     kbuild@...ts.01.org, lkp@...el.com, kbuild-all@...ts.01.org,
        "Darrick J. Wong" <darrick.wong@...cle.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [kbuild] [djwong-xfs:djwong-wtf 349/351]
 fs/xfs/xfs_bmap_util.c:1372 xfs_map_free_extent() warn: missing error code
 'error'

On Mon, Mar 21, 2022 at 02:59:08PM -0700, Darrick J. Wong wrote:
> On Mon, Mar 21, 2022 at 10:33:02AM +0300, Dan Carpenter wrote:
> > b82670045aab66 Darrick J. Wong 2022-01-06  1365  
> > b82670045aab66 Darrick J. Wong 2022-01-06  1366  	error = xfs_alloc_find_freesp(tp, pag, cursor, end_agbno, &len);
> > b82670045aab66 Darrick J. Wong 2022-01-06  1367  	if (error)
> > b82670045aab66 Darrick J. Wong 2022-01-06  1368  		goto out_cancel;
> > b82670045aab66 Darrick J. Wong 2022-01-06  1369  
> > b82670045aab66 Darrick J. Wong 2022-01-06  1370  	/* Bail out if the cursor is beyond what we asked for. */
> > b82670045aab66 Darrick J. Wong 2022-01-06  1371  	if (*cursor >= end_agbno)
> > b82670045aab66 Darrick J. Wong 2022-01-06 @1372  		goto out_cancel;
> > 
> > This looks like it should have an error = -EINVAL;
> 
> Nope.  If xfs_alloc_find_freesp moves @cursor goes beyond end_agbno, we
> want to exit early so that the xfs_map_free_extent caller will return to
> userspace.
> 
> --D

I'm generally pretty happy with this static checker rule.  Returning
success on a failure path almost always results if something bad like a
NULL deref or a use after free.  But false positives are a real risk
because it's tempting to add an error code to this and introduce a bug.

Smatch will not print the warning if error is set within 4 lines of the
goto.
	error = 0;
	if (*cursor >= end_agbno)
		goto out_cancel;

Another option is that people have started adding comments to these
blocks in response to the checker warning.

Or if you had a different idea about how to silence the checker warning
I can also probably implement that.

regards,
dan carpenter