[PATCH v2 bpf 1/2] bpf: prevent re-mmap()'ing BPF map as writable for initially r/o mapping

[PATCH v2 bpf 1/2] bpf: prevent re-mmap()'ing BPF map as writable for initially r/o mapping

[Andrii Nakryiko]

VM_MAYWRITE flag during initial memory mapping determines if already mmap()'ed
pages can be later remapped as writable ones through mprotect() call. To
prevent user application to rewrite contents of memory-mapped as read-only and
subsequently frozen BPF map, remove VM_MAYWRITE flag completely on initially
read-only mapping.

Alternatively, we could treat any memory-mapping on unfrozen map as writable
and bump writecnt instead. But there is little legitimate reason to map
BPF map as read-only and then re-mmap() it as writable through mprotect(),
instead of just mmap()'ing it as read/write from the very beginning.

Also, at the suggestion of Jann Horn, drop unnecessary refcounting in mmap
operations. We can just rely on VMA holding reference to BPF map's file
properly.

Fixes: fc9702273e2e ("bpf: Add mmap() support for BPF_MAP_TYPE_ARRAY")
Reported-by: Jann Horn <jannh@google.com>
Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
 kernel/bpf/syscall.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 64783da34202..d85f37239540 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -586,9 +586,7 @@ static void bpf_map_mmap_open(struct vm_area_struct *vma)
 {
 	struct bpf_map *map = vma->vm_file->private_data;
 
-	bpf_map_inc_with_uref(map);
-
-	if (vma->vm_flags & VM_WRITE) {
+	if (vma->vm_flags & VM_MAYWRITE) {
 		mutex_lock(&map->freeze_mutex);
 		map->writecnt++;
 		mutex_unlock(&map->freeze_mutex);
@@ -600,13 +598,11 @@ static void bpf_map_mmap_close(struct vm_area_struct *vma)
 {
 	struct bpf_map *map = vma->vm_file->private_data;
 
-	if (vma->vm_flags & VM_WRITE) {
+	if (vma->vm_flags & VM_MAYWRITE) {
 		mutex_lock(&map->freeze_mutex);
 		map->writecnt--;
 		mutex_unlock(&map->freeze_mutex);
 	}
-
-	bpf_map_put_with_uref(map);
 }
 
 static const struct vm_operations_struct bpf_map_default_vmops = {
@@ -635,14 +631,16 @@ static int bpf_map_mmap(struct file *filp, struct vm_area_struct *vma)
 	/* set default open/close callbacks */
 	vma->vm_ops = &bpf_map_default_vmops;
 	vma->vm_private_data = map;
+	vma->vm_flags &= ~VM_MAYEXEC;
+	if (!(vma->vm_flags & VM_WRITE))
+		/* disallow re-mapping with PROT_WRITE */
+		vma->vm_flags &= ~VM_MAYWRITE;
 
 	err = map->ops->map_mmap(map, vma);
 	if (err)
 		goto out;
 
-	bpf_map_inc_with_uref(map);
-
-	if (vma->vm_flags & VM_WRITE)
+	if (vma->vm_flags & VM_MAYWRITE)
 		map->writecnt++;
 out:
 	mutex_unlock(&map->freeze_mutex);
-- 
2.24.1

[PATCH v2 bpf 2/2] selftests/bpf: validate frozen map contents stays frozen

[Andrii Nakryiko]

Test that frozen and mmap()'ed BPF map can't be mprotect()'ed as writable or
executable memory. Also validate that "downgrading" from writable to read-only
doesn't screw up internal writable count accounting for the purposes of map
freezing.

Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
 tools/testing/selftests/bpf/prog_tests/mmap.c | 62 ++++++++++++++++++-
 1 file changed, 60 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/mmap.c b/tools/testing/selftests/bpf/prog_tests/mmap.c
index 16a814eb4d64..56d80adcf4bd 100644
--- a/tools/testing/selftests/bpf/prog_tests/mmap.c
+++ b/tools/testing/selftests/bpf/prog_tests/mmap.c
@@ -19,15 +19,16 @@ void test_mmap(void)
 	const size_t map_sz = roundup_page(sizeof(struct map_data));
 	const int zero = 0, one = 1, two = 2, far = 1500;
 	const long page_size = sysconf(_SC_PAGE_SIZE);
-	int err, duration = 0, i, data_map_fd;
+	int err, duration = 0, i, data_map_fd, data_map_id, tmp_fd;
 	struct bpf_map *data_map, *bss_map;
 	void *bss_mmaped = NULL, *map_mmaped = NULL, *tmp1, *tmp2;
 	struct test_mmap__bss *bss_data;
+	struct bpf_map_info map_info;
+	__u32 map_info_sz = sizeof(map_info);
 	struct map_data *map_data;
 	struct test_mmap *skel;
 	__u64 val = 0;
 
-
 	skel = test_mmap__open_and_load();
 	if (CHECK(!skel, "skel_open_and_load", "skeleton open/load failed\n"))
 		return;
@@ -36,6 +37,14 @@ void test_mmap(void)
 	data_map = skel->maps.data_map;
 	data_map_fd = bpf_map__fd(data_map);
 
+	/* get map's ID */
+	memset(&map_info, 0, map_info_sz);
+	err = bpf_obj_get_info_by_fd(data_map_fd, &map_info, &map_info_sz);
+	if (CHECK(err, "map_get_info", "failed %d\n", errno))
+		goto cleanup;
+	data_map_id = map_info.id;
+
+	/* mmap BSS map */
 	bss_mmaped = mmap(NULL, bss_sz, PROT_READ | PROT_WRITE, MAP_SHARED,
 			  bpf_map__fd(bss_map), 0);
 	if (CHECK(bss_mmaped == MAP_FAILED, "bss_mmap",
@@ -98,6 +107,10 @@ void test_mmap(void)
 		  "data_map freeze succeeded: err=%d, errno=%d\n", err, errno))
 		goto cleanup;
 
+	err = mprotect(map_mmaped, map_sz, PROT_READ);
+	if (CHECK(err, "mprotect_ro", "mprotect to r/o failed %d\n", errno))
+		goto cleanup;
+
 	/* unmap R/W mapping */
 	err = munmap(map_mmaped, map_sz);
 	map_mmaped = NULL;
@@ -111,6 +124,12 @@ void test_mmap(void)
 		map_mmaped = NULL;
 		goto cleanup;
 	}
+	err = mprotect(map_mmaped, map_sz, PROT_WRITE);
+	if (CHECK(!err, "mprotect_wr", "mprotect() succeeded unexpectedly!\n"))
+		goto cleanup;
+	err = mprotect(map_mmaped, map_sz, PROT_EXEC);
+	if (CHECK(!err, "mprotect_ex", "mprotect() succeeded unexpectedly!\n"))
+		goto cleanup;
 	map_data = map_mmaped;
 
 	/* map/unmap in a loop to test ref counting */
@@ -197,6 +216,45 @@ void test_mmap(void)
 	CHECK_FAIL(map_data->val[far] != 3 * 321);
 
 	munmap(tmp2, 4 * page_size);
+
+	tmp1 = mmap(NULL, map_sz, PROT_READ, MAP_SHARED, data_map_fd, 0);
+	if (CHECK(tmp1 == MAP_FAILED, "last_mmap", "failed %d\n", errno))
+		goto cleanup;
+
+	test_mmap__destroy(skel);
+	skel = NULL;
+	CHECK_FAIL(munmap(bss_mmaped, bss_sz));
+	bss_mmaped = NULL;
+	CHECK_FAIL(munmap(map_mmaped, map_sz));
+	map_mmaped = NULL;
+
+	/* map should be still held by active mmap */
+	tmp_fd = bpf_map_get_fd_by_id(data_map_id);
+	if (CHECK(tmp_fd < 0, "get_map_by_id", "failed %d\n", errno)) {
+		munmap(tmp1, map_sz);
+		goto cleanup;
+	}
+	close(tmp_fd);
+
+	/* this should release data map finally */
+	munmap(tmp1, map_sz);
+
+	/* we need to wait for RCU grace period */
+	for (i = 0; i < 10000; i++) {
+		__u32 id = data_map_id - 1;
+		if (bpf_map_get_next_id(id, &id) || id > data_map_id)
+			break;
+		usleep(1);
+	}
+
+	/* should fail to get map FD by non-existing ID */
+	tmp_fd = bpf_map_get_fd_by_id(data_map_id);
+	if (CHECK(tmp_fd >= 0, "get_map_by_id_after",
+		  "unexpectedly succeeded %d\n", tmp_fd)) {
+		close(tmp_fd);
+		goto cleanup;
+	}
+
 cleanup:
 	if (bss_mmaped)
 		CHECK_FAIL(munmap(bss_mmaped, bss_sz));
-- 
2.24.1

Re: [PATCH v2 bpf 1/2] bpf: prevent re-mmap()'ing BPF map as writable for initially r/o mapping

[Jann Horn]

On Fri, Apr 10, 2020 at 10:26 PM Andrii Nakryiko <andriin@fb.com> wrote:
> VM_MAYWRITE flag during initial memory mapping determines if already mmap()'ed
> pages can be later remapped as writable ones through mprotect() call. To
> prevent user application to rewrite contents of memory-mapped as read-only and
> subsequently frozen BPF map, remove VM_MAYWRITE flag completely on initially
> read-only mapping.
>
> Alternatively, we could treat any memory-mapping on unfrozen map as writable
> and bump writecnt instead. But there is little legitimate reason to map
> BPF map as read-only and then re-mmap() it as writable through mprotect(),
> instead of just mmap()'ing it as read/write from the very beginning.
>
> Also, at the suggestion of Jann Horn, drop unnecessary refcounting in mmap
> operations. We can just rely on VMA holding reference to BPF map's file
> properly.
>
> Fixes: fc9702273e2e ("bpf: Add mmap() support for BPF_MAP_TYPE_ARRAY")
> Reported-by: Jann Horn <jannh@google.com>
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>

Reviewed-by: Jann Horn <jannh@google.com>

(in the sense that I think this patch is good and correct, but does
not fix the entire problem in the bigger picture)

Re: [PATCH v2 bpf 1/2] bpf: prevent re-mmap()'ing BPF map as writable for initially r/o mapping

[Andrii Nakryiko]

On Tue, Apr 14, 2020 at 9:58 AM Jann Horn <jannh@google.com> wrote:
>
> On Fri, Apr 10, 2020 at 10:26 PM Andrii Nakryiko <andriin@fb.com> wrote:
> > VM_MAYWRITE flag during initial memory mapping determines if already mmap()'ed
> > pages can be later remapped as writable ones through mprotect() call. To
> > prevent user application to rewrite contents of memory-mapped as read-only and
> > subsequently frozen BPF map, remove VM_MAYWRITE flag completely on initially
> > read-only mapping.
> >
> > Alternatively, we could treat any memory-mapping on unfrozen map as writable
> > and bump writecnt instead. But there is little legitimate reason to map
> > BPF map as read-only and then re-mmap() it as writable through mprotect(),
> > instead of just mmap()'ing it as read/write from the very beginning.
> >
> > Also, at the suggestion of Jann Horn, drop unnecessary refcounting in mmap
> > operations. We can just rely on VMA holding reference to BPF map's file
> > properly.
> >
> > Fixes: fc9702273e2e ("bpf: Add mmap() support for BPF_MAP_TYPE_ARRAY")
> > Reported-by: Jann Horn <jannh@google.com>
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
>
> Reviewed-by: Jann Horn <jannh@google.com>
>
> (in the sense that I think this patch is good and correct, but does
> not fix the entire problem in the bigger picture)

I agree, we'll continue discussion on the other thread, but this
should be applied as a bug fix anyways.

Re: [PATCH v2 bpf 1/2] bpf: prevent re-mmap()'ing BPF map as writable for initially r/o mapping

[Daniel Borkmann]

On 4/14/20 8:28 PM, Andrii Nakryiko wrote:
> On Tue, Apr 14, 2020 at 9:58 AM Jann Horn <jannh@google.com> wrote:
>> On Fri, Apr 10, 2020 at 10:26 PM Andrii Nakryiko <andriin@fb.com> wrote:
>>> VM_MAYWRITE flag during initial memory mapping determines if already mmap()'ed
>>> pages can be later remapped as writable ones through mprotect() call. To
>>> prevent user application to rewrite contents of memory-mapped as read-only and
>>> subsequently frozen BPF map, remove VM_MAYWRITE flag completely on initially
>>> read-only mapping.
>>>
>>> Alternatively, we could treat any memory-mapping on unfrozen map as writable
>>> and bump writecnt instead. But there is little legitimate reason to map
>>> BPF map as read-only and then re-mmap() it as writable through mprotect(),
>>> instead of just mmap()'ing it as read/write from the very beginning.
>>>
>>> Also, at the suggestion of Jann Horn, drop unnecessary refcounting in mmap
>>> operations. We can just rely on VMA holding reference to BPF map's file
>>> properly.
>>>
>>> Fixes: fc9702273e2e ("bpf: Add mmap() support for BPF_MAP_TYPE_ARRAY")
>>> Reported-by: Jann Horn <jannh@google.com>
>>> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
>>
>> Reviewed-by: Jann Horn <jannh@google.com>
>>
>> (in the sense that I think this patch is good and correct, but does
>> not fix the entire problem in the bigger picture)
> 
> I agree, we'll continue discussion on the other thread, but this
> should be applied as a bug fix anyways.

Applied, thanks!