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 for Android: free password hash cracker in your pocket

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

Message-ID: <20240819113747.31d1ae79@gandalf.local.home>
Date: Mon, 19 Aug 2024 11:37:47 -0400
From: Steven Rostedt <rostedt@...dmis.org>
To: Jiri Olsa <olsajiri@...il.com>
Cc: 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, 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.

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

-- Steve