| Message ID | 20201026125227.54520-2-a.nogikh@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | security: add fault injection to LSM hooks | expand |
On 10/26/2020 5:52 AM, Aleksandr Nogikh wrote: > From: Aleksandr Nogikh <nogikh@google.com> > > Add a fault injection capability to call_int_hook macro. This will > facilitate testing of fault tolerance of the code that invokes > security hooks as well as the fault tolerance of the LSM > implementations themselves. > > Add a KConfig option (CONFIG_FAIL_LSM_HOOKS) that controls whether the > capability is enabled. In order to enable configuration from the user > space, add the standard debugfs entries for fault injection (if > CONFIG_FAULT_INJECTION_DEBUG_FS is enabled). > > Signed-off-by: Aleksandr Nogikh <nogikh@google.com> > --- > v2: > - Renamed should_fail_lsm_hook() to should_fail_lsm_hook(). > --- > lib/Kconfig.debug | 6 +++++ > security/security.c | 53 ++++++++++++++++++++++++++++++++++++++++++--- > 2 files changed, 56 insertions(+), 3 deletions(-) > > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug > index 537cf3c2937d..80d289591e29 100644 > --- a/lib/Kconfig.debug > +++ b/lib/Kconfig.debug > @@ -1803,6 +1803,12 @@ config FAIL_MAKE_REQUEST > help > Provide fault-injection capability for disk IO. > > +config FAIL_LSM_HOOKS > + bool "Fault-injection capability for LSM hooks" > + depends on FAULT_INJECTION > + help > + Provide fault-injection capability for LSM hooks. > + > config FAIL_IO_TIMEOUT > bool "Fault-injection capability for faking disk interrupts" > depends on FAULT_INJECTION && BLOCK > diff --git a/security/security.c b/security/security.c > index 69ff6e2e2cd4..1105ad0f6891 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -28,6 +28,7 @@ > #include <linux/backing-dev.h> > #include <linux/string.h> > #include <linux/msg.h> > +#include <linux/fault-inject.h> > #include <net/flow.h> > > #define MAX_LSM_EVM_XATTR 2 > @@ -669,6 +670,51 @@ static void __init lsm_early_task(struct task_struct *task) > panic("%s: Early task alloc failed.\n", __func__); > } > > + > +#ifdef CONFIG_FAIL_LSM_HOOKS > + > +static struct { > + struct fault_attr attr; > + int retval; > +} fail_lsm_hooks = { > + .attr = FAULT_ATTR_INITIALIZER, > + .retval = -EACCES > +}; > + > +static int __init setup_fail_lsm_hooks(char *str) > +{ > + return setup_fault_attr(&fail_lsm_hooks.attr, str); > +} > +__setup("fail_lsm_hooks=", setup_fail_lsm_hooks); > + > +static int lsm_hooks_inject_fail(void) > +{ > + return should_fail(&fail_lsm_hooks.attr, 1) ? fail_lsm_hooks.retval : 0; > +} > + > +#ifdef CONFIG_FAULT_INJECTION_DEBUG_FS > + > +static int __init fail_lsm_hooks_debugfs(void) > +{ > + umode_t mode = S_IFREG | 0600; > + struct dentry *dir; > + > + dir = fault_create_debugfs_attr("fail_lsm_hooks", NULL, > + &fail_lsm_hooks.attr); > + debugfs_create_u32("retval", mode, dir, &fail_lsm_hooks.retval); > + return 0; > +} > + > +late_initcall(fail_lsm_hooks_debugfs); > + > +#endif /* CONFIG_FAULT_INJECTION_DEBUG_FS */ > + > +#else > + > +static inline int lsm_hooks_inject_fail(void) { return 0; } > + > +#endif /* CONFIG_FAIL_LSM_HOOKS */ > + > /* > * The default value of the LSM hook is defined in linux/lsm_hook_defs.h and > * can be accessed with: > @@ -707,16 +753,17 @@ static void __init lsm_early_task(struct task_struct *task) > } while (0) > > #define call_int_hook(FUNC, IRC, ...) ({ \ > - int RC = IRC; \ > - do { \ > + int RC = lsm_hooks_inject_fail(); \ > + if (RC == 0) { \ Injecting the failure here will prevent the loaded LSM hooks from being called. > struct security_hook_list *P; \ > + RC = IRC; \ > \ > hlist_for_each_entry(P, &security_hook_heads.FUNC, list) { \ > RC = P->hook.FUNC(__VA_ARGS__); \ > if (RC != 0) \ > break; \ > } \ > - } while (0); \ > + } \ Injecting the failure here would allow the loaded LSM hooks to be called. It shouldn't make a difference, but hooks with side-effects are always possible. I don't have an issue either way. > RC; \ > }) >
(resending the previous message in a plain/text mode) On Mon, Oct 26, 2020 at 7:20 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > [...] > > - int RC = IRC; \ > > - do { \ > > + int RC = lsm_hooks_inject_fail(); \ > > + if (RC == 0) { \ > > Injecting the failure here will prevent the loaded LSM hooks from > being called. In this RFC, fault injection was intentionally placed before the code that invokes LSM hooks. The reasoning was that it would simultaneously check how the kernel code reacts to LSM denials and the effect of fault injections on LSM modules. > > > struct security_hook_list *P; \ > > + RC = IRC; \ > > \ > > hlist_for_each_entry(P, &security_hook_heads.FUNC, list) { \ > > RC = P->hook.FUNC(__VA_ARGS__); \ > > if (RC != 0) \ > > break; \ > > } \ > > - } while (0); \ > > + } \ > > Injecting the failure here would allow the loaded LSM hooks to > be called. It shouldn't make a difference, but hooks with side-effects > are always possible. I don't have an issue either way. > > > RC; \ > > }) > > > Should we expect LSM modules to properly handle the cases when their hooks with side effects were not invoked (unlike the selinux crash that is described in the cover letter)? From the source code it seems that a failure/denial from one module prevents the execution of the subsequent hooks, so this looks like a realistic scenario. If that is not true in general and depends on the specific active modules, then it probably makes sense to introduce an option to control whether to inject faults at the beginning of call_int_hook() or after the hooks have been invoked.
On 10/27/2020 10:29 AM, Aleksandr Nogikh wrote: > (resending the previous message in a plain/text mode) > > On Mon, Oct 26, 2020 at 7:20 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > [...] >>> - int RC = IRC; \ >>> - do { \ >>> + int RC = lsm_hooks_inject_fail(); \ >>> + if (RC == 0) { \ >> Injecting the failure here will prevent the loaded LSM hooks from >> being called. > In this RFC, fault injection was intentionally placed before the code that > invokes LSM hooks. The reasoning was that it would simultaneously check > how the kernel code reacts to LSM denials and the effect of fault injections > on LSM modules. > >>> struct security_hook_list *P; \ >>> + RC = IRC; \ >>> \ >>> hlist_for_each_entry(P, &security_hook_heads.FUNC, list) { \ >>> RC = P->hook.FUNC(__VA_ARGS__); \ >>> if (RC != 0) \ >>> break; \ >>> } \ >>> - } while (0); \ >>> + } \ >> Injecting the failure here would allow the loaded LSM hooks to >> be called. It shouldn't make a difference, but hooks with side-effects >> are always possible. I don't have an issue either way. >> >>> RC; \ >>> }) >>> > Should we expect LSM modules to properly handle the cases when their > hooks with side effects were not invoked (unlike the selinux crash that > is described in the cover letter)? From the source code it seems that a > failure/denial from one module prevents the execution of the subsequent > hooks, so this looks like a realistic scenario. Yes. Security modules have to accept the possibility that something ahead of them in the stack will fail. This may be a DAC check, a capability check or another security module. > If that is not true in general and depends on the specific active modules, > then it probably makes sense to introduce an option to control whether to > inject faults at the beginning of call_int_hook() or after the hooks have > been invoked. If you want to do that you could implement it as an LSM. You could place it anywhere in the stack that way. Based on what I see with the BPF lsm that might be more work than it is worth.
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index 537cf3c2937d..80d289591e29 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -1803,6 +1803,12 @@ config FAIL_MAKE_REQUEST help Provide fault-injection capability for disk IO. +config FAIL_LSM_HOOKS + bool "Fault-injection capability for LSM hooks" + depends on FAULT_INJECTION + help + Provide fault-injection capability for LSM hooks. + config FAIL_IO_TIMEOUT bool "Fault-injection capability for faking disk interrupts" depends on FAULT_INJECTION && BLOCK diff --git a/security/security.c b/security/security.c index 69ff6e2e2cd4..1105ad0f6891 100644 --- a/security/security.c +++ b/security/security.c @@ -28,6 +28,7 @@ #include <linux/backing-dev.h> #include <linux/string.h> #include <linux/msg.h> +#include <linux/fault-inject.h> #include <net/flow.h> #define MAX_LSM_EVM_XATTR 2 @@ -669,6 +670,51 @@ static void __init lsm_early_task(struct task_struct *task) panic("%s: Early task alloc failed.\n", __func__); } + +#ifdef CONFIG_FAIL_LSM_HOOKS + +static struct { + struct fault_attr attr; + int retval; +} fail_lsm_hooks = { + .attr = FAULT_ATTR_INITIALIZER, + .retval = -EACCES +}; + +static int __init setup_fail_lsm_hooks(char *str) +{ + return setup_fault_attr(&fail_lsm_hooks.attr, str); +} +__setup("fail_lsm_hooks=", setup_fail_lsm_hooks); + +static int lsm_hooks_inject_fail(void) +{ + return should_fail(&fail_lsm_hooks.attr, 1) ? fail_lsm_hooks.retval : 0; +} + +#ifdef CONFIG_FAULT_INJECTION_DEBUG_FS + +static int __init fail_lsm_hooks_debugfs(void) +{ + umode_t mode = S_IFREG | 0600; + struct dentry *dir; + + dir = fault_create_debugfs_attr("fail_lsm_hooks", NULL, + &fail_lsm_hooks.attr); + debugfs_create_u32("retval", mode, dir, &fail_lsm_hooks.retval); + return 0; +} + +late_initcall(fail_lsm_hooks_debugfs); + +#endif /* CONFIG_FAULT_INJECTION_DEBUG_FS */ + +#else + +static inline int lsm_hooks_inject_fail(void) { return 0; } + +#endif /* CONFIG_FAIL_LSM_HOOKS */ + /* * The default value of the LSM hook is defined in linux/lsm_hook_defs.h and * can be accessed with: @@ -707,16 +753,17 @@ static void __init lsm_early_task(struct task_struct *task) } while (0) #define call_int_hook(FUNC, IRC, ...) ({ \ - int RC = IRC; \ - do { \ + int RC = lsm_hooks_inject_fail(); \ + if (RC == 0) { \ struct security_hook_list *P; \ + RC = IRC; \ \ hlist_for_each_entry(P, &security_hook_heads.FUNC, list) { \ RC = P->hook.FUNC(__VA_ARGS__); \ if (RC != 0) \ break; \ } \ - } while (0); \ + } \ RC; \ })