diff mbox series

[RFC] panic: Add new taint flag for fault injection

Message ID 166991263326.311919.16890937584677289681.stgit@devnote3
State New
Headers
Series [RFC] panic: Add new taint flag for fault injection |

Commit Message

Masami Hiramatsu (Google) Dec. 1, 2022, 4:37 p.m. UTC 
  From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Since the function error injection framework in the fault injection
subsystem can change the function code flow forcibly, it may cause
unexpected behavior (but that is the purpose of this feature).
To identify this in the kernel oops message, add a new taint flag
for this, and set it if it is (and similar things in BPF) used.

Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 include/linux/panic.h    |    3 ++-
 kernel/fail_function.c   |    2 ++
 kernel/panic.c           |    1 +
 kernel/trace/bpf_trace.c |    2 ++
 4 files changed, 7 insertions(+), 1 deletion(-)

Comments

Kees Cook Dec. 1, 2022, 4:39 p.m. UTC | #1 
On Fri, Dec 02, 2022 at 01:37:13AM +0900, Masami Hiramatsu (Google) wrote:
> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> Since the function error injection framework in the fault injection
> subsystem can change the function code flow forcibly, it may cause
> unexpected behavior (but that is the purpose of this feature).
> To identify this in the kernel oops message, add a new taint flag
> for this, and set it if it is (and similar things in BPF) used.

Why is hooking through BPF considered to be "fault injection" here?
Steven Rostedt Dec. 1, 2022, 4:40 p.m. UTC | #2 
On Fri,  2 Dec 2022 01:37:13 +0900
"Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:

> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> Since the function error injection framework in the fault injection
> subsystem can change the function code flow forcibly, it may cause
> unexpected behavior (but that is the purpose of this feature).
> To identify this in the kernel oops message, add a new taint flag
> for this, and set it if it is (and similar things in BPF) used.
> 
> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
>

Acked-by: Steven Rostedt (Google) <rostedt@goodmis.org>

-- Steve
Steven Rostedt Dec. 1, 2022, 4:48 p.m. UTC | #3 
On Thu, 1 Dec 2022 08:39:28 -0800
Kees Cook <keescook@chromium.org> wrote:

> On Fri, Dec 02, 2022 at 01:37:13AM +0900, Masami Hiramatsu (Google) wrote:
> > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > 
> > Since the function error injection framework in the fault injection
> > subsystem can change the function code flow forcibly, it may cause
> > unexpected behavior (but that is the purpose of this feature).
> > To identify this in the kernel oops message, add a new taint flag
> > for this, and set it if it is (and similar things in BPF) used.  
> 
> Why is hooking through BPF considered to be "fault injection" here?
> 

Have you not been reading this thread?

-- Steve
Kees Cook Dec. 1, 2022, 4:53 p.m. UTC | #4 
On Thu, Dec 01, 2022 at 11:48:48AM -0500, Steven Rostedt wrote:
> On Thu, 1 Dec 2022 08:39:28 -0800
> Kees Cook <keescook@chromium.org> wrote:
> 
> > On Fri, Dec 02, 2022 at 01:37:13AM +0900, Masami Hiramatsu (Google) wrote:
> > > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > > 
> > > Since the function error injection framework in the fault injection
> > > subsystem can change the function code flow forcibly, it may cause
> > > unexpected behavior (but that is the purpose of this feature).
> > > To identify this in the kernel oops message, add a new taint flag
> > > for this, and set it if it is (and similar things in BPF) used.  
> > 
> > Why is hooking through BPF considered to be "fault injection" here?
> > 
> 
> Have you not been reading this thread?

I skimmed it -- trying to catch up from turkey week. If this was already
covered, then please ignore me. It just wasn't obvious from the commit
log why it was included.
Steven Rostedt Dec. 1, 2022, 7:14 p.m. UTC | #5 
On Thu, 1 Dec 2022 08:53:02 -0800
Kees Cook <keescook@chromium.org> wrote:

> > Have you not been reading this thread?  
> 
> I skimmed it -- trying to catch up from turkey week. If this was already
> covered, then please ignore me. It just wasn't obvious from the commit
> log why it was included.

That's a better request :-)

That is, please add why this is needed for BPF (and also include a Link:
tag to this thread).

-- Steve
Chris Mason Dec. 1, 2022, 9 p.m. UTC | #6 
On 12/1/22 2:14 PM, Steven Rostedt wrote:
> On Thu, 1 Dec 2022 08:53:02 -0800
> Kees Cook <keescook@chromium.org> wrote:
> 
>>> Have you not been reading this thread?  
>>
>> I skimmed it -- trying to catch up from turkey week. If this was already
>> covered, then please ignore me. It just wasn't obvious from the commit
>> log why it was included.
> 
> That's a better request :-)
> 
> That is, please add why this is needed for BPF (and also include a Link:
> tag to this thread).

Sorry, I'm completely failing to parse.  Is this directed at Kees or
Benjamin?  I'm also not sure what the this is in "why this is needed for
BPF"?

-chris
Linus Torvalds Dec. 1, 2022, 9:18 p.m. UTC | #7 
On Thu, Dec 1, 2022 at 1:00 PM Chris Mason <clm@meta.com> wrote:
>
> On 12/1/22 2:14 PM, Steven Rostedt wrote:
> >
> > That is, please add why this is needed for BPF (and also include a Link:
> > tag to this thread).
>
> Sorry, I'm completely failing to parse.  Is this directed at Kees or
> Benjamin?  I'm also not sure what the this is in "why this is needed for
> BPF"?

It's not at all "needed for bpf".

There are mis-uses of error injection that have nothing to do with
error injection in linux-next, and some people have argued that said
mis-uses are a valid.

They aren't. They need fixing. Thankfully they haven't made it
upstream, and I most definitely do not want random users mis-using
"error injection" to inject random bpf code for non-error cases.

              Linus
Steven Rostedt Dec. 1, 2022, 9:25 p.m. UTC | #8 
On Thu, 1 Dec 2022 16:00:03 -0500
Chris Mason <clm@meta.com> wrote:

> On 12/1/22 2:14 PM, Steven Rostedt wrote:
> > On Thu, 1 Dec 2022 08:53:02 -0800
> > Kees Cook <keescook@chromium.org> wrote:
> >   
> >>> Have you not been reading this thread?    
> >>
> >> I skimmed it -- trying to catch up from turkey week. If this was already
> >> covered, then please ignore me. It just wasn't obvious from the commit
> >> log why it was included.  
> > 
> > That's a better request :-)
> > 
> > That is, please add why this is needed for BPF (and also include a Link:
> > tag to this thread).  
> 
> Sorry, I'm completely failing to parse.  Is this directed at Kees or
> Benjamin?  I'm also not sure what the this is in "why this is needed for
> BPF"?
> 

It was directed towards Kees. I don't even know who "Benjamin" is. I don't
see a "Benjamin" in the Cc list.

And "this" is for:

--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -2137,6 +2137,8 @@ int perf_event_attach_bpf_prog(struct perf_event *event,
 		goto unlock;
 
 	/* set the new array to event->tp_event and set event->prog */
+	if (prog->kprobe_override)
+		add_taint(TAINT_FAULT_INJECTED, LOCKDEP_NOW_UNRELIABLE);
 	event->prog = prog;
 	event->bpf_cookie = bpf_cookie;
 	rcu_assign_pointer(event->tp_event->prog_array, new_array);

-- Steve
Steven Rostedt Dec. 1, 2022, 9:29 p.m. UTC | #9 
On Thu, 1 Dec 2022 16:25:26 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> > Sorry, I'm completely failing to parse.  Is this directed at Kees or
> > Benjamin?  I'm also not sure what the this is in "why this is needed for
> > BPF"?
> >   
> 
> It was directed towards Kees. I don't even know who "Benjamin" is. I don't
> see a "Benjamin" in the Cc list.

Oh, I see a Benjamin replied to another branch of the email thread.

May I suggest getting a better email client ;-)  One that has proper
threading where it is obvious which email is being replied to.

-- Steve
Masami Hiramatsu (Google) Dec. 2, 2022, 12:46 a.m. UTC | #10 
On Thu, 1 Dec 2022 14:14:26 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Thu, 1 Dec 2022 08:53:02 -0800
> Kees Cook <keescook@chromium.org> wrote:
> 
> > > Have you not been reading this thread?  
> > 
> > I skimmed it -- trying to catch up from turkey week. If this was already
> > covered, then please ignore me. It just wasn't obvious from the commit
> > log why it was included.
> 
> That's a better request :-)
> 
> That is, please add why this is needed for BPF (and also include a Link:
> tag to this thread).

OK, let me update this :)

Thank you!

> 
> -- Steve
Christoph Hellwig Dec. 2, 2022, 6:17 a.m. UTC | #11 
On Thu, Dec 01, 2022 at 01:18:09PM -0800, Linus Torvalds wrote:
> They aren't. They need fixing. Thankfully they haven't made it
> upstream, and I most definitely do not want random users mis-using
> "error injection" to inject random bpf code for non-error cases.

Which seem to be what HID-BPF is all about.  And if I see linux-next
reports correctly that actually got merged despite all the oustanding
objections.
diff mbox series

Patch

diff --git a/include/linux/panic.h b/include/linux/panic.h
index c7759b3f2045..2b03a02d86be 100644
--- a/include/linux/panic.h
+++ b/include/linux/panic.h
@@ -69,7 +69,8 @@  static inline void set_arch_panic_timeout(int timeout, int arch_default_timeout)
 #define TAINT_AUX			16
 #define TAINT_RANDSTRUCT		17
 #define TAINT_TEST			18
-#define TAINT_FLAGS_COUNT		19
+#define TAINT_FAULT_INJECTED		19
+#define TAINT_FLAGS_COUNT		20
 #define TAINT_FLAGS_MAX			((1UL << TAINT_FLAGS_COUNT) - 1)
 
 struct taint_flag {
diff --git a/kernel/fail_function.c b/kernel/fail_function.c
index a7ccd2930c5f..80a743f14a2c 100644
--- a/kernel/fail_function.c
+++ b/kernel/fail_function.c
@@ -9,6 +9,7 @@ 
 #include <linux/kprobes.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/panic.h>
 #include <linux/slab.h>
 #include <linux/uaccess.h>
 
@@ -298,6 +299,7 @@  static ssize_t fei_write(struct file *file, const char __user *buffer,
 		fei_attr_free(attr);
 		goto out;
 	}
+	add_taint(TAINT_FAULT_INJECTED, LOCKDEP_NOW_UNRELIABLE);
 	fei_debugfs_add_attr(attr);
 	list_add_tail(&attr->list, &fei_attr_list);
 	ret = count;
diff --git a/kernel/panic.c b/kernel/panic.c
index da323209f583..e396a5fd9bb6 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -426,6 +426,7 @@  const struct taint_flag taint_flags[TAINT_FLAGS_COUNT] = {
 	[ TAINT_AUX ]			= { 'X', ' ', true },
 	[ TAINT_RANDSTRUCT ]		= { 'T', ' ', true },
 	[ TAINT_TEST ]			= { 'N', ' ', true },
+	[ TAINT_FAULT_INJECTED ]	= { 'J', ' ', false },
 };
 
 /**
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 1ed08967fb97..de0614d9796c 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -2137,6 +2137,8 @@  int perf_event_attach_bpf_prog(struct perf_event *event,
 		goto unlock;
 
 	/* set the new array to event->tp_event and set event->prog */
+	if (prog->kprobe_override)
+		add_taint(TAINT_FAULT_INJECTED, LOCKDEP_NOW_UNRELIABLE);
 	event->prog = prog;
 	event->bpf_cookie = bpf_cookie;
 	rcu_assign_pointer(event->tp_event->prog_array, new_array);