| Message ID | 20240507221045.551537-6-kpsingh@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | Reduce overhead of LSMs with static calls | expand |
On Wed, May 08, 2024 at 12:10:45AM +0200, KP Singh wrote: > [...] > +/** > + * security_toggle_hook - Toggle the state of the LSM hook. > + * @hook_addr: The address of the hook to be toggled. > + * @state: Whether to enable for disable the hook. > + * > + * Returns 0 on success, -EINVAL if the address is not found. > + */ > +int security_toggle_hook(void *hook_addr, bool state) > +{ > + struct lsm_static_call *scalls = ((void *)&static_calls_table); > + unsigned long num_entries = > + (sizeof(static_calls_table) / sizeof(struct lsm_static_call)); > + int i; > + > + for (i = 0; i < num_entries; i++) { > + if (!scalls[i].hl) > + continue; > + > + if (scalls[i].hl->hook.lsm_func_addr != hook_addr) > + continue; > + > + if (state) > + static_branch_enable(scalls[i].active); > + else > + static_branch_disable(scalls[i].active); > + return 0; > + } > + return -EINVAL; > +} First of all: patches 1-4 are great. They have a measurable performance benefit; let's get those in. But here I come to patch 5 where I will suggest the exact opposite of what Paul said in v9 for patch 5. :P I don't want to have a global function that can be used to disable LSMs. We got an entire distro (RedHat) to change their SELinux configurations to get rid of CONFIG_SECURITY_SELINUX_DISABLE (and therefore CONFIG_SECURITY_WRITABLE_HOOKS), via commit f22f9aaf6c3d ("selinux: remove the runtime disable functionality"). We cannot reintroduce that, and I'm hoping Paul will agree, given this reminder of LSM history. :) Run-time hook changing should be BPF_LSM specific, if it exists at all.
On Tue, May 7, 2024 at 8:01 PM Kees Cook <keescook@chromium.org> wrote: > > On Wed, May 08, 2024 at 12:10:45AM +0200, KP Singh wrote: > > [...] > > +/** > > + * security_toggle_hook - Toggle the state of the LSM hook. > > + * @hook_addr: The address of the hook to be toggled. > > + * @state: Whether to enable for disable the hook. > > + * > > + * Returns 0 on success, -EINVAL if the address is not found. > > + */ > > +int security_toggle_hook(void *hook_addr, bool state) > > +{ > > + struct lsm_static_call *scalls = ((void *)&static_calls_table); > > + unsigned long num_entries = > > + (sizeof(static_calls_table) / sizeof(struct lsm_static_call)); > > + int i; > > + > > + for (i = 0; i < num_entries; i++) { > > + if (!scalls[i].hl) > > + continue; > > + > > + if (scalls[i].hl->hook.lsm_func_addr != hook_addr) > > + continue; > > + > > + if (state) > > + static_branch_enable(scalls[i].active); > > + else > > + static_branch_disable(scalls[i].active); > > + return 0; > > + } > > + return -EINVAL; > > +} > > First of all: patches 1-4 are great. They have a measurable performance > benefit; let's get those in. > > But here I come to patch 5 where I will suggest the exact opposite of > what Paul said in v9 for patch 5. :P For those looking up v9 of the patchset, you'll be looking for patch *4*, not patch 5, as there were only four patches in the v9 series. Patch 4/5 in the v10 series is a new addition to the stack. Beyond that, I'm guessing you are referring to my comment regarding bpf_lsm_toggle_hook() Kees? The one that starts with "More ugh. If we are going to solve things this way ..."? > I don't want to have a global function that can be used to disable LSMs. > We got an entire distro (RedHat) to change their SELinux configurations > to get rid of CONFIG_SECURITY_SELINUX_DISABLE (and therefore > CONFIG_SECURITY_WRITABLE_HOOKS), via commit f22f9aaf6c3d ("selinux: > remove the runtime disable functionality"). We cannot reintroduce that, > and I'm hoping Paul will agree, given this reminder of LSM history. :) > > Run-time hook changing should be BPF_LSM specific, if it exists at all. I don't want individual LSMs manipulating the LSM hook state directly; they go through the LSM layer to register their hooks, they should go through the LSM layer to unregister or enable/disable their hooks. I'm going to be pretty inflexible on this point. Honestly, I see this more as a problem in the BPF LSM design (although one might argue it's an implementation issue?), just as I saw the SELinux runtime disable as a problem. If you're upset with the runtime hook disable, and you should be, fix the BPF LSM, don't force more bad architecture on the LSM layer.
On Tue, May 07, 2024 at 09:45:09PM -0400, Paul Moore wrote: > I don't want individual LSMs manipulating the LSM hook state directly; > they go through the LSM layer to register their hooks, they should go > through the LSM layer to unregister or enable/disable their hooks. > I'm going to be pretty inflexible on this point. No other LSMs unregister or disable hooks. :) Let's drop patch 5; 1-4 stand alone. > Honestly, I see this more as a problem in the BPF LSM design (although > one might argue it's an implementation issue?), just as I saw the > SELinux runtime disable as a problem. If you're upset with the > runtime hook disable, and you should be, fix the BPF LSM, don't force > more bad architecture on the LSM layer. We'll have to come back to this later. It's a separate (but closely related) issue.
> On 8 May 2024, at 03:45, Paul Moore <paul@paul-moore.com> wrote: > > On Tue, May 7, 2024 at 8:01 PM Kees Cook <keescook@chromium.org> wrote: >> >> On Wed, May 08, 2024 at 12:10:45AM +0200, KP Singh wrote: >>> [...] >>> +/** >>> + * security_toggle_hook - Toggle the state of the LSM hook. >>> + * @hook_addr: The address of the hook to be toggled. >>> + * @state: Whether to enable for disable the hook. >>> + * >>> + * Returns 0 on success, -EINVAL if the address is not found. >>> + */ >>> +int security_toggle_hook(void *hook_addr, bool state) >>> +{ >>> + struct lsm_static_call *scalls = ((void *)&static_calls_table); >>> + unsigned long num_entries = >>> + (sizeof(static_calls_table) / sizeof(struct lsm_static_call)); >>> + int i; >>> + >>> + for (i = 0; i < num_entries; i++) { >>> + if (!scalls[i].hl) >>> + continue; >>> + >>> + if (scalls[i].hl->hook.lsm_func_addr != hook_addr) >>> + continue; >>> + >>> + if (state) >>> + static_branch_enable(scalls[i].active); >>> + else >>> + static_branch_disable(scalls[i].active); >>> + return 0; >>> + } >>> + return -EINVAL; >>> +} >> >> First of all: patches 1-4 are great. They have a measurable performance >> benefit; let's get those in. >> >> But here I come to patch 5 where I will suggest the exact opposite of >> what Paul said in v9 for patch 5. :P > > For those looking up v9 of the patchset, you'll be looking for patch > *4*, not patch 5, as there were only four patches in the v9 series. > Patch 4/5 in the v10 series is a new addition to the stack. > > Beyond that, I'm guessing you are referring to my comment regarding > bpf_lsm_toggle_hook() Kees? The one that starts with "More ugh. If > we are going to solve things this way ..."? > >> I don't want to have a global function that can be used to disable LSMs. >> We got an entire distro (RedHat) to change their SELinux configurations >> to get rid of CONFIG_SECURITY_SELINUX_DISABLE (and therefore >> CONFIG_SECURITY_WRITABLE_HOOKS), via commit f22f9aaf6c3d ("selinux: >> remove the runtime disable functionality"). We cannot reintroduce that, >> and I'm hoping Paul will agree, given this reminder of LSM history. :) >> >> Run-time hook changing should be BPF_LSM specific, if it exists at all. One idea here is that only LSM hooks with default_state = false can be toggled. This would also any ROPs that try to abuse this function. Maybe we can call "default_disabled" .toggleable (or dynamic) and change the corresponding LSM_INIT_TOGGLEABLE. Kees, Paul, this may be a fair middle ground? Something like: diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index 4bd1d47bb9dc..5c0918ed6b80 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -117,7 +117,7 @@ struct security_hook_list { struct lsm_static_call *scalls; union security_list_options hook; const struct lsm_id *lsmid; - bool default_enabled; + bool toggleable; } __randomize_layout; /* @@ -168,14 +168,18 @@ static inline struct xattr *lsm_get_xattr_slot(struct xat> { \ .scalls = static_calls_table.NAME, \ .hook = { .NAME = HOOK }, \ - .default_enabled = true \ + .toggleable = false \ } -#define LSM_HOOK_INIT_DISABLED(NAME, HOOK) \ +/* + * Toggleable LSM hooks are enabled at runtime with + * security_toggle_hook and are initialized as inactive. + */ +#define LSM_HOOK_INIT_TOGGLEABLE(NAME, HOOK) \ { \ .scalls = static_calls_table.NAME, \ .hook = { .NAME = HOOK }, \ - .default_enabled = false \ + .toggleable = true \ } extern char *lsm_names; diff --git a/security/bpf/hooks.c b/security/bpf/hooks.c index ed864f7430a3..ba1c3a19fb12 100644 --- a/security/bpf/hooks.c +++ b/security/bpf/hooks.c @@ -9,7 +9,7 @@ static struct security_hook_list bpf_lsm_hooks[] __ro_after_init = { #define LSM_HOOK(RET, DEFAULT, NAME, ...) \ - LSM_HOOK_INIT_DISABLED(NAME, bpf_lsm_##NAME), + LSM_HOOK_INIT_TOGGLEABLE(NAME, bpf_lsm_##NAME), #include <linux/lsm_hook_defs.h> #undef LSM_HOOK LSM_HOOK_INIT(inode_free_security, bpf_inode_storage_free), + * security_toggle_hook and are initialized as inactive. + */ +#define LSM_HOOK_INIT_TOGGLEABLE(NAME, HOOK) \ { \ .scalls = static_calls_table.NAME, \ .hook = { .NAME = HOOK }, \ - .default_enabled = false \ + .toggleable = true \ } extern char *lsm_names; diff --git a/security/bpf/hooks.c b/security/bpf/hooks.c index ed864f7430a3..ba1c3a19fb12 100644 --- a/security/bpf/hooks.c +++ b/security/bpf/hooks.c @@ -9,7 +9,7 @@ static struct security_hook_list bpf_lsm_hooks[] __ro_after_init = { #define LSM_HOOK(RET, DEFAULT, NAME, ...) \ - LSM_HOOK_INIT_DISABLED(NAME, bpf_lsm_##NAME), + LSM_HOOK_INIT_TOGGLEABLE(NAME, bpf_lsm_##NAME), #include <linux/lsm_hook_defs.h> #undef LSM_HOOK LSM_HOOK_INIT(inode_free_security, bpf_inode_storage_free), kpsingh@kpsingh:~/projects/linux$ git diff diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index 4bd1d47bb9dc..5c0918ed6b80 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -117,7 +117,7 @@ struct security_hook_list { struct lsm_static_call *scalls; union security_list_options hook; const struct lsm_id *lsmid; - bool default_enabled; + bool toggleable; } __randomize_layout; /* @@ -168,14 +168,18 @@ static inline struct xattr *lsm_get_xattr_slot(struct xattr *xattrs, { \ .scalls = static_calls_table.NAME, \ .hook = { .NAME = HOOK }, \ - .default_enabled = true \ + .toggleable = false \ } -#define LSM_HOOK_INIT_DISABLED(NAME, HOOK) \ +/* + * Toggleable LSM hooks are enabled at runtime with + * security_toggle_hook and are initialized as inactive. + */ +#define LSM_HOOK_INIT_TOGGLEABLE(NAME, HOOK) \ { \ .scalls = static_calls_table.NAME, \ .hook = { .NAME = HOOK }, \ - .default_enabled = false \ + .toggleable = true \ } extern char *lsm_names; diff --git a/security/bpf/hooks.c b/security/bpf/hooks.c index ed864f7430a3..ba1c3a19fb12 100644 --- a/security/bpf/hooks.c +++ b/security/bpf/hooks.c @@ -9,7 +9,7 @@ static struct security_hook_list bpf_lsm_hooks[] __ro_after_init = { #define LSM_HOOK(RET, DEFAULT, NAME, ...) \ - LSM_HOOK_INIT_DISABLED(NAME, bpf_lsm_##NAME), + LSM_HOOK_INIT_TOGGLEABLE(NAME, bpf_lsm_##NAME), #include <linux/lsm_hook_defs.h> #undef LSM_HOOK LSM_HOOK_INIT(inode_free_security, bpf_inode_storage_free), diff --git a/security/security.c b/security/security.c index b3a92a67f325..a89eb8fe302b 100644 --- a/security/security.c +++ b/security/security.c @@ -407,7 +407,8 @@ static void __init lsm_static_call_init(struct security_hook_list *hl) __static_call_update(scall->key, scall->trampoline, hl->hook.lsm_func_addr); scall->hl = hl; - if (hl->default_enabled) + /* Toggleable hooks are inactive by default */ + if (!hl->toggleable) static_branch_enable(scall->active); return; } @@ -901,6 +902,9 @@ int security_toggle_hook(void *hook_addr, bool state) int i; for (i = 0; i < num_entries; i++) { + if (!scalls[i].hl->toggleable) + continue; + if (!scalls[i].hl) continue; - KP > > I don't want individual LSMs manipulating the LSM hook state directly; > they go through the LSM layer to register their hooks, they should go > through the LSM layer to unregister or enable/disable their hooks. > I'm going to be pretty inflexible on this point. > > Honestly, I see this more as a problem in the BPF LSM design (although > one might argue it's an implementation issue?), just as I saw the > SELinux runtime disable as a problem. If you're upset with the > runtime hook disable, and you should be, fix the BPF LSM, don't force > more bad architecture on the LSM layer. > > -- > paul-moore.com
On Wed, May 08, 2024 at 09:00:42AM +0200, KP Singh wrote: > > > > On 8 May 2024, at 03:45, Paul Moore <paul@paul-moore.com> wrote: > > > > On Tue, May 7, 2024 at 8:01 PM Kees Cook <keescook@chromium.org> wrote: > >> > >> On Wed, May 08, 2024 at 12:10:45AM +0200, KP Singh wrote: > >>> [...] > >>> +/** > >>> + * security_toggle_hook - Toggle the state of the LSM hook. > >>> + * @hook_addr: The address of the hook to be toggled. > >>> + * @state: Whether to enable for disable the hook. > >>> + * > >>> + * Returns 0 on success, -EINVAL if the address is not found. > >>> + */ > >>> +int security_toggle_hook(void *hook_addr, bool state) > >>> +{ > >>> + struct lsm_static_call *scalls = ((void *)&static_calls_table); > >>> + unsigned long num_entries = > >>> + (sizeof(static_calls_table) / sizeof(struct lsm_static_call)); > >>> + int i; > >>> + > >>> + for (i = 0; i < num_entries; i++) { > >>> + if (!scalls[i].hl) > >>> + continue; > >>> + > >>> + if (scalls[i].hl->hook.lsm_func_addr != hook_addr) > >>> + continue; > >>> + > >>> + if (state) > >>> + static_branch_enable(scalls[i].active); > >>> + else > >>> + static_branch_disable(scalls[i].active); > >>> + return 0; > >>> + } > >>> + return -EINVAL; > >>> +} > >> > >> First of all: patches 1-4 are great. They have a measurable performance > >> benefit; let's get those in. > >> > >> But here I come to patch 5 where I will suggest the exact opposite of > >> what Paul said in v9 for patch 5. :P > > > > For those looking up v9 of the patchset, you'll be looking for patch > > *4*, not patch 5, as there were only four patches in the v9 series. > > Patch 4/5 in the v10 series is a new addition to the stack. > > > > Beyond that, I'm guessing you are referring to my comment regarding > > bpf_lsm_toggle_hook() Kees? The one that starts with "More ugh. If > > we are going to solve things this way ..."? > > > >> I don't want to have a global function that can be used to disable LSMs. > >> We got an entire distro (RedHat) to change their SELinux configurations > >> to get rid of CONFIG_SECURITY_SELINUX_DISABLE (and therefore > >> CONFIG_SECURITY_WRITABLE_HOOKS), via commit f22f9aaf6c3d ("selinux: > >> remove the runtime disable functionality"). We cannot reintroduce that, > >> and I'm hoping Paul will agree, given this reminder of LSM history. :) > >> > >> Run-time hook changing should be BPF_LSM specific, if it exists at all. > > > One idea here is that only LSM hooks with default_state = false can be toggled. > > This would also any ROPs that try to abuse this function. Maybe we can call "default_disabled" .toggleable (or dynamic) > > and change the corresponding LSM_INIT_TOGGLEABLE. Kees, Paul, this may be a fair middle ground? > > Something like: > > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h > index 4bd1d47bb9dc..5c0918ed6b80 100644 > --- a/include/linux/lsm_hooks.h > +++ b/include/linux/lsm_hooks.h > @@ -117,7 +117,7 @@ struct security_hook_list { > struct lsm_static_call *scalls; > union security_list_options hook; > const struct lsm_id *lsmid; > - bool default_enabled; > + bool toggleable; > } __randomize_layout; > > /* > @@ -168,14 +168,18 @@ static inline struct xattr *lsm_get_xattr_slot(struct xattr *xattrs, > { \ > .scalls = static_calls_table.NAME, \ > .hook = { .NAME = HOOK }, \ > - .default_enabled = true \ > + .toggleable = false \ > } > > -#define LSM_HOOK_INIT_DISABLED(NAME, HOOK) \ > +/* > + * Toggleable LSM hooks are enabled at runtime with > + * security_toggle_hook and are initialized as inactive. > + */ > +#define LSM_HOOK_INIT_TOGGLEABLE(NAME, HOOK) \ > { \ > .scalls = static_calls_table.NAME, \ > .hook = { .NAME = HOOK }, \ > - .default_enabled = false \ > + .toggleable = true \ > } > > extern char *lsm_names; > diff --git a/security/bpf/hooks.c b/security/bpf/hooks.c > index ed864f7430a3..ba1c3a19fb12 100644 > --- a/security/bpf/hooks.c > +++ b/security/bpf/hooks.c > @@ -9,7 +9,7 @@ > > static struct security_hook_list bpf_lsm_hooks[] __ro_after_init = { > #define LSM_HOOK(RET, DEFAULT, NAME, ...) \ > - LSM_HOOK_INIT_DISABLED(NAME, bpf_lsm_##NAME), > + LSM_HOOK_INIT_TOGGLEABLE(NAME, bpf_lsm_##NAME), > #include <linux/lsm_hook_defs.h> > #undef LSM_HOOK > LSM_HOOK_INIT(inode_free_security, bpf_inode_storage_free), > diff --git a/security/security.c b/security/security.c > index b3a92a67f325..a89eb8fe302b 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -407,7 +407,8 @@ static void __init lsm_static_call_init(struct security_hook_list *hl) > __static_call_update(scall->key, scall->trampoline, > hl->hook.lsm_func_addr); > scall->hl = hl; > - if (hl->default_enabled) > + /* Toggleable hooks are inactive by default */ > + if (!hl->toggleable) > static_branch_enable(scall->active); > return; > } > @@ -901,6 +902,9 @@ int security_toggle_hook(void *hook_addr, bool state) > int i; > > for (i = 0; i < num_entries; i++) { > + if (!scalls[i].hl->toggleable) > + continue; > + > if (!scalls[i].hl) > continue; Yeah, I like this! It's a routine that is walking read-only data to make the choice, and it's specific to a pre-defined characteristic that an LSM would need to opt into. My concerns are addressed! Thanks! :)
On Tue, May 7, 2024 at 10:35 PM Kees Cook <keescook@chromium.org> wrote: > > On Tue, May 07, 2024 at 09:45:09PM -0400, Paul Moore wrote: > > I don't want individual LSMs manipulating the LSM hook state directly; > > they go through the LSM layer to register their hooks, they should go > > through the LSM layer to unregister or enable/disable their hooks. > > I'm going to be pretty inflexible on this point. > > No other LSMs unregister or disable hooks. :) To be clear, it doesn't really matter if it is all of the LSMs or just one; preserving the interface abstraction as much as possible is worthwhile and good. > > Honestly, I see this more as a problem in the BPF LSM design (although > > one might argue it's an implementation issue?), just as I saw the > > SELinux runtime disable as a problem. If you're upset with the > > runtime hook disable, and you should be, fix the BPF LSM, don't force > > more bad architecture on the LSM layer. > > We'll have to come back to this later. It's a separate (but closely > related) issue. It's a moot point given KP's latest suggestion, but just to give some insight on priorities, correctness is always my primary concern and while the performance improvement in this patchset is a nice win, the most interesting part to me was that it provided a solution for the empty-BPF-LSM-hook problem that has been an ongoing source of problems. Yes, we have made a number of improvements in that area, and I expect those to continue, but selectively enabling/disabling the BPF LSM hook implementations is a big step forward.
On Wed, May 8, 2024 at 3:00 AM KP Singh <kpsingh@kernel.org> wrote: > One idea here is that only LSM hooks with default_state = false can be toggled. > > This would also any ROPs that try to abuse this function. Maybe we can call "default_disabled" .toggleable (or dynamic) > > and change the corresponding LSM_INIT_TOGGLEABLE. Kees, Paul, this may be a fair middle ground? Seems reasonable to me, although I think it's worth respinning to get a proper look at it in context. Some naming bikeshedding below ... > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h > index 4bd1d47bb9dc..5c0918ed6b80 100644 > --- a/include/linux/lsm_hooks.h > +++ b/include/linux/lsm_hooks.h > @@ -117,7 +117,7 @@ struct security_hook_list { > struct lsm_static_call *scalls; > union security_list_options hook; > const struct lsm_id *lsmid; > - bool default_enabled; > + bool toggleable; > } __randomize_layout; How about inverting the boolean and using something like 'fixed' instead of 'toggleable'?
> On 9 May 2024, at 16:24, Paul Moore <paul@paul-moore.com> wrote: > > On Wed, May 8, 2024 at 3:00 AM KP Singh <kpsingh@kernel.org> wrote: >> One idea here is that only LSM hooks with default_state = false can be toggled. >> >> This would also any ROPs that try to abuse this function. Maybe we can call "default_disabled" .toggleable (or dynamic) >> >> and change the corresponding LSM_INIT_TOGGLEABLE. Kees, Paul, this may be a fair middle ground? > > Seems reasonable to me, although I think it's worth respinning to get > a proper look at it in context. Some naming bikeshedding below ... > >> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h >> index 4bd1d47bb9dc..5c0918ed6b80 100644 >> --- a/include/linux/lsm_hooks.h >> +++ b/include/linux/lsm_hooks.h >> @@ -117,7 +117,7 @@ struct security_hook_list { >> struct lsm_static_call *scalls; >> union security_list_options hook; >> const struct lsm_id *lsmid; >> - bool default_enabled; >> + bool toggleable; >> } __randomize_layout; > > How about inverting the boolean and using something like 'fixed' > instead of 'toggleable'? > I would prefer not changing the all the other LSM_HOOK_INIT calls as we change the default behaviour then. How about calling it "dynamic" LSM_HOOK_INIT_DYNAMIC and call the boolean dynamic - KP > -- > paul-moore.com
On Fri, May 10, 2024 at 7:23 AM KP Singh <kpsingh@kernel.org> wrote: > > > > > On 9 May 2024, at 16:24, Paul Moore <paul@paul-moore.com> wrote: > > > > On Wed, May 8, 2024 at 3:00 AM KP Singh <kpsingh@kernel.org> wrote: > >> One idea here is that only LSM hooks with default_state = false can be toggled. > >> > >> This would also any ROPs that try to abuse this function. Maybe we can call "default_disabled" .toggleable (or dynamic) > >> > >> and change the corresponding LSM_INIT_TOGGLEABLE. Kees, Paul, this may be a fair middle ground? > > > > Seems reasonable to me, although I think it's worth respinning to get > > a proper look at it in context. Some naming bikeshedding below ... > > > >> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h > >> index 4bd1d47bb9dc..5c0918ed6b80 100644 > >> --- a/include/linux/lsm_hooks.h > >> +++ b/include/linux/lsm_hooks.h > >> @@ -117,7 +117,7 @@ struct security_hook_list { > >> struct lsm_static_call *scalls; > >> union security_list_options hook; > >> const struct lsm_id *lsmid; > >> - bool default_enabled; > >> + bool toggleable; > >> } __randomize_layout; > > > > How about inverting the boolean and using something like 'fixed' > > instead of 'toggleable'? > > > > I would prefer not changing the all the other LSM_HOOK_INIT calls as we change the default behaviour then. How about calling it "dynamic" > > LSM_HOOK_INIT_DYNAMIC and call the boolean dynamic > Paul, others, any preferences here? - KP > - KP > > > -- > > paul-moore.com >
> On 15 May 2024, at 10:08, KP Singh <kpsingh@kernel.org> wrote: > > On Fri, May 10, 2024 at 7:23 AM KP Singh <kpsingh@kernel.org> wrote: >> >> >> >>> On 9 May 2024, at 16:24, Paul Moore <paul@paul-moore.com> wrote: >>> >>> On Wed, May 8, 2024 at 3:00 AM KP Singh <kpsingh@kernel.org> wrote: >>>> One idea here is that only LSM hooks with default_state = false can be toggled. >>>> >>>> This would also any ROPs that try to abuse this function. Maybe we can call "default_disabled" .toggleable (or dynamic) >>>> >>>> and change the corresponding LSM_INIT_TOGGLEABLE. Kees, Paul, this may be a fair middle ground? >>> >>> Seems reasonable to me, although I think it's worth respinning to get >>> a proper look at it in context. Some naming bikeshedding below ... >>> >>>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h >>>> index 4bd1d47bb9dc..5c0918ed6b80 100644 >>>> --- a/include/linux/lsm_hooks.h >>>> +++ b/include/linux/lsm_hooks.h >>>> @@ -117,7 +117,7 @@ struct security_hook_list { >>>> struct lsm_static_call *scalls; >>>> union security_list_options hook; >>>> const struct lsm_id *lsmid; >>>> - bool default_enabled; >>>> + bool toggleable; >>>> } __randomize_layout; >>> >>> How about inverting the boolean and using something like 'fixed' >>> instead of 'toggleable'? >>> >> >> I would prefer not changing the all the other LSM_HOOK_INIT calls as we change the default behaviour then. How about calling it "dynamic" >> >> LSM_HOOK_INIT_DYNAMIC and call the boolean dynamic >> > > Paul, others, any preferences here? I will throw another in the mix, LSM_HOOK_RUNTIME which captures the nature nicely. (i.e. these hooks are enabled / disabled at runtime). Thoughts? > > - KP > >> - KP >> >>> -- >>> paul-moore.com
On 5/15/2024 9:44 AM, KP Singh wrote: > >> On 15 May 2024, at 10:08, KP Singh <kpsingh@kernel.org> wrote: >> >> On Fri, May 10, 2024 at 7:23 AM KP Singh <kpsingh@kernel.org> wrote: >>> >>> >>>> On 9 May 2024, at 16:24, Paul Moore <paul@paul-moore.com> wrote: >>>> >>>> On Wed, May 8, 2024 at 3:00 AM KP Singh <kpsingh@kernel.org> wrote: >>>>> One idea here is that only LSM hooks with default_state = false can be toggled. >>>>> >>>>> This would also any ROPs that try to abuse this function. Maybe we can call "default_disabled" .toggleable (or dynamic) >>>>> >>>>> and change the corresponding LSM_INIT_TOGGLEABLE. Kees, Paul, this may be a fair middle ground? >>>> Seems reasonable to me, although I think it's worth respinning to get >>>> a proper look at it in context. Some naming bikeshedding below ... >>>> >>>>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h >>>>> index 4bd1d47bb9dc..5c0918ed6b80 100644 >>>>> --- a/include/linux/lsm_hooks.h >>>>> +++ b/include/linux/lsm_hooks.h >>>>> @@ -117,7 +117,7 @@ struct security_hook_list { >>>>> struct lsm_static_call *scalls; >>>>> union security_list_options hook; >>>>> const struct lsm_id *lsmid; >>>>> - bool default_enabled; >>>>> + bool toggleable; >>>>> } __randomize_layout; >>>> How about inverting the boolean and using something like 'fixed' >>>> instead of 'toggleable'? >>>> >>> I would prefer not changing the all the other LSM_HOOK_INIT calls as we change the default behaviour then. How about calling it "dynamic" >>> >>> LSM_HOOK_INIT_DYNAMIC and call the boolean dynamic >>> >> Paul, others, any preferences here? > I will throw another in the mix, LSM_HOOK_RUNTIME which captures the nature nicely. (i.e. these hooks are enabled / disabled at runtime). Thoughts? I think the bike shed should be painted blue. Sorry. Seriously, I like LSM_HOOK_RUNTIME.
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index 5db244308c92..4bd1d47bb9dc 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -110,11 +110,14 @@ struct lsm_id { * @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 struct lsm_id *lsmid; + bool default_enabled; } __randomize_layout; /* @@ -164,7 +167,15 @@ static inline struct xattr *lsm_get_xattr_slot(struct xattr *xattrs, #define LSM_HOOK_INIT(NAME, HOOK) \ { \ .scalls = static_calls_table.NAME, \ - .hook = { .NAME = HOOK } \ + .hook = { .NAME = HOOK }, \ + .default_enabled = true \ + } + +#define LSM_HOOK_INIT_DISABLED(NAME, HOOK) \ + { \ + .scalls = static_calls_table.NAME, \ + .hook = { .NAME = HOOK }, \ + .default_enabled = false \ } extern char *lsm_names; @@ -206,4 +217,17 @@ extern struct lsm_info __start_early_lsm_info[], __end_early_lsm_info[]; extern int lsm_inode_alloc(struct inode *inode); extern struct lsm_static_calls_table static_calls_table __ro_after_init; +#ifdef CONFIG_SECURITY + +int security_toggle_hook(void *addr, bool value); + +#else + +static inline int security_toggle_hook(void *addr, bool value) +{ + return -EINVAL; +} + +#endif /* CONFIG_SECURITY */ + #endif /* ! __LINUX_LSM_HOOKS_H */ diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c index db7599c59c78..5758c5681023 100644 --- a/kernel/bpf/trampoline.c +++ b/kernel/bpf/trampoline.c @@ -521,6 +521,21 @@ static enum bpf_tramp_prog_type bpf_attach_type_to_tramp(struct bpf_prog *prog) } } +static int bpf_trampoline_toggle_lsm(struct bpf_trampoline *tr, + enum bpf_tramp_prog_type kind) +{ + struct bpf_tramp_link *link; + bool found = false; + + hlist_for_each_entry(link, &tr->progs_hlist[kind], tramp_hlist) { + if (link->link.prog->type == BPF_PROG_TYPE_LSM) { + found = true; + break; + } + } + return security_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; @@ -560,11 +575,22 @@ static int __bpf_trampoline_link_prog(struct bpf_tramp_link *link, struct bpf_tr hlist_add_head(&link->tramp_hlist, &tr->progs_hlist[kind]); tr->progs_cnt[kind]++; - err = bpf_trampoline_update(tr, true /* lock_direct_mutex */); - if (err) { - hlist_del_init(&link->tramp_hlist); - tr->progs_cnt[kind]--; + + if (link->link.prog->type == BPF_PROG_TYPE_LSM) { + err = bpf_trampoline_toggle_lsm(tr, kind); + if (err) + goto cleanup; } + + err = bpf_trampoline_update(tr, true /* lock_direct_mutex */); + if (err) + goto cleanup; + + return 0; + +cleanup: + hlist_del_init(&link->tramp_hlist); + tr->progs_cnt[kind]--; return err; } @@ -593,6 +619,12 @@ static int __bpf_trampoline_unlink_prog(struct bpf_tramp_link *link, struct bpf_ } hlist_del_init(&link->tramp_hlist); tr->progs_cnt[kind]--; + + if (link->link.prog->type == BPF_PROG_TYPE_LSM) { + err = bpf_trampoline_toggle_lsm(tr, kind); + WARN(err, "BUG: unable to toggle BPF LSM hook"); + } + return bpf_trampoline_update(tr, true /* lock_direct_mutex */); } diff --git a/security/bpf/hooks.c b/security/bpf/hooks.c index 57b9ffd53c98..ed864f7430a3 100644 --- a/security/bpf/hooks.c +++ b/security/bpf/hooks.c @@ -9,7 +9,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), diff --git a/security/security.c b/security/security.c index 491b807a8a63..b3a92a67f325 100644 --- a/security/security.c +++ b/security/security.c @@ -407,7 +407,8 @@ static void __init lsm_static_call_init(struct security_hook_list *hl) __static_call_update(scall->key, scall->trampoline, hl->hook.lsm_func_addr); scall->hl = hl; - static_branch_enable(scall->active); + if (hl->default_enabled) + static_branch_enable(scall->active); return; } scall++; @@ -885,6 +886,36 @@ int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, u32 *uctx_len, return rc; } +/** + * security_toggle_hook - Toggle the state of the LSM hook. + * @hook_addr: The address of the hook to be toggled. + * @state: Whether to enable for disable the hook. + * + * Returns 0 on success, -EINVAL if the address is not found. + */ +int security_toggle_hook(void *hook_addr, bool state) +{ + struct lsm_static_call *scalls = ((void *)&static_calls_table); + unsigned long num_entries = + (sizeof(static_calls_table) / sizeof(struct lsm_static_call)); + int i; + + for (i = 0; i < num_entries; i++) { + if (!scalls[i].hl) + continue; + + if (scalls[i].hl->hook.lsm_func_addr != hook_addr) + continue; + + if (state) + static_branch_enable(scalls[i].active); + else + static_branch_disable(scalls[i].active); + return 0; + } + return -EINVAL; +} + /* * The default value of the LSM hook is defined in linux/lsm_hook_defs.h and * can be accessed with:
BPF LSM hooks have side-effects (even when a default value is returned), as some hooks end up behaving differently due to the very presence of the hook. The static keys guarding the BPF LSM hooks are disabled by default and enabled only when a BPF program is attached implementing the hook logic. This avoids the issue of the side-effects and also the minor overhead associated with the empty callback. security_file_ioctl: 0xffffffff818f0e30 <+0>: endbr64 0xffffffff818f0e34 <+4>: nopl 0x0(%rax,%rax,1) 0xffffffff818f0e39 <+9>: push %rbp 0xffffffff818f0e3a <+10>: push %r14 0xffffffff818f0e3c <+12>: push %rbx 0xffffffff818f0e3d <+13>: mov %rdx,%rbx 0xffffffff818f0e40 <+16>: mov %esi,%ebp 0xffffffff818f0e42 <+18>: mov %rdi,%r14 0xffffffff818f0e45 <+21>: jmp 0xffffffff818f0e57 <security_file_ioctl+39> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Static key enabled for SELinux 0xffffffff818f0e47 <+23>: xchg %ax,%ax ^^^^^^^^^^^^^^ Static key disabled for BPF. This gets patched when a BPF LSM program is attached 0xffffffff818f0e49 <+25>: xor %eax,%eax 0xffffffff818f0e4b <+27>: xchg %ax,%ax 0xffffffff818f0e4d <+29>: pop %rbx 0xffffffff818f0e4e <+30>: pop %r14 0xffffffff818f0e50 <+32>: pop %rbp 0xffffffff818f0e51 <+33>: cs jmp 0xffffffff82c00000 <__x86_return_thunk> 0xffffffff818f0e57 <+39>: endbr64 0xffffffff818f0e5b <+43>: mov %r14,%rdi 0xffffffff818f0e5e <+46>: mov %ebp,%esi 0xffffffff818f0e60 <+48>: mov %rbx,%rdx 0xffffffff818f0e63 <+51>: call 0xffffffff819033c0 <selinux_file_ioctl> 0xffffffff818f0e68 <+56>: test %eax,%eax 0xffffffff818f0e6a <+58>: jne 0xffffffff818f0e4d <security_file_ioctl+29> 0xffffffff818f0e6c <+60>: jmp 0xffffffff818f0e47 <security_file_ioctl+23> 0xffffffff818f0e6e <+62>: endbr64 0xffffffff818f0e72 <+66>: mov %r14,%rdi 0xffffffff818f0e75 <+69>: mov %ebp,%esi 0xffffffff818f0e77 <+71>: mov %rbx,%rdx 0xffffffff818f0e7a <+74>: call 0xffffffff8141e3b0 <bpf_lsm_file_ioctl> 0xffffffff818f0e7f <+79>: test %eax,%eax 0xffffffff818f0e81 <+81>: jne 0xffffffff818f0e4d <security_file_ioctl+29> 0xffffffff818f0e83 <+83>: jmp 0xffffffff818f0e49 <security_file_ioctl+25> 0xffffffff818f0e85 <+85>: endbr64 0xffffffff818f0e89 <+89>: mov %r14,%rdi 0xffffffff818f0e8c <+92>: mov %ebp,%esi 0xffffffff818f0e8e <+94>: mov %rbx,%rdx 0xffffffff818f0e91 <+97>: pop %rbx 0xffffffff818f0e92 <+98>: pop %r14 0xffffffff818f0e94 <+100>: pop %rbp 0xffffffff818f0e95 <+101>: ret Signed-off-by: KP Singh <kpsingh@kernel.org> --- include/linux/lsm_hooks.h | 26 ++++++++++++++++++++++++- kernel/bpf/trampoline.c | 40 +++++++++++++++++++++++++++++++++++---- security/bpf/hooks.c | 2 +- security/security.c | 33 +++++++++++++++++++++++++++++++- 4 files changed, 94 insertions(+), 7 deletions(-)