On 7/9/2024 5:36 AM, KP Singh wrote:
> [...]
>
>>> --- 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().
I really dislike it. The security prefix (even with __) tells the
developer in security.c that the code is used elsewhere. How about
lsm_hook_current_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
