On Jun 29, 2024 KP Singh <kpsingh@xxxxxxxxxx> 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.
>
> Reviewed-by: Casey Schaufler <casey@xxxxxxxxxxxxxxxx>
> Reviewed-by: Kees Cook <keescook@xxxxxxxxxxxx>
> Signed-off-by: KP Singh <kpsingh@xxxxxxxxxx>
> Reviewed-by: John Johansen <john.johansen@xxxxxxxxxxxxx>
> ---
> security/security.c | 248 +++++++++++++++++++++++++-------------------
> 1 file changed, 144 insertions(+), 104 deletions(-)
>
> diff --git a/security/security.c b/security/security.c
> index e0ec185cf125..4f0f35857217 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -948,10 +948,48 @@ 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
See my comments below about security_getselfattr(), I think we can drop
the current_lsmid() macro. If we really must keep it, we need to rename
it to something else as it clashes too much with the other current_XXX()
macros/functions which are useful outside of our wacky macros.
> +#define __CALL_HOOK(NUM, HOOK, RC, BLOCK_BEFORE, BLOCK_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; \
> + BLOCK_BEFORE \
> + RC = static_call(LSM_STATIC_CALL(HOOK, NUM))(__VA_ARGS__); \
> + BLOCK_AFTER \
> + } \
> +} while (0);
> +
> +#define lsm_for_each_hook(HOOK, RC, BLOCK_AFTER, ...) \
> + LSM_LOOP_UNROLL(__CALL_HOOK, HOOK, RC, ;, BLOCK_AFTER, __VA_ARGS__)
> +
> +#define call_hook_with_lsmid(HOOK, LSMID, ...) \
> +({ \
> + __label__ out; \
> + int RC = LSM_RET_DEFAULT(HOOK); \
> + \
> + LSM_LOOP_UNROLL(__CALL_HOOK, HOOK, RC, \
> + /* BLOCK BEFORE INVOCATION */ \
> + { \
> + if (current_lsmid() != LSMID) \
> + continue; \
> + }, \
> + /* END BLOCK BEFORE INVOCATION */ \
> + /* BLOCK AFTER INVOCATION */ \
> + { \
> + goto out; \
> + }, \
> + /* END BLOCK AFTER INVOCATION */ \
> + __VA_ARGS__); \
> +out: \
> + RC; \
> +})
>
> /* Security operations */
...
> @@ -1581,15 +1629,19 @@ 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,
> + /* BLOCK AFTER INVOCATION */
> + {
> + if (rc != LSM_RET_DEFAULT(sb_set_mnt_opts))
> + goto out;
> + },
> + /* END BLOCK AFTER INVOCATION */
> + sb, mnt_opts, kern_flags, set_kern_flags);
> +
> +out:
> return rc;
> }
> EXPORT_SYMBOL(security_sb_set_mnt_opts);
I know I was the one who asked to implement the static_calls for *all*
of the LSM functions - thank you for doing that - but I think we can
all agree that some of the resulting code is pretty awful. I'm probably
missing something important, but would an apporach similar to the pseudo
code below work?
#define call_int_hook_special(HOOK, RC, LABEL, ...) \
LSM_LOOP_UNROLL(HOOK##_SPECIAL, RC, HOOK, LABEL, __VA_ARGS__)
int security_sb_set_mnt_opts(...)
{
int rc = LSM_RET_DEFAULT(sb_set_mnt_opts);
#define sb_set_mnt_opts_SPECIAL \
do { \
if (rc != LSM_RET_DEFAULT(sb_set_mnt_opts)) \
goto out; \
} while (0)
rc = call_int_hook_special(sb_set_mnt_opts, rc, out, ...);
out:
return rc;
}
> @@ -4040,7 +4099,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;
> @@ -4078,31 +4136,42 @@ 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,
> + /* BLOCK BEFORE INVOCATION */
> + {
> + if (single && lctx.id != current_lsmid())
> + continue;
> + entrysize = left;
> + if (base)
> + uctx = (struct lsm_ctx __user *)(base + total);
> + },
> + /* END BLOCK BEFORE INVOCATION */
> + /* BLOCK AFTER INVOCATION */
> + {
> + 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;
> + }
> + },
> + /* END BLOCK AFTER INVOCATION */
> + attr, uctx, &entrysize, flags);
> +
> +out:
>
> - total += entrysize;
> - count += rc;
> - if (single)
> - break;
> - }
> if (put_user(total, size))
> return -EFAULT;
> if (toobig)
I think we may need to admit defeat with security_getselfattr() and
leave it as-is, the above is just too ugly to live. I'd suggest
adding a comment explaining that it wasn't converted due to complexity
and the resulting awfulness.
--
paul-moore.com
