On Thu, Jan 28, 2021 at 04:29:51PM -0800, Andy Lutomirski wrote: > On Thu, Jan 28, 2021 at 4:26 PM Alexei Starovoitov > <alexei.starovoitov@xxxxxxxxx> 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@xxxxxxxxx> 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. are you saying that NULL is a _user_ pointer?! It's NULL. All zeros. probe_read_kernel(NULL) was returning EFAULT on it and should continue doing so.
