diff mbox series

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

Message ID 20220612213227.3881769-2-willy@infradead.org (mailing list archive)
State New
Headers show
Series Fixes for usercopy | expand

Commit Message

Matthew Wilcox June 12, 2022, 9:32 p.m. UTC
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(-)

Comments

Uladzislau Rezki June 13, 2022, 10 a.m. UTC | #1
> 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
Baoquan He June 13, 2022, 11:52 a.m. UTC | #2
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
>
Uladzislau Rezki June 13, 2022, 12:56 p.m. UTC | #3
> 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
Kees Cook June 13, 2022, 4:23 p.m. UTC | #4
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?
Matthew Wilcox June 13, 2022, 4:44 p.m. UTC | #5
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?
Uladzislau Rezki June 13, 2022, 5:02 p.m. UTC | #6
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
Kees Cook June 13, 2022, 5:04 p.m. UTC | #7
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.
diff mbox series

Patch

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;