Message-ID: <20221207040146.zhm3kyduqp7kosqa@macbook-pro-6.dhcp.thefacebook.com>
Date:   Tue, 6 Dec 2022 20:01:46 -0800
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Masami Hiramatsu <mhiramat@...nel.org>
Cc:     LKML <linux-kernel@...r.kernel.org>, bpf@...r.kernel.org,
        Borislav Petkov <bp@...en8.de>,
        Steven Rostedt <rostedt@...dmis.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Kees Cook <keescook@...omium.org>,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        KP Singh <kpsingh@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Florent Revest <revest@...omium.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Christoph Hellwig <hch@...radead.org>,
        Chris Mason <clm@...a.com>
Subject: Re: [PATCH v2] panic: Taint kernel if fault injection has been used

On Tue, Dec 06, 2022 at 04:20:35PM +0900, Masami Hiramatsu wrote:
> On Mon, 5 Dec 2022 18:17:00 -0800
> Alexei Starovoitov <alexei.starovoitov@...il.com> wrote:
> 
> > On Mon, Dec 05, 2022 at 07:59:21AM +0900, Masami Hiramatsu wrote:
> > > On Sun, 4 Dec 2022 14:30:01 -0800
> > > Alexei Starovoitov <alexei.starovoitov@...il.com> wrote:
> > > 
> > > > On Mon, Dec 05, 2022 at 07:22:44AM +0900, Masami Hiramatsu (Google) wrote:
> > > > > From: Masami Hiramatsu (Google) <mhiramat@...nel.org>
> > > > > 
> > > > > Since the function error injection framework in the fault injection
> > > > > subsystem can change the function code flow forcibly, it may cause
> > > > > unexpected behavior (and that is the purpose of this feature) even
> > > > > if it is applied to the ALLOW_ERROR_INJECTION functions.
> > > > > So this feature must be used only for debugging or testing purpose.
> > > > 
> > > > The whole idea of tainting for kernel debugging is questionable.
> > > > There are many other *inject* kconfigs and other debug flags
> > > > for link lists, RCU, sleeping, etc.
> > > > None of them taint the kernel.
> > > > 
> > > > > To identify this in the kernel oops message, add a new taint flag
> > > > 
> > > > Have you ever seen a single oops message because of this particular
> > > > error injection?
> > > 
> > > No, but there is no guarantee that the FEI doesn't cause any issue
> > > in the future too. If it happens, we need to know the precise
> > > information about what FEI/bpf does.
> > > FEI is a kind of temporal Livepatch for testing. If Livepatch taints
> > > the kernel, why doesn't the FEI taint it too?
> > 
> > Live patching can replace an arbitrary function and the kernel has
> > no visibility into what KLP module is doing.
> > While 'bpf error injection' is predictable.
> 
> No, not much predictable because the kernel code can be changed.
> 
> > The functions marked with [BPF_]ALLOW_ERROR_INJECTION can return errors
> > in the normal execution. So the callers of these functions have to deal with errors.
> 
> Right, but it might change something before checking the input, and
> if it rejects the sane input, the caller may go into unexpected
> status (e.g. the caller already checked input value, and does not
> expect the call is fail). Such behaviors are buggy, yes. And the
> FEI is designed for finding such buggy behavior.
> (e.g. injecting error, but the caller passed successfully, it
> means the caller code has some issue.)
> 
> > If kernel panics on such injected error it potentially would have paniced
> > on it anyway.
> 
> Yes, but that doesn't cover all cases. If the function doesn't have
> any internal state but returns an error according to the input,
> FEI can make it return an error even if the input is correct.
> And if it cause a kernel panic, that is a panic that must not
> happen without FEI.
> 
> Thus, the ALLOW_ERROR_INJECTION should only be applied to the
> function which has so-called 'side-effect', e.g. memory allocation,
> external data (except for input data) read, etc. that could cause
> an error regardless of the input value. Then the caller must
> handle such errors.

Not quite. I think you're confusing functions with 'side effect'
with 'pure' functions.
Your point about 'checking args before the call' applies to pure functions.
We have some too: git grep __pure.
Most of the time the compiler can identify the functions that return
the same value with the same args, but sometimes it needs help
and we mark such functions.
Clearly such functions should never be marked as 'error inject'
because changing return value of such function might lead to
wrong code and _it will not be a kernel bug_.
The compiler optimizations rely on the function being pure.
Live kernel patching should be very careful with __pure functions too.
No idea whether they do this now or not.

Of course, there is a category of functions (with or without side effects)
which return values should not be changed by error injecton mechanism.
That's a given. Applying ALLOW_ERROR_INJECTION should not to be taken lightly.

> 
> > At this point crash dump might be necessary to debug.
> 
> Yes. So the TAINT flag can help. Please consider that the TAINT flag
> doesn't mean you are guilty, but this is just a hint for debugging.
> (good for the first triage)

I think you misunderstand the reason behind 'tainted' flags.
It's 'hint for debugging' only on the surface.
See Documentation/admin-guide/tainted-kernels.rst
"... That's why bug reports
from tainted kernels will often be ignored by developers, hence try to reproduce
problems with an untainted kernel."

When 'error injection' finds a kernel bug the kernel developers need to
look into it regardless whether it's syzbot error injection
or whatever other mechanism.

To change the topic to something ... else...

We've just hit this panic using rethook.
[   49.235708] ==================================================================
[   49.236243] BUG: KASAN: use-after-free in rethook_try_get+0x7e/0x380
[   49.236693] Read of size 8 at addr ffff888102e62c88 by task test_progs/1688
[   49.240398]  kasan_report+0x90/0x190
[   49.240934]  rethook_try_get+0x7e/0x380
[   49.244885]  fprobe_handler.part.1+0x119/0x1f0
[   49.245505]  arch_ftrace_ops_list_func+0x17d/0x1d0
[   49.246544]  ftrace_regs_call+0x5/0x52
[   49.247411]  bpf_fentry_test1+0x5/0x10

[   49.262578] Allocated by task 1692:
[   49.262804]  kasan_save_stack+0x1c/0x40
[   49.263059]  kasan_set_track+0x21/0x30
[   49.263335]  __kasan_kmalloc+0x7a/0x90
[   49.263624]  rethook_alloc+0x2c/0xa0
[   49.263879]  fprobe_init_rethook+0x6d/0x170
[   49.264154]  register_fprobe_ips+0xae/0x130

[   49.265938] Freed by task 0:
[   49.266153]  kasan_save_stack+0x1c/0x40
[   49.266440]  kasan_set_track+0x21/0x30
[   49.266705]  kasan_save_free_info+0x26/0x40
[   49.266995]  __kasan_slab_free+0x103/0x190
[   49.267282]  __kmem_cache_free+0x1b7/0x3a0
[   49.267559]  rcu_core+0x4d8/0xd50

[   49.268181] Last potentially related work creation:
[   49.268565]  kasan_save_stack+0x1c/0x40
[   49.268898]  __kasan_record_aux_stack+0xa1/0xb0
[   49.269260]  call_rcu+0x47/0x360
[   49.269526]  unregister_fprobe+0x47/0x80

[   49.281382] general protection fault, probably for non-canonical address 0x57e006e00000000: 0000 [#1] PREEMPT SMP KASAN
[   49.282226] CPU: 6 PID: 1688 Comm: test_progs Tainted: G    B      O       6.1.0-rc7-01508-gf0c5a2d9f234 #4343
[   49.283751] RIP: 0010:rethook_trampoline_handler+0xff/0x1d0
[   49.289900] Call Trace:
[   49.290083]  <TASK>
[   49.290248]  arch_rethook_trampoline_callback+0x6c/0xa0
[   49.290631]  arch_rethook_trampoline+0x2c/0x50
[   49.290964]  ? lock_release+0xad/0x3f0
[   49.291245]  ? bpf_prog_test_run_tracing+0x235/0x380
[   49.291609]  trace_clock_x86_tsc+0x10/0x10

This is just running bpf selftests in parallel mode on 16-cpu VM on bpf-next.
Notice 'Tained' flags.
Please take a look.

Thanks!

Powered by blists - more mailing lists