Re: [PATCH] error-injection: Add prompt for function error injection

[Google]

On Mon, 21 Nov 2022 15:36:08 -0800
Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx> wrote:

> On Mon, Nov 21, 2022 at 11:32 AM Borislav Petkov <bp@xxxxxxxxx> wrote:
> >
> > On Mon, Nov 21, 2022 at 10:44:03AM -0500, Steven Rostedt wrote:
> > > From: "Steven Rostedt (Google)" <rostedt@xxxxxxxxxxx>
> > >
> > > The config to be able to inject error codes into any function annotated
> > > with ALLOW_ERROR_INJECTION() is enabled when CONFIG_FUNCTION_ERROR_INJECTION
> > > is enabled. But unfortunately, this is always enabled on x86 when KPROBES
> > > is enabled, and there's no way to turn it off.
> > >
> > > As kprobes is useful for observability of the kernel, it is useful to have
> > > it enabled in production environments. But error injection should be
> > > avoided. Add a prompt to the config to allow it to be disabled even when
> > > kprobes is enabled, and get rid of the "def_bool y".
> > >
> > > This is a kernel debug feature (it's in Kconfig.debug), and should have
> > > never been something enabled by default.
> > >
> > > Cc: stable@xxxxxxxxxxxxxxx
> > > Fixes: 540adea3809f6 ("error-injection: Separate error-injection from kprobe")
> > > Signed-off-by: Steven Rostedt (Google) <rostedt@xxxxxxxxxxx>
> > > ---
> > >  lib/Kconfig.debug | 8 +++++++-
> > >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > As stated on another thread, debugging production kernels where folks
> > have been injecting errors into functions is not something anyone would
> > like to and have to do. Especially if from looking at system dumps, it
> > is not even clear what has been injected and why, rendering the system
> > unstable and the issue probably unreproducible.
> >
> > Acked-by: Borislav Petkov <bp@xxxxxxx>
> 
> The commit log is bogus and the lack of understanding what
> bpf and error injection hooks are used for expressed
> in this thread is quite sad.
> Disabling error injection makes the system _less_ secure.

Why? I thought this was only used for testing. Or, are you
using this for changing the kernel behavior in production
environment?

For example, the commit 540adea3809f6 ("error-injection: Separate
error-injection from kprobe") specifies that some btrfs functions
to whitelist, which is I thought only for the testing btrfs.

Now it seems more functions related to syscalls registered to
the whitelist. (I didn't notice that...) If it is intended to
filter syscalls, I recommend you to use secomp instead of this.

> But giving people an option to reduce security is a decision
> that every distro and data center has to make on their own.

This function-level override should be used carefully just for
testing linux kernel code. For forcibly filtering some functionality,
it should use LSM or have another facility based on jump label.
(yeah, if it is for security, why can you use LSM?)


Thank you,

-- 
Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>