Re: [PATCH v2] NFSv4.2: fix nfs4_listxattr size accounting

On Fri, Jul 10, 2026 at 5:55 PM Paul Moore <[email protected]> wrote:
>
> On Fri, Jul 10, 2026 at 5:54 PM Paul Moore <[email protected]> wrote:
> > On Thu, Jul 9, 2026 at 8:33 AM Stephen Smalley
> > <[email protected]> wrote:
> > > On Wed, Jul 8, 2026 at 4:11 PM Paul Moore <[email protected]> wrote:
> > > > On Wed, Jul 8, 2026 at 2:54 PM Stephen Smalley
> > > > <[email protected]> wrote:
> > > > > On Tue, Jul 7, 2026 at 4:01 PM Paul Moore <[email protected]> wrote:
> > > > > >
> > > > > > On Tue, Jul 7, 2026 at 3:12 PM Anna Schumaker <[email protected]> wrote:
> > > > > > > On Tue, Jul 7, 2026, at 2:48 PM, Paul Moore wrote:
> > > > > > > > On Tue, Jul 7, 2026 at 11:24 AM Achilles Gaikwad
> > > > > > > > <[email protected]> wrote:
> > > > > > > >>
> > > > > > > >> A call to listxattr() with a buffer size of 0 returns the actual
> > > > > > > >> size of the buffer needed for a subsequent call. On an NFSv4.2
> > > > > > > >> mount this triggers the following oops:
> > > > > > > >>
> > > > > > > >>   [  399.768687] BUG: kernel NULL pointer dereference, address: 0000000000000000
> > > > > > > >>   [  399.768705] RIP: 0010:_copy_from_pages+0x44/0xe0
> > > > > > > >>   [  399.768722] Call Trace:
> > > > > > > >>   [  399.768723]  nfs4_xattr_alloc_entry+0x1bf/0x1e0
> > > > > > > >>   [  399.768730]  nfs4_xattr_cache_set_list+0x43/0x1f0
> > > > > > > >>   [  399.768731]  nfs4_listxattr+0x21f/0x250
> > > > > > > >>   [  399.768733]  vfs_listxattr+0x55/0xa0
> > > > > > > >>   [  399.768736]  listxattr+0x23/0x160
> > > > > > > >>   [  399.768737]  path_listxattrat+0xba/0x1e0
> > > > > > > >>   [  399.768739]  do_syscall_64+0xe2/0x680
> > > > > > > >>
> > > > > > > >> security_inode_listsecurity() (via the xattr_list_one() helper) now
> > > > > > > >> decrements the remaining size even when the buffer pointer is NULL, so
> > > > > > > >> in the size-query case, 'left' underflows to a huge size_t value. As a
> > > > > > > >> result, nfs4_listxattr_nfs4_user() treats the NULL buffer as a real one,
> > > > > > > >> leading to a NULL pointer dereference in _copy_from_pages().
> > > > > > > >>
> > > > > > > >> security_inode_listsecurity() does not return the number of bytes
> > > > > > > >> it added to the list, so the code derived it as
> > > > > > > >> 'size - error - left'. That is also wrong in the size-query case:
> > > > > > > >> the generic_listxattr() contribution is only subtracted from 'left'
> > > > > > > >> when a buffer is present. Thus, the query result comes up short by
> > > > > > > >> exactly that contribution (e.g., "system.nfs4_acl" on a mount with
> > > > > > > >> ACL support), and a caller that allocates the returned size gets
> > > > > > > >> -ERANGE on the subsequent call.
> > > > > > > >>
> > > > > > > >> Declare 'left' as ssize_t, use a scratch copy to measure security
> > > > > > > >> hook consumption, and only decrement 'left' if a buffer is present.
> > > > > > > >>
> > > > > > > >> Fixes: f71ece9712b7 ("security,fs,nfs,net: update security_inode_listsecurity() interface")
> > > > > > > >> Suggested-by: Paul Moore <[email protected]>
> > > > > > > >> Signed-off-by: Achilles Gaikwad <[email protected]>
> > > > > > > >> ---
> > > > > > > >> Changes in v2:
> > > > > > > >>  - Use a scratch variable to track security label size directly,
> > > > > > > >>    replacing the old formula that undercounted the size-query case.
> > > > > > > >>  - Drop the now-unneeded NULL-buffer special case for
> > > > > > > >>    nfs4_listxattr_nfs4_user().
> > > > > > > >>  - Retitled from "fix nfs4_listxattr NULL pointer dereference"
> > > > > > > >>    (the same accounting bug caused both the oops and the undercount).
> > > > > > > >> v1: https://lore.kernel.org/linux-nfs/[email protected]/
> > > > > > > >>  fs/nfs/nfs4proc.c | 10 +++++++---
> > > > > > > >>  1 file changed, 7 insertions(+), 3 deletions(-)
> > > > > > > >
> > > > > > > > [CC'd the LSM and SELinux lists for visibility]
> > > > > > > >
> > > > > > > > Unfortunately my testing was unsuccessful due to an NFS problem that
> > > > > > > > started with the v7.2 merge window that I haven't had the time to
> > > > > > > > bisect yet.  Assuming the NFS folks are okay with this change, I
> > > > > > > > figure they will want to send it up to Linus via their tree, if not
> > > > > > > > let me know and I can send this up via the LSM tree.
> > > > > > >
> > > > > > > Yeah, we'll send it through the NFS tree.
> > > > > >
> > > > > > Thanks Anna.
> > > > > >
> > > > > > > I'll be curious to hear
> > > > > > > what problem you're hitting, and what patch is the culprit once you
> > > > > > > do that bisect!
> > > > > >
> > > > > > Yes, me too :)
> > > > > >
> > > > > > I'm still working through a review backlog so it might be a bit before
> > > > > > I have a chance, but in case anyone wants to test it out, it's easily
> > > > > > reproduced using the selinux-testsuite and the NFS tests:
> > > > > >
> > > > > > https://github.com/SELinuxProject/selinux-testsuite#nfs
> > > > >
> > > > > They seem to pass for me with and without the patch (they don't
> > > > > exercise listxattr AFAIK).
> > > > > This was on the current selinux/dev branch, v7.2-rc1 based.
> > > >
> > > > They work for me on vanilla v7.1 and fail somewhere before vanilla
> > > > v7.2-rc1 (still bisecting).
> > > >
> > > > I wonder if there is an interaction problem with a recent userspace
> > > > update.  What distro/userspace are you running for your tests?  I'm
> > > > doing my testing on a relatively recent Rawhide.
> > >
> > > I was on F44, so yes, it could be a difference in e.g. coreutils or
> > > other userspace on rawhide that is tickling this particular bug.
> >
> > I haven't had a chance to look into this yet, but the git bisect just
> > produced the commit below as the offending commit:
> >
> >    commit 01c2305795a3b6b164df48e72b12022a68fd60c1
> >    Author: Jeff Layton <[email protected]>
> >    Date:   Wed Mar 25 10:40:32 2026 -0400
>
> Ooops, nevermind, scratch that - I still have one more kernel to test.

... and scratch that, the offending commit was that one.

   commit 01c2305795a3b6b164df48e72b12022a68fd60c1
   Author: Jeff Layton <[email protected]>
   Date:   Wed Mar 25 10:40:32 2026 -0400

   nfsd: add netlink upcall for the nfsd.fh cache

   Add netlink-based cache upcall support for the expkey (nfsd.fh) cache,
   following the same pattern as the existing svc_export netlink support.

   Add expkey to the cache-type enum, a new expkey attribute-set with
   client, fsidtype, fsid, negative, expiry, and path fields, and the
   expkey-get-reqs / expkey-set-reqs operations to the nfsd YAML spec
   and generated headers.

   Implement nfsd_nl_expkey_get_reqs_dumpit() which snapshots pending
   expkey cache requests and sends each entry's seqno, client name,
   fsidtype, and fsid over netlink.

   Implement nfsd_nl_expkey_set_reqs_doit() which parses expkey cache
   responses from userspace (client, fsidtype, fsid, expiry, and path
   or negative flag) and updates the cache via svc_expkey_lookup() /
   svc_expkey_update().

   Wire up the expkey_notify() callback in svc_expkey_cache_template
   so cache misses trigger NFSD_CMD_CACHE_NOTIFY multicast events with
   NFSD_CACHE_TYPE_EXPKEY.

   Signed-off-by: Jeff Layton <[email protected]>
   Signed-off-by: Chuck Lever <[email protected]>

-- 
paul-moore.com