| Message ID | 20230928202410.3765062-5-kpsingh@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | Reduce overhead of LSMs with static calls | expand |
On Thu, Sep 28, 2023 at 10:24:09PM +0200, KP Singh wrote: SNIP > diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c > index e97aeda3a86b..df9699bce372 100644 > --- a/kernel/bpf/trampoline.c > +++ b/kernel/bpf/trampoline.c > @@ -13,6 +13,7 @@ > #include <linux/bpf_verifier.h> > #include <linux/bpf_lsm.h> > #include <linux/delay.h> > +#include <linux/bpf_lsm.h> > > /* dummy _ops. The verifier will operate on target program's ops. */ > const struct bpf_verifier_ops bpf_extension_verifier_ops = { > @@ -514,7 +515,7 @@ static int __bpf_trampoline_link_prog(struct bpf_tramp_link *link, struct bpf_tr > { > enum bpf_tramp_prog_type kind; > struct bpf_tramp_link *link_exiting; > - int err = 0; > + int err = 0, num_lsm_progs = 0; > int cnt = 0, i; > > kind = bpf_attach_type_to_tramp(link->link.prog); > @@ -545,8 +546,14 @@ static int __bpf_trampoline_link_prog(struct bpf_tramp_link *link, struct bpf_tr > continue; > /* prog already linked */ > return -EBUSY; > + > + if (link_exiting->link.prog->type == BPF_PROG_TYPE_LSM) > + num_lsm_progs++; this looks wrong, it's never reached.. seems like we should add separate hlist_for_each_entry loop over trampoline's links for this check/init of num_lsm_progs ? jirka > } > > + if (!num_lsm_progs && link->link.prog->type == BPF_PROG_TYPE_LSM) > + bpf_lsm_toggle_hook(tr->func.addr, true); > + > hlist_add_head(&link->tramp_hlist, &tr->progs_hlist[kind]); > tr->progs_cnt[kind]++; > err = bpf_trampoline_update(tr, true /* lock_direct_mutex */); > @@ -569,8 +576,10 @@ int bpf_trampoline_link_prog(struct bpf_tramp_link *link, struct bpf_trampoline > > static int __bpf_trampoline_unlink_prog(struct bpf_tramp_link *link, struct bpf_trampoline *tr) > { > + struct bpf_tramp_link *link_exiting; > enum bpf_tramp_prog_type kind; > - int err; > + bool lsm_link_found = false; > + int err, num_lsm_progs = 0; > > kind = bpf_attach_type_to_tramp(link->link.prog); > if (kind == BPF_TRAMP_REPLACE) { > @@ -580,8 +589,24 @@ static int __bpf_trampoline_unlink_prog(struct bpf_tramp_link *link, struct bpf_ > tr->extension_prog = NULL; > return err; > } > + > + if (link->link.prog->type == BPF_PROG_TYPE_LSM) { > + hlist_for_each_entry(link_exiting, &tr->progs_hlist[kind], > + tramp_hlist) { > + if (link_exiting->link.prog->type == BPF_PROG_TYPE_LSM) > + num_lsm_progs++; > + > + if (link_exiting->link.prog == link->link.prog) > + lsm_link_found = true; > + } > + } > + > hlist_del_init(&link->tramp_hlist); > tr->progs_cnt[kind]--; > + > + if (lsm_link_found && num_lsm_progs == 1) > + bpf_lsm_toggle_hook(tr->func.addr, false); > + > return bpf_trampoline_update(tr, true /* lock_direct_mutex */); > } > > diff --git a/security/bpf/hooks.c b/security/bpf/hooks.c > index cfaf1d0e6a5f..1957244196d0 100644 > --- a/security/bpf/hooks.c > +++ b/security/bpf/hooks.c > @@ -8,7 +8,7 @@ > > static struct security_hook_list bpf_lsm_hooks[] __ro_after_init = { > #define LSM_HOOK(RET, DEFAULT, NAME, ...) \ > - LSM_HOOK_INIT(NAME, bpf_lsm_##NAME), > + LSM_HOOK_INIT_DISABLED(NAME, bpf_lsm_##NAME), > #include <linux/lsm_hook_defs.h> > #undef LSM_HOOK > LSM_HOOK_INIT(inode_free_security, bpf_inode_storage_free), > @@ -32,3 +32,26 @@ DEFINE_LSM(bpf) = { > .init = bpf_lsm_init, > .blobs = &bpf_lsm_blob_sizes > }; > + > +void bpf_lsm_toggle_hook(void *addr, bool value) > +{ > + struct lsm_static_call *scalls; > + struct security_hook_list *h; > + int i, j; > + > + for (i = 0; i < ARRAY_SIZE(bpf_lsm_hooks); i++) { > + h = &bpf_lsm_hooks[i]; > + scalls = h->scalls; > + if (h->hook.lsm_callback == addr) > + continue; > + > + for (j = 0; j < MAX_LSM_COUNT; j++) { > + if (scalls[j].hl != h) > + continue; > + if (value) > + static_branch_enable(scalls[j].active); > + else > + static_branch_disable(scalls[j].active); > + } > + } > +} > diff --git a/security/security.c b/security/security.c > index c2c2cf6b711f..d1ee72e563cc 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -382,7 +382,8 @@ static void __init lsm_static_call_init(struct security_hook_list *hl) > __static_call_update(scall->key, scall->trampoline, > hl->hook.lsm_callback); > scall->hl = hl; > - static_branch_enable(scall->active); > + if (hl->default_state) > + static_branch_enable(scall->active); > return; > } > scall++; > -- > 2.42.0.582.g8ccd20d70d-goog > >
On Thu, Oct 5, 2023 at 10:09 AM Jiri Olsa <olsajiri@gmail.com> wrote: > > On Thu, Sep 28, 2023 at 10:24:09PM +0200, KP Singh wrote: > > SNIP > > > diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c > > index e97aeda3a86b..df9699bce372 100644 > > --- a/kernel/bpf/trampoline.c > > +++ b/kernel/bpf/trampoline.c > > @@ -13,6 +13,7 @@ > > #include <linux/bpf_verifier.h> > > #include <linux/bpf_lsm.h> > > #include <linux/delay.h> > > +#include <linux/bpf_lsm.h> > > > > /* dummy _ops. The verifier will operate on target program's ops. */ > > const struct bpf_verifier_ops bpf_extension_verifier_ops = { > > @@ -514,7 +515,7 @@ static int __bpf_trampoline_link_prog(struct bpf_tramp_link *link, struct bpf_tr > > { > > enum bpf_tramp_prog_type kind; > > struct bpf_tramp_link *link_exiting; > > - int err = 0; > > + int err = 0, num_lsm_progs = 0; > > int cnt = 0, i; > > > > kind = bpf_attach_type_to_tramp(link->link.prog); > > @@ -545,8 +546,14 @@ static int __bpf_trampoline_link_prog(struct bpf_tramp_link *link, struct bpf_tr > > continue; > > /* prog already linked */ > > return -EBUSY; > > + > > + if (link_exiting->link.prog->type == BPF_PROG_TYPE_LSM) > > + num_lsm_progs++; > > this looks wrong, it's never reached.. seems like we should add separate > hlist_for_each_entry loop over trampoline's links for this check/init of > num_lsm_progs ? > > jirka Good catch, I missed this during my rebase, after https://lore.kernel.org/bpf/20220510205923.3206889-2-kuifeng@fb.com/ this condition is basically never reached. I will do a general loop over to count LSM programs and toggle the hook to true (and same for unlink). - KP [...]
On Thu, Oct 5, 2023 at 3:26 PM KP Singh <kpsingh@kernel.org> wrote: > > On Thu, Oct 5, 2023 at 10:09 AM Jiri Olsa <olsajiri@gmail.com> wrote: > > > > On Thu, Sep 28, 2023 at 10:24:09PM +0200, KP Singh wrote: > > > > SNIP > > > > > diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c > > > index e97aeda3a86b..df9699bce372 100644 > > > --- a/kernel/bpf/trampoline.c > > > +++ b/kernel/bpf/trampoline.c > > > @@ -13,6 +13,7 @@ > > > #include <linux/bpf_verifier.h> > > > #include <linux/bpf_lsm.h> > > > #include <linux/delay.h> > > > +#include <linux/bpf_lsm.h> > > > > > > /* dummy _ops. The verifier will operate on target program's ops. */ > > > const struct bpf_verifier_ops bpf_extension_verifier_ops = { > > > @@ -514,7 +515,7 @@ static int __bpf_trampoline_link_prog(struct bpf_tramp_link *link, struct bpf_tr > > > { > > > enum bpf_tramp_prog_type kind; > > > struct bpf_tramp_link *link_exiting; I think this is a typo here. It should be existing, no? > > > - int err = 0; > > > + int err = 0, num_lsm_progs = 0; > > > int cnt = 0, i; > > > > > > kind = bpf_attach_type_to_tramp(link->link.prog); > > > @@ -545,8 +546,14 @@ static int __bpf_trampoline_link_prog(struct bpf_tramp_link *link, struct bpf_tr > > > continue; > > > /* prog already linked */ > > > return -EBUSY; > > > + > > > + if (link_exiting->link.prog->type == BPF_PROG_TYPE_LSM) > > > + num_lsm_progs++; > > > > this looks wrong, it's never reached.. seems like we should add separate > > hlist_for_each_entry loop over trampoline's links for this check/init of > > num_lsm_progs ? > > > > jirka > > Good catch, I missed this during my rebase, after > https://lore.kernel.org/bpf/20220510205923.3206889-2-kuifeng@fb.com/ > this condition is basically never reached. I will do a general loop > over to count LSM programs and toggle the hook to true (and same for > unlink). > > - KP > > [...]
On Thu, Oct 05, 2023 at 03:27:35PM +0200, KP Singh wrote: > On Thu, Oct 5, 2023 at 3:26 PM KP Singh <kpsingh@kernel.org> wrote: > > > > On Thu, Oct 5, 2023 at 10:09 AM Jiri Olsa <olsajiri@gmail.com> wrote: > > > > > > On Thu, Sep 28, 2023 at 10:24:09PM +0200, KP Singh wrote: > > > > > > SNIP > > > > > > > diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c > > > > index e97aeda3a86b..df9699bce372 100644 > > > > --- a/kernel/bpf/trampoline.c > > > > +++ b/kernel/bpf/trampoline.c > > > > @@ -13,6 +13,7 @@ > > > > #include <linux/bpf_verifier.h> > > > > #include <linux/bpf_lsm.h> > > > > #include <linux/delay.h> > > > > +#include <linux/bpf_lsm.h> > > > > > > > > /* dummy _ops. The verifier will operate on target program's ops. */ > > > > const struct bpf_verifier_ops bpf_extension_verifier_ops = { > > > > @@ -514,7 +515,7 @@ static int __bpf_trampoline_link_prog(struct bpf_tramp_link *link, struct bpf_tr > > > > { > > > > enum bpf_tramp_prog_type kind; > > > > struct bpf_tramp_link *link_exiting; > > I think this is a typo here. It should be existing, no? yes, I was wondering about that as well ;-) jirka > > > > > - int err = 0; > > > > + int err = 0, num_lsm_progs = 0; > > > > int cnt = 0, i; > > > > > > > > kind = bpf_attach_type_to_tramp(link->link.prog); > > > > @@ -545,8 +546,14 @@ static int __bpf_trampoline_link_prog(struct bpf_tramp_link *link, struct bpf_tr > > > > continue; > > > > /* prog already linked */ > > > > return -EBUSY; > > > > + > > > > + if (link_exiting->link.prog->type == BPF_PROG_TYPE_LSM) > > > > + num_lsm_progs++; > > > > > > this looks wrong, it's never reached.. seems like we should add separate > > > hlist_for_each_entry loop over trampoline's links for this check/init of > > > num_lsm_progs ? > > > > > > jirka > > > > Good catch, I missed this during my rebase, after > > https://lore.kernel.org/bpf/20220510205923.3206889-2-kuifeng@fb.com/ > > this condition is basically never reached. I will do a general loop > > over to count LSM programs and toggle the hook to true (and same for > > unlink). > > > > - KP > > > > [...]
On Thu, Oct 5, 2023 at 3:52 PM Jiri Olsa <olsajiri@gmail.com> wrote: > > On Thu, Oct 05, 2023 at 03:27:35PM +0200, KP Singh wrote: > > On Thu, Oct 5, 2023 at 3:26 PM KP Singh <kpsingh@kernel.org> wrote: > > > > > > On Thu, Oct 5, 2023 at 10:09 AM Jiri Olsa <olsajiri@gmail.com> wrote: > > > > > > > > On Thu, Sep 28, 2023 at 10:24:09PM +0200, KP Singh wrote: > > > > > > > > SNIP > > > > > > > > > diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c > > > > > index e97aeda3a86b..df9699bce372 100644 > > > > > --- a/kernel/bpf/trampoline.c > > > > > +++ b/kernel/bpf/trampoline.c > > > > > @@ -13,6 +13,7 @@ > > > > > #include <linux/bpf_verifier.h> > > > > > #include <linux/bpf_lsm.h> > > > > > #include <linux/delay.h> > > > > > +#include <linux/bpf_lsm.h> > > > > > > > > > > /* dummy _ops. The verifier will operate on target program's ops. */ > > > > > const struct bpf_verifier_ops bpf_extension_verifier_ops = { > > > > > @@ -514,7 +515,7 @@ static int __bpf_trampoline_link_prog(struct bpf_tramp_link *link, struct bpf_tr > > > > > { > > > > > enum bpf_tramp_prog_type kind; > > > > > struct bpf_tramp_link *link_exiting; > > > > I think this is a typo here. It should be existing, no? > > yes, I was wondering about that as well ;-) > > jirka > > > > > > > > - int err = 0; > > > > > + int err = 0, num_lsm_progs = 0; > > > > > int cnt = 0, i; > > > > > > > > > > kind = bpf_attach_type_to_tramp(link->link.prog); > > > > > @@ -545,8 +546,14 @@ static int __bpf_trampoline_link_prog(struct bpf_tramp_link *link, struct bpf_tr > > > > > continue; > > > > > /* prog already linked */ > > > > > return -EBUSY; > > > > > + > > > > > + if (link_exiting->link.prog->type == BPF_PROG_TYPE_LSM) > > > > > + num_lsm_progs++; > > > > > > > > this looks wrong, it's never reached.. seems like we should add separate > > > > hlist_for_each_entry loop over trampoline's links for this check/init of > > > > num_lsm_progs ? > > > > > > > > jirka > > > > > > Good catch, I missed this during my rebase, after > > > https://lore.kernel.org/bpf/20220510205923.3206889-2-kuifeng@fb.com/ > > > this condition is basically never reached. I will do a general loop > > > over to count LSM programs and toggle the hook to true (and same for > > > unlink). So, there is something that is unclear about this code, i.e. what happens when there is an error from bpf_trampoline_update fails and the link and unlink seem to have different expectations: * link seems to go back to the linked list and removes the trampoline and restores the refcount: [...] err = bpf_trampoline_update(tr, true /* lock_direct_mutex */); if (err) { hlist_del_init(&link->tramp_hlist); tr->progs_cnt[kind]--; } return err; } * unlink does restore the side effect (i.e. it does not put the removed trampoline back and increments the refcount). hlist_del_init(&link->tramp_hlist); tr->progs_cnt[kind]--; return bpf_trampoline_update(tr, true /* lock_direct_mutex */); However, I think I will make it simpler and enforce the invariant that if an LSM program is attached, the hook is enabled and vice versa. How about: diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c index df9699bce372..4f31384b5637 100644 --- a/kernel/bpf/trampoline.c +++ b/kernel/bpf/trampoline.c @@ -511,11 +511,30 @@ static enum bpf_tramp_prog_type bpf_attach_type_to_tramp(struct bpf_prog *prog) } } +static void bpf_trampoline_toggle_lsm(struct bpf_trampoline *tr, + enum bpf_tramp_prog_type kind) +{ + struct bpf_tramp_link *link; + volatile bool found = false; + + /* Loop through the links and if any LSM program is attached, ensure + * that the hook is enabled. + */ + hlist_for_each_entry(link, &tr->progs_hlist[kind], tramp_hlist) { + if (link->link.prog->type == BPF_PROG_TYPE_LSM) { + found = true; + break; + } + } + + bpf_lsm_toggle_hook(tr->func.addr, found); +} + static int __bpf_trampoline_link_prog(struct bpf_tramp_link *link, struct bpf_trampoline *tr) { enum bpf_tramp_prog_type kind; struct bpf_tramp_link *link_exiting; - int err = 0, num_lsm_progs = 0; + int err = 0; int cnt = 0, i; kind = bpf_attach_type_to_tramp(link->link.prog); @@ -547,15 +566,14 @@ static int __bpf_trampoline_link_prog(struct bpf_tramp_link *link, struct bpf_tr /* prog already linked */ return -EBUSY; - if (link_exiting->link.prog->type == BPF_PROG_TYPE_LSM) - num_lsm_progs++; } - if (!num_lsm_progs && link->link.prog->type == BPF_PROG_TYPE_LSM) - bpf_lsm_toggle_hook(tr->func.addr, true); - hlist_add_head(&link->tramp_hlist, &tr->progs_hlist[kind]); tr->progs_cnt[kind]++; + + if (link->link.prog->type == BPF_PROG_TYPE_LSM) + bpf_trampoline_toggle_lsm(tr, kind); + err = bpf_trampoline_update(tr, true /* lock_direct_mutex */); if (err) { hlist_del_init(&link->tramp_hlist); @@ -578,7 +596,6 @@ static int __bpf_trampoline_unlink_prog(struct bpf_tramp_link *link, struct bpf_ { struct bpf_tramp_link *link_exiting; enum bpf_tramp_prog_type kind; - bool lsm_link_found = false; int err, num_lsm_progs = 0; kind = bpf_attach_type_to_tramp(link->link.prog); @@ -595,18 +612,14 @@ static int __bpf_trampoline_unlink_prog(struct bpf_tramp_link *link, struct bpf_ tramp_hlist) { if (link_exiting->link.prog->type == BPF_PROG_TYPE_LSM) num_lsm_progs++; - - if (link_exiting->link.prog == link->link.prog) - lsm_link_found = true; } } hlist_del_init(&link->tramp_hlist); tr->progs_cnt[kind]--; - if (lsm_link_found && num_lsm_progs == 1) - bpf_lsm_toggle_hook(tr->func.addr, false); - + if (link->link.prog->type == BPF_PROG_TYPE_LSM) + bpf_trampoline_toggle_lsm(tr, kind); return bpf_trampoline_update(tr, true /* lock_direct_mutex */); } - KP
On Thu, Oct 05, 2023 at 06:07:28PM +0200, KP Singh wrote: SNIP > diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c > index df9699bce372..4f31384b5637 100644 > --- a/kernel/bpf/trampoline.c > +++ b/kernel/bpf/trampoline.c > @@ -511,11 +511,30 @@ static enum bpf_tramp_prog_type > bpf_attach_type_to_tramp(struct bpf_prog *prog) > } > } > > +static void bpf_trampoline_toggle_lsm(struct bpf_trampoline *tr, > + enum bpf_tramp_prog_type kind) > +{ > + struct bpf_tramp_link *link; > + volatile bool found = false; > + > + /* Loop through the links and if any LSM program is attached, ensure > + * that the hook is enabled. > + */ > + hlist_for_each_entry(link, &tr->progs_hlist[kind], tramp_hlist) { > + if (link->link.prog->type == BPF_PROG_TYPE_LSM) { > + found = true; > + break; > + } > + } > + > + bpf_lsm_toggle_hook(tr->func.addr, found); > +} > + > static int __bpf_trampoline_link_prog(struct bpf_tramp_link *link, > struct bpf_trampoline *tr) > { > enum bpf_tramp_prog_type kind; > struct bpf_tramp_link *link_exiting; > - int err = 0, num_lsm_progs = 0; > + int err = 0; > int cnt = 0, i; > > kind = bpf_attach_type_to_tramp(link->link.prog); > @@ -547,15 +566,14 @@ static int __bpf_trampoline_link_prog(struct > bpf_tramp_link *link, struct bpf_tr > /* prog already linked */ > return -EBUSY; > > - if (link_exiting->link.prog->type == BPF_PROG_TYPE_LSM) > - num_lsm_progs++; > } > > - if (!num_lsm_progs && link->link.prog->type == BPF_PROG_TYPE_LSM) > - bpf_lsm_toggle_hook(tr->func.addr, true); > - > hlist_add_head(&link->tramp_hlist, &tr->progs_hlist[kind]); > tr->progs_cnt[kind]++; > + > + if (link->link.prog->type == BPF_PROG_TYPE_LSM) > + bpf_trampoline_toggle_lsm(tr, kind); how about keeping BPF_PROG_TYPE_LSM progs type count of attached programs in bpf_trampoline and toggle lsm on first coming in and last going out? also the trampoline attach is actually made in bpf_trampoline_update, so I wonder it'd make more sense to put it in there, but it's already complicated, so it actually might be easier in here jirka > + > err = bpf_trampoline_update(tr, true /* lock_direct_mutex */); > if (err) { > hlist_del_init(&link->tramp_hlist); > @@ -578,7 +596,6 @@ static int __bpf_trampoline_unlink_prog(struct > bpf_tramp_link *link, struct bpf_ > { > struct bpf_tramp_link *link_exiting; > enum bpf_tramp_prog_type kind; > - bool lsm_link_found = false; > int err, num_lsm_progs = 0; > > kind = bpf_attach_type_to_tramp(link->link.prog); > @@ -595,18 +612,14 @@ static int __bpf_trampoline_unlink_prog(struct > bpf_tramp_link *link, struct bpf_ > tramp_hlist) { > if (link_exiting->link.prog->type == BPF_PROG_TYPE_LSM) > num_lsm_progs++; > - > - if (link_exiting->link.prog == link->link.prog) > - lsm_link_found = true; > } > } > > hlist_del_init(&link->tramp_hlist); > tr->progs_cnt[kind]--; > > - if (lsm_link_found && num_lsm_progs == 1) > - bpf_lsm_toggle_hook(tr->func.addr, false); > - > + if (link->link.prog->type == BPF_PROG_TYPE_LSM) > + bpf_trampoline_toggle_lsm(tr, kind); > return bpf_trampoline_update(tr, true /* lock_direct_mutex */); > } > > > - KP
On Fri, Oct 06, 2023 at 09:27:57AM +0200, Jiri Olsa wrote: SNIP > > static int __bpf_trampoline_link_prog(struct bpf_tramp_link *link, > > struct bpf_trampoline *tr) > > { > > enum bpf_tramp_prog_type kind; > > struct bpf_tramp_link *link_exiting; > > - int err = 0, num_lsm_progs = 0; > > + int err = 0; > > int cnt = 0, i; > > > > kind = bpf_attach_type_to_tramp(link->link.prog); > > @@ -547,15 +566,14 @@ static int __bpf_trampoline_link_prog(struct > > bpf_tramp_link *link, struct bpf_tr > > /* prog already linked */ > > return -EBUSY; > > > > - if (link_exiting->link.prog->type == BPF_PROG_TYPE_LSM) > > - num_lsm_progs++; > > } > > > > - if (!num_lsm_progs && link->link.prog->type == BPF_PROG_TYPE_LSM) > > - bpf_lsm_toggle_hook(tr->func.addr, true); > > - > > hlist_add_head(&link->tramp_hlist, &tr->progs_hlist[kind]); > > tr->progs_cnt[kind]++; > > + > > + if (link->link.prog->type == BPF_PROG_TYPE_LSM) > > + bpf_trampoline_toggle_lsm(tr, kind); > > how about keeping BPF_PROG_TYPE_LSM progs type count of attached programs > in bpf_trampoline and toggle lsm on first coming in and last going out? hm we actually allow other tracing program types to attach to bpf_lsm_* functions, so I wonder we should toggle the lsm hook for each program type (for bpf_lsm_* trampolines) because they'd expect the hook is called but I'm not sure it's a valid use case to have like normal fentry program attached to bpf_lsm_XXX function jirka > > also the trampoline attach is actually made in bpf_trampoline_update, > so I wonder it'd make more sense to put it in there, but it's already > complicated, so it actually might be easier in here > > jirka > > > + > > err = bpf_trampoline_update(tr, true /* lock_direct_mutex */); > > if (err) { > > hlist_del_init(&link->tramp_hlist); > > @@ -578,7 +596,6 @@ static int __bpf_trampoline_unlink_prog(struct > > bpf_tramp_link *link, struct bpf_ > > { > > struct bpf_tramp_link *link_exiting; > > enum bpf_tramp_prog_type kind; > > - bool lsm_link_found = false; > > int err, num_lsm_progs = 0; > > > > kind = bpf_attach_type_to_tramp(link->link.prog); > > @@ -595,18 +612,14 @@ static int __bpf_trampoline_unlink_prog(struct > > bpf_tramp_link *link, struct bpf_ > > tramp_hlist) { > > if (link_exiting->link.prog->type == BPF_PROG_TYPE_LSM) > > num_lsm_progs++; > > - > > - if (link_exiting->link.prog == link->link.prog) > > - lsm_link_found = true; > > } > > } > > > > hlist_del_init(&link->tramp_hlist); > > tr->progs_cnt[kind]--; > > > > - if (lsm_link_found && num_lsm_progs == 1) > > - bpf_lsm_toggle_hook(tr->func.addr, false); > > - > > + if (link->link.prog->type == BPF_PROG_TYPE_LSM) > > + bpf_trampoline_toggle_lsm(tr, kind); > > return bpf_trampoline_update(tr, true /* lock_direct_mutex */); > > } > > > > > > - KP
On Fri, Oct 6, 2023 at 11:05 AM Jiri Olsa <olsajiri@gmail.com> wrote: > > On Fri, Oct 06, 2023 at 09:27:57AM +0200, Jiri Olsa wrote: > > SNIP > > > > static int __bpf_trampoline_link_prog(struct bpf_tramp_link *link, > > > struct bpf_trampoline *tr) > > > { > > > enum bpf_tramp_prog_type kind; > > > struct bpf_tramp_link *link_exiting; > > > - int err = 0, num_lsm_progs = 0; > > > + int err = 0; > > > int cnt = 0, i; > > > > > > kind = bpf_attach_type_to_tramp(link->link.prog); > > > @@ -547,15 +566,14 @@ static int __bpf_trampoline_link_prog(struct > > > bpf_tramp_link *link, struct bpf_tr > > > /* prog already linked */ > > > return -EBUSY; > > > > > > - if (link_exiting->link.prog->type == BPF_PROG_TYPE_LSM) > > > - num_lsm_progs++; > > > } > > > > > > - if (!num_lsm_progs && link->link.prog->type == BPF_PROG_TYPE_LSM) > > > - bpf_lsm_toggle_hook(tr->func.addr, true); > > > - > > > hlist_add_head(&link->tramp_hlist, &tr->progs_hlist[kind]); > > > tr->progs_cnt[kind]++; > > > + > > > + if (link->link.prog->type == BPF_PROG_TYPE_LSM) > > > + bpf_trampoline_toggle_lsm(tr, kind); > > > > how about keeping BPF_PROG_TYPE_LSM progs type count of attached programs > > in bpf_trampoline and toggle lsm on first coming in and last going out? > > hm we actually allow other tracing program types to attach to bpf_lsm_* > functions, so I wonder we should toggle the lsm hook for each program > type (for bpf_lsm_* trampolines) because they'd expect the hook is called Tracing is about tracing, attaching a tracing program to bpf_lsm_ that changes the actual trace here is not something I would recommend. Infact, I have used tracing programs to figure out whether bpf_lsm_* is being called to debug stuff. Tracing users can always attach to security_* if they like. - KP > > but I'm not sure it's a valid use case to have like normal fentry program > attached to bpf_lsm_XXX function > > jirka > > > > > also the trampoline attach is actually made in bpf_trampoline_update, > > so I wonder it'd make more sense to put it in there, but it's already > > complicated, so it actually might be easier in here > > > > jirka > > > > > + > > > err = bpf_trampoline_update(tr, true /* lock_direct_mutex */); > > > if (err) { > > > hlist_del_init(&link->tramp_hlist); > > > @@ -578,7 +596,6 @@ static int __bpf_trampoline_unlink_prog(struct > > > bpf_tramp_link *link, struct bpf_ > > > { > > > struct bpf_tramp_link *link_exiting; > > > enum bpf_tramp_prog_type kind; > > > - bool lsm_link_found = false; > > > int err, num_lsm_progs = 0; > > > > > > kind = bpf_attach_type_to_tramp(link->link.prog); > > > @@ -595,18 +612,14 @@ static int __bpf_trampoline_unlink_prog(struct > > > bpf_tramp_link *link, struct bpf_ > > > tramp_hlist) { > > > if (link_exiting->link.prog->type == BPF_PROG_TYPE_LSM) > > > num_lsm_progs++; > > > - > > > - if (link_exiting->link.prog == link->link.prog) > > > - lsm_link_found = true; > > > } > > > } > > > > > > hlist_del_init(&link->tramp_hlist); > > > tr->progs_cnt[kind]--; > > > > > > - if (lsm_link_found && num_lsm_progs == 1) > > > - bpf_lsm_toggle_hook(tr->func.addr, false); > > > - > > > + if (link->link.prog->type == BPF_PROG_TYPE_LSM) > > > + bpf_trampoline_toggle_lsm(tr, kind); > > > return bpf_trampoline_update(tr, true /* lock_direct_mutex */); > > > } > > > > > > > > > - KP >
On Fri, Oct 6, 2023 at 12:57 PM KP Singh <kpsingh@kernel.org> wrote: > > On Fri, Oct 6, 2023 at 11:05 AM Jiri Olsa <olsajiri@gmail.com> wrote: > > > > On Fri, Oct 06, 2023 at 09:27:57AM +0200, Jiri Olsa wrote: > > > > SNIP > > > > > > static int __bpf_trampoline_link_prog(struct bpf_tramp_link *link, > > > > struct bpf_trampoline *tr) > > > > { > > > > enum bpf_tramp_prog_type kind; > > > > struct bpf_tramp_link *link_exiting; > > > > - int err = 0, num_lsm_progs = 0; > > > > + int err = 0; > > > > int cnt = 0, i; > > > > > > > > kind = bpf_attach_type_to_tramp(link->link.prog); > > > > @@ -547,15 +566,14 @@ static int __bpf_trampoline_link_prog(struct > > > > bpf_tramp_link *link, struct bpf_tr > > > > /* prog already linked */ > > > > return -EBUSY; > > > > > > > > - if (link_exiting->link.prog->type == BPF_PROG_TYPE_LSM) > > > > - num_lsm_progs++; > > > > } > > > > > > > > - if (!num_lsm_progs && link->link.prog->type == BPF_PROG_TYPE_LSM) > > > > - bpf_lsm_toggle_hook(tr->func.addr, true); > > > > - > > > > hlist_add_head(&link->tramp_hlist, &tr->progs_hlist[kind]); > > > > tr->progs_cnt[kind]++; > > > > + > > > > + if (link->link.prog->type == BPF_PROG_TYPE_LSM) > > > > + bpf_trampoline_toggle_lsm(tr, kind); > > > > > > how about keeping BPF_PROG_TYPE_LSM progs type count of attached programs > > > in bpf_trampoline and toggle lsm on first coming in and last going out? > > > > hm we actually allow other tracing program types to attach to bpf_lsm_* > > functions, so I wonder we should toggle the lsm hook for each program > > type (for bpf_lsm_* trampolines) because they'd expect the hook is called > > Tracing is about tracing, attaching a tracing program to bpf_lsm_ that > changes the actual trace here is not something I would recommend. > Infact, I have used tracing programs to figure out whether bpf_lsm_* > is being called to debug stuff. Tracing users can always attach to > security_* if they like. > I will rev up with this fix and send it out as I will be unavailable for the next 2 weeks. - KP > - KP > > > > > but I'm not sure it's a valid use case to have like normal fentry program > > attached to bpf_lsm_XXX function > > > > jirka > > > > > > > > also the trampoline attach is actually made in bpf_trampoline_update, > > > so I wonder it'd make more sense to put it in there, but it's already > > > complicated, so it actually might be easier in here > > > > > > jirka > > > > > > > + > > > > err = bpf_trampoline_update(tr, true /* lock_direct_mutex */); > > > > if (err) { > > > > hlist_del_init(&link->tramp_hlist); > > > > @@ -578,7 +596,6 @@ static int __bpf_trampoline_unlink_prog(struct > > > > bpf_tramp_link *link, struct bpf_ > > > > { > > > > struct bpf_tramp_link *link_exiting; > > > > enum bpf_tramp_prog_type kind; > > > > - bool lsm_link_found = false; > > > > int err, num_lsm_progs = 0; > > > > > > > > kind = bpf_attach_type_to_tramp(link->link.prog); > > > > @@ -595,18 +612,14 @@ static int __bpf_trampoline_unlink_prog(struct > > > > bpf_tramp_link *link, struct bpf_ > > > > tramp_hlist) { > > > > if (link_exiting->link.prog->type == BPF_PROG_TYPE_LSM) > > > > num_lsm_progs++; > > > > - > > > > - if (link_exiting->link.prog == link->link.prog) > > > > - lsm_link_found = true; > > > > } > > > > } > > > > > > > > hlist_del_init(&link->tramp_hlist); > > > > tr->progs_cnt[kind]--; > > > > > > > > - if (lsm_link_found && num_lsm_progs == 1) > > > > - bpf_lsm_toggle_hook(tr->func.addr, false); > > > > - > > > > + if (link->link.prog->type == BPF_PROG_TYPE_LSM) > > > > + bpf_trampoline_toggle_lsm(tr, kind); > > > > return bpf_trampoline_update(tr, true /* lock_direct_mutex */); > > > > } > > > > > > > > > > > > - KP > >
diff --git a/include/linux/bpf.h b/include/linux/bpf.h index 08b10a422560..9a81e0396aaa 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -1159,6 +1159,7 @@ struct bpf_attach_target_info { struct module *tgt_mod; const char *tgt_name; const struct btf_type *tgt_type; + bool is_lsm_target; }; #define BPF_DISPATCHER_MAX 48 /* Fits in 2048B */ diff --git a/include/linux/bpf_lsm.h b/include/linux/bpf_lsm.h index 1de7ece5d36d..5bbc31ac948c 100644 --- a/include/linux/bpf_lsm.h +++ b/include/linux/bpf_lsm.h @@ -29,6 +29,7 @@ int bpf_lsm_verify_prog(struct bpf_verifier_log *vlog, bool bpf_lsm_is_sleepable_hook(u32 btf_id); bool bpf_lsm_is_trusted(const struct bpf_prog *prog); +void bpf_lsm_toggle_hook(void *addr, bool value); static inline struct bpf_storage_blob *bpf_inode( const struct inode *inode) @@ -78,6 +79,10 @@ static inline void bpf_lsm_find_cgroup_shim(const struct bpf_prog *prog, { } +static inline void bpf_lsm_toggle_hook(void *addr, bool value) +{ +} + #endif /* CONFIG_BPF_LSM */ #endif /* _LINUX_BPF_LSM_H */ diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index c77a1859214d..57ffe4eb6d30 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -97,11 +97,14 @@ struct lsm_static_calls_table { * @scalls: The beginning of the array of static calls assigned to this hook. * @hook: The callback for the hook. * @lsm: The name of the lsm that owns this hook. + * @default_state: The state of the LSM hook when initialized. If set to false, + * the static key guarding the hook will be set to disabled. */ struct security_hook_list { struct lsm_static_call *scalls; union security_list_options hook; const char *lsm; + bool default_state; } __randomize_layout; /* @@ -151,7 +154,15 @@ static inline struct xattr *lsm_get_xattr_slot(struct xattr *xattrs, #define LSM_HOOK_INIT(NAME, CALLBACK) \ { \ .scalls = static_calls_table.NAME, \ - .hook = { .NAME = CALLBACK } \ + .hook = { .NAME = CALLBACK }, \ + .default_state = true \ + } + +#define LSM_HOOK_INIT_DISABLED(NAME, CALLBACK) \ + { \ + .scalls = static_calls_table.NAME, \ + .hook = { .NAME = CALLBACK }, \ + .default_state = false \ } extern char *lsm_names; diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c index e97aeda3a86b..df9699bce372 100644 --- a/kernel/bpf/trampoline.c +++ b/kernel/bpf/trampoline.c @@ -13,6 +13,7 @@ #include <linux/bpf_verifier.h> #include <linux/bpf_lsm.h> #include <linux/delay.h> +#include <linux/bpf_lsm.h> /* dummy _ops. The verifier will operate on target program's ops. */ const struct bpf_verifier_ops bpf_extension_verifier_ops = { @@ -514,7 +515,7 @@ static int __bpf_trampoline_link_prog(struct bpf_tramp_link *link, struct bpf_tr { enum bpf_tramp_prog_type kind; struct bpf_tramp_link *link_exiting; - int err = 0; + int err = 0, num_lsm_progs = 0; int cnt = 0, i; kind = bpf_attach_type_to_tramp(link->link.prog); @@ -545,8 +546,14 @@ static int __bpf_trampoline_link_prog(struct bpf_tramp_link *link, struct bpf_tr continue; /* prog already linked */ return -EBUSY; + + if (link_exiting->link.prog->type == BPF_PROG_TYPE_LSM) + num_lsm_progs++; } + if (!num_lsm_progs && link->link.prog->type == BPF_PROG_TYPE_LSM) + bpf_lsm_toggle_hook(tr->func.addr, true); + hlist_add_head(&link->tramp_hlist, &tr->progs_hlist[kind]); tr->progs_cnt[kind]++; err = bpf_trampoline_update(tr, true /* lock_direct_mutex */); @@ -569,8 +576,10 @@ int bpf_trampoline_link_prog(struct bpf_tramp_link *link, struct bpf_trampoline static int __bpf_trampoline_unlink_prog(struct bpf_tramp_link *link, struct bpf_trampoline *tr) { + struct bpf_tramp_link *link_exiting; enum bpf_tramp_prog_type kind; - int err; + bool lsm_link_found = false; + int err, num_lsm_progs = 0; kind = bpf_attach_type_to_tramp(link->link.prog); if (kind == BPF_TRAMP_REPLACE) { @@ -580,8 +589,24 @@ static int __bpf_trampoline_unlink_prog(struct bpf_tramp_link *link, struct bpf_ tr->extension_prog = NULL; return err; } + + if (link->link.prog->type == BPF_PROG_TYPE_LSM) { + hlist_for_each_entry(link_exiting, &tr->progs_hlist[kind], + tramp_hlist) { + if (link_exiting->link.prog->type == BPF_PROG_TYPE_LSM) + num_lsm_progs++; + + if (link_exiting->link.prog == link->link.prog) + lsm_link_found = true; + } + } + hlist_del_init(&link->tramp_hlist); tr->progs_cnt[kind]--; + + if (lsm_link_found && num_lsm_progs == 1) + bpf_lsm_toggle_hook(tr->func.addr, false); + return bpf_trampoline_update(tr, true /* lock_direct_mutex */); } diff --git a/security/bpf/hooks.c b/security/bpf/hooks.c index cfaf1d0e6a5f..1957244196d0 100644 --- a/security/bpf/hooks.c +++ b/security/bpf/hooks.c @@ -8,7 +8,7 @@ static struct security_hook_list bpf_lsm_hooks[] __ro_after_init = { #define LSM_HOOK(RET, DEFAULT, NAME, ...) \ - LSM_HOOK_INIT(NAME, bpf_lsm_##NAME), + LSM_HOOK_INIT_DISABLED(NAME, bpf_lsm_##NAME), #include <linux/lsm_hook_defs.h> #undef LSM_HOOK LSM_HOOK_INIT(inode_free_security, bpf_inode_storage_free), @@ -32,3 +32,26 @@ DEFINE_LSM(bpf) = { .init = bpf_lsm_init, .blobs = &bpf_lsm_blob_sizes }; + +void bpf_lsm_toggle_hook(void *addr, bool value) +{ + struct lsm_static_call *scalls; + struct security_hook_list *h; + int i, j; + + for (i = 0; i < ARRAY_SIZE(bpf_lsm_hooks); i++) { + h = &bpf_lsm_hooks[i]; + scalls = h->scalls; + if (h->hook.lsm_callback == addr) + continue; + + for (j = 0; j < MAX_LSM_COUNT; j++) { + if (scalls[j].hl != h) + continue; + if (value) + static_branch_enable(scalls[j].active); + else + static_branch_disable(scalls[j].active); + } + } +} diff --git a/security/security.c b/security/security.c index c2c2cf6b711f..d1ee72e563cc 100644 --- a/security/security.c +++ b/security/security.c @@ -382,7 +382,8 @@ static void __init lsm_static_call_init(struct security_hook_list *hl) __static_call_update(scall->key, scall->trampoline, hl->hook.lsm_callback); scall->hl = hl; - static_branch_enable(scall->active); + if (hl->default_state) + static_branch_enable(scall->active); return; } scall++;