diff mbox series

[SRU,Groovy,Focal/linux-oem-5.10/Hirsute,2/2] bpf: Prevent writable memory-mapping of read-only ringbuf pages

Message ID 20210527001150.38562-3-cascardo@canonical.com
State New
Headers show
Series CVE-2021-3489 fixups | expand

Commit Message

Thadeu Lima de Souza Cascardo May 27, 2021, 12:11 a.m. UTC 
From: Andrii Nakryiko <andrii@kernel.org>

Only the very first page of BPF ringbuf that contains consumer position
counter is supposed to be mapped as writeable by user-space. Producer
position is read-only and can be modified only by the kernel code. BPF ringbuf
data pages are read-only as well and are not meant to be modified by
user-code to maintain integrity of per-record headers.

This patch allows to map only consumer position page as writeable and
everything else is restricted to be read-only. remap_vmalloc_range()
internally adds VM_DONTEXPAND, so all the established memory mappings can't be
extended, which prevents any future violations through mremap()'ing.

Fixes: 457f44363a88 ("bpf: Implement BPF ring buffer and verifier support for it")
Reported-by: Ryota Shiga (Flatt Security)
Reported-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Acked-by: Alexei Starovoitov <ast@kernel.org>
(cherry picked from commit 04ea3086c4d73da7009de1e84962a904139af219 bpf.git)
CVE-2021-3489
Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
---
 kernel/bpf/ringbuf.c | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

Comments

Tim Gardner May 27, 2021, 11:48 a.m. UTC | #1 
The Fixed-by URL in https://ubuntu.com/security/cve-2021-3489 is a 
little weird, but if you tease out the SHA1 from the URL, the commit 
referenced is quite different then this patch. I also noticed that you 
got that patch merged upstream as a fix, yet we were also carrying a 
SAUCE patch that you're reverting here. I'm a little confused.

rtg

On 5/26/21 6:11 PM, Thadeu Lima de Souza Cascardo wrote:
> From: Andrii Nakryiko <andrii@kernel.org>
> 
> Only the very first page of BPF ringbuf that contains consumer position
> counter is supposed to be mapped as writeable by user-space. Producer
> position is read-only and can be modified only by the kernel code. BPF ringbuf
> data pages are read-only as well and are not meant to be modified by
> user-code to maintain integrity of per-record headers.
> 
> This patch allows to map only consumer position page as writeable and
> everything else is restricted to be read-only. remap_vmalloc_range()
> internally adds VM_DONTEXPAND, so all the established memory mappings can't be
> extended, which prevents any future violations through mremap()'ing.
> 
> Fixes: 457f44363a88 ("bpf: Implement BPF ring buffer and verifier support for it")
> Reported-by: Ryota Shiga (Flatt Security)
> Reported-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> Acked-by: Alexei Starovoitov <ast@kernel.org>
> (cherry picked from commit 04ea3086c4d73da7009de1e84962a904139af219 bpf.git)
> CVE-2021-3489
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> ---
>   kernel/bpf/ringbuf.c | 21 ++++++++-------------
>   1 file changed, 8 insertions(+), 13 deletions(-)
> 
> diff --git a/kernel/bpf/ringbuf.c b/kernel/bpf/ringbuf.c
> index 1619afe00ded..e1da496135c6 100644
> --- a/kernel/bpf/ringbuf.c
> +++ b/kernel/bpf/ringbuf.c
> @@ -247,25 +247,20 @@ static int ringbuf_map_get_next_key(struct bpf_map *map, void *key,
>   	return -ENOTSUPP;
>   }
>   
> -static size_t bpf_ringbuf_mmap_page_cnt(const struct bpf_ringbuf *rb)
> -{
> -	size_t data_pages = (rb->mask + 1) >> PAGE_SHIFT;
> -
> -	/* consumer page + producer page + 2 x data pages */
> -	return RINGBUF_POS_PAGES + 2 * data_pages;
> -}
> -
>   static int ringbuf_map_mmap(struct bpf_map *map, struct vm_area_struct *vma)
>   {
>   	struct bpf_ringbuf_map *rb_map;
> -	size_t mmap_sz;
>   
>   	rb_map = container_of(map, struct bpf_ringbuf_map, map);
> -	mmap_sz = bpf_ringbuf_mmap_page_cnt(rb_map->rb) << PAGE_SHIFT;
> -
> -	if (vma->vm_pgoff * PAGE_SIZE + (vma->vm_end - vma->vm_start) > mmap_sz)
> -		return -EINVAL;
>   
> +	if (vma->vm_flags & VM_WRITE) {
> +		/* allow writable mapping for the consumer_pos only */
> +		if (vma->vm_pgoff != 0 || vma->vm_end - vma->vm_start != PAGE_SIZE)
> +			return -EPERM;
> +	} else {
> +		vma->vm_flags &= ~VM_MAYWRITE;
> +	}
> +	/* remap_vmalloc_range() checks size and offset constraints */
>   	return remap_vmalloc_range(vma, rb_map->rb,
>   				   vma->vm_pgoff + RINGBUF_PGOFF);
>   }
>
Thadeu Lima de Souza Cascardo May 27, 2021, 11:54 a.m. UTC | #2 
On Thu, May 27, 2021 at 05:48:58AM -0600, Tim Gardner wrote:
> The Fixed-by URL in https://ubuntu.com/security/cve-2021-3489 is a little
> weird, but if you tease out the SHA1 from the URL, the commit referenced is
> quite different then this patch. I also noticed that you got that patch
> merged upstream as a fix, yet we were also carrying a SAUCE patch that
> you're reverting here. I'm a little confused.
> 
> rtg
> 

The vulnerability was indeed fixed by 4b81ccebaeee885ab1aa1438133f2991e3a2b6ea
("bpf, ringbuf: Deny reserve of buffers larger than ringbuf"), which we also
carry as SAUCE. I haven't touched that part of the fix.

However, allowing the producer page and data pages to be writable by userspace
would also allow other abuses, so that was fixed as well as part of the same
CVE. That change is what we applied with an error here, so I reverted the SAUCE
version and applied the one without the error.

Cascardo.

> On 5/26/21 6:11 PM, Thadeu Lima de Souza Cascardo wrote:
> > From: Andrii Nakryiko <andrii@kernel.org>
> > 
> > Only the very first page of BPF ringbuf that contains consumer position
> > counter is supposed to be mapped as writeable by user-space. Producer
> > position is read-only and can be modified only by the kernel code. BPF ringbuf
> > data pages are read-only as well and are not meant to be modified by
> > user-code to maintain integrity of per-record headers.
> > 
> > This patch allows to map only consumer position page as writeable and
> > everything else is restricted to be read-only. remap_vmalloc_range()
> > internally adds VM_DONTEXPAND, so all the established memory mappings can't be
> > extended, which prevents any future violations through mremap()'ing.
> > 
> > Fixes: 457f44363a88 ("bpf: Implement BPF ring buffer and verifier support for it")
> > Reported-by: Ryota Shiga (Flatt Security)
> > Reported-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> > Acked-by: Alexei Starovoitov <ast@kernel.org>
> > (cherry picked from commit 04ea3086c4d73da7009de1e84962a904139af219 bpf.git)
> > CVE-2021-3489
> > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> > ---
> >   kernel/bpf/ringbuf.c | 21 ++++++++-------------
> >   1 file changed, 8 insertions(+), 13 deletions(-)
> > 
> > diff --git a/kernel/bpf/ringbuf.c b/kernel/bpf/ringbuf.c
> > index 1619afe00ded..e1da496135c6 100644
> > --- a/kernel/bpf/ringbuf.c
> > +++ b/kernel/bpf/ringbuf.c
> > @@ -247,25 +247,20 @@ static int ringbuf_map_get_next_key(struct bpf_map *map, void *key,
> >   	return -ENOTSUPP;
> >   }
> > -static size_t bpf_ringbuf_mmap_page_cnt(const struct bpf_ringbuf *rb)
> > -{
> > -	size_t data_pages = (rb->mask + 1) >> PAGE_SHIFT;
> > -
> > -	/* consumer page + producer page + 2 x data pages */
> > -	return RINGBUF_POS_PAGES + 2 * data_pages;
> > -}
> > -
> >   static int ringbuf_map_mmap(struct bpf_map *map, struct vm_area_struct *vma)
> >   {
> >   	struct bpf_ringbuf_map *rb_map;
> > -	size_t mmap_sz;
> >   	rb_map = container_of(map, struct bpf_ringbuf_map, map);
> > -	mmap_sz = bpf_ringbuf_mmap_page_cnt(rb_map->rb) << PAGE_SHIFT;
> > -
> > -	if (vma->vm_pgoff * PAGE_SIZE + (vma->vm_end - vma->vm_start) > mmap_sz)
> > -		return -EINVAL;
> > +	if (vma->vm_flags & VM_WRITE) {
> > +		/* allow writable mapping for the consumer_pos only */
> > +		if (vma->vm_pgoff != 0 || vma->vm_end - vma->vm_start != PAGE_SIZE)
> > +			return -EPERM;
> > +	} else {
> > +		vma->vm_flags &= ~VM_MAYWRITE;
> > +	}
> > +	/* remap_vmalloc_range() checks size and offset constraints */
> >   	return remap_vmalloc_range(vma, rb_map->rb,
> >   				   vma->vm_pgoff + RINGBUF_PGOFF);
> >   }
> > 
> 
> -- 
> -----------
> Tim Gardner
> Canonical, Inc
diff mbox series

Patch

diff --git a/kernel/bpf/ringbuf.c b/kernel/bpf/ringbuf.c
index 1619afe00ded..e1da496135c6 100644
--- a/kernel/bpf/ringbuf.c
+++ b/kernel/bpf/ringbuf.c
@@ -247,25 +247,20 @@  static int ringbuf_map_get_next_key(struct bpf_map *map, void *key,
 	return -ENOTSUPP;
 }
 
-static size_t bpf_ringbuf_mmap_page_cnt(const struct bpf_ringbuf *rb)
-{
-	size_t data_pages = (rb->mask + 1) >> PAGE_SHIFT;
-
-	/* consumer page + producer page + 2 x data pages */
-	return RINGBUF_POS_PAGES + 2 * data_pages;
-}
-
 static int ringbuf_map_mmap(struct bpf_map *map, struct vm_area_struct *vma)
 {
 	struct bpf_ringbuf_map *rb_map;
-	size_t mmap_sz;
 
 	rb_map = container_of(map, struct bpf_ringbuf_map, map);
-	mmap_sz = bpf_ringbuf_mmap_page_cnt(rb_map->rb) << PAGE_SHIFT;
-
-	if (vma->vm_pgoff * PAGE_SIZE + (vma->vm_end - vma->vm_start) > mmap_sz)
-		return -EINVAL;
 
+	if (vma->vm_flags & VM_WRITE) {
+		/* allow writable mapping for the consumer_pos only */
+		if (vma->vm_pgoff != 0 || vma->vm_end - vma->vm_start != PAGE_SIZE)
+			return -EPERM;
+	} else {
+		vma->vm_flags &= ~VM_MAYWRITE;
+	}
+	/* remap_vmalloc_range() checks size and offset constraints */
 	return remap_vmalloc_range(vma, rb_map->rb,
 				   vma->vm_pgoff + RINGBUF_PGOFF);
 }