| Message ID | 20240129133058.1627971-1-omosnace@redhat.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | security: fix no-op hook logic in security_inode_{set,remove}xattr() | expand |
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.
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);
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.
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?
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); }
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 --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;
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(-)