On Thu, Jan 28, 2021 at 4:26 PM Alexei Starovoitov
[off-list ref] wrote:

On Thu, Jan 28, 2021 at 04:18:24PM -0800, Andy Lutomirski wrote:

quoted

On Thu, Jan 28, 2021 at 4:11 PM Alexei Starovoitov
[off-list ref] wrote:

quoted

On Thu, Jan 28, 2021 at 03:51:13PM -0800, Andy Lutomirski wrote:

quoted

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.

quoted

quoted

quoted

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