From: Casey Schaufler <casey@schaufler-ca.com>
To: KP Singh <kpsingh@kernel.org>,
linux-security-module@vger.kernel.org, bpf@vger.kernel.org
Cc: ast@kernel.org, paul@paul-moore.com, andrii@kernel.org,
keescook@chromium.org, daniel@iogearbox.net, renauld@google.com,
revest@chromium.org, song@kernel.org,
Casey Schaufler <casey@schaufler-ca.com>
Subject: Re: [PATCH v11 4/5] security: Update non standard hooks to use static calls
Date: Fri, 10 May 2024 10:08:03 -0700 [thread overview]
Message-ID: <62a282e2-4cd5-4222-a223-9d38822a6b38@schaufler-ca.com> (raw)
In-Reply-To: <20240509201421.905965-5-kpsingh@kernel.org>
On 5/9/2024 1:14 PM, KP Singh wrote:
> There are some LSM hooks which do not use the common pattern followed
> by other LSM hooks and thus cannot use call_{int, void}_hook macros and
> instead use lsm_for_each_hook macro which still results in indirect
> call.
>
> There is one additional generalizable pattern where a hook matching an
> lsmid is called and the indirect calls for these are addressed with the
> newly added call_hook_with_lsmid macro which internally uses an
> implementation similar to call_int_hook but has an additional check that
> matches the lsmid.
>
> For the generic case the lsm_for_each_hook macro is updated to accept
> logic before and after the invocation of the LSM hook (static call) in
> the unrolled loop.
>
> Signed-off-by: KP Singh <kpsingh@kernel.org>
A couple of comments below, nonetheless ...
Reviewed-by: Casey Schaufler <casey@schaufler-ca.com>
> ---
> security/security.c | 229 ++++++++++++++++++++++++--------------------
> 1 file changed, 125 insertions(+), 104 deletions(-)
>
> diff --git a/security/security.c b/security/security.c
> index 39ffe949e509..491b807a8a63 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -945,10 +945,41 @@ out: \
> RC; \
> })
>
> -#define lsm_for_each_hook(scall, NAME) \
> - for (scall = static_calls_table.NAME; \
> - scall - static_calls_table.NAME < MAX_LSM_COUNT; scall++) \
> - if (static_key_enabled(&scall->active->key))
> +/*
> + * Can be used in the context passed to lsm_for_each_hook to get the lsmid of the
> + * current hook
> + */
> +#define current_lsmid() _hook_lsmid
> +
> +#define __CALL_HOOK(NUM, HOOK, RC, BODY_BEFORE, BODY_AFTER, ...) \
> +do { \
> + int __maybe_unused _hook_lsmid; \
> + \
> + if (static_branch_unlikely(&SECURITY_HOOK_ACTIVE_KEY(HOOK, NUM))) { \
> + _hook_lsmid = static_calls_table.HOOK[NUM].hl->lsmid->id; \
> + BODY_BEFORE \
> + RC = static_call(LSM_STATIC_CALL(HOOK, NUM))(__VA_ARGS__); \
> + BODY_AFTER \
> + } \
> +} while (0);
> +
> +#define lsm_for_each_hook(HOOK, RC, BODY, ...) \
> + LSM_LOOP_UNROLL(__CALL_HOOK, HOOK, RC, ;, BODY, __VA_ARGS__)
> +
> +#define call_hook_with_lsmid(HOOK, LSMID, ...) \
> +({ \
> + __label__ out; \
> + int RC = LSM_RET_DEFAULT(HOOK); \
> + \
> + LSM_LOOP_UNROLL(__CALL_HOOK, HOOK, RC, { \
> + if (current_lsmid() != LSMID) \
> + continue; \
> + }, { \
> + goto out; \
> + }, __VA_ARGS__); \
> +out: \
> + RC; \
> +})
I like how clean these macros look ...
>
> /* Security operations */
>
> @@ -1184,7 +1215,6 @@ int security_settime64(const struct timespec64 *ts, const struct timezone *tz)
> */
> int security_vm_enough_memory_mm(struct mm_struct *mm, long pages)
> {
> - struct lsm_static_call *scall;
> int cap_sys_admin = 1;
> int rc;
>
> @@ -1195,13 +1225,18 @@ int security_vm_enough_memory_mm(struct mm_struct *mm, long pages)
> * agree that it should be set it will. If any module
> * thinks it should not be set it won't.
> */
> - lsm_for_each_hook(scall, vm_enough_memory) {
> - rc = scall->hl->hook.vm_enough_memory(mm, pages);
> - if (rc <= 0) {
> - cap_sys_admin = 0;
> - break;
> - }
> - }
> +
> + lsm_for_each_hook(
> + vm_enough_memory, rc,
> + {
> + if (rc <= 0) {
> + cap_sys_admin = 0;
> + goto out;
> + }
> + },
> + mm, pages);
> +
> +out:
... but the invocations are quite hideous. Because this looks like code that's
messed up I would like to see it commented, perhaps something like
+
+ lsm_for_each_hook(
+ vm_enough_memory, rc,
+ /* BLOCK BEFORE */
+ {
+ if (rc <= 0) {
+ cap_sys_admin = 0;
+ goto out;
+ }
+ },
+ /* END BLOCK BEFORE */
+ mm, pages);
+
+out:
> return __vm_enough_memory(mm, pages, cap_sys_admin);
> }
>
> @@ -1343,17 +1378,19 @@ int security_fs_context_dup(struct fs_context *fc, struct fs_context *src_fc)
> int security_fs_context_parse_param(struct fs_context *fc,
> struct fs_parameter *param)
> {
> - struct lsm_static_call *scall;
> - int trc;
> + int trc = LSM_RET_DEFAULT(fs_context_parse_param);
> int rc = -ENOPARAM;
>
> - lsm_for_each_hook(scall, fs_context_parse_param) {
> - trc = scall->hl->hook.fs_context_parse_param(fc, param);
> - if (trc == 0)
> - rc = 0;
> - else if (trc != -ENOPARAM)
> - return trc;
> - }
> + lsm_for_each_hook(
> + fs_context_parse_param, trc,
> + {
> + if (trc == 0)
> + rc = 0;
> + else if (trc != -ENOPARAM)
> + return trc;
> + },
> + fc, param);
> +
> return rc;
> }
>
> @@ -1578,15 +1615,17 @@ int security_sb_set_mnt_opts(struct super_block *sb,
> unsigned long kern_flags,
> unsigned long *set_kern_flags)
> {
> - struct lsm_static_call *scall;
> int rc = mnt_opts ? -EOPNOTSUPP : LSM_RET_DEFAULT(sb_set_mnt_opts);
>
> - lsm_for_each_hook(scall, sb_set_mnt_opts) {
> - rc = scall->hl->hook.sb_set_mnt_opts(sb, mnt_opts, kern_flags,
> - set_kern_flags);
> - if (rc != LSM_RET_DEFAULT(sb_set_mnt_opts))
> - break;
> - }
> + lsm_for_each_hook(
> + sb_set_mnt_opts, rc,
> + {
> + if (rc != LSM_RET_DEFAULT(sb_set_mnt_opts))
> + goto out;
> + },
> + sb, mnt_opts, kern_flags, set_kern_flags);
> +
> +out:
> return rc;
> }
> EXPORT_SYMBOL(security_sb_set_mnt_opts);
> @@ -1777,7 +1816,6 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
> const struct qstr *qstr,
> const initxattrs initxattrs, void *fs_data)
> {
> - struct lsm_static_call *scall;
> struct xattr *new_xattrs = NULL;
> int ret = -EOPNOTSUPP, xattr_count = 0;
>
> @@ -1795,18 +1833,19 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
> return -ENOMEM;
> }
>
> - lsm_for_each_hook(scall, inode_init_security) {
> - ret = scall->hl->hook.inode_init_security(inode, dir, qstr, new_xattrs,
> - &xattr_count);
> - if (ret && ret != -EOPNOTSUPP)
> - goto out;
> + lsm_for_each_hook(
> + inode_init_security, ret,
> + {
> /*
> * As documented in lsm_hooks.h, -EOPNOTSUPP in this context
> * means that the LSM is not willing to provide an xattr, not
> * that it wants to signal an error. Thus, continue to invoke
> * the remaining LSMs.
> */
> - }
> + if (ret && ret != -EOPNOTSUPP)
> + goto out;
> + },
> + inode, dir, qstr, new_xattrs, &xattr_count);
>
> /* If initxattrs() is NULL, xattr_count is zero, skip the call. */
> if (!xattr_count)
> @@ -3601,16 +3640,19 @@ int security_task_prctl(int option, unsigned long arg2, unsigned long arg3,
> {
> int thisrc;
> int rc = LSM_RET_DEFAULT(task_prctl);
> - struct lsm_static_call *scall;
> -
> - lsm_for_each_hook(scall, task_prctl) {
> - thisrc = scall->hl->hook.task_prctl(option, arg2, arg3, arg4, arg5);
> - if (thisrc != LSM_RET_DEFAULT(task_prctl)) {
> - rc = thisrc;
> - if (thisrc != 0)
> - break;
> - }
> - }
> +
> + lsm_for_each_hook(
> + task_prctl, thisrc,
> + {
> + if (thisrc != LSM_RET_DEFAULT(task_prctl)) {
> + rc = thisrc;
> + if (thisrc != 0)
> + goto out;
> + }
> + },
> + option, arg2, arg3, arg4, arg5);
> +
> +out:
> return rc;
> }
>
> @@ -4010,7 +4052,6 @@ EXPORT_SYMBOL(security_d_instantiate);
> int security_getselfattr(unsigned int attr, struct lsm_ctx __user *uctx,
> u32 __user *size, u32 flags)
> {
> - struct lsm_static_call *scall;
> struct lsm_ctx lctx = { .id = LSM_ID_UNDEF, };
> u8 __user *base = (u8 __user *)uctx;
> u32 entrysize;
> @@ -4048,31 +4089,40 @@ int security_getselfattr(unsigned int attr, struct lsm_ctx __user *uctx,
> * In the usual case gather all the data from the LSMs.
> * In the single case only get the data from the LSM specified.
> */
> - lsm_for_each_hook(scall, getselfattr) {
> - if (single && lctx.id != scall->hl->lsmid->id)
> - continue;
> - entrysize = left;
> - if (base)
> - uctx = (struct lsm_ctx __user *)(base + total);
> - rc = scall->hl->hook.getselfattr(attr, uctx, &entrysize, flags);
> - if (rc == -EOPNOTSUPP) {
> - rc = 0;
> - continue;
> - }
> - if (rc == -E2BIG) {
> - rc = 0;
> - left = 0;
> - toobig = true;
> - } else if (rc < 0)
> - return rc;
> - else
> - left -= entrysize;
> + LSM_LOOP_UNROLL(
> + __CALL_HOOK, getselfattr, rc,
> + /* BODY_BEFORE */
> + {
> + if (single && lctx.id != current_lsmid())
> + continue;
> + entrysize = left;
> + if (base)
> + uctx = (struct lsm_ctx __user *)(base + total);
> + },
> + /* BODY_AFTER */
> + {
> + if (rc == -EOPNOTSUPP) {
> + rc = 0;
> + } else {
> + if (rc == -E2BIG) {
> + rc = 0;
> + left = 0;
> + toobig = true;
> + } else if (rc < 0)
> + return rc;
> + else
> + left -= entrysize;
> +
> + total += entrysize;
> + count += rc;
> + if (single)
> + goto out;
> + }
> + },
> + attr, uctx, &entrysize, flags);
> +
> +out:
>
> - total += entrysize;
> - count += rc;
> - if (single)
> - break;
> - }
> if (put_user(total, size))
> return -EFAULT;
> if (toobig)
> @@ -4103,9 +4153,8 @@ int security_getselfattr(unsigned int attr, struct lsm_ctx __user *uctx,
> int security_setselfattr(unsigned int attr, struct lsm_ctx __user *uctx,
> u32 size, u32 flags)
> {
> - struct lsm_static_call *scall;
> struct lsm_ctx *lctx;
> - int rc = LSM_RET_DEFAULT(setselfattr);
> + int rc;
> u64 required_len;
>
> if (flags)
> @@ -4126,11 +4175,7 @@ int security_setselfattr(unsigned int attr, struct lsm_ctx __user *uctx,
> goto free_out;
> }
>
> - lsm_for_each_hook(scall, setselfattr)
> - if ((scall->hl->lsmid->id) == lctx->id) {
> - rc = scall->hl->hook.setselfattr(attr, lctx, size, flags);
> - break;
> - }
> + rc = call_hook_with_lsmid(setselfattr, lctx->id, attr, lctx, size, flags);
>
> free_out:
> kfree(lctx);
> @@ -4151,14 +4196,7 @@ int security_setselfattr(unsigned int attr, struct lsm_ctx __user *uctx,
> int security_getprocattr(struct task_struct *p, int lsmid, const char *name,
> char **value)
> {
> - struct lsm_static_call *scall;
> -
> - lsm_for_each_hook(scall, getprocattr) {
> - if (lsmid != 0 && lsmid != scall->hl->lsmid->id)
> - continue;
> - return scall->hl->hook.getprocattr(p, name, value);
> - }
> - return LSM_RET_DEFAULT(getprocattr);
> + return call_hook_with_lsmid(getprocattr, lsmid, p, name, value);
> }
>
> /**
> @@ -4175,14 +4213,7 @@ int security_getprocattr(struct task_struct *p, int lsmid, const char *name,
> */
> int security_setprocattr(int lsmid, const char *name, void *value, size_t size)
> {
> - struct lsm_static_call *scall;
> -
> - lsm_for_each_hook(scall, setprocattr) {
> - if (lsmid != 0 && lsmid != scall->hl->lsmid->id)
> - continue;
> - return scall->hl->hook.setprocattr(name, value, size);
> - }
> - return LSM_RET_DEFAULT(setprocattr);
> + return call_hook_with_lsmid(setprocattr, lsmid, name, value, size);
> }
>
> /**
> @@ -5267,23 +5298,13 @@ int security_xfrm_state_pol_flow_match(struct xfrm_state *x,
> struct xfrm_policy *xp,
> const struct flowi_common *flic)
> {
> - struct lsm_static_call *scall;
> - int rc = LSM_RET_DEFAULT(xfrm_state_pol_flow_match);
> -
> /*
> * Since this function is expected to return 0 or 1, the judgment
> * becomes difficult if multiple LSMs supply this call. Fortunately,
> * we can use the first LSM's judgment because currently only SELinux
> * supplies this call.
> - *
> - * For speed optimization, we explicitly break the loop rather than
> - * using the macro
> */
> - lsm_for_each_hook(scall, xfrm_state_pol_flow_match) {
> - rc = scall->hl->hook.xfrm_state_pol_flow_match(x, xp, flic);
> - break;
> - }
> - return rc;
> + return call_int_hook(xfrm_state_pol_flow_match, x, xp, flic);
> }
>
> /**
next prev parent reply other threads:[~2024-05-10 17:18 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-09 20:14 [PATCH v11 0/5] Reduce overhead of LSMs with static calls KP Singh
2024-05-09 20:14 ` [PATCH v11 1/5] kernel: Add helper macros for loop unrolling KP Singh
2024-05-09 20:14 ` [PATCH v11 2/5] security: Count the LSMs enabled at compile time KP Singh
2024-05-10 16:47 ` Casey Schaufler
2024-05-09 20:14 ` [PATCH v11 3/5] security: Replace indirect LSM hook calls with static calls KP Singh
2024-05-09 20:14 ` [PATCH v11 4/5] security: Update non standard hooks to use " KP Singh
2024-05-10 17:08 ` Casey Schaufler [this message]
2024-05-13 19:01 ` Kees Cook
2024-05-09 20:14 ` [PATCH v11 5/5] bpf: Only enable BPF LSM hooks when an LSM program is attached KP Singh
2024-05-09 21:55 ` Tetsuo Handa
2024-05-10 17:11 ` Casey Schaufler
2024-05-13 19:02 ` Kees Cook
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=62a282e2-4cd5-4222-a223-9d38822a6b38@schaufler-ca.com \
--to=casey@schaufler-ca.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=keescook@chromium.org \
--cc=kpsingh@kernel.org \
--cc=linux-security-module@vger.kernel.org \
--cc=paul@paul-moore.com \
--cc=renauld@google.com \
--cc=revest@chromium.org \
--cc=song@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).