On 12-Feb 14:27, Daniel Borkmann wrote:
> On 2/12/20 3:45 AM, Alexei Starovoitov wrote:
> > On Wed, Feb 12, 2020 at 01:09:07AM +0100, Daniel Borkmann wrote:
> > >
> > > Another approach could be to have a special nop inside call_int_hook()
> > > macro which would then get patched to avoid these situations. Somewhat
> > > similar like static keys where it could be defined anywhere in text but
> > > with updating of call_int_hook()'s RC for the verdict.
> >
> > Sounds nice in theory. I couldn't quite picture how that would look
> > in the code, so I hacked:
> > diff --git a/security/security.c b/security/security.c
> > index 565bc9b67276..ce4bc1e5e26c 100644
> > --- a/security/security.c
> > +++ b/security/security.c
> > @@ -28,6 +28,7 @@
> > #include <linux/string.h>
> > #include <linux/msg.h>
> > #include <net/flow.h>
> > +#include <linux/jump_label.h>
> >
> > #define MAX_LSM_EVM_XATTR 2
> >
> > @@ -678,12 +679,26 @@ static void __init lsm_early_task(struct task_struct *task)
> > * This is a hook that returns a value.
> > */
> >
> > +#define LSM_HOOK_NAME(FUNC) \
> > + DEFINE_STATIC_KEY_FALSE(bpf_lsm_key_##FUNC);
> > +#include <linux/lsm_hook_names.h>
> > +#undef LSM_HOOK_NAME
> > +__diag_push();
> > +__diag_ignore(GCC, 8, "-Wstrict-prototypes", "");
> > +#define LSM_HOOK_NAME(FUNC) \
> > + int bpf_lsm_call_##FUNC() {return 0;}
> > +#include <linux/lsm_hook_names.h>
> > +#undef LSM_HOOK_NAME
> > +__diag_pop();
> > +
> > #define call_void_hook(FUNC, ...) \
> > do { \
> > struct security_hook_list *P; \
> > \
> > hlist_for_each_entry(P, &security_hook_heads.FUNC, list) \
> > P->hook.FUNC(__VA_ARGS__); \
> > + if (static_branch_unlikely(&bpf_lsm_key_##FUNC)) \
> > + (void)bpf_lsm_call_##FUNC(__VA_ARGS__); \
> > } while (0)
> >
> > #define call_int_hook(FUNC, IRC, ...) ({ \
> > @@ -696,6 +711,8 @@ static void __init lsm_early_task(struct task_struct *task)
> > if (RC != 0) \
> > break; \
> > } \
> > + if (RC == IRC && static_branch_unlikely(&bpf_lsm_key_##FUNC)) \
> > + RC = bpf_lsm_call_##FUNC(__VA_ARGS__); \
>
> Nit: the `RC == IRC` test could be moved behind the static_branch_unlikely() so
> that it would be bypassed when not enabled.
>
> > } while (0); \
> > RC; \
> > })
> >
> > The assembly looks good from correctness and performance points.
> > union security_list_options can be split into lsm_hook_names.h too
> > to avoid __diag_ignore. Is that what you have in mind?
> > I don't see how one can improve call_int_hook() macro without
> > full refactoring of linux/lsm_hooks.h
> > imo static_key doesn't have to be there in the first set. We can add this
> > optimization later.
>
> Yes, like the above diff looks good, and then we'd dynamically attach the program
> at bpf_lsm_call_##FUNC()'s fexit hook for a direct jump, so all the security_blah()
> internals could stay as-is which then might also address Jann's concerns wrt
> concrete annotation as well as potential locking changes inside security_blah().
> Agree that patching out via static key could be optional but since you were talking
> about avoiding indirect jumps..
I like this approach as well. Will give it a go and update the
patches. Thanks a lot for your inputs!
- KP
>
> Thanks,
> Daniel
