linux-kernel - Re: [PATCH v1] NFS: Fix possible NULL pointer dereference in nfs_inode_remove

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]

Message-ID: <aPiJIBTsQit5jyUg@stanley.mountain>
Date: Wed, 22 Oct 2025 10:34:56 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Trond Myklebust <trondmy@...nel.org>
Cc: liubaolin <liubaolin12138@....com>, anna@...nel.org,
	linux-nfs@...r.kernel.org, linux-kernel@...r.kernel.org,
	Baolin Liu <liubaolin@...inos.cn>
Subject: Re: [PATCH v1] NFS: Fix possible NULL pointer dereference in
 nfs_inode_remove_request()

On Tue, Oct 21, 2025 at 11:15:21PM -0400, Trond Myklebust wrote:
> On Wed, 2025-10-22 at 10:44 +0800, liubaolin wrote:
> > > Sorry, I didn’t actually see any case where req->wb_head == NULL. 
> > > I found this through a smatch warning that pointed out a potential
> > > null pointer dereference. 
> > > Instead of removing the NULL folio check, I prefer to keep it to
> > > prevent this potential issue. Checking pointer validity before use
> > > is a good practice. 
> > > From a maintenance perspective, we can’t rule out the possibility
> > > that future changes might introduce a req->wb_head == NULL case, so
> > > I suggest keeping the NULL folio check.
> > 
> 
> I think you need to look at how smatch works in these situations. It is
> not looking at the call chain, but is rather looking at how the
> function is structured.
> Specifically, as I understand it, smatch looks at whether a test for a
> NULL pointer exists, and whether it is placed before or after the
> pointer is dereferenced. So it has nothing to say about whether the
> check is needed; all it says is that *if* the check is needed, then it
> should be placed differently.
> Dan Carpenter, please correct me if my information above is outdated...

Yes.  That's the gist of it.

However Smatch can tell that the check is not needed then the warning
won't be printed.  In this case, Smatch breaks the return values from
nfs_page_to_folio() down like this, and it thinks folio can be NULL.

fs/nfs/write.c | nfs_page_to_folio | 340 |      0-u64max|        INTERNAL | -1 |                  175 | struct folio*(*)(struct nfs_page*) |
fs/nfs/write.c | nfs_page_to_folio | 340 |      0-u64max|     PARAM_LIMIT |  0 |                    $ |         4096-ptr_max |
fs/nfs/write.c | nfs_page_to_folio | 340 |      0-u64max|   PARAM_COMPARE | -1 |                    $ |      == $0->wb_folio |
fs/nfs/write.c | nfs_page_to_folio | 340 |      0-u64max|        STMT_CNT | -1 |                      |                   22 |
fs/nfs/write.c | nfs_page_to_folio | 341 |  4096-ptr_max|        INTERNAL | -1 |                  175 | struct folio*(*)(struct nfs_page*) |
fs/nfs/write.c | nfs_page_to_folio | 341 |  4096-ptr_max|     PARAM_LIMIT |  0 |                    $ |         4096-ptr_max |
fs/nfs/write.c | nfs_page_to_folio | 341 |  4096-ptr_max|     PARAM_LIMIT |  0 |          $->wb_folio |         4096-ptr_max |
fs/nfs/write.c | nfs_page_to_folio | 341 |  4096-ptr_max|   PARAM_COMPARE | -1 |                    $ |      == $0->wb_folio |
fs/nfs/write.c | nfs_page_to_folio | 341 |  4096-ptr_max|        STMT_CNT | -1 |                      |                   22 |
fs/nfs/write.c | nfs_page_to_folio | 342 |             0|        INTERNAL | -1 |                  176 | struct folio*(*)(struct nfs_page*) |
fs/nfs/write.c | nfs_page_to_folio | 342 |             0|     PARAM_LIMIT |  0 |                    $ |         4096-ptr_max |
fs/nfs/write.c | nfs_page_to_folio | 342 |             0|        STMT_CNT | -1 |                      |                   22 |

But Smatch is taking short cuts in its analysis and it doesn't track
bit tests so it's going to be wrong sometimes.

> 
> So in this case, since we've never seen a case where the NULL check is
> violated, and an analysis of the call chain doesn't show up any
> (remaining) cases where that NULL pointer test is needed, my
> recommendation is that we just remove the test going forward.

Removing the check is probably the correct response to these warnings
more often than not.

regards,
dan carpenter