Re: [PATCH 2/2] mm/frame-vec: use FOLL_LONGTERM

Re: [PATCH 2/2] mm/frame-vec: use FOLL_LONGTERM

[Jason Gunthorpe]

When get_vaddr_frames() does its hacky follow_pfn() loop it should never
be allowed to extract a struct page from a normal VMA. This could allow a
serious use-after-free problem on any kernel memory.

Restrict this to only work on VMA's with one of VM_IO | VM_PFNMAP
set. This limits the use-after-free problem to only IO memory, which while
still serious, is an improvement.

Cc: stable@vger.kernel.org
Fixes: 8025e5ddf9c1 ("[media] mm: Provide new get_vaddr_frames() helper")
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 mm/frame_vector.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/mm/frame_vector.c b/mm/frame_vector.c
index 10f82d5643b6de..26cb20544b6c37 100644
--- a/mm/frame_vector.c
+++ b/mm/frame_vector.c
@@ -99,6 +99,10 @@ int get_vaddr_frames(unsigned long start, unsigned int nr_frames,
 		if (ret >= nr_frames || start < vma->vm_end)
 			break;
 		vma = find_vma_intersection(mm, start, start + 1);
+		if (!(vma->vm_flags & (VM_IO | VM_PFNMAP))) {
+			ret = -EINVAL;
+			goto out;
+		}
 	} while (vma && vma->vm_flags & (VM_IO | VM_PFNMAP));
 out:
 	if (locked)
-- 
2.28.0

Re: [PATCH 2/2] mm/frame-vec: use FOLL_LONGTERM

[Jason Gunthorpe]

On Mon, Oct 05, 2020 at 02:38:54PM -0300, Jason Gunthorpe wrote:
> When get_vaddr_frames() does its hacky follow_pfn() loop it should never
> be allowed to extract a struct page from a normal VMA. This could allow a
> serious use-after-free problem on any kernel memory.
> 
> Restrict this to only work on VMA's with one of VM_IO | VM_PFNMAP
> set. This limits the use-after-free problem to only IO memory, which while
> still serious, is an improvement.
> 
> Cc: stable@vger.kernel.org
> Fixes: 8025e5ddf9c1 ("[media] mm: Provide new get_vaddr_frames() helper")
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  mm/frame_vector.c | 4 ++++
>  1 file changed, 4 insertions(+)

woops, this subject got badly corrupted when I was editing the CC
list, it was supposed to be:

[PATCH] mm/gpu: frame_vector: require all VMAs to be VM_PFNMAP

Andrew please let me know if you need a resend

Sorry,
Jason

Re: [PATCH 2/2] mm/frame-vec: use FOLL_LONGTERM

[Andrew Morton]

On Mon, 5 Oct 2020 14:47:47 -0300 Jason Gunthorpe <jgg@nvidia.com> wrote:

> Andrew please let me know if you need a resend

Andrew is rather confused.

Can we please identify the minimal patch(es) which are needed for 5.9
and -stable? 

Re: [PATCH 2/2] mm/frame-vec: use FOLL_LONGTERM

[Jason Gunthorpe]

On Mon, Oct 05, 2020 at 08:36:00PM -0700, Andrew Morton wrote:
> On Mon, 5 Oct 2020 14:47:47 -0300 Jason Gunthorpe <jgg@nvidia.com> wrote:
> 
> > Andrew please let me know if you need a resend
> 
> Andrew is rather confused.
> 
> Can we please identify the minimal patch(es) which are needed for 5.9
> and -stable? 

Please ignore this one, it was sent in haste

Sorry,
Jason

Re: [PATCH 2/2] mm/frame-vec: use FOLL_LONGTERM

[Jan Kara]

On Mon 05-10-20 14:38:54, Jason Gunthorpe wrote:
> When get_vaddr_frames() does its hacky follow_pfn() loop it should never
> be allowed to extract a struct page from a normal VMA. This could allow a
> serious use-after-free problem on any kernel memory.
> 
> Restrict this to only work on VMA's with one of VM_IO | VM_PFNMAP
> set. This limits the use-after-free problem to only IO memory, which while
> still serious, is an improvement.
> 
> Cc: stable@vger.kernel.org
> Fixes: 8025e5ddf9c1 ("[media] mm: Provide new get_vaddr_frames() helper")
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  mm/frame_vector.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/mm/frame_vector.c b/mm/frame_vector.c
> index 10f82d5643b6de..26cb20544b6c37 100644
> --- a/mm/frame_vector.c
> +++ b/mm/frame_vector.c
> @@ -99,6 +99,10 @@ int get_vaddr_frames(unsigned long start, unsigned int nr_frames,
>  		if (ret >= nr_frames || start < vma->vm_end)
>  			break;
>  		vma = find_vma_intersection(mm, start, start + 1);
> +		if (!(vma->vm_flags & (VM_IO | VM_PFNMAP))) {
> +			ret = -EINVAL;
> +			goto out;
> +		}
>  	} while (vma && vma->vm_flags & (VM_IO | VM_PFNMAP));

Hum, I fail to see how this helps. If vma has no VM_IO or VM_PFNMAP flag,
we'd exit the loop (to out: label) anyway due to the loop termination
condition and why not return the frames we already have? Furthermore
find_vma_intersection() can return NULL which would oops in your check
then. What am I missing?

								Honza

>  out:
>  	if (locked)
> -- 
> 2.28.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 2/2] mm/frame-vec: use FOLL_LONGTERM

[Jason Gunthorpe]

On Mon, Oct 05, 2020 at 07:53:08PM +0200, Jan Kara wrote:
> On Mon 05-10-20 14:38:54, Jason Gunthorpe wrote:
> > When get_vaddr_frames() does its hacky follow_pfn() loop it should never
> > be allowed to extract a struct page from a normal VMA. This could allow a
> > serious use-after-free problem on any kernel memory.
> > 
> > Restrict this to only work on VMA's with one of VM_IO | VM_PFNMAP
> > set. This limits the use-after-free problem to only IO memory, which while
> > still serious, is an improvement.
> > 
> > Cc: stable@vger.kernel.org
> > Fixes: 8025e5ddf9c1 ("[media] mm: Provide new get_vaddr_frames() helper")
> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> >  mm/frame_vector.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/mm/frame_vector.c b/mm/frame_vector.c
> > index 10f82d5643b6de..26cb20544b6c37 100644
> > +++ b/mm/frame_vector.c
> > @@ -99,6 +99,10 @@ int get_vaddr_frames(unsigned long start, unsigned int nr_frames,
> >  		if (ret >= nr_frames || start < vma->vm_end)
> >  			break;
> >  		vma = find_vma_intersection(mm, start, start + 1);
> > +		if (!(vma->vm_flags & (VM_IO | VM_PFNMAP))) {
> > +			ret = -EINVAL;
> > +			goto out;
> > +		}
> >  	} while (vma && vma->vm_flags & (VM_IO | VM_PFNMAP));
> 
> Hum, I fail to see how this helps. If vma has no VM_IO or VM_PFNMAP flag,
> we'd exit the loop (to out: label) anyway due to the loop termination
> condition and why not return the frames we already have? Furthermore
> find_vma_intersection() can return NULL which would oops in your check
> then. What am I missing?

Oh, nothing, you are right. It just didn't read naturally because
hitting the wrong kind of VMA should be an error condition :\

Sorry again,
Jason

Re: [PATCH 2/2] mm/frame-vec: use FOLL_LONGTERM

[Daniel Vetter]

On Mon, Oct 5, 2020 at 7:58 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
>
> On Mon, Oct 05, 2020 at 07:53:08PM +0200, Jan Kara wrote:
> > On Mon 05-10-20 14:38:54, Jason Gunthorpe wrote:
> > > When get_vaddr_frames() does its hacky follow_pfn() loop it should never
> > > be allowed to extract a struct page from a normal VMA. This could allow a
> > > serious use-after-free problem on any kernel memory.
> > >
> > > Restrict this to only work on VMA's with one of VM_IO | VM_PFNMAP
> > > set. This limits the use-after-free problem to only IO memory, which while
> > > still serious, is an improvement.
> > >
> > > Cc: stable@vger.kernel.org
> > > Fixes: 8025e5ddf9c1 ("[media] mm: Provide new get_vaddr_frames() helper")
> > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> > >  mm/frame_vector.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/mm/frame_vector.c b/mm/frame_vector.c
> > > index 10f82d5643b6de..26cb20544b6c37 100644
> > > +++ b/mm/frame_vector.c
> > > @@ -99,6 +99,10 @@ int get_vaddr_frames(unsigned long start, unsigned int nr_frames,
> > >             if (ret >= nr_frames || start < vma->vm_end)
> > >                     break;
> > >             vma = find_vma_intersection(mm, start, start + 1);
> > > +           if (!(vma->vm_flags & (VM_IO | VM_PFNMAP))) {
> > > +                   ret = -EINVAL;
> > > +                   goto out;
> > > +           }
> > >     } while (vma && vma->vm_flags & (VM_IO | VM_PFNMAP));
> >
> > Hum, I fail to see how this helps. If vma has no VM_IO or VM_PFNMAP flag,
> > we'd exit the loop (to out: label) anyway due to the loop termination
> > condition and why not return the frames we already have? Furthermore
> > find_vma_intersection() can return NULL which would oops in your check
> > then. What am I missing?
>
> Oh, nothing, you are right. It just didn't read naturally because
> hitting the wrong kind of VMA should be an error condition :\

Afaik these mmio maps should all be VM_DONTEXPAND (or at least the
ones in drivers/gpu are all), so not sure why we need the loop here.
But maybe there's some drivers that don't set that, or have other
funny things going on with userspace piecing the mmap together, and
I'm not going to audit them all :-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 2/2] mm/frame-vec: use FOLL_LONGTERM

[Daniel Vetter]

On Mon, Oct 5, 2020 at 7:58 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
>
> On Mon, Oct 05, 2020 at 07:53:08PM +0200, Jan Kara wrote:
> > On Mon 05-10-20 14:38:54, Jason Gunthorpe wrote:
> > > When get_vaddr_frames() does its hacky follow_pfn() loop it should never
> > > be allowed to extract a struct page from a normal VMA. This could allow a
> > > serious use-after-free problem on any kernel memory.
> > >
> > > Restrict this to only work on VMA's with one of VM_IO | VM_PFNMAP
> > > set. This limits the use-after-free problem to only IO memory, which while
> > > still serious, is an improvement.
> > >
> > > Cc: stable@vger.kernel.org
> > > Fixes: 8025e5ddf9c1 ("[media] mm: Provide new get_vaddr_frames() helper")
> > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> > >  mm/frame_vector.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/mm/frame_vector.c b/mm/frame_vector.c
> > > index 10f82d5643b6de..26cb20544b6c37 100644
> > > +++ b/mm/frame_vector.c
> > > @@ -99,6 +99,10 @@ int get_vaddr_frames(unsigned long start, unsigned int nr_frames,
> > >             if (ret >= nr_frames || start < vma->vm_end)
> > >                     break;
> > >             vma = find_vma_intersection(mm, start, start + 1);
> > > +           if (!(vma->vm_flags & (VM_IO | VM_PFNMAP))) {
> > > +                   ret = -EINVAL;
> > > +                   goto out;
> > > +           }
> > >     } while (vma && vma->vm_flags & (VM_IO | VM_PFNMAP));
> >
> > Hum, I fail to see how this helps. If vma has no VM_IO or VM_PFNMAP flag,
> > we'd exit the loop (to out: label) anyway due to the loop termination
> > condition and why not return the frames we already have? Furthermore
> > find_vma_intersection() can return NULL which would oops in your check
> > then. What am I missing?
>
> Oh, nothing, you are right. It just didn't read naturally because
> hitting the wrong kind of VMA should be an error condition :\

Also follow_pfn checks for this same conditionat already too, so this
isn't really stopping anything bad from happening.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch