[PATCH 0/3] Fixes for usercopy

[PATCH 0/3] Fixes for usercopy

[Matthew Wilcox (Oracle)]

Kees, I'm hoping you'll take these through your tree.  I think they're
all reasonable fixes to go into 5.19.  The first one is essential;
it fixes two different bugs that people have hit.

Matthew Wilcox (Oracle) (3):
  usercopy: Handle vm_map_ram() areas
  usercopy: Cast pointer to an integer once
  usercopy: Make usercopy resilient against ridiculously large copies

 include/linux/vmalloc.h |  1 +
 mm/usercopy.c           | 24 +++++++++++++-----------
 mm/vmalloc.c            |  2 +-
 3 files changed, 15 insertions(+), 12 deletions(-)

-- 
2.35.1

[PATCH 1/3] usercopy: Handle vm_map_ram() areas

[Matthew Wilcox (Oracle)]

vmalloc does not allocate a vm_struct for vm_map_ram() areas.  That causes
us to deny usercopies from those areas.  This affects XFS which uses
vm_map_ram() for its directories.

Fix this by calling find_vmap_area() instead of find_vm_area().

Fixes: 0aef499f3172 ("mm/usercopy: Detect vmalloc overruns")
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 include/linux/vmalloc.h | 1 +
 mm/usercopy.c           | 8 +++++---
 mm/vmalloc.c            | 2 +-
 3 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/include/linux/vmalloc.h b/include/linux/vmalloc.h
index b159c2789961..096d48aa3437 100644
--- a/include/linux/vmalloc.h
+++ b/include/linux/vmalloc.h
@@ -215,6 +215,7 @@ extern struct vm_struct *__get_vm_area_caller(unsigned long size,
 void free_vm_area(struct vm_struct *area);
 extern struct vm_struct *remove_vm_area(const void *addr);
 extern struct vm_struct *find_vm_area(const void *addr);
+struct vmap_area *find_vmap_area(unsigned long addr);
 
 static inline bool is_vm_area_hugepages(const void *addr)
 {
diff --git a/mm/usercopy.c b/mm/usercopy.c
index baeacc735b83..fdd1bed3b90a 100644
--- a/mm/usercopy.c
+++ b/mm/usercopy.c
@@ -173,7 +173,7 @@ static inline void check_heap_object(const void *ptr, unsigned long n,
 	}
 
 	if (is_vmalloc_addr(ptr)) {
-		struct vm_struct *area = find_vm_area(ptr);
+		struct vmap_area *area = find_vmap_area((unsigned long)ptr);
 		unsigned long offset;
 
 		if (!area) {
@@ -181,8 +181,10 @@ static inline void check_heap_object(const void *ptr, unsigned long n,
 			return;
 		}
 
-		offset = ptr - area->addr;
-		if (offset + n > get_vm_area_size(area))
+		/* XXX: We should also abort for free vmap_areas */
+
+		offset = (unsigned long)ptr - area->va_start;
+		if ((unsigned long)ptr + n > area->va_end)
 			usercopy_abort("vmalloc", NULL, to_user, offset, n);
 		return;
 	}
diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index 07db42455dd4..effd1ff6a4b4 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -1798,7 +1798,7 @@ static void free_unmap_vmap_area(struct vmap_area *va)
 	free_vmap_area_noflush(va);
 }
 
-static struct vmap_area *find_vmap_area(unsigned long addr)
+struct vmap_area *find_vmap_area(unsigned long addr)
 {
 	struct vmap_area *va;
 
-- 
2.35.1

[PATCH 2/3] usercopy: Cast pointer to an integer once

[Matthew Wilcox (Oracle)]

Get rid of a lot of annoying casts by setting 'addr' once at the top
of the function.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 mm/usercopy.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/mm/usercopy.c b/mm/usercopy.c
index fdd1bed3b90a..31deee7dd2f5 100644
--- a/mm/usercopy.c
+++ b/mm/usercopy.c
@@ -161,19 +161,20 @@ static inline void check_bogus_address(const unsigned long ptr, unsigned long n,
 static inline void check_heap_object(const void *ptr, unsigned long n,
 				     bool to_user)
 {
+	uintptr_t addr = (uintptr_t)ptr;
 	struct folio *folio;
 
 	if (is_kmap_addr(ptr)) {
-		unsigned long page_end = (unsigned long)ptr | (PAGE_SIZE - 1);
+		unsigned long page_end = addr | (PAGE_SIZE - 1);
 
-		if ((unsigned long)ptr + n - 1 > page_end)
+		if (addr + n - 1 > page_end)
 			usercopy_abort("kmap", NULL, to_user,
 					offset_in_page(ptr), n);
 		return;
 	}
 
 	if (is_vmalloc_addr(ptr)) {
-		struct vmap_area *area = find_vmap_area((unsigned long)ptr);
+		struct vmap_area *area = find_vmap_area(addr);
 		unsigned long offset;
 
 		if (!area) {
@@ -183,8 +184,8 @@ static inline void check_heap_object(const void *ptr, unsigned long n,
 
 		/* XXX: We should also abort for free vmap_areas */
 
-		offset = (unsigned long)ptr - area->va_start;
-		if ((unsigned long)ptr + n > area->va_end)
+		offset = addr - area->va_start;
+		if (addr + n > area->va_end)
 			usercopy_abort("vmalloc", NULL, to_user, offset, n);
 		return;
 	}
-- 
2.35.1

[PATCH 3/3] usercopy: Make usercopy resilient against ridiculously large copies

[Matthew Wilcox (Oracle)]

If 'n' is so large that it's negative, we might wrap around and mistakenly
think that the copy is OK when it's not.  Such a copy would probably
crash, but just doing the arithmetic in a more simple way lets us detect
and refuse this case.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 mm/usercopy.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/mm/usercopy.c b/mm/usercopy.c
index 31deee7dd2f5..ff16083cf1c8 100644
--- a/mm/usercopy.c
+++ b/mm/usercopy.c
@@ -162,20 +162,18 @@ static inline void check_heap_object(const void *ptr, unsigned long n,
 				     bool to_user)
 {
 	uintptr_t addr = (uintptr_t)ptr;
+	unsigned long offset;
 	struct folio *folio;
 
 	if (is_kmap_addr(ptr)) {
-		unsigned long page_end = addr | (PAGE_SIZE - 1);
-
-		if (addr + n - 1 > page_end)
-			usercopy_abort("kmap", NULL, to_user,
-					offset_in_page(ptr), n);
+		offset = offset_in_page(ptr);
+		if (n > PAGE_SIZE - offset)
+			usercopy_abort("kmap", NULL, to_user, offset, n);
 		return;
 	}
 
 	if (is_vmalloc_addr(ptr)) {
 		struct vmap_area *area = find_vmap_area(addr);
-		unsigned long offset;
 
 		if (!area) {
 			usercopy_abort("vmalloc", "no area", to_user, 0, n);
@@ -184,9 +182,10 @@ static inline void check_heap_object(const void *ptr, unsigned long n,
 
 		/* XXX: We should also abort for free vmap_areas */
 
-		offset = addr - area->va_start;
-		if (addr + n > area->va_end)
+		if (n > area->va_end - addr) {
+			offset = addr - area->va_start;
 			usercopy_abort("vmalloc", NULL, to_user, offset, n);
+		}
 		return;
 	}
 
@@ -199,8 +198,8 @@ static inline void check_heap_object(const void *ptr, unsigned long n,
 		/* Check slab allocator for flags and size. */
 		__check_heap_object(ptr, n, folio_slab(folio), to_user);
 	} else if (folio_test_large(folio)) {
-		unsigned long offset = ptr - folio_address(folio);
-		if (offset + n > folio_size(folio))
+		offset = ptr - folio_address(folio);
+		if (n > folio_size(folio) - offset)
 			usercopy_abort("page alloc", NULL, to_user, offset, n);
 	}
 }
-- 
2.35.1

Re: [PATCH 0/3] Fixes for usercopy

[Zorro Lang]

On Sun, Jun 12, 2022 at 10:32:24PM +0100, Matthew Wilcox (Oracle) wrote:
> Kees, I'm hoping you'll take these through your tree.  I think they're
> all reasonable fixes to go into 5.19.  The first one is essential;
> it fixes two different bugs that people have hit.
> 
> Matthew Wilcox (Oracle) (3):
>   usercopy: Handle vm_map_ram() areas
>   usercopy: Cast pointer to an integer once
>   usercopy: Make usercopy resilient against ridiculously large copies

Hi Matthew,

[Quick response] ...
After merging this patchset onto linux v5.19-rc2, I can't reproduce bug [1]
anymore. I'd like to keep the test running, if it find other issues I'll
feedback.

Thanks,
Zorro

[1]
https://bugzilla.kernel.org/show_bug.cgi?id=216073

> 
>  include/linux/vmalloc.h |  1 +
>  mm/usercopy.c           | 24 +++++++++++++-----------
>  mm/vmalloc.c            |  2 +-
>  3 files changed, 15 insertions(+), 12 deletions(-)
> 
> -- 
> 2.35.1
> 

Re: [PATCH 2/3] usercopy: Cast pointer to an integer once

[Uladzislau Rezki]

On Sun, Jun 12, 2022 at 10:32:26PM +0100, Matthew Wilcox (Oracle) wrote:
> Get rid of a lot of annoying casts by setting 'addr' once at the top
> of the function.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>  mm/usercopy.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/usercopy.c b/mm/usercopy.c
> index fdd1bed3b90a..31deee7dd2f5 100644
> --- a/mm/usercopy.c
> +++ b/mm/usercopy.c
> @@ -161,19 +161,20 @@ static inline void check_bogus_address(const unsigned long ptr, unsigned long n,
>  static inline void check_heap_object(const void *ptr, unsigned long n,
>  				     bool to_user)
>  {
> +	uintptr_t addr = (uintptr_t)ptr;
>  	struct folio *folio;
>  
>  	if (is_kmap_addr(ptr)) {
> -		unsigned long page_end = (unsigned long)ptr | (PAGE_SIZE - 1);
> +		unsigned long page_end = addr | (PAGE_SIZE - 1);
>  
> -		if ((unsigned long)ptr + n - 1 > page_end)
> +		if (addr + n - 1 > page_end)
>  			usercopy_abort("kmap", NULL, to_user,
>  					offset_in_page(ptr), n);
>  		return;
>  	}
>  
>  	if (is_vmalloc_addr(ptr)) {
> -		struct vmap_area *area = find_vmap_area((unsigned long)ptr);
> +		struct vmap_area *area = find_vmap_area(addr);
>  		unsigned long offset;
>  
>  		if (!area) {
> @@ -183,8 +184,8 @@ static inline void check_heap_object(const void *ptr, unsigned long n,
>  
>  		/* XXX: We should also abort for free vmap_areas */
>  
> -		offset = (unsigned long)ptr - area->va_start;
> -		if ((unsigned long)ptr + n > area->va_end)
> +		offset = addr - area->va_start;
> +		if (addr + n > area->va_end)
>  			usercopy_abort("vmalloc", NULL, to_user, offset, n);
>  		return;
>  	}
> -- 
> 2.35.1
> 
Looks good to me: Reviewed-by: Uladzislau Rezki (Sony) <urezki@gmail.com>

--
Uladzislau Rezki

Re: [PATCH 3/3] usercopy: Make usercopy resilient against ridiculously large copies

[Uladzislau Rezki]

> If 'n' is so large that it's negative, we might wrap around and mistakenly
> think that the copy is OK when it's not.  Such a copy would probably
> crash, but just doing the arithmetic in a more simple way lets us detect
> and refuse this case.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>  mm/usercopy.c | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/mm/usercopy.c b/mm/usercopy.c
> index 31deee7dd2f5..ff16083cf1c8 100644
> --- a/mm/usercopy.c
> +++ b/mm/usercopy.c
> @@ -162,20 +162,18 @@ static inline void check_heap_object(const void *ptr, unsigned long n,
>  				     bool to_user)
>  {
>  	uintptr_t addr = (uintptr_t)ptr;
> +	unsigned long offset;
>  	struct folio *folio;
>  
>  	if (is_kmap_addr(ptr)) {
> -		unsigned long page_end = addr | (PAGE_SIZE - 1);
> -
> -		if (addr + n - 1 > page_end)
> -			usercopy_abort("kmap", NULL, to_user,
> -					offset_in_page(ptr), n);
> +		offset = offset_in_page(ptr);
> +		if (n > PAGE_SIZE - offset)
> +			usercopy_abort("kmap", NULL, to_user, offset, n);
>  		return;
>  	}
>  
>  	if (is_vmalloc_addr(ptr)) {
>  		struct vmap_area *area = find_vmap_area(addr);
> -		unsigned long offset;
>  
>  		if (!area) {
>  			usercopy_abort("vmalloc", "no area", to_user, 0, n);
> @@ -184,9 +182,10 @@ static inline void check_heap_object(const void *ptr, unsigned long n,
>  
>  		/* XXX: We should also abort for free vmap_areas */
>  
> -		offset = addr - area->va_start;
> -		if (addr + n > area->va_end)
> +		if (n > area->va_end - addr) {
> +			offset = addr - area->va_start;
>  			usercopy_abort("vmalloc", NULL, to_user, offset, n);
> +		}
>  		return;
>  	}
>  
> @@ -199,8 +198,8 @@ static inline void check_heap_object(const void *ptr, unsigned long n,
>  		/* Check slab allocator for flags and size. */
>  		__check_heap_object(ptr, n, folio_slab(folio), to_user);
>  	} else if (folio_test_large(folio)) {
> -		unsigned long offset = ptr - folio_address(folio);
> -		if (offset + n > folio_size(folio))
> +		offset = ptr - folio_address(folio);
> +		if (n > folio_size(folio) - offset)
>  			usercopy_abort("page alloc", NULL, to_user, offset, n);
>  	}
>  }
> -- 
> 2.35.1
> 
Looks good to me: Reviewed-by: Uladzislau Rezki (Sony) <urezki@gmail.com>

--
Uladzislau Rezki

Re: [PATCH 1/3] usercopy: Handle vm_map_ram() areas

[Uladzislau Rezki]

> vmalloc does not allocate a vm_struct for vm_map_ram() areas.  That causes
> us to deny usercopies from those areas.  This affects XFS which uses
> vm_map_ram() for its directories.
> 
> Fix this by calling find_vmap_area() instead of find_vm_area().
> 
> Fixes: 0aef499f3172 ("mm/usercopy: Detect vmalloc overruns")
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>  include/linux/vmalloc.h | 1 +
>  mm/usercopy.c           | 8 +++++---
>  mm/vmalloc.c            | 2 +-
>  3 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/vmalloc.h b/include/linux/vmalloc.h
> index b159c2789961..096d48aa3437 100644
> --- a/include/linux/vmalloc.h
> +++ b/include/linux/vmalloc.h
> @@ -215,6 +215,7 @@ extern struct vm_struct *__get_vm_area_caller(unsigned long size,
>  void free_vm_area(struct vm_struct *area);
>  extern struct vm_struct *remove_vm_area(const void *addr);
>  extern struct vm_struct *find_vm_area(const void *addr);
> +struct vmap_area *find_vmap_area(unsigned long addr);
Make it "extern" since it becomes globally visible?

--
Uladzislau Rezki

Re: [PATCH 1/3] usercopy: Handle vm_map_ram() areas

[Baoquan He]

On 06/13/22 at 12:00pm, Uladzislau Rezki wrote:
> > vmalloc does not allocate a vm_struct for vm_map_ram() areas.  That causes
> > us to deny usercopies from those areas.  This affects XFS which uses
> > vm_map_ram() for its directories.
> > 
> > Fix this by calling find_vmap_area() instead of find_vm_area().
> > 
> > Fixes: 0aef499f3172 ("mm/usercopy: Detect vmalloc overruns")
> > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> > ---
> >  include/linux/vmalloc.h | 1 +
> >  mm/usercopy.c           | 8 +++++---
> >  mm/vmalloc.c            | 2 +-
> >  3 files changed, 7 insertions(+), 4 deletions(-)
> > 
> > diff --git a/include/linux/vmalloc.h b/include/linux/vmalloc.h
> > index b159c2789961..096d48aa3437 100644
> > --- a/include/linux/vmalloc.h
> > +++ b/include/linux/vmalloc.h
> > @@ -215,6 +215,7 @@ extern struct vm_struct *__get_vm_area_caller(unsigned long size,
> >  void free_vm_area(struct vm_struct *area);
> >  extern struct vm_struct *remove_vm_area(const void *addr);
> >  extern struct vm_struct *find_vm_area(const void *addr);
> > +struct vmap_area *find_vmap_area(unsigned long addr);
> Make it "extern" since it becomes globally visible?

extern is not suggested any more to add for function declaration in
header file, and removing it doesn't impact thing.

> 
> --
> Uladzislau Rezki
> 

Re: [PATCH 1/3] usercopy: Handle vm_map_ram() areas

[Uladzislau Rezki]

> On 06/13/22 at 12:00pm, Uladzislau Rezki wrote:
> > > vmalloc does not allocate a vm_struct for vm_map_ram() areas.  That causes
> > > us to deny usercopies from those areas.  This affects XFS which uses
> > > vm_map_ram() for its directories.
> > > 
> > > Fix this by calling find_vmap_area() instead of find_vm_area().
> > > 
> > > Fixes: 0aef499f3172 ("mm/usercopy: Detect vmalloc overruns")
> > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> > > ---
> > >  include/linux/vmalloc.h | 1 +
> > >  mm/usercopy.c           | 8 +++++---
> > >  mm/vmalloc.c            | 2 +-
> > >  3 files changed, 7 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/include/linux/vmalloc.h b/include/linux/vmalloc.h
> > > index b159c2789961..096d48aa3437 100644
> > > --- a/include/linux/vmalloc.h
> > > +++ b/include/linux/vmalloc.h
> > > @@ -215,6 +215,7 @@ extern struct vm_struct *__get_vm_area_caller(unsigned long size,
> > >  void free_vm_area(struct vm_struct *area);
> > >  extern struct vm_struct *remove_vm_area(const void *addr);
> > >  extern struct vm_struct *find_vm_area(const void *addr);
> > > +struct vmap_area *find_vmap_area(unsigned long addr);
> > Make it "extern" since it becomes globally visible?
> 
> extern is not suggested any more to add for function declaration in
> header file, and removing it doesn't impact thing.
> 
OK, thanks for the hint: Reviewed-by: Uladzislau Rezki (Sony) <urezki@gmail.com>

--
Uladzislau Rezki

Re: [PATCH 2/3] usercopy: Cast pointer to an integer once

[Kees Cook]

On Mon, Jun 13, 2022 at 11:51:24AM +0200, Uladzislau Rezki wrote:
> On Sun, Jun 12, 2022 at 10:32:26PM +0100, Matthew Wilcox (Oracle) wrote:
> > Get rid of a lot of annoying casts by setting 'addr' once at the top
> > of the function.
> > 
> > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> > ---
> >  mm/usercopy.c | 11 ++++++-----
> >  1 file changed, 6 insertions(+), 5 deletions(-)
> > 
> > diff --git a/mm/usercopy.c b/mm/usercopy.c
> > index fdd1bed3b90a..31deee7dd2f5 100644
> > --- a/mm/usercopy.c
> > +++ b/mm/usercopy.c
> > @@ -161,19 +161,20 @@ static inline void check_bogus_address(const unsigned long ptr, unsigned long n,
> >  static inline void check_heap_object(const void *ptr, unsigned long n,
> >  				     bool to_user)
> >  {
> > +	uintptr_t addr = (uintptr_t)ptr;
> >  	struct folio *folio;
> >  
> >  	if (is_kmap_addr(ptr)) {
> > -		unsigned long page_end = (unsigned long)ptr | (PAGE_SIZE - 1);
> > +		unsigned long page_end = addr | (PAGE_SIZE - 1);
> >  
> > -		if ((unsigned long)ptr + n - 1 > page_end)
> > +		if (addr + n - 1 > page_end)
> >  			usercopy_abort("kmap", NULL, to_user,
> >  					offset_in_page(ptr), n);
> >  		return;
> >  	}
> >  
> >  	if (is_vmalloc_addr(ptr)) {
> > -		struct vmap_area *area = find_vmap_area((unsigned long)ptr);
> > +		struct vmap_area *area = find_vmap_area(addr);
> >  		unsigned long offset;
> >  
> >  		if (!area) {
> > @@ -183,8 +184,8 @@ static inline void check_heap_object(const void *ptr, unsigned long n,
> >  
> >  		/* XXX: We should also abort for free vmap_areas */
> >  
> > -		offset = (unsigned long)ptr - area->va_start;
> > -		if ((unsigned long)ptr + n > area->va_end)
> > +		offset = addr - area->va_start;
> > +		if (addr + n > area->va_end)
> >  			usercopy_abort("vmalloc", NULL, to_user, offset, n);
> >  		return;
> >  	}
> > -- 
> > 2.35.1
> > 
> Looks good to me: Reviewed-by: Uladzislau Rezki (Sony) <urezki@gmail.com>

For the future, please put your tags ("Reviewed-by") on a separate line
or the workflow tools (b4, patchwork, etc) won't see it. :)

-- 
Kees Cook

Re: [PATCH 1/3] usercopy: Handle vm_map_ram() areas

[Kees Cook]

On Sun, Jun 12, 2022 at 10:32:25PM +0100, Matthew Wilcox (Oracle) wrote:
> vmalloc does not allocate a vm_struct for vm_map_ram() areas.  That causes
> us to deny usercopies from those areas.  This affects XFS which uses
> vm_map_ram() for its directories.
> 
> Fix this by calling find_vmap_area() instead of find_vm_area().

Thanks for the fixes!

> [...]
> +		/* XXX: We should also abort for free vmap_areas */

What's needed to detect this?

-- 
Kees Cook

Re: [PATCH 0/3] Fixes for usercopy

[Kees Cook]

On Sun, 12 Jun 2022 22:32:24 +0100, Matthew Wilcox (Oracle) wrote:
> Kees, I'm hoping you'll take these through your tree.  I think they're
> all reasonable fixes to go into 5.19.  The first one is essential;
> it fixes two different bugs that people have hit.
> 
> Matthew Wilcox (Oracle) (3):
>   usercopy: Handle vm_map_ram() areas
>   usercopy: Cast pointer to an integer once
>   usercopy: Make usercopy resilient against ridiculously large copies
> 
> [...]

Applied to for-next/hardening, thanks!

[1/3] usercopy: Handle vm_map_ram() areas
      https://git.kernel.org/kees/c/751ad8bdde7f
[2/3] usercopy: Cast pointer to an integer once
      https://git.kernel.org/kees/c/de2ae8f5331a
[3/3] usercopy: Make usercopy resilient against ridiculously large copies
      https://git.kernel.org/kees/c/630b2014e60e

-- 
Kees Cook

Re: [PATCH 2/3] usercopy: Cast pointer to an integer once

[Uladzislau Rezki]

>
> For the future, please put your tags ("Reviewed-by") on a separate line
> or the workflow tools (b4, patchwork, etc) won't see it. :)
>
OK. Thanks :)

-- 
Uladzislau Rezki

Re: [PATCH 1/3] usercopy: Handle vm_map_ram() areas

[Matthew Wilcox]

On Mon, Jun 13, 2022 at 09:23:15AM -0700, Kees Cook wrote:
> On Sun, Jun 12, 2022 at 10:32:25PM +0100, Matthew Wilcox (Oracle) wrote:
> > vmalloc does not allocate a vm_struct for vm_map_ram() areas.  That causes
> > us to deny usercopies from those areas.  This affects XFS which uses
> > vm_map_ram() for its directories.
> > 
> > Fix this by calling find_vmap_area() instead of find_vm_area().
> 
> Thanks for the fixes!
> 
> > [...]
> > +		/* XXX: We should also abort for free vmap_areas */
> 
> What's needed to detect this?

I'm not entirely sure.  I only just learned of the existence of this
struct ;-)

        /*
         * The following two variables can be packed, because
         * a vmap_area object can be either:
         *    1) in "free" tree (root is free_vmap_area_root)
         *    2) or "busy" tree (root is vmap_area_root)
         */
        union {
                unsigned long subtree_max_size; /* in "free" tree */
                struct vm_struct *vm;           /* in "busy" tree */
        };

Hmm.  Actually, we only search vmap_area_root, so I suppose it can't
be a free area.  So this XXX can be removed, as we'll get NULL back
if we've got a pointer to a free area.  Ulad, do I have this right?

Re: [PATCH 1/3] usercopy: Handle vm_map_ram() areas

[Uladzislau Rezki]

On Mon, Jun 13, 2022 at 05:44:35PM +0100, Matthew Wilcox wrote:
> On Mon, Jun 13, 2022 at 09:23:15AM -0700, Kees Cook wrote:
> > On Sun, Jun 12, 2022 at 10:32:25PM +0100, Matthew Wilcox (Oracle) wrote:
> > > vmalloc does not allocate a vm_struct for vm_map_ram() areas.  That causes
> > > us to deny usercopies from those areas.  This affects XFS which uses
> > > vm_map_ram() for its directories.
> > > 
> > > Fix this by calling find_vmap_area() instead of find_vm_area().
> > 
> > Thanks for the fixes!
> > 
> > > [...]
> > > +		/* XXX: We should also abort for free vmap_areas */
> > 
> > What's needed to detect this?
> 
> I'm not entirely sure.  I only just learned of the existence of this
> struct ;-)
> 
>         /*
>          * The following two variables can be packed, because
>          * a vmap_area object can be either:
>          *    1) in "free" tree (root is free_vmap_area_root)
>          *    2) or "busy" tree (root is vmap_area_root)
>          */
>         union {
>                 unsigned long subtree_max_size; /* in "free" tree */
>                 struct vm_struct *vm;           /* in "busy" tree */
>         };
> 
> Hmm.  Actually, we only search vmap_area_root, so I suppose it can't
> be a free area.  So this XXX can be removed, as we'll get NULL back
> if we've got a pointer to a free area.  Ulad, do I have this right?
>
Yep, we find here only allocated areas which bind to the "vmap_area_root"
tree, so it can not be a freed area. So we will not get a pointer to the
free area :)

--
Uladzislau Rezki

Re: [PATCH 1/3] usercopy: Handle vm_map_ram() areas

[Kees Cook]

On Mon, Jun 13, 2022 at 07:02:10PM +0200, Uladzislau Rezki wrote:
> On Mon, Jun 13, 2022 at 05:44:35PM +0100, Matthew Wilcox wrote:
> > On Mon, Jun 13, 2022 at 09:23:15AM -0700, Kees Cook wrote:
> > > On Sun, Jun 12, 2022 at 10:32:25PM +0100, Matthew Wilcox (Oracle) wrote:
> > > > vmalloc does not allocate a vm_struct for vm_map_ram() areas.  That causes
> > > > us to deny usercopies from those areas.  This affects XFS which uses
> > > > vm_map_ram() for its directories.
> > > > 
> > > > Fix this by calling find_vmap_area() instead of find_vm_area().
> > > 
> > > Thanks for the fixes!
> > > 
> > > > [...]
> > > > +		/* XXX: We should also abort for free vmap_areas */
> > > 
> > > What's needed to detect this?
> > 
> > I'm not entirely sure.  I only just learned of the existence of this
> > struct ;-)
> > 
> >         /*
> >          * The following two variables can be packed, because
> >          * a vmap_area object can be either:
> >          *    1) in "free" tree (root is free_vmap_area_root)
> >          *    2) or "busy" tree (root is vmap_area_root)
> >          */
> >         union {
> >                 unsigned long subtree_max_size; /* in "free" tree */
> >                 struct vm_struct *vm;           /* in "busy" tree */
> >         };
> > 
> > Hmm.  Actually, we only search vmap_area_root, so I suppose it can't
> > be a free area.  So this XXX can be removed, as we'll get NULL back
> > if we've got a pointer to a free area.  Ulad, do I have this right?
> >
> Yep, we find here only allocated areas which bind to the "vmap_area_root"
> tree, so it can not be a freed area. So we will not get a pointer to the
> free area :)

Thanks!

I've tweaked the patch to drop the XXX comment.

-- 
Kees Cook