linux-kernel - Re: [PATCH v2] security,fs,nfs,net: update security_inode

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] [day] [month] [year] [list]
Message-ID: <CAEjxPJ4T4srp91xsfbVd357Fhwb6Mx_3RGxCHT8Tnk_zk38m+g@mail.gmail.com>
Date: Wed, 3 Dec 2025 15:01:45 -0500
From: Stephen Smalley <stephen.smalley.work@...il.com>
To: Paul Moore <paul@...l-moore.com>
Cc: Anna Schumaker <anna.schumaker@...cle.com>, Trond Myklebust <trondmy@...nel.org>, 
	Anna Schumaker <anna@...nel.org>, Jakub Kicinski <kuba@...nel.org>, 
	Casey Schaufler <casey@...aufler-ca.com>, Alexander Viro <viro@...iv.linux.org.uk>, 
	Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>, James Morris <jmorris@...ei.org>, 
	"Serge E. Hallyn" <serge@...lyn.com>, Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>, 
	Willem de Bruijn <willemb@...gle.com>, "David S. Miller" <davem@...emloft.net>, 
	Simon Horman <horms@...nel.org>, Ondrej Mosnacek <omosnace@...hat.com>, linux-nfs@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org, 
	linux-security-module@...r.kernel.org, netdev@...r.kernel.org, 
	selinux@...r.kernel.org
Subject: Re: [PATCH v2] security,fs,nfs,net: update security_inode_listsecurity()
 interface

On Wed, Dec 3, 2025 at 1:08 PM Stephen Smalley
<stephen.smalley.work@...il.com> wrote:
>
> On Wed, Dec 3, 2025 at 10:55 AM Paul Moore <paul@...l-moore.com> wrote:
> >
> > On Wed, Dec 3, 2025 at 10:35 AM Stephen Smalley
> > <stephen.smalley.work@...il.com> wrote:
> > > On Wed, Jul 23, 2025 at 10:10 PM Paul Moore <paul@...l-moore.com> wrote:
> > > > On Thu, Jun 19, 2025 at 5:18 PM Paul Moore <paul@...l-moore.com> wrote:
> > > > > On Tue, May 27, 2025 at 5:03 PM Anna Schumaker
> > > > > <anna.schumaker@...cle.com> wrote:
> > > > > > On 5/20/25 5:31 PM, Paul Moore wrote:
> > > > > > > On Tue, Apr 29, 2025 at 7:34 PM Paul Moore <paul@...l-moore.com> wrote:
> > > > > > >> On Mon, Apr 28, 2025 at 4:15 PM Stephen Smalley
> > > > > > >> <stephen.smalley.work@...il.com> wrote:
> > > > > > >>>
> > > > > > >>> Update the security_inode_listsecurity() interface to allow
> > > > > > >>> use of the xattr_list_one() helper and update the hook
> > > > > > >>> implementations.
> > > > > > >>>
> > > > > > >>> Link: https://lore.kernel.org/selinux/20250424152822.2719-1-stephen.smalley.work@gmail.com/
> > > > > > >>>
> > > > > > >>> Signed-off-by: Stephen Smalley <stephen.smalley.work@...il.com>
> > > > > > >>> ---
> > > > > > >>> This patch is relative to the one linked above, which in theory is on
> > > > > > >>> vfs.fixes but doesn't appear to have been pushed when I looked.
> > > > > > >>>
> > > > > > >>>  fs/nfs/nfs4proc.c             | 10 ++++++----
> > > > > > >>>  fs/xattr.c                    | 19 +++++++------------
> > > > > > >>>  include/linux/lsm_hook_defs.h |  4 ++--
> > > > > > >>>  include/linux/security.h      |  5 +++--
> > > > > > >>>  net/socket.c                  | 17 +++++++----------
> > > > > > >>>  security/security.c           | 16 ++++++++--------
> > > > > > >>>  security/selinux/hooks.c      | 10 +++-------
> > > > > > >>>  security/smack/smack_lsm.c    | 13 ++++---------
> > > > > > >>>  8 files changed, 40 insertions(+), 54 deletions(-)
> > > > > > >>
> > > > > > >> Thanks Stephen.  Once we get ACKs from the NFS, netdev, and Smack
> > > > > > >> folks I can pull this into the LSM tree.
> > > > > > >
> > > > > > > Gentle ping for Trond, Anna, Jakub, and Casey ... can I get some ACKs
> > > > > > > on this patch?  It's a little late for the upcoming merge window, but
> > > > > > > I'd like to merge this via the LSM tree after the merge window closes.
> > > > > >
> > > > > > For the NFS change:
> > > > > >     Acked-by: Anna Schumaker <anna.schumaker@...cle.com>
> > > > >
> > > > > Hi Anna,
> > > > >
> > > > > Thanks for reviewing the patch.  Unfortunately when merging the patch
> > > > > today and fixing up some merge conflicts I bumped into an odd case in
> > > > > the NFS space and I wanted to check with you on how you would like to
> > > > > resolve it.
> > > > >
> > > > > Commit 243fea134633 ("NFSv4.2: fix listxattr to return selinux
> > > > > security label")[1] adds a direct call to
> > > > > security_inode_listsecurity() in nfs4_listxattr(), despite the
> > > > > existing nfs4_listxattr_nfs4_label() call which calls into the same
> > > > > LSM hook, although that call is conditional on the server supporting
> > > > > NFS_CAP_SECURITY_LABEL.  Based on a quick search, it appears the only
> > > > > caller for nfs4_listxattr_nfs4_label() is nfs4_listxattr() so I'm
> > > > > wondering if there isn't some room for improvement here.
> > > > >
> > > > > I think there are two obvious options, and I'm curious about your
> > > > > thoughts on which of these you would prefer, or if there is another
> > > > > third option that you would like to see merged.
> > > > >
> > > > > Option #1:
> > > > > Essentially back out commit 243fea134633, removing the direct LSM call
> > > > > in nfs4_listxattr() and relying on the nfs4_listxattr_nfs4_label() for
> > > > > the LSM/SELinux xattrs.  I think we would want to remove the
> > > > > NFS_CAP_SECURITY_LABEL check and build nfs4_listxattr_nfs4_label()
> > > > > regardless of CONFIG_NFS_V4_SECURITY_LABEL.
> > > > >
> > > > > Option #2:
> > > > > Remove nfs4_listxattr_nfs4_label() entirely and keep the direct LSM
> > > > > call in nfs4_listxattr(), with the required changes for this patch.
> > > > >
> > > > > Thoughts?
> > > > >
> > > > > [1] https://lore.kernel.org/all/20250425180921.86702-1-okorniev@redhat.com/
> > > >
> > > > A gentle ping on the question above for the NFS folks.  If I don't
> > > > hear anything I'll hack up something and send it out for review, but I
> > > > thought it would nice if we could sort out the proper fix first.
> > >
> > > Raising this thread back up again to see if the NFS folks have a
> > > preference on option #1 or #2 above, or
> > > something else altogether. Should returning of the security.selinux
> > > xattr name from listxattr() be dependent on
> > > NFS_CAP_SECURITY_LABEL being set by the server and should it be
> > > dependent on CONFIG_NFS_V4_SECURITY_LABEL?
> >
> > Thanks for bringing this back up Stephen, it would be good to get this resolved.
>
> On second look, I realized that commit 243fea134633 ("NFSv4.2: fix
> listxattr to return selinux security label") was likely motivated by
> the same issue as commit 8b0ba61df5a1c44e2b3cf6 ("fs/xattr.c: fix
> simple_xattr_list to always include security.* xattrs"), i.e. the
> coreutils change that switched ls -Z from unconditionally calling
> getxattr("security.selinux") (via libselinux getfilecon(3)) to only
> doing so if listxattr() returns the "security.selinux" xattr name.
> Hence, we want the call to security_inode_listsecurity() to be
> unconditional, which favors option #2. My only residual question
> though is that commit 243fea134633 put the call _after_ fetching the
> user.* xattr names, whereas the nfs4_listxattr_nfs4_label() returns it
> _before_ any user.* xattrs are appended. I'd be inclined to move up
> the security_inode_listsecurity() call to replace the
> nfs4_listxattr_nfs4_label() call along with option #2.

I've made an attempt to unify the two security_inode_listsecurity()
hook calls in the nfs4 code into a single, unconditional call from
nfs4_listxattr(), which can be found here:
https://lore.kernel.org/selinux/20251203195728.8592-1-stephen.smalley.work@gmail.com/T/#u

If this is deemed acceptable by the NFS folks, then I can re-base this
patch on top of that one.