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

On Thu, Jan 28, 2021 at 04:45:41PM -0800, Andy Lutomirski wrote:

quoted

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

quoted

On Thu, Jan 28, 2021 at 04:29:51PM -0800, Andy Lutomirski wrote:

quoted

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.

probe_read_kernel() does not exist.  get_kernel_nofault() returns -ERANGE.

That was an old name. bpf_probe_read_kernel() is using copy_from_kernel_nofault() now.

quoted

And yes, NULL is a user pointer.  I can write you a little Linux
program that maps some real valid data at user address 0.  As I noted

are you sure? I thought mmap of addr zero was disallowed long ago.

Quite sure.

#define _GNU_SOURCE

#include <stdio.h>
#include <sys/mman.h>
#include <err.h>

int main()
{
    int *ptr = mmap(0, 4096, PROT_READ | PROT_WRITE, MAP_ANONYMOUS |
MAP_PRIVATE | MAP_FIXED, -1, 0);
    if (ptr == MAP_FAILED)
        err(1, "mmap");

    *ptr = 1;
    printf("I just wrote %d to %p\n", *ptr, ptr);
    return 0;
}

Whether this works or not depends on a complicated combination of
sysctl settings, process capabilities, and whether SELinux feels like
adding its own restrictions.  But it does work on current kernels.

quoted

when I first analyzed this bug, because NULL is a user address, bpf is
incorrectly triggering the *user* fault handling code, and that code
is objecting.

I propose the following fix to the x86 code.  I'll send it as a real
patch tomorrow.

https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=x86/fixes&id=f61282777772f375bba7130ae39ccbd7e83878b2

You realize that you propose to panic kernels for all existing tracing users, right?

Do you have a specific security concern with treating fault on NULL special?

The security concern is probably not that severe because of the
aforementioned restrictions, but I haven't analyzed it very carefully.
But I do have a *functionality* concern.  As the original email that
prompted this whole discussion noted, the current x86 fault code does
not do what you want it to do if SMAP is off.  The fact that it mostly
works with SMAP on is luck.  With SMAP off, we will behave erratically
at best.

What exactly could the fault code even do to fix this up?  Something like:

if (addr == 0 && SMAP off && error_code says it's kernel mode && we
don't have permission to map NULL) {
  special care for bpf;
}

This seems arbitrary and fragile.  And it's not obviously
implementable safely without taking locks, which we really ought not
to be doing from inside arbitrary bpf programs.  Keep in mind that, if
SMAP is unavailable, the fault code literally does not know whether
the page fault originated form a valid uaccess region.

There's also always the possibility that you simultaneously run bpf
and something like Wine or DOSEMU2 that actually maps the zero page,
in which case the effect of the bpf code is going to be quite erratic
and will depend on which mm is loaded.

--Andy