[PATCH v13 4/5] security: Update non standard hooks to use static calls

Casey Schaufler casey at schaufler-ca.com
Tue Jul 9 16:53:21 UTC 2024


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



More information about the Linux-security-module-archive mailing list