diff mbox series

selinux,smack: remove the capability checks in the removexattr hooks

Message ID 20240703211134.349950-2-paul@paul-moore.com (mailing list archive)
State Handled Elsewhere
Delegated to: Paul Moore
Headers show
Series selinux,smack: remove the capability checks in the removexattr hooks | expand

Commit Message

Paul Moore July 3, 2024, 9:11 p.m. UTC
Commit 61df7b828204 ("lsm: fixup the inode xattr capability handling")
moved the responsibility of doing the inode xattr capability checking
out of the individual LSMs and into the LSM framework itself.
Unfortunately, while the original commit added the capability checks
to both the setxattr and removexattr code in the LSM framework, it
only removed the setxattr capability checks from the individual LSMs,
leaving duplicated removexattr capability checks in both the SELinux
and Smack code.

This patch removes the duplicated code from SELinux and Smack.

Fixes: 61df7b828204 ("lsm: fixup the inode xattr capability handling")
Signed-off-by: Paul Moore <paul@paul-moore.com>
---
 security/selinux/hooks.c   | 10 ++--------
 security/smack/smack_lsm.c |  3 +--
 2 files changed, 3 insertions(+), 10 deletions(-)

Comments

Paul Moore July 3, 2024, 9:14 p.m. UTC | #1
On Wed, Jul 3, 2024 at 5:11 PM Paul Moore <paul@paul-moore.com> wrote:
>
> Commit 61df7b828204 ("lsm: fixup the inode xattr capability handling")
> moved the responsibility of doing the inode xattr capability checking
> out of the individual LSMs and into the LSM framework itself.
> Unfortunately, while the original commit added the capability checks
> to both the setxattr and removexattr code in the LSM framework, it
> only removed the setxattr capability checks from the individual LSMs,
> leaving duplicated removexattr capability checks in both the SELinux
> and Smack code.
>
> This patch removes the duplicated code from SELinux and Smack.
>
> Fixes: 61df7b828204 ("lsm: fixup the inode xattr capability handling")
> Signed-off-by: Paul Moore <paul@paul-moore.com>
> ---
>  security/selinux/hooks.c   | 10 ++--------
>  security/smack/smack_lsm.c |  3 +--
>  2 files changed, 3 insertions(+), 10 deletions(-)

FYI, this is still untested as my test kernel is compiling now, but I
wanted to get this out onto the list before the holiday in the US for
folks (/me looks at Casey for the Smack bits) to look at and
potentially review.
Casey Schaufler July 3, 2024, 9:55 p.m. UTC | #2
On 7/3/2024 2:14 PM, Paul Moore wrote:
> On Wed, Jul 3, 2024 at 5:11 PM Paul Moore <paul@paul-moore.com> wrote:
>> Commit 61df7b828204 ("lsm: fixup the inode xattr capability handling")
>> moved the responsibility of doing the inode xattr capability checking
>> out of the individual LSMs and into the LSM framework itself.
>> Unfortunately, while the original commit added the capability checks
>> to both the setxattr and removexattr code in the LSM framework, it
>> only removed the setxattr capability checks from the individual LSMs,
>> leaving duplicated removexattr capability checks in both the SELinux
>> and Smack code.
>>
>> This patch removes the duplicated code from SELinux and Smack.
>>
>> Fixes: 61df7b828204 ("lsm: fixup the inode xattr capability handling")
>> Signed-off-by: Paul Moore <paul@paul-moore.com>
>> ---
>>  security/selinux/hooks.c   | 10 ++--------
>>  security/smack/smack_lsm.c |  3 +--
>>  2 files changed, 3 insertions(+), 10 deletions(-)
> FYI, this is still untested as my test kernel is compiling now, but I
> wanted to get this out onto the list before the holiday in the US for
> folks (/me looks at Casey for the Smack bits)

Let me know how your test goes, and then I'll have a closer look.

>  to look at and
> potentially review.
>
Paul Moore July 3, 2024, 11 p.m. UTC | #3
On Wed, Jul 3, 2024 at 5:55 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 7/3/2024 2:14 PM, Paul Moore wrote:
> > On Wed, Jul 3, 2024 at 5:11 PM Paul Moore <paul@paul-moore.com> wrote:
> >> Commit 61df7b828204 ("lsm: fixup the inode xattr capability handling")
> >> moved the responsibility of doing the inode xattr capability checking
> >> out of the individual LSMs and into the LSM framework itself.
> >> Unfortunately, while the original commit added the capability checks
> >> to both the setxattr and removexattr code in the LSM framework, it
> >> only removed the setxattr capability checks from the individual LSMs,
> >> leaving duplicated removexattr capability checks in both the SELinux
> >> and Smack code.
> >>
> >> This patch removes the duplicated code from SELinux and Smack.
> >>
> >> Fixes: 61df7b828204 ("lsm: fixup the inode xattr capability handling")
> >> Signed-off-by: Paul Moore <paul@paul-moore.com>
> >> ---
> >>  security/selinux/hooks.c   | 10 ++--------
> >>  security/smack/smack_lsm.c |  3 +--
> >>  2 files changed, 3 insertions(+), 10 deletions(-)
> > FYI, this is still untested as my test kernel is compiling now, but I
> > wanted to get this out onto the list before the holiday in the US for
> > folks (/me looks at Casey for the Smack bits)
>
> Let me know how your test goes, and then I'll have a closer look.

It looks good - my SELinux test system booted up, appears to be
running normally, and all of the selinux-testsuite tests pass.
Casey Schaufler July 3, 2024, 11:13 p.m. UTC | #4
On 7/3/2024 2:11 PM, Paul Moore wrote:
> Commit 61df7b828204 ("lsm: fixup the inode xattr capability handling")
> moved the responsibility of doing the inode xattr capability checking
> out of the individual LSMs and into the LSM framework itself.
> Unfortunately, while the original commit added the capability checks
> to both the setxattr and removexattr code in the LSM framework, it
> only removed the setxattr capability checks from the individual LSMs,
> leaving duplicated removexattr capability checks in both the SELinux
> and Smack code.
>
> This patch removes the duplicated code from SELinux and Smack.
>
> Fixes: 61df7b828204 ("lsm: fixup the inode xattr capability handling")
> Signed-off-by: Paul Moore <paul@paul-moore.com>

Acked-by: Casey Schaufler <casey@schaufler-ca.com>

> ---
>  security/selinux/hooks.c   | 10 ++--------
>  security/smack/smack_lsm.c |  3 +--
>  2 files changed, 3 insertions(+), 10 deletions(-)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 2daa0961b7f1..c41bf07d4b06 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3356,15 +3356,9 @@ static int selinux_inode_listxattr(struct dentry *dentry)
>  static int selinux_inode_removexattr(struct mnt_idmap *idmap,
>  				     struct dentry *dentry, const char *name)
>  {
> -	if (strcmp(name, XATTR_NAME_SELINUX)) {
> -		int rc = cap_inode_removexattr(idmap, dentry, name);
> -		if (rc)
> -			return rc;
> -
> -		/* Not an attribute we recognize, so just check the
> -		   ordinary setattr permission. */
> +	/* if not a selinux xattr, only check the ordinary setattr perm */
> +	if (strcmp(name, XATTR_NAME_SELINUX))
>  		return dentry_has_perm(current_cred(), dentry, FILE__SETATTR);
> -	}
>  
>  	if (!selinux_initialized())
>  		return 0;
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index a19a94f27766..9f8a8ffb5dde 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -1461,8 +1461,7 @@ static int smack_inode_removexattr(struct mnt_idmap *idmap,
>  	    strcmp(name, XATTR_NAME_SMACKMMAP) == 0) {
>  		if (!smack_privileged(CAP_MAC_ADMIN))
>  			rc = -EPERM;
> -	} else
> -		rc = cap_inode_removexattr(idmap, dentry, name);
> +	}
>  
>  	if (rc != 0)
>  		return rc;
Paul Moore July 5, 2024, 5:11 p.m. UTC | #5
On Wed, Jul 3, 2024 at 7:13 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 7/3/2024 2:11 PM, Paul Moore wrote:
> > Commit 61df7b828204 ("lsm: fixup the inode xattr capability handling")
> > moved the responsibility of doing the inode xattr capability checking
> > out of the individual LSMs and into the LSM framework itself.
> > Unfortunately, while the original commit added the capability checks
> > to both the setxattr and removexattr code in the LSM framework, it
> > only removed the setxattr capability checks from the individual LSMs,
> > leaving duplicated removexattr capability checks in both the SELinux
> > and Smack code.
> >
> > This patch removes the duplicated code from SELinux and Smack.
> >
> > Fixes: 61df7b828204 ("lsm: fixup the inode xattr capability handling")
> > Signed-off-by: Paul Moore <paul@paul-moore.com>
>
> Acked-by: Casey Schaufler <casey@schaufler-ca.com>

Thanks Casey.  As this is a pretty minor fix, I'm going to go ahead
and merge it into lsm/dev so it will go up to Linus during the next
merge window; if anyone has any objections to that please let me know
soon.
diff mbox series

Patch

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 2daa0961b7f1..c41bf07d4b06 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3356,15 +3356,9 @@  static int selinux_inode_listxattr(struct dentry *dentry)
 static int selinux_inode_removexattr(struct mnt_idmap *idmap,
 				     struct dentry *dentry, const char *name)
 {
-	if (strcmp(name, XATTR_NAME_SELINUX)) {
-		int rc = cap_inode_removexattr(idmap, dentry, name);
-		if (rc)
-			return rc;
-
-		/* Not an attribute we recognize, so just check the
-		   ordinary setattr permission. */
+	/* if not a selinux xattr, only check the ordinary setattr perm */
+	if (strcmp(name, XATTR_NAME_SELINUX))
 		return dentry_has_perm(current_cred(), dentry, FILE__SETATTR);
-	}
 
 	if (!selinux_initialized())
 		return 0;
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index a19a94f27766..9f8a8ffb5dde 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -1461,8 +1461,7 @@  static int smack_inode_removexattr(struct mnt_idmap *idmap,
 	    strcmp(name, XATTR_NAME_SMACKMMAP) == 0) {
 		if (!smack_privileged(CAP_MAC_ADMIN))
 			rc = -EPERM;
-	} else
-		rc = cap_inode_removexattr(idmap, dentry, name);
+	}
 
 	if (rc != 0)
 		return rc;