Re: [PATCH bpf-next v9 7/8] bpf: lsm: Add selftests for BPF_PROG_TYPE_LSM

[KP Singh <kpsingh@chromium.org>]

Thanks Jann and Alexei,

On 02-Apr 07:15, Jann Horn wrote:
> On Thu, Apr 2, 2020 at 6:30 AM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> > On Thu, Apr 02, 2020 at 06:03:57AM +0200, KP Singh wrote:
> > > On 01-Apr 17:09, Alexei Starovoitov wrote:
> > > > > +            unsigned long reqprot, unsigned long prot, int ret)
> > > > > +{

[...]

> >
> > and see exactly the same values as bpf side (at least it was nice to see
> > that all CO-RE logic is working as expected :))
> >
> > [   24.787442] start 523000 39b9000
> >
> > I think it has something to do with the way test_progs is linked.
> > But the problem is in condition itself.
> > I suspect you copy-pasted it from selinux_file_mprotect() ?
> > I think it's incorrect there as well.
> > Did we just discover a way to side step selinux protection?
> > Try objdump -h test_progs|grep bss
> > the number I see in vma->vm_start is the beginning of .bss rounded to page boundary.
> > I wonder where your 55dc6e8df000 is coming from.
> 
> I suspect that you're using different versions of libc, or something's
> different in the memory layout, or something like that. The brk region
> is used for memory allocations using brk(), but memory allocations
> using mmap() land outside it. At least some versions of libc try to
> allocate memory for malloc() with brk(), then fall back to mmap() if
> that fails because there's something else behind the current end of
> the brk region; but I think there might also be versions of libc that
> directly use mmap() and don't even try to use brk().

Yeah missed this that heap can also be allocated using mmap:

Quoting the manual:

"""
Normally, malloc() allocates memory from the heap, and adjusts the
size of the heap as required, using sbrk(2).   When  allocating blocks
of  memory larger than MMAP_THRESHOLD bytes, the glibc malloc()
implementation allocates the memory as a private anonymous mapping
using mmap(2).  MMAP_THRESHOLD is 128 kB by default, but is adjustable
using mallopt(3).  Prior to Linux  4.7  allocations performed  using
mmap(2) were unaffected by the RLIMIT_DATA resource limit; since Linux
4.7, this limit is also enforced for allocations performed using
mmap(2).
"""

So it seems like we might have separate MMAP_THRESHOLD or resource
limits.

I updated my test case to check for mmaps on the stack instead:

diff --git a/tools/testing/selftests/bpf/prog_tests/test_lsm.c b/tools/testing/selftests/bpf/prog_tests/test_lsm.c
index 1e4c258de09d..64c13c850611 100644
--- a/tools/testing/selftests/bpf/prog_tests/test_lsm.c
+++ b/tools/testing/selftests/bpf/prog_tests/test_lsm.c
@@ -15,8 +15,13 @@
 
 char *CMD_ARGS[] = {"true", NULL};
 
-int heap_mprotect(void)
+#define PAGE_SIZE 4096
+#define GET_PAGE_ADDR(ADDR)                                    \
+       (char *)(((unsigned long) ADDR) & ~(PAGE_SIZE-1))
+
+int stack_mprotect(void)
 {
+
        void *buf;
        long sz;
        int ret;
@@ -25,12 +30,9 @@ int heap_mprotect(void)
        if (sz < 0)
                return sz;
 
-       buf = memalign(sz, 2 * sz);
-       if (buf == NULL)
-               return -ENOMEM;
-
-       ret = mprotect(buf, sz, PROT_READ | PROT_WRITE | PROT_EXEC);
-       free(buf);
+       buf = alloca(PAGE_SIZE * 3);
+       ret = mprotect(GET_PAGE_ADDR(buf + PAGE_SIZE), PAGE_SIZE,
+                      PROT_READ | PROT_WRITE | PROT_EXEC);
        return ret;
 }
 
@@ -73,8 +75,8 @@ void test_test_lsm(void)
 
        skel->bss->monitored_pid = getpid();
 
-       err = heap_mprotect();
-       if (CHECK(errno != EPERM, "heap_mprotect", "want errno=EPERM, got %d\n",
+       err = stack_mprotect();
+       if (CHECK(errno != EPERM, "stack_mprotect", "want err=EPERM, got %d\n",
                  errno))
                goto close_prog;
 
diff --git a/tools/testing/selftests/bpf/progs/lsm.c b/tools/testing/selftests/bpf/progs/lsm.c
index a4e3c223028d..b4598d4bc4f7 100644
--- a/tools/testing/selftests/bpf/progs/lsm.c
+++ b/tools/testing/selftests/bpf/progs/lsm.c
@@ -23,12 +23,12 @@ int BPF_PROG(test_int_hook, struct vm_area_struct *vma,
                return ret;
 
        __u32 pid = bpf_get_current_pid_tgid() >> 32;
-       int is_heap = 0;
+       int is_stack = 0;
 
-       is_heap = (vma->vm_start >= vma->vm_mm->start_brk &&
-                  vma->vm_end <= vma->vm_mm->brk);
+       is_stack = (vma->vm_start <= vma->vm_mm->start_stack &&
+                   vma->vm_end >= vma->vm_mm->start_stack);
 
-       if (is_heap && monitored_pid == pid) {
+       if (is_stack && monitored_pid == pid) {
                mprotect_count++;
                ret = -EPERM;
        }

and the the logic seems to work for me. Do you think we could use
this instead?

- KP

> 
> So yeah, security checks based on the brk region aren't exactly
> useful; but e.g. in SELinux, both cases have appropriate checks. The
> brk region gets SECCLASS_PROCESS:PROCESS__EXECHEAP, anonymous mmap
> allocations get SECCLASS_PROCESS:PROCESS__EXECMEM in
> file_map_prot_check() instead. (This makes *some* amount of sense -
> although not a lot - because for the brk region you know that it comes
> from something like malloc(), while an anonymous mmap() allocation
> might reasonably be used for JIT executable memory.)
> 
> In other words, you may want to pick something different as test case,
> since the behavior here depends on libc.