From: KP Singh <kpsingh@chromium.org>
To: Jann Horn <jannh@google.com>
Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com>,
KP Singh <kpsingh@chromium.org>, bpf <bpf@vger.kernel.org>,
Andrii Nakryiko <andriin@fb.com>,
Brendan Jackman <jackmanb@google.com>,
Florent Revest <revest@google.com>,
Thomas Garnier <thgarnie@google.com>,
Daniel Borkmann <daniel@iogearbox.net>,
Kees Cook <keescook@chromium.org>, Paul Turner <pjt@google.com>,
Florent Revest <revest@chromium.org>,
Brendan Jackman <jackmanb@chromium.org>
Subject: Re: [PATCH bpf-next v9 7/8] bpf: lsm: Add selftests for BPF_PROG_TYPE_LSM
Date: Thu, 2 Apr 2020 13:53:06 +0200 [thread overview]
Message-ID: <20200402115306.GA100892@google.com> (raw)
In-Reply-To: <CAG48ez3SdOVbzJQgo-p=rhKhPdJxjUdraEE6WK5GP3GdenWAAQ@mail.gmail.com>
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.
next prev parent reply other threads:[~2020-04-02 11:53 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-29 0:43 [PATCH bpf-next v9 0/8] MAC and Audit policy using eBPF (KRSI) KP Singh
2020-03-29 0:43 ` [PATCH bpf-next v9 1/8] bpf: Introduce BPF_PROG_TYPE_LSM KP Singh
2020-03-29 0:43 ` [PATCH bpf-next v9 2/8] security: Refactor declaration of LSM hooks KP Singh
2020-03-29 0:43 ` [PATCH bpf-next v9 3/8] bpf: lsm: provide attachment points for BPF LSM programs KP Singh
2020-03-29 0:43 ` [PATCH bpf-next v9 4/8] bpf: lsm: Implement attach, detach and execution KP Singh
2020-03-29 0:43 ` [PATCH bpf-next v9 5/8] bpf: lsm: Initialize the BPF LSM hooks KP Singh
2020-03-29 0:43 ` [PATCH bpf-next v9 6/8] tools/libbpf: Add support for BPF_PROG_TYPE_LSM KP Singh
2020-03-29 0:43 ` [PATCH bpf-next v9 7/8] bpf: lsm: Add selftests " KP Singh
2020-04-02 0:09 ` Alexei Starovoitov
2020-04-02 4:03 ` KP Singh
2020-04-02 4:30 ` Alexei Starovoitov
2020-04-02 5:15 ` Jann Horn
2020-04-02 11:53 ` KP Singh [this message]
2020-04-02 14:38 ` Jann Horn
2020-04-02 14:40 ` KP Singh
2020-04-02 15:57 ` Alexei Starovoitov
2020-03-29 0:43 ` [PATCH bpf-next v9 8/8] bpf: lsm: Add Documentation KP Singh
2020-03-29 23:46 ` [PATCH bpf-next v9 0/8] MAC and Audit policy using eBPF (KRSI) Daniel Borkmann
2020-04-29 12:31 ` Mikko Ylinen
2020-04-29 12:34 ` KP Singh
2020-04-29 12:45 ` Mikko Ylinen
2020-04-29 16:17 ` KP Singh
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200402115306.GA100892@google.com \
--to=kpsingh@chromium.org \
--cc=alexei.starovoitov@gmail.com \
--cc=andriin@fb.com \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=jackmanb@chromium.org \
--cc=jackmanb@google.com \
--cc=jannh@google.com \
--cc=keescook@chromium.org \
--cc=pjt@google.com \
--cc=revest@chromium.org \
--cc=revest@google.com \
--cc=thgarnie@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).