Re: [PATCH bpf] x86/bpf: handle bpf-program-triggered exceptions properly

From: Andy Lutomirski <luto@kernel.org>
To: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: Andy Lutomirski <luto@kernel.org>, Yonghong Song <yhs@fb.com>,
	Jann Horn <jannh@google.com>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Andrii Nakryiko <andriin@fb.com>, Martin KaFai Lau <kafai@fb.com>,
	bpf <bpf@vger.kernel.org>, Alexei Starovoitov <ast@kernel.org>,
	kernel-team <kernel-team@fb.com>, X86 ML <x86@kernel.org>,
	KP Singh <kpsingh@kernel.org>
Subject: Re: [PATCH bpf] x86/bpf: handle bpf-program-triggered exceptions properly
Date: Thu, 28 Jan 2021 16:29:51 -0800	[thread overview]
Message-ID: <CALCETrUQuf6FX9EmuZur7vRwbeZBmoKeSYb9Rvx2ETp76SukOg@mail.gmail.com> (raw)
In-Reply-To: <20210129002642.iqlbssmp267zv7f2@ast-mbp.dhcp.thefacebook.com>

On Thu, Jan 28, 2021 at 4:26 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Thu, Jan 28, 2021 at 04:18:24PM -0800, Andy Lutomirski wrote:
> > On Thu, Jan 28, 2021 at 4:11 PM Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> > >
> > > On Thu, Jan 28, 2021 at 03:51:13PM -0800, Andy Lutomirski wrote:
> > > >
> > > > Okay, so I guess you're trying to inline probe_read_kernel().  But
> > > > that means you have to inline a valid implementation.  In particular,
> > > > you need to check that you're accessing *kernel* memory.  Just like
> > >
> > > That check is on the verifier side. It only does it for kernel
> > > pointers with known types.
> > > In a sequnce a->b->c the verifier guarantees that 'a' is valid
> > > kernel pointer and it's also !null. Then it guarantees that offsetof(b)
> > > points to valid kernel field which is also a pointer.
> > > What it doesn't check that b != null, so
> > > that users don't have to write silly code with 'if (p)' after every
> > > dereference.
> >
> > That sounds like a verifier and/or JIT bug.  If you have a pointer p
> > (doesn't matter whether p is what you call a or a->b) and you have not
> > confirmed that p points to the kernel range, you may not generate a
> > load from that pointer.
>
> Please read the explanation again. It's an inlined probe_kernel_read.

Can you point me at the uninlined implementation?  Does it still
exist?  I see get_kernel_nofault(), which is currently buggy, and I
will fix it.

>
> > >
> > > > how get_user() validates that the pointer points into user memory,
> > > > your helper should bounds check the pointer.  On x86, you could check
> > > > the high bit.
> > > >
> > > > As an extra complication, we should really add logic to
> > > > get_kernel_nofault() to verify that the pointer points into actual
> > > > memory as opposed to MMIO space (or future incoherent MKTME space or
> > > > something like that, sigh).  This will severely complicate inlining
> > > > it.  And we should *really* make the same fix to get_kernel_nofault()
> > > > -- it should validate that the pointer is a kernel pointer.
> > > >
> > > > Is this really worth inlining instead of having the BPF JIT generate
> > > > an out of line call to a real C function?  That would let us put in a
> > > > sane implementation.
> > >
> > > It's out of the question.
> > > JIT cannot generate a helper call for single bpf insn without huge overhead.
> > > All registers are used. It needs full save/restore, stack increase, etc.
> > >
> > > Anyhow I bet the bug we're discussing has nothing to do with bpf and jit.
> > > Something got changed and now probe_kernel_read(NULL) warns on !SMAP.
> > > This is something to debug.
> >
> > The bug is in bpf.
>
> If you don't care to debug please don't provide wrong guesses.

BPF generated a NULL pointer dereference (where NULL is a user
pointer) and expected it to recover cleanly. What exactly am I
supposed to debug?  IMO the only thing wrong with the x86 code is that
it doesn't complain more loudly.  I will fix that, too.

--Andy