diff mbox series

security: fix no-op hook logic in security_inode_{set,remove}xattr()

Message ID 20240129133058.1627971-1-omosnace@redhat.com (mailing list archive)
State Handled Elsewhere
Delegated to: Paul Moore
Headers show
Series security: fix no-op hook logic in security_inode_{set,remove}xattr() | expand

Commit Message

Ondrej Mosnacek Jan. 29, 2024, 1:30 p.m. UTC
These two hooks currently work like this:
1. If no LSM registers the hook, cap_inode_{set,remove}xattr() is
   called.
2. If an LSM hook call returns 0, the loop continues to call further
   LSMs (and only stops on an error return value).
3. The "default" return value is 0, so e.g. the default BPF LSM hook
   just returns 0.

This works if BPF LSM is enabled along with SELinux or SMACK (or not
enabled at all), but if it's the only LSM implementing the hook, then
the cap_inode_{set,remove}xattr() is erroneously skipped.

Fix this by using 1 as the default return value and make the loop
recognize it as a no-op return value (i.e. if an LSM returns this value
it is treated as if it wasn't called at all). The final logic is similar
to that of security_fs_context_parse_param().

Fixes: 98e828a0650f ("security: Refactor declaration of LSM hooks")
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 include/linux/lsm_hook_defs.h |  4 ++--
 security/security.c           | 45 +++++++++++++++++++++++++----------
 2 files changed, 35 insertions(+), 14 deletions(-)

Comments

Paul Moore Jan. 31, 2024, 12:09 a.m. UTC | #1
On Mon, Jan 29, 2024 at 8:31 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> These two hooks currently work like this:
> 1. If no LSM registers the hook, cap_inode_{set,remove}xattr() is
>    called.
> 2. If an LSM hook call returns 0, the loop continues to call further
>    LSMs (and only stops on an error return value).
> 3. The "default" return value is 0, so e.g. the default BPF LSM hook
>    just returns 0.
>
> This works if BPF LSM is enabled along with SELinux or SMACK (or not
> enabled at all), but if it's the only LSM implementing the hook, then
> the cap_inode_{set,remove}xattr() is erroneously skipped.
>
> Fix this by using 1 as the default return value and make the loop
> recognize it as a no-op return value (i.e. if an LSM returns this value
> it is treated as if it wasn't called at all). The final logic is similar
> to that of security_fs_context_parse_param().
>
> Fixes: 98e828a0650f ("security: Refactor declaration of LSM hooks")
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
>  include/linux/lsm_hook_defs.h |  4 ++--
>  security/security.c           | 45 +++++++++++++++++++++++++----------
>  2 files changed, 35 insertions(+), 14 deletions(-)

Thanks for working on this Ondrej, I've got a couple of thoughts on
the approach taken here, but we definitely need to fix this.

My first thought is that we really should move the
cap_inode_setxattr() and cap_inode_removexattr() calls in security.c
over to using the LSM hook infrastructure just as we do with other
capability hooks in commoncap.c:

  LSM_HOOK_INIT(inode_setxattr, cap_inode_setxattr);
  LSM_HOOK_INIT(inode_removexattr, cap_inode_removexattr);

... of course we will need to adjust cap_inode_setxattr to take (and
ignore the idmap) parameter, but that is easy enough.  It looks like
cap_inode_removexattr() can be used as-is.  Modifications to the only
two LSMs, SELinux and Smack, which explicitly call out to these
capability hooks looks rather straightforward as well.  Doing this
should simplify the LSM hooks significantly, and lower the chance of a
future LSM mistakenly not doing the required capability calls.  There
should also be a slight performance bump for the few (one? two?)
people running both SELinux and Smack in a production environment.

My second thought is that we *really* need to add to the function
header block comment/description for both these hooks.  Of course the
details here will change depending on the bits above about the
capability hooks, but if we need any special handling like you're
proposing here we really should document it in the hook's header
block.
Paul Moore Jan. 31, 2024, 12:33 a.m. UTC | #2
On Tue, Jan 30, 2024 at 7:09 PM Paul Moore <paul@paul-moore.com> wrote:
> On Mon, Jan 29, 2024 at 8:31 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> >
> > These two hooks currently work like this:
> > 1. If no LSM registers the hook, cap_inode_{set,remove}xattr() is
> >    called.
> > 2. If an LSM hook call returns 0, the loop continues to call further
> >    LSMs (and only stops on an error return value).
> > 3. The "default" return value is 0, so e.g. the default BPF LSM hook
> >    just returns 0.
> >
> > This works if BPF LSM is enabled along with SELinux or SMACK (or not
> > enabled at all), but if it's the only LSM implementing the hook, then
> > the cap_inode_{set,remove}xattr() is erroneously skipped.
> >
> > Fix this by using 1 as the default return value and make the loop
> > recognize it as a no-op return value (i.e. if an LSM returns this value
> > it is treated as if it wasn't called at all). The final logic is similar
> > to that of security_fs_context_parse_param().
> >
> > Fixes: 98e828a0650f ("security: Refactor declaration of LSM hooks")
> > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > ---
> >  include/linux/lsm_hook_defs.h |  4 ++--
> >  security/security.c           | 45 +++++++++++++++++++++++++----------
> >  2 files changed, 35 insertions(+), 14 deletions(-)
>
> Thanks for working on this Ondrej, I've got a couple of thoughts on
> the approach taken here, but we definitely need to fix this.
>
> My first thought is that we really should move the
> cap_inode_setxattr() and cap_inode_removexattr() calls in security.c
> over to using the LSM hook infrastructure just as we do with other
> capability hooks in commoncap.c:
>
>   LSM_HOOK_INIT(inode_setxattr, cap_inode_setxattr);
>   LSM_HOOK_INIT(inode_removexattr, cap_inode_removexattr);
>
> ... of course we will need to adjust cap_inode_setxattr to take (and
> ignore the idmap) parameter, but that is easy enough.  It looks like
> cap_inode_removexattr() can be used as-is.  Modifications to the only
> two LSMs, SELinux and Smack, which explicitly call out to these
> capability hooks looks rather straightforward as well.  Doing this
> should simplify the LSM hooks significantly, and lower the chance of a
> future LSM mistakenly not doing the required capability calls.  There
> should also be a slight performance bump for the few (one? two?)
> people running both SELinux and Smack in a production environment.
>
> My second thought is that we *really* need to add to the function
> header block comment/description for both these hooks.  Of course the
> details here will change depending on the bits above about the
> capability hooks, but if we need any special handling like you're
> proposing here we really should document it in the hook's header
> block.

A completely untested, other than compiling security/, patch is below
demonstrating what I was thinking.  I've also attached the same patch
in case anyone wants to actually try it out as the cut-n-paste version
below is surely whitespace damaged.  I will warn you that this was
hastily thrown together so it is very likely I screwed something up :)

diff --git a/include/linux/security.h b/include/linux/security.h
index d0eb20f90b26..2d3c0af33b65 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -156,7 +156,8 @@ extern int cap_capset(struct cred *new, const
struct cred *old,
        const kernel_cap_t *inheritable,
        const kernel_cap_t *permitted);
 extern int cap_bprm_creds_from_file(struct linux_binprm *bprm, const
struct file *file);
-int cap_inode_setxattr(struct dentry *dentry, const char *name,
+int cap_inode_setxattr(struct mnt_idmap *idmap,
+        struct dentry *dentry, const char *name,
         const void *value, size_t size, int flags);
 int cap_inode_removexattr(struct mnt_idmap *idmap,
    struct dentry *dentry, const char *name);
@@ -888,7 +889,7 @@ static inline int security_inode_setxattr(struct
mnt_idmap *idmap,
  struct dentry *dentry, const char *name, const void *value,
  size_t size, int flags)
 {
- return cap_inode_setxattr(dentry, name, value, size, flags);
+ return cap_inode_setxattr(idmap, dentry, name, value, size, flags);
 }

 static inline int security_inode_set_acl(struct mnt_idmap *idmap,
diff --git a/security/commoncap.c b/security/commoncap.c
index 162d96b3a676..34caf0e19b2f 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -975,6 +975,7 @@ int cap_bprm_creds_from_file(struct linux_binprm
*bprm, const struct file *file)

 /**
  * cap_inode_setxattr - Determine whether an xattr may be altered
+ * @idmap: idmap of the mount
  * @dentry: The inode/dentry being altered
  * @name: The name of the xattr to be changed
  * @value: The value that the xattr will be changed to
@@ -987,7 +988,8 @@ int cap_bprm_creds_from_file(struct linux_binprm
*bprm, const struct file *file)
  * This is used to make sure security xattrs don't get updated or set by those
  * who aren't privileged to do so.
  */
-int cap_inode_setxattr(struct dentry *dentry, const char *name,
+int cap_inode_setxattr(struct mnt_idmap *idmap,
+        struct dentry *dentry, const char *name,
         const void *value, size_t size, int flags)
 {
  struct user_namespace *user_ns = dentry->d_sb->s_user_ns;
@@ -1457,6 +1459,8 @@ static struct security_hook_list
capability_hooks[] __ro_after_init = {
  LSM_HOOK_INIT(inode_need_killpriv, cap_inode_need_killpriv),
  LSM_HOOK_INIT(inode_killpriv, cap_inode_killpriv),
  LSM_HOOK_INIT(inode_getsecurity, cap_inode_getsecurity),
+ LSM_HOOK_INIT(inode_setxattr, cap_inode_setxattr),
+ LSM_HOOK_INIT(inode_removexattr, cap_inode_removexattr),
  LSM_HOOK_INIT(mmap_addr, cap_mmap_addr),
  LSM_HOOK_INIT(mmap_file, cap_mmap_file),
  LSM_HOOK_INIT(task_fix_setuid, cap_task_fix_setuid),
diff --git a/security/security.c b/security/security.c
index 3aaad75c9ce8..6425d177b301 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2258,15 +2258,9 @@ int security_inode_setxattr(struct mnt_idmap *idmap,

  if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
  return 0;
- /*
- * SELinux and Smack integrate the cap call,
- * so assume that all LSMs supplying this call do so.
- */
- ret = call_int_hook(inode_setxattr, 1, idmap, dentry, name, value,
-     size, flags);

- if (ret == 1)
- ret = cap_inode_setxattr(dentry, name, value, size, flags);
+ ret = call_int_hook(inode_setxattr, 0, idmap, dentry, name, value,
+     size, flags);
  if (ret)
  return ret;
  ret = ima_inode_setxattr(dentry, name, value, size);
@@ -2421,13 +2415,8 @@ int security_inode_removexattr(struct mnt_idmap *idmap,

  if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
  return 0;
- /*
- * SELinux and Smack integrate the cap call,
- * so assume that all LSMs supplying this call do so.
- */
- ret = call_int_hook(inode_removexattr, 1, idmap, dentry, name);
- if (ret == 1)
- ret = cap_inode_removexattr(idmap, dentry, name);
+
+ ret = call_int_hook(inode_removexattr, 0, idmap, dentry, name);
  if (ret)
  return ret;
  ret = ima_inode_removexattr(dentry, name);
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index a6bf90ace84c..49cb331a0d84 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3193,10 +3193,6 @@ static int selinux_inode_setxattr(struct
mnt_idmap *idmap,
  int rc = 0;

  if (strcmp(name, XATTR_NAME_SELINUX)) {
- rc = cap_inode_setxattr(dentry, name, value, size, flags);
- if (rc)
- return rc;
-
  /* Not an attribute we recognize, so just check the
     ordinary setattr permission. */
  return dentry_has_perm(current_cred(), dentry, FILE__SETATTR);
@@ -3350,10 +3346,6 @@ 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. */
  return dentry_has_perm(current_cred(), dentry, FILE__SETATTR);
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 0fdbf04cc258..34b74e442412 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -1317,8 +1317,7 @@ static int smack_inode_setxattr(struct mnt_idmap *idmap,
  if (size != TRANS_TRUE_SIZE ||
      strncmp(value, TRANS_TRUE, TRANS_TRUE_SIZE) != 0)
  rc = -EINVAL;
- } else
- rc = cap_inode_setxattr(dentry, name, value, size, flags);
+ }

  if (check_priv && !smack_privileged(CAP_MAC_ADMIN))
  rc = -EPERM;
@@ -1426,12 +1425,8 @@ static int smack_inode_removexattr(struct
mnt_idmap *idmap,
      strcmp(name, XATTR_NAME_SMACKTRANSMUTE) == 0 ||
      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;
+ return -EPERM;
+ }

  smk_ad_init(&ad, __func__, LSM_AUDIT_DATA_DENTRY);
  smk_ad_setfield_u_fs_path_dentry(&ad, dentry);
Paul Moore Jan. 31, 2024, 2:19 a.m. UTC | #3
On Tue, Jan 30, 2024 at 7:33 PM Paul Moore <paul@paul-moore.com> wrote:
> On Tue, Jan 30, 2024 at 7:09 PM Paul Moore <paul@paul-moore.com> wrote:
> > On Mon, Jan 29, 2024 at 8:31 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > >
> > > These two hooks currently work like this:
> > > 1. If no LSM registers the hook, cap_inode_{set,remove}xattr() is
> > >    called.
> > > 2. If an LSM hook call returns 0, the loop continues to call further
> > >    LSMs (and only stops on an error return value).
> > > 3. The "default" return value is 0, so e.g. the default BPF LSM hook
> > >    just returns 0.
> > >
> > > This works if BPF LSM is enabled along with SELinux or SMACK (or not
> > > enabled at all), but if it's the only LSM implementing the hook, then
> > > the cap_inode_{set,remove}xattr() is erroneously skipped.
> > >
> > > Fix this by using 1 as the default return value and make the loop
> > > recognize it as a no-op return value (i.e. if an LSM returns this value
> > > it is treated as if it wasn't called at all). The final logic is similar
> > > to that of security_fs_context_parse_param().
> > >
> > > Fixes: 98e828a0650f ("security: Refactor declaration of LSM hooks")
> > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > > ---
> > >  include/linux/lsm_hook_defs.h |  4 ++--
> > >  security/security.c           | 45 +++++++++++++++++++++++++----------
> > >  2 files changed, 35 insertions(+), 14 deletions(-)
> >
> > Thanks for working on this Ondrej, I've got a couple of thoughts on
> > the approach taken here, but we definitely need to fix this.
> >
> > My first thought is that we really should move the
> > cap_inode_setxattr() and cap_inode_removexattr() calls in security.c
> > over to using the LSM hook infrastructure just as we do with other
> > capability hooks in commoncap.c:
> >
> >   LSM_HOOK_INIT(inode_setxattr, cap_inode_setxattr);
> >   LSM_HOOK_INIT(inode_removexattr, cap_inode_removexattr);
> >
> > ... of course we will need to adjust cap_inode_setxattr to take (and
> > ignore the idmap) parameter, but that is easy enough.  It looks like
> > cap_inode_removexattr() can be used as-is.  Modifications to the only
> > two LSMs, SELinux and Smack, which explicitly call out to these
> > capability hooks looks rather straightforward as well.  Doing this
> > should simplify the LSM hooks significantly, and lower the chance of a
> > future LSM mistakenly not doing the required capability calls.  There
> > should also be a slight performance bump for the few (one? two?)
> > people running both SELinux and Smack in a production environment.
> >
> > My second thought is that we *really* need to add to the function
> > header block comment/description for both these hooks.  Of course the
> > details here will change depending on the bits above about the
> > capability hooks, but if we need any special handling like you're
> > proposing here we really should document it in the hook's header
> > block.
>
> A completely untested, other than compiling security/, patch is below
> demonstrating what I was thinking ...

... I built a kernel and did a quick test that failed spectacularly :)

Without looking too closely, I'm guessing I forgot to take into
account that SELinux and Smack don't normally apply the capability
checks if it is one of their xattrs, and installing the capability
checks as hooks means they are always checked regardless of the other
LSMs.

Bummer.

I'll come back to this tomorrow with some fresh eyes.
Paul Moore Feb. 1, 2024, 11:52 p.m. UTC | #4
On Tue, Jan 30, 2024 at 9:19 PM Paul Moore <paul@paul-moore.com> wrote:
>
> I'll come back to this tomorrow with some fresh eyes.

My apologies, "tomorrow" turned into "the day after tomorrow" (as it
often does) ...

I've been struggling with the idea that there are individual LSMs
still calling into the capability hooks instead of leveraging the LSM
stacking infrastructure, and the "magic" involved to make it all work.
While your patch looks like it should restore proper behavior - that's
good! - I keep thinking that we can, and should, do better.

The only thing that I coming up with is to create two new LSM hooks,
in addition to the existing 'inode_setxattr' hook.  The new LSM hooks
would be 'inode_setxattr_owned' and 'inode_setxattr_cap'.  The _owned
hook would simply check the xattr name and return a positive value if
the LSM "owned" the xattr, e.g. XATTR_NAME_SELINUX for SELinux, and
zero otherwise.  The _cap hook would only be used by the capabilities
code (or something similar), and would match up with
cap_inode_setxattr().  With these two new hooks I think we could do
something like this:

int security_inode_setxattr(...)
{
  owned = false
  hook_loop(inode_setxattr_owned) {
    trc = hook->inode_setxattr_owned(name);
    if (trc > 0) {
      owned = true;
      break;
    }
  }
  if (owned) {
    hook_loop(inode_setxattr) {
      /* run the existing inode_setxattr hooks, e.g. SELinux and Smack */
    }
  } else {
    hook_loop(inode_setxattr_cap) {
      /* run the capability setxattr hooks, e.g. commoncap.c */
    }
  }
}

... with security_inode_removexattr() following a similar pattern.

I will admit that there is some duplication in having to check the
xattr twice (once in _owned, again in inode_setxattr), and the
multiple hook approach is less than ideal, but this seems much less
fragile to me.

Thoughts?
Casey Schaufler Feb. 2, 2024, 12:10 a.m. UTC | #5
On 2/1/2024 3:52 PM, Paul Moore wrote:
> On Tue, Jan 30, 2024 at 9:19 PM Paul Moore <paul@paul-moore.com> wrote:
>> I'll come back to this tomorrow with some fresh eyes.
> My apologies, "tomorrow" turned into "the day after tomorrow" (as it
> often does) ...
>
> I've been struggling with the idea that there are individual LSMs
> still calling into the capability hooks instead of leveraging the LSM
> stacking infrastructure, and the "magic" involved to make it all work.
> While your patch looks like it should restore proper behavior - that's
> good! - I keep thinking that we can, and should, do better.

Apology for attaching a patch rather than inlining it.
I've attached patch #38 from the current stacking set.
It addresses the issue.

>
> The only thing that I coming up with is to create two new LSM hooks,
> in addition to the existing 'inode_setxattr' hook.  The new LSM hooks
> would be 'inode_setxattr_owned' and 'inode_setxattr_cap'.  The _owned
> hook would simply check the xattr name and return a positive value if
> the LSM "owned" the xattr, e.g. XATTR_NAME_SELINUX for SELinux, and
> zero otherwise.  The _cap hook would only be used by the capabilities
> code (or something similar), and would match up with
> cap_inode_setxattr().  With these two new hooks I think we could do
> something like this:
>
> int security_inode_setxattr(...)
> {
>   owned = false
>   hook_loop(inode_setxattr_owned) {
>     trc = hook->inode_setxattr_owned(name);
>     if (trc > 0) {
>       owned = true;
>       break;
>     }
>   }
>   if (owned) {
>     hook_loop(inode_setxattr) {
>       /* run the existing inode_setxattr hooks, e.g. SELinux and Smack */
>     }
>   } else {
>     hook_loop(inode_setxattr_cap) {
>       /* run the capability setxattr hooks, e.g. commoncap.c */
>     }
>   }
> }
>
> .. with security_inode_removexattr() following a similar pattern.
>
> I will admit that there is some duplication in having to check the
> xattr twice (once in _owned, again in inode_setxattr), and the
> multiple hook approach is less than ideal, but this seems much less
> fragile to me.
>
> Thoughts?
>
From 644ac239cbbdee3d4fc3ba0173c85b34382670c6 Mon Sep 17 00:00:00 2001
From: Casey Schaufler <casey@schaufler-ca.com>
Date: Thu, 26 Oct 2023 12:52:55 -0700
Subject: [PATCH v39 38/42] LSM: Correct handling of ENOSYS in inode_setxattr

The usual "bail on fail" behavior of LSM hooks doesn't
work for security_inode_setxattr(). Modules are allowed
to return -ENOSYS if the attribute specified isn't one
they manage. Fix the code to accommodate this unusal case.
This requires changes to the hooks in SELinux and Smack.

Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
---
 security/security.c        | 29 +++++++++++++++--------------
 security/selinux/hooks.c   |  7 ++-----
 security/smack/smack_lsm.c | 10 +++++-----
 3 files changed, 22 insertions(+), 24 deletions(-)

diff --git a/security/security.c b/security/security.c
index 64cdf0e09832..b1a849e8589c 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2346,24 +2346,25 @@ int security_inode_setxattr(struct mnt_idmap *idmap,
 			    struct dentry *dentry, const char *name,
 			    const void *value, size_t size, int flags)
 {
-	int ret;
+	struct security_hook_list *hp;
+	int rc = -ENOSYS;
 
 	if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
 		return 0;
-	/*
-	 * SELinux and Smack integrate the cap call,
-	 * so assume that all LSMs supplying this call do so.
-	 */
-	ret = call_int_hook(inode_setxattr, 1, idmap, dentry, name, value,
-			    size, flags);
 
-	if (ret == 1)
-		ret = cap_inode_setxattr(dentry, name, value, size, flags);
-	if (ret)
-		return ret;
-	ret = ima_inode_setxattr(dentry, name, value, size);
-	if (ret)
-		return ret;
+	hlist_for_each_entry(hp, &security_hook_heads.inode_setxattr, list) {
+		rc = hp->hook.inode_setxattr(idmap, dentry, name, value, size,
+					     flags);
+		if (rc != -ENOSYS)
+			break;
+	}
+	if (rc == -ENOSYS)
+		rc = cap_inode_setxattr(dentry, name, value, size, flags);
+	if (rc)
+		return rc;
+	rc = ima_inode_setxattr(dentry, name, value, size);
+	if (rc)
+		return rc;
 	return evm_inode_setxattr(idmap, dentry, name, value, size);
 }
 
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 46dee63eec12..4ac4b536c568 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3207,13 +3207,10 @@ static int selinux_inode_setxattr(struct mnt_idmap *idmap,
 	int rc = 0;
 
 	if (strcmp(name, XATTR_NAME_SELINUX)) {
-		rc = cap_inode_setxattr(dentry, name, value, size, flags);
-		if (rc)
-			return rc;
-
 		/* Not an attribute we recognize, so just check the
 		   ordinary setattr permission. */
-		return dentry_has_perm(current_cred(), dentry, FILE__SETATTR);
+		rc = dentry_has_perm(current_cred(), dentry, FILE__SETATTR);
+		return rc ? rc : -ENOSYS;
 	}
 
 	if (!selinux_initialized())
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 61bd3f626e7d..02b9aa200ad4 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -1340,7 +1340,7 @@ static int smack_inode_setxattr(struct mnt_idmap *idmap,
 		    strncmp(value, TRANS_TRUE, TRANS_TRUE_SIZE) != 0)
 			rc = -EINVAL;
 	} else
-		rc = cap_inode_setxattr(dentry, name, value, size, flags);
+		rc = -ENOSYS;
 
 	if (check_priv && !smack_privileged(CAP_MAC_ADMIN))
 		rc = -EPERM;
@@ -1354,11 +1354,11 @@ static int smack_inode_setxattr(struct mnt_idmap *idmap,
 			rc = -EINVAL;
 	}
 
-	smk_ad_init(&ad, __func__, LSM_AUDIT_DATA_DENTRY);
-	smk_ad_setfield_u_fs_path_dentry(&ad, dentry);
-
 	if (rc == 0) {
-		rc = smk_curacc(smk_of_inode(d_backing_inode(dentry)), MAY_WRITE, &ad);
+		smk_ad_init(&ad, __func__, LSM_AUDIT_DATA_DENTRY);
+		smk_ad_setfield_u_fs_path_dentry(&ad, dentry);
+		rc = smk_curacc(smk_of_inode(d_backing_inode(dentry)),
+				MAY_WRITE, &ad);
 		rc = smk_bu_inode(d_backing_inode(dentry), MAY_WRITE, rc);
 	}
Paul Moore Feb. 2, 2024, 2:50 a.m. UTC | #6
On Thu, Feb 1, 2024 at 7:11 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 2/1/2024 3:52 PM, Paul Moore wrote:
> > On Tue, Jan 30, 2024 at 9:19 PM Paul Moore <paul@paul-moore.com> wrote:
> >> I'll come back to this tomorrow with some fresh eyes.
> > My apologies, "tomorrow" turned into "the day after tomorrow" (as it
> > often does) ...
> >
> > I've been struggling with the idea that there are individual LSMs
> > still calling into the capability hooks instead of leveraging the LSM
> > stacking infrastructure, and the "magic" involved to make it all work.
> > While your patch looks like it should restore proper behavior - that's
> > good! - I keep thinking that we can, and should, do better.
>
> Apology for attaching a patch rather than inlining it.
> I've attached patch #38 from the current stacking set.
> It addresses the issue.

Archive link:
https://lore.kernel.org/linux-security-module/20231215221636.105680-39-casey@schaufler-ca.com/

It still relies on checking for special return values, which I'm
starting to sour on as it has been the source of problems in the past.
At some point in the future (likely distant future) I want to spend a
day to see what sort of changes it would take to convert all of the
LSM hooks that return an int value into a 0-on-success,
negative-errno-otherwise format where the negative error codes have no
special meaning to the LSM layer ... and if that would even be
desirable in each case.

> > The only thing that I coming up with is to create two new LSM hooks,
> > in addition to the existing 'inode_setxattr' hook.  The new LSM hooks
> > would be 'inode_setxattr_owned' and 'inode_setxattr_cap'.  The _owned
> > hook would simply check the xattr name and return a positive value if
> > the LSM "owned" the xattr, e.g. XATTR_NAME_SELINUX for SELinux, and
> > zero otherwise.  The _cap hook would only be used by the capabilities
> > code (or something similar), and would match up with
> > cap_inode_setxattr().  With these two new hooks I think we could do
> > something like this:
> >
> > int security_inode_setxattr(...)
> > {
> >   owned = false
> >   hook_loop(inode_setxattr_owned) {
> >     trc = hook->inode_setxattr_owned(name);
> >     if (trc > 0) {
> >       owned = true;
> >       break;
> >     }
> >   }
> >   if (owned) {
> >     hook_loop(inode_setxattr) {
> >       /* run the existing inode_setxattr hooks, e.g. SELinux and Smack */
> >     }
> >   } else {
> >     hook_loop(inode_setxattr_cap) {
> >       /* run the capability setxattr hooks, e.g. commoncap.c */
> >     }
> >   }
> > }
> >
> > .. with security_inode_removexattr() following a similar pattern.
> >
> > I will admit that there is some duplication in having to check the
> > xattr twice (once in _owned, again in inode_setxattr), and the
> > multiple hook approach is less than ideal, but this seems much less
> > fragile to me.
> >
> > Thoughts?
> >
diff mbox series

Patch

diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 76458b6d53da..a281db62b665 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -137,14 +137,14 @@  LSM_HOOK(int, 0, inode_follow_link, struct dentry *dentry, struct inode *inode,
 LSM_HOOK(int, 0, inode_permission, struct inode *inode, int mask)
 LSM_HOOK(int, 0, inode_setattr, struct dentry *dentry, struct iattr *attr)
 LSM_HOOK(int, 0, inode_getattr, const struct path *path)
-LSM_HOOK(int, 0, inode_setxattr, struct mnt_idmap *idmap,
+LSM_HOOK(int, 1, inode_setxattr, struct mnt_idmap *idmap,
 	 struct dentry *dentry, const char *name, const void *value,
 	 size_t size, int flags)
 LSM_HOOK(void, LSM_RET_VOID, inode_post_setxattr, struct dentry *dentry,
 	 const char *name, const void *value, size_t size, int flags)
 LSM_HOOK(int, 0, inode_getxattr, struct dentry *dentry, const char *name)
 LSM_HOOK(int, 0, inode_listxattr, struct dentry *dentry)
-LSM_HOOK(int, 0, inode_removexattr, struct mnt_idmap *idmap,
+LSM_HOOK(int, 1, inode_removexattr, struct mnt_idmap *idmap,
 	 struct dentry *dentry, const char *name)
 LSM_HOOK(int, 0, inode_set_acl, struct mnt_idmap *idmap,
 	 struct dentry *dentry, const char *acl_name, struct posix_acl *kacl)
diff --git a/security/security.c b/security/security.c
index 3aaad75c9ce8..cedd6c150bdd 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2254,7 +2254,8 @@  int security_inode_setxattr(struct mnt_idmap *idmap,
 			    struct dentry *dentry, const char *name,
 			    const void *value, size_t size, int flags)
 {
-	int ret;
+	struct security_hook_list *hp;
+	int ret = LSM_RET_DEFAULT(inode_setxattr);
 
 	if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
 		return 0;
@@ -2262,13 +2263,22 @@  int security_inode_setxattr(struct mnt_idmap *idmap,
 	 * SELinux and Smack integrate the cap call,
 	 * so assume that all LSMs supplying this call do so.
 	 */
-	ret = call_int_hook(inode_setxattr, 1, idmap, dentry, name, value,
-			    size, flags);
-
-	if (ret == 1)
+	hlist_for_each_entry(hp, &security_hook_heads.inode_setxattr,
+			     list) {
+		int trc = hp->hook.inode_setxattr(idmap, dentry, name,
+						  value, size, flags);
+		if (trc == LSM_RET_DEFAULT(inode_setxattr))
+			continue;
+		if (trc)
+			return trc;
+		ret = 0;
+	}
+	/* rc can only be either LSM_RET_DEFAULT(...) or 0 here */
+	if (ret == LSM_RET_DEFAULT(inode_setxattr)) {
 		ret = cap_inode_setxattr(dentry, name, value, size, flags);
-	if (ret)
-		return ret;
+		if (ret)
+			return ret;
+	}
 	ret = ima_inode_setxattr(dentry, name, value, size);
 	if (ret)
 		return ret;
@@ -2417,7 +2427,8 @@  int security_inode_listxattr(struct dentry *dentry)
 int security_inode_removexattr(struct mnt_idmap *idmap,
 			       struct dentry *dentry, const char *name)
 {
-	int ret;
+	struct security_hook_list *hp;
+	int ret = LSM_RET_DEFAULT(inode_removexattr);
 
 	if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
 		return 0;
@@ -2425,11 +2436,21 @@  int security_inode_removexattr(struct mnt_idmap *idmap,
 	 * SELinux and Smack integrate the cap call,
 	 * so assume that all LSMs supplying this call do so.
 	 */
-	ret = call_int_hook(inode_removexattr, 1, idmap, dentry, name);
-	if (ret == 1)
+	hlist_for_each_entry(hp, &security_hook_heads.inode_removexattr,
+			     list) {
+		int trc = hp->hook.inode_removexattr(idmap, dentry, name);
+		if (trc == LSM_RET_DEFAULT(inode_removexattr))
+			continue;
+		if (trc)
+			return trc;
+		ret = 0;
+	}
+	/* rc can only be either LSM_RET_DEFAULT(...) or 0 here */
+	if (ret == LSM_RET_DEFAULT(inode_removexattr)) {
 		ret = cap_inode_removexattr(idmap, dentry, name);
-	if (ret)
-		return ret;
+		if (ret)
+			return ret;
+	}
 	ret = ima_inode_removexattr(dentry, name);
 	if (ret)
 		return ret;