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] [day] [month] [year] [list]

Message-ID: <Zv11JnaQIlV8BCnB@krava>
Date: Wed, 2 Oct 2024 18:30:30 +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 Tue, Aug 20, 2024 at 11:05:07AM -0400, Steven Rostedt wrote:
> On Tue, 20 Aug 2024 12:17:31 +0200
> Jiri Olsa <olsajiri@...il.com> wrote:
> 
> > > 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
> 
> Better than a kernel crash, isn't it?  I thought the guarantee of BPF was
> not to ever crash the kernel. Crashing user space may be bad, but not
> always fatal, and something that can be fixed by fixng the BPF program that
> was loaded.
> 
> > 
> > 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
> 
> These are not mutually exclusive. I think you want both. Adding annotation
> is going to be a whack-a-mole game as new tracepoints will always be
> created with new possibly NULL parameters and even old tracepoints can add
> that too. There's nothing to stop that.
> 
> The exception table logic will prevent any missed checks from causing a
> kernel crash, and your annotations will keep user space from crashing.
> 
> -- Steve

sorry for delay.. reviving this after plumbers and other stuff that got in a way

Steven,
we were discussing this in plumbers and you had an idea on doing this
automatically through objtool.. IIRC you meant tracking instructions
that carry argument pointers for NULL checks

AFAICS we'd need to do roughly:
  - for each tracepoint we'd need to interpret one of the functions
    where TP_fast_assign macro gets unwinded:
      perf_trace_##call
      trace_custom_event_raw_event_##call
      trace_event_raw_event_##call
  - we can't tell at this point which argument is kernel object,
    so we'd need to check all arguments (assuming we can get their count)
  - store argument info (if it has null check) into some elf tables and
    use those later in bpf verifier
  - it's all arch specific 

on first look it seems hard and fragile (given it's arch specific)
but I might be easily wrong with above.. do you have an idea on how
this could work?

thanks,
jirka