[PATCH v13 4/5] security: Update non standard hooks to use static calls
KP Singh
kpsingh at kernel.org
Tue Jul 9 12:36:09 UTC 2024
[...]
> > --- 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.
call_hook_with_lsmid is a pattern used by quite a few hooks, happy to
update the name.
What do you think about __security_hook_lsm_id().
> > +#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?
This does not work.
The special macro you are defining does not have the static_call
invocation and if you add that bit it's basically the __CALL_HOOK
macro or __CALL_STATIC_INT, __CALL_STATIC_VOID macro inlined
everywhere, I tried implementing it but it gets very dirty.
>
> #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.
>
I think your position on fixing everything is actually a valid one for
security, which is why I did not contest it.
The security_getselfattr is called very close to the syscall boundary
and the closer to the boundary the call is, the greater control the
attacker has over arguments and the easier it is to mount the attack.
This is why LSM indirect calls are a lucrative target because they
happen fairly early in the transition from user to kernel.
security_getselfattr is literally just in a SYSCALL_DEFINE
>From a security perspective we should not leave this open.
- KP
> --
> paul-moore.com
More information about the Linux-security-module-archive
mailing list