| 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
| ||
|
Message-ID: <d2877eb6c54ec197e5102aa78dffd2a6a0f3d1cc.camel@kernel.org>
Date: Tue, 21 Oct 2025 23:26:33 -0400
From: Trond Myklebust <trondmy@...nel.org>
To: liubaolin <liubaolin12138@....com>, anna@...nel.org, Dan Carpenter
<dan.carpenter@...aro.org>
Cc: 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, 2025-10-21 at 23:15 -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...
>
> 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.
>
> We should not need to add a "Tested" or "stable" tag, since this test
> is harmless, and so the change is just an optimisation.
Sorry. I meant to say there is no need to add a "Fixes" or a "Cc:
stable" tag...
>
> >
> > 在 2025/10/17 23:02, Trond Myklebust 写道:
> > > On Fri, 2025-10-17 at 14:57 +0800, liubaolin wrote:
> > > > [You don't often get email from liubaolin12138@....com. Learn
> > > > why
> > > > this is important at
> > > > https://aka.ms/LearnAboutSenderIdentification ]
> > > >
> > > > > This modification addresses a potential issue detected by
> > > > > Smatch
> > > > > during a scan of the NFS code. After reviewing the relevant
> > > > > code, I
> > > > > confirmed that the change is required to remove the potential
> > > > > risk.
> > > >
> > > >
> > >
> > > I'm sorry, but I'm still not seeing why we can't just remove the
> > > check
> > > for a NULL folio.
> > >
> > > Under what circumstances do you see us calling
> > > nfs_inode_remove_request() with a request that has req->wb_head
> > > ==
> > > NULL? I'm asking for a concrete example.
> > >
> > > >
> > > > 在 2025/10/13 12:47, Trond Myklebust 写道:
> > > > > On Sun, 2025-10-12 at 16:39 +0800, Baolin Liu wrote:
> > > > > > [You don't often get email from liubaolin12138@....com.
> > > > > > Learn
> > > > > > why
> > > > > > this is important at
> > > > > > https://aka.ms/LearnAboutSenderIdentification ]
> > > > > >
> > > > > > From: Baolin Liu <liubaolin@...inos.cn>
> > > > > >
> > > > > > nfs_page_to_folio(req->wb_head) may return NULL in certain
> > > > > > conditions,
> > > > > > but the function dereferences folio->mapping and calls
> > > > > > folio_end_dropbehind(folio) unconditionally. This may cause
> > > > > > a
> > > > > > NULL
> > > > > > pointer dereference crash.
> > > > > >
> > > > > > Fix this by checking folio before using it or calling
> > > > > > folio_end_dropbehind().
> > > > > >
> > > > > > Signed-off-by: Baolin Liu <liubaolin@...inos.cn>
> > > > > > ---
> > > > > > fs/nfs/write.c | 11 ++++++-----
> > > > > > 1 file changed, 6 insertions(+), 5 deletions(-)
> > > > > >
> > > > > > diff --git a/fs/nfs/write.c b/fs/nfs/write.c
> > > > > > index 0fb6905736d5..e148308c1923 100644
> > > > > > --- a/fs/nfs/write.c
> > > > > > +++ b/fs/nfs/write.c
> > > > > > @@ -739,17 +739,18 @@ static void
> > > > > > nfs_inode_remove_request(struct
> > > > > > nfs_page *req)
> > > > > > nfs_page_group_lock(req);
> > > > > > if (nfs_page_group_sync_on_bit_locked(req,
> > > > > > PG_REMOVE)) {
> > > > > > struct folio *folio =
> > > > > > nfs_page_to_folio(req-
> > > > > > > wb_head);
> > > > > > - struct address_space *mapping = folio-
> > > > > > > mapping;
> > > > > >
> > > > > > - spin_lock(&mapping->i_private_lock);
> > > > > > if (likely(folio)) {
> > > > > > + struct address_space *mapping =
> > > > > > folio-
> > > > > > > mapping;
> > > > > > +
> > > > > > + spin_lock(&mapping-
> > > > > > >i_private_lock);
> > > > > > folio->private = NULL;
> > > > > > folio_clear_private(folio);
> > > > > > clear_bit(PG_MAPPED, &req-
> > > > > > >wb_head-
> > > > > > > wb_flags);
> > > > > > - }
> > > > > > - spin_unlock(&mapping->i_private_lock);
> > > > > > + spin_unlock(&mapping-
> > > > > > > i_private_lock);
> > > > > >
> > > > > > - folio_end_dropbehind(folio);
> > > > > > + folio_end_dropbehind(folio);
> > > > > > + }
> > > > > > }
> > > > > > nfs_page_group_unlock(req);
> > > > > >
> > > > > > --
> > > > > > 2.39.2
> > > > > >
> > > > >
> > > > > What reason is there to believe that we can ever call
> > > > > nfs_inode_remove_request() with a NULL value for req-
> > > > > >wb_head-
> > > > > > wb_folio, or even with a NULL value for req->wb_head-
> > > > > > > wb_folio-
> > > > > > mapping?
> > > > >
> > > > >
> > > >
> > >
Powered by blists - more mailing lists