| Message ID | 20240629084331.3807368-5-kpsingh@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | Reduce overhead of LSMs with static calls | expand |
On Jun 29, 2024 KP Singh <kpsingh@kernel.org> 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@schaufler-ca.com> > Reviewed-by: Kees Cook <keescook@chromium.org> > Signed-off-by: KP Singh <kpsingh@kernel.org> > Reviewed-by: John Johansen <john.johansen@canonical.com> > --- > 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
[...] > > --- 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
On Tue, Jul 9, 2024 at 8:36 AM KP Singh <kpsingh@kernel.org> wrote: > > > --- a/security/security.c > > > +++ b/security/security.c ... > > > -#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 guess we can't get rid of it due to the crazy macro stuff with loop unrolling, BEFORE/AFTER blocks, etc. Ooof. If you were looking for another example of why I don't really like these patches, this would be a good candidate. More below ... > > 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. Thanks for testing it out. Perhaps trying to move all of these hooks to use the static_call approach was a mistake. I realize you're doing your best adapting the static_call API to support multiple LSMs, but it just doesn't look like a good fit to me for the "unconventional" hooks here in this patch. > > 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 I recognize that your comments are in reference to that last flaw rooted in the hardware that used indirect calls at an attack vector, but wasn't that resolved through other means? I never saw the PoC or had time to follow up on whatever mitigation was ultimately merged (if any). However, my understanding is that the move to static_calls is not strictly necessary to patch over that particular hardware flaw, it is just a we-really-want-this for either a performance or a non-specific security reason; pick your favorite of the two based on your audience. Regardless, since none of the previous suggestions/options proved to be workable, I'm going to suggest we just kill this patch too and move forward with the others. I had hoped we could get the changes in this patch cleaned up, but it doesn't look like that is going to be the case, or at least not within a week or two, so let's drop it and we can always reconsider this in the future if a cleaner implementation is presented.
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
On Tue, Jul 9, 2024 at 12:53 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > 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()? See my reply. There is enough ugliness in converting the hooks in this particular patch that I think we need to shelve this patch too.
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 + +#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 */ @@ -1187,7 +1225,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; @@ -1198,13 +1235,20 @@ 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, + /* BLOCK AFTER INVOCATION */ + { + if (rc <= 0) { + cap_sys_admin = 0; + goto out; + } + }, + /* END BLOCK AFTER INVOCATION */ + mm, pages); + +out: return __vm_enough_memory(mm, pages, cap_sys_admin); } @@ -1346,17 +1390,21 @@ 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, + /* BLOCK AFTER INVOCATION */ + { + if (trc == 0) + rc = 0; + else if (trc != -ENOPARAM) + return trc; + }, + /* END BLOCK AFTER INVOCATION */ + fc, param); + return rc; } @@ -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); @@ -1780,7 +1832,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; @@ -1798,18 +1849,21 @@ 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, + /* BLOCK AFTER INVOCATION */ + { /* * 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; + }, + /* END BLOCK AFTER INVOCATION */ + inode, dir, qstr, new_xattrs, &xattr_count); /* If initxattrs() is NULL, xattr_count is zero, skip the call. */ if (!xattr_count) @@ -3631,16 +3685,21 @@ 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, + /* BLOCK AFTER INVOCATION */ + { + if (thisrc != LSM_RET_DEFAULT(task_prctl)) { + rc = thisrc; + if (thisrc != 0) + goto out; + } + }, + /* END BLOCK AFTER INVOCATION */ + option, arg2, arg3, arg4, arg5); + +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) @@ -4133,9 +4202,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) @@ -4156,11 +4224,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); @@ -4181,14 +4245,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); } /** @@ -4205,14 +4262,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); } /** @@ -5328,23 +5378,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); } /**
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@schaufler-ca.com> Reviewed-by: Kees Cook <keescook@chromium.org> Signed-off-by: KP Singh <kpsingh@kernel.org> --- security/security.c | 248 +++++++++++++++++++++++++------------------- 1 file changed, 144 insertions(+), 104 deletions(-)