[RFC PATCH v2 0/2] security: add fault injection to LSM hooks

[RFC PATCH v2 0/2] security: add fault injection to LSM hooks

[Aleksandr Nogikh]

From: Aleksandr Nogikh <nogikh@google.com>

Fault injection capabilities[Documentation/fault-injection/fault-injection.rst]
facilitate testing of the stability of the Linux kernel by providing
means to force a number of kernel interfaces to return error
codes. This RFC proposes adding such fault injection capability into
LSM hooks.

The intent is to make it possible to test whether the existing kernel
code properly handles negative return values of LSM hooks. Syzbot
[https://github.com/google/syzkaller/blob/master/docs/syzbot.md] will
automatically do that with the aid of instrumentation tools once these
changes are merged.

Is the attached implementation consistent with the ideas behind LSM
stacking in its current state? In particular, is it reasonable to
expect the existing LSMs to operate normally when they are active and
such fault injection happens?

Local fuzzing of a Linux kernel with this patch has almost instantly
led to two crashes. I'm not sure whether they correspond to actual
issues as the LSM fault injection implementation (and the concept
itself) can be wrong. Here they are:

1. "general protection fault in selinux_inode_free_security". This is
caused by executing security_inode_free() when a fault was injected to
inode_alloc_security() and therefore selinux_inode_alloc_security()
was not executed. In this case, the subsequent inode_free_security()
call executes list_del_init() on an uninitialized list. Theoretically,
this may happen if some other LSM precedes selinux in the hooks list
and its inode_alloc_security hook fails.

A fault was injected to this call_int_hook():
https://elixir.bootlin.com/linux/v5.9/source/security/security.c#L975

Below you can find a call trace for the subsequent crash.
__list_del_entry include/linux/list.h:132 [inline]
list_del_init include/linux/list.h:204 [inline]
inode_free_security security/selinux/hooks.c:337 [inline]
selinux_inode_free_security+0xf0/0x290 security/selinux/hooks.c:2839
security_inode_free+0x46/0xc0 security/security.c:1042
security_inode_alloc+0x161/0x1a0 security/security.c:1027
inode_init_always+0x5a7/0xd10 fs/inode.c:171
alloc_inode+0x82/0x230 fs/inode.c:239
new_inode_pseudo+0x14/0xe0 fs/inode.c:928
sock_alloc+0x3c/0x260 net/socket.c:573
__sock_create+0xb9/0x780 net/socket.c:1391
sock_create net/socket.c:1478 [inline]
__sys_socket+0xef/0x200 net/socket.c:1520
__do_sys_socket net/socket.c:1529 [inline]
__se_sys_socket net/socket.c:1527 [inline]
__x64_sys_socket+0x6f/0xb0 net/socket.c:1527
do_syscall_64+0x2d/0x70 arch/x86/entry/common.c:46
entry_SYSCALL_64_after_hwframe+0x44/0xa9

2. BUG_ON inside security_skb_classify_flow(). Why is it needed there?
https://elixir.bootlin.com/linux/v5.9/source/security/security.c#L2426

---
v2:
* Renamed should_fail_lsm_hook() to should_fail_lsm_hook().
* Extended the documentation.

Aleksandr Nogikh (2):
  security: add fault injection capability
  docs: add fail_lsm_hooks info to fault-injection.rst

 .../fault-injection/fault-injection.rst       |  6 +++
 lib/Kconfig.debug                             |  6 +++
 security/security.c                           | 53 +++++++++++++++++--
 3 files changed, 62 insertions(+), 3 deletions(-)


base-commit: 2ef991b5fdbe828dc8fb8af473dab160729570ed
-- 
2.29.0.rc1.297.gfa9743e501-goog

[RFC PATCH v2 1/2] security: add fault injection capability

[Aleksandr Nogikh]

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) {								\
 		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;							\
 })
 
-- 
2.29.0.rc1.297.gfa9743e501-goog

[RFC PATCH v2 2/2] docs: add fail_lsm_hooks info to fault-injection.rst

[Aleksandr Nogikh]

From: Aleksandr Nogikh <nogikh@google.com>

Describe fail_lsm_hooks fault injection capability.

Signed-off-by: Aleksandr Nogikh <nogikh@google.com>
---
v2:
- Added this commit.
---
 Documentation/fault-injection/fault-injection.rst | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/fault-injection/fault-injection.rst b/Documentation/fault-injection/fault-injection.rst
index 31ecfe44e5b4..48705adfbc18 100644
--- a/Documentation/fault-injection/fault-injection.rst
+++ b/Documentation/fault-injection/fault-injection.rst
@@ -48,6 +48,12 @@ Available fault injection capabilities
   status code is NVME_SC_INVALID_OPCODE with no retry. The status code and
   retry flag can be set via the debugfs.
 
+- fail_lsm_hooks
+
+  injects failures into LSM hooks. When a fault is injected, actual hooks
+  are not executed and a code from /sys/kernel/debug/fail_lsm_hooks/retval
+  is returned (the default value is -EACCES).
+
 
 Configure fault-injection capabilities behavior
 -----------------------------------------------
-- 
2.29.0.rc1.297.gfa9743e501-goog

Re: [RFC PATCH v2 1/2] security: add fault injection capability

[Casey Schaufler]

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;							\
>  })
>  

Re: [RFC PATCH v2 2/2] docs: add fail_lsm_hooks info to fault-injection.rst

[Akinobu Mita]

2020年10月26日(月) 21:52 Aleksandr Nogikh <a.nogikh@gmail.com>:
>
> From: Aleksandr Nogikh <nogikh@google.com>
>
> Describe fail_lsm_hooks fault injection capability.
>
> Signed-off-by: Aleksandr Nogikh <nogikh@google.com>
> ---
> v2:
> - Added this commit.
> ---
>  Documentation/fault-injection/fault-injection.rst | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/Documentation/fault-injection/fault-injection.rst b/Documentation/fault-injection/fault-injection.rst
> index 31ecfe44e5b4..48705adfbc18 100644
> --- a/Documentation/fault-injection/fault-injection.rst
> +++ b/Documentation/fault-injection/fault-injection.rst
> @@ -48,6 +48,12 @@ Available fault injection capabilities
>    status code is NVME_SC_INVALID_OPCODE with no retry. The status code and
>    retry flag can be set via the debugfs.
>
> +- fail_lsm_hooks
> +
> +  injects failures into LSM hooks. When a fault is injected, actual hooks
> +  are not executed and a code from /sys/kernel/debug/fail_lsm_hooks/retval
> +  is returned (the default value is -EACCES).

In addition to this global one, what do you think about per-hook fault
injection,
i.e. /sys/kernel/debug/fail_lsm_hooks/<FUNC>/retval ?

In this case, we need a fault_attr for each hook. (Maybe, we can use the same
technique that is used to define security_hook_heads).

Re: [RFC PATCH v2 1/2] security: add fault injection capability

[Aleksandr Nogikh]

(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.

Re: [RFC PATCH v2 2/2] docs: add fail_lsm_hooks info to fault-injection.rst

[Aleksandr Nogikh]

On Tue, Oct 27, 2020 at 6:31 PM Akinobu Mita <akinobu.mita@gmail.com> wrote:
>
[...]
> In addition to this global one, what do you think about per-hook fault
> injection,
> i.e. /sys/kernel/debug/fail_lsm_hooks/<FUNC>/retval ?

I was thinking about this, but decided to begin with a simple version
that could definitely be useful in practice (for syzbot/syzkaller it is just
necessary to have a fault injection capability that will be triggered via
fail-nth). If per-hook fault injection can also be useful to someone, I
can try to add it as well.

> In this case, we need a fault_attr for each hook. (Maybe, we can use the same
> technique that is used to define security_hook_heads).

Yes, that technique should help to implement the feature in a very concise
way. Thanks for the suggestion.

Re: [RFC PATCH v2 1/2] security: add fault injection capability

[Casey Schaufler]

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.

Re: [RFC PATCH v2 2/2] docs: add fail_lsm_hooks info to fault-injection.rst

[Dmitry Vyukov]

On Tue, Oct 27, 2020 at 6:34 PM Aleksandr Nogikh <nogikh@google.com> wrote:
> [...]
> > In addition to this global one, what do you think about per-hook fault
> > injection,
> > i.e. /sys/kernel/debug/fail_lsm_hooks/<FUNC>/retval ?
>
> I was thinking about this, but decided to begin with a simple version
> that could definitely be useful in practice (for syzbot/syzkaller it is just
> necessary to have a fault injection capability that will be triggered via
> fail-nth). If per-hook fault injection can also be useful to someone, I
> can try to add it as well.

Yes, before we add it, it would be useful to have a clear use case
(otherwise we can add an unused thing, or implement it in a way that
slightly misses the use case).
Note that fail-nth allows to fail a single concrete site for testing,
though it's not super convenient for this as one would need to figure
out the right N first. But as a one-off test it should do.


> > In this case, we need a fault_attr for each hook. (Maybe, we can use the same
> > technique that is used to define security_hook_heads).
>
> Yes, that technique should help to implement the feature in a very concise
> way. Thanks for the suggestion.