linux-kernel - Re: NULL pointer deref when running BPF monitor program (6.11.0-rc1)

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZsRtOzhicxAhkmoN@krava>
Date: Tue, 20 Aug 2024 12:17:31 +0200
From: Jiri Olsa <olsajiri@...il.com>
To: Steven Rostedt <rostedt@...dmis.org>
Cc: Jiri Olsa <olsajiri@...il.com>,
	Alexei Starovoitov <alexei.starovoitov@...il.com>,
	Juri Lelli <juri.lelli@...hat.com>, bpf <bpf@...r.kernel.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Artem Savkov <asavkov@...hat.com>,
	"Jose E. Marchesi" <jose.marchesi@...cle.com>
Subject: Re: NULL pointer deref when running BPF monitor program (6.11.0-rc1)

On Mon, Aug 19, 2024 at 11:37:47AM -0400, Steven Rostedt wrote:
> On Mon, 19 Aug 2024 13:47:20 +0200
> Jiri Olsa <olsajiri@...il.com> wrote:
> 
> > verifier assumes that programs attached to the tracepoint can access
> > pointer arguments without checking them for null and some of those
> > programs most likely access such arguments directly
> 
> Hmm, so the verifier made a wrong assumption :-/  That's because that was
> never a requirement for tracepoint arguments and several can easily be
> NULL. That's why the macros have NULL checks for all arguments. For
> example, see include/trace/stages/stage5_get_offsets.h:
> 
>   static inline const char *__string_src(const char *str)
>   {
>        if (!str)
>                return EVENT_NULL_STR;
>        return str;
>   }
> 
> 
> How does the verifier handle accessing function arguments? Because a
> tracepoint call is no different.

verifier is checking program's access to function arguments which in
case of tracepoint is access to context (btf_ctx_access function)

> 
> > 
> > changing that globally and require bpf program to do null check for all
> > pointer arguments will make verifier fail to load existing programs
> > 
> > > 
> > > If you had a macro around the parameter:
> > > 
> > > 		TP_PROTO(struct task_struct *tsk, struct task_struct *__nullable(pi_task)),
> > > 
> > > Could having that go through another macro pass in trace_events.h work?
> > > That is, could we associate the trace event with "nullable" parameters
> > > that could be stored someplace else for you?  
> > 
> > IIUC you mean to store extra data for each tracepoint that would
> > annotate the argument? as Alexei pointed out earlier it might be
> > too much, because we'd be fine with just adding suffix to annotated
> > arguments in __bpf_trace_##call:
> > 
> > 	__bpf_trace_##call(void *__data, proto)                                 \
> > 	{                                                                       \
> > 		CONCATENATE(bpf_trace_run, COUNT_ARGS(args))(__data, CAST_TO_U64(args));        \
> > 	}
> > 
> > with that verifier could easily get suffix information from BTF and
> > once gcc implements btf_type_tag we can easily switch to that
> 
> Could it be possible that the verifier could add to the exception table for
> all accesses to tracepoint arguments? Then if there's a NULL pointer
> dereference, the kernel will not crash but the exception can be sent to the
> user space process instead? That is, it sends SIGSEV to the task accessing
> NULL when it shouldn't.

hm, but that would mean random process that would happened to trigger
the tracepoint would segfault, right? I don't think we can do that

it seems better to teach verifier which tracepoint arguments can be NULL
and deny load of the bpf program that would not check such argument properly

jirka