On Thu, Aug 29, 2024 at 7:15 AM Jeff Layton <jlayton@xxxxxxxxxx> wrote: > > On Wed, 2024-08-28 at 15:51 -0400, Scott Mayhew wrote: > > Marek Gresko reports that the root user on an NFS client is able to > > change the security labels on files on an NFS filesystem that is > > exported with root squashing enabled. > > > > The end of the kerneldoc comment for __vfs_setxattr_noperm() states: > > > > * This function requires the caller to lock the inode's i_mutex before it > > * is executed. It also assumes that the caller will make the appropriate > > * permission checks. > > > > nfsd_setattr() does do permissions checking via fh_verify() and > > nfsd_permission(), but those don't do all the same permissions checks > > that are done by security_inode_setxattr() and its related LSM hooks do. > > > > Since nfsd_setattr() is the only consumer of security_inode_setsecctx(), > > simplest solution appears to be to replace the call to > > __vfs_setxattr_noperm() with a call to __vfs_setxattr_locked(). This > > fixes the above issue and has the added benefit of causing nfsd to > > recall conflicting delegations on a file when a client tries to change > > its security label. > > > > Reported-by: Marek Gresko <marek.gresko@xxxxxxxxxxxxxx> > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=218809 > > Signed-off-by: Scott Mayhew <smayhew@xxxxxxxxxx> > > --- > > security/selinux/hooks.c | 4 ++-- > > security/smack/smack_lsm.c | 4 ++-- > > 2 files changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > index bfa61e005aac..400eca4ad0fb 100644 > > --- a/security/selinux/hooks.c > > +++ b/security/selinux/hooks.c > > @@ -6660,8 +6660,8 @@ static int selinux_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen > > */ > > static int selinux_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen) > > { > > - return __vfs_setxattr_noperm(&nop_mnt_idmap, dentry, XATTR_NAME_SELINUX, > > - ctx, ctxlen, 0); > > + return __vfs_setxattr_locked(&nop_mnt_idmap, dentry, XATTR_NAME_SELINUX, > > + ctx, ctxlen, 0, NULL); > > } > > > > static int selinux_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen) > > diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c > > index 4164699cd4f6..002a1b9ed83a 100644 > > --- a/security/smack/smack_lsm.c > > +++ b/security/smack/smack_lsm.c > > @@ -4880,8 +4880,8 @@ static int smack_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen) > > > > static int smack_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen) > > { > > - return __vfs_setxattr_noperm(&nop_mnt_idmap, dentry, XATTR_NAME_SMACK, > > - ctx, ctxlen, 0); > > + return __vfs_setxattr_locked(&nop_mnt_idmap, dentry, XATTR_NAME_SMACK, > > + ctx, ctxlen, 0, NULL); > > } > > > > static int smack_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen) > > Reviewed-by: Jeff Layton <jlayton@xxxxxxxxxx> Reviewed-by: Stephen Smalley <stephen.smalley.work@xxxxxxxxx> Tested-by: Stephen Smalley <stephen.smalley.work@xxxxxxxxx> Passes all the NFS tests in the selinux-testsuite, and also correctly denies root the ability to relabel a file on a root_squash mount but allows a normal user to do so (as expected).
