[PATCH] omap_vout: find_vma() needs ->mmap_sem held

[PATCH] omap_vout: find_vma() needs ->mmap_sem held

[Al Viro]

	Walking rbtree while it's modified is a Bad Idea(tm); besides,
the result of find_vma() can be freed just as it's getting returned
to caller.  Fortunately, it's easy to fix - just take ->mmap_sem a bit
earlier (and don't bother with find_vma() at all if virtp >= PAGE_OFFSET -
in that case we don't even look at its result).

Cc: stable@vger.kernel.org [2.6.35]
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
diff --git a/drivers/media/platform/omap/omap_vout.c b/drivers/media/platform/omap/omap_vout.c
index 9935040..984512f 100644
--- a/drivers/media/platform/omap/omap_vout.c
+++ b/drivers/media/platform/omap/omap_vout.c
@@ -207,19 +207,21 @@ static u32 omap_vout_uservirt_to_phys(u32 virtp)
 	struct vm_area_struct *vma;
 	struct mm_struct *mm = current->mm;
 
-	vma = find_vma(mm, virtp);
 	/* For kernel direct-mapped memory, take the easy way */
-	if (virtp >= PAGE_OFFSET) {
-		physp = virt_to_phys((void *) virtp);
+	if (virtp >= PAGE_OFFSET)
+		return virt_to_phys((void *) virtp);
+
+	down_read(&current->mm->mmap_sem);
+	vma = find_vma(mm, virtp);
 	} else if (vma && (vma->vm_flags & VM_IO) && vma->vm_pgoff) {
 		/* this will catch, kernel-allocated, mmaped-to-usermode
 		   addresses */
 		physp = (vma->vm_pgoff << PAGE_SHIFT) + (virtp - vma->vm_start);
+		up_read(&current->mm->mmap_sem);
 	} else {
 		/* otherwise, use get_user_pages() for general userland pages */
 		int res, nr_pages = 1;
 		struct page *pages;
-		down_read(&current->mm->mmap_sem);
 
 		res = get_user_pages(current, current->mm, virtp, nr_pages, 1,
 				0, &pages, NULL);

Re: [PATCH] omap_vout: find_vma() needs ->mmap_sem held

[Al Viro]

On Sat, Dec 15, 2012 at 08:12:37PM +0000, Al Viro wrote:
> 	Walking rbtree while it's modified is a Bad Idea(tm); besides,
> the result of find_vma() can be freed just as it's getting returned
> to caller.  Fortunately, it's easy to fix - just take ->mmap_sem a bit
> earlier (and don't bother with find_vma() at all if virtp >= PAGE_OFFSET -
> in that case we don't even look at its result).

	While we are at it, what prevents VIDIOC_PREPARE_BUF calling
v4l_prepare_buf() -> (e.g) vb2_ioctl_prepare_buf() -> vb2_prepare_buf() ->
__buf_prepare() -> __qbuf_userptr() -> vb2_vmalloc_get_userptr() -> find_vma(),
AFAICS without having taken ->mmap_sem anywhere in process?  The code flow
is bloody convoluted and depends on a bunch of things done by initialization,
so I certainly might've missed something...

Re: [PATCH] omap_vout: find_vma() needs ->mmap_sem held

[Paul Bolle]

On Sat, 2012-12-15 at 20:12 +0000, Al Viro wrote:
> 	Walking rbtree while it's modified is a Bad Idea(tm); besides,
> the result of find_vma() can be freed just as it's getting returned
> to caller.  Fortunately, it's easy to fix - just take ->mmap_sem a bit
> earlier (and don't bother with find_vma() at all if virtp >= PAGE_OFFSET -
> in that case we don't even look at its result).
> 
> Cc: stable@vger.kernel.org [2.6.35]
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
> diff --git a/drivers/media/platform/omap/omap_vout.c b/drivers/media/platform/omap/omap_vout.c
> index 9935040..984512f 100644
> --- a/drivers/media/platform/omap/omap_vout.c
> +++ b/drivers/media/platform/omap/omap_vout.c
> @@ -207,19 +207,21 @@ static u32 omap_vout_uservirt_to_phys(u32 virtp)
>  	struct vm_area_struct *vma;
>  	struct mm_struct *mm = current->mm;
>  
> -	vma = find_vma(mm, virtp);
>  	/* For kernel direct-mapped memory, take the easy way */
> -	if (virtp >= PAGE_OFFSET) {
> -		physp = virt_to_phys((void *) virtp);
> +	if (virtp >= PAGE_OFFSET)
> +		return virt_to_phys((void *) virtp);
> +
> +	down_read(&current->mm->mmap_sem);
> +	vma = find_vma(mm, virtp);
>  	} else if (vma && (vma->vm_flags & VM_IO) && vma->vm_pgoff) {

Shouldn't that line become 
    if (vma && (vma->vm_flags & VM_IO) && vma->vm_pgoff) {

so that this actually compiles?

>  		/* this will catch, kernel-allocated, mmaped-to-usermode
>  		   addresses */
>  		physp = (vma->vm_pgoff << PAGE_SHIFT) + (virtp - vma->vm_start);
> +		up_read(&current->mm->mmap_sem);
>  	} else {
>  		/* otherwise, use get_user_pages() for general userland pages */
>  		int res, nr_pages = 1;
>  		struct page *pages;
> -		down_read(&current->mm->mmap_sem);
>  
>  		res = get_user_pages(current, current->mm, virtp, nr_pages, 1,
>  				0, &pages, NULL);


Paul Bolle

Re: [PATCH] omap_vout: find_vma() needs ->mmap_sem held

[Al Viro]

On Sun, Dec 16, 2012 at 09:01:10PM +0100, Paul Bolle wrote:
> > +	vma = find_vma(mm, virtp);
> >  	} else if (vma && (vma->vm_flags & VM_IO) && vma->vm_pgoff) {
> 
> Shouldn't that line become 
>     if (vma && (vma->vm_flags & VM_IO) && vma->vm_pgoff) {
> 
> so that this actually compiles?

*Do'h*

Yes, it should.  Mea culpa...

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
diff --git a/drivers/media/platform/omap/omap_vout.c b/drivers/media/platform/omap/omap_vout.c
index 9935040..cb564d0 100644
--- a/drivers/media/platform/omap/omap_vout.c
+++ b/drivers/media/platform/omap/omap_vout.c
@@ -207,19 +207,21 @@ static u32 omap_vout_uservirt_to_phys(u32 virtp)
 	struct vm_area_struct *vma;
 	struct mm_struct *mm = current->mm;
 
-	vma = find_vma(mm, virtp);
 	/* For kernel direct-mapped memory, take the easy way */
-	if (virtp >= PAGE_OFFSET) {
-		physp = virt_to_phys((void *) virtp);
-	} else if (vma && (vma->vm_flags & VM_IO) && vma->vm_pgoff) {
+	if (virtp >= PAGE_OFFSET)
+		return virt_to_phys((void *) virtp);
+
+	down_read(&current->mm->mmap_sem);
+	vma = find_vma(mm, virtp);
+	if (vma && (vma->vm_flags & VM_IO) && vma->vm_pgoff) {
 		/* this will catch, kernel-allocated, mmaped-to-usermode
 		   addresses */
 		physp = (vma->vm_pgoff << PAGE_SHIFT) + (virtp - vma->vm_start);
+		up_read(&current->mm->mmap_sem);
 	} else {
 		/* otherwise, use get_user_pages() for general userland pages */
 		int res, nr_pages = 1;
 		struct page *pages;
-		down_read(&current->mm->mmap_sem);
 
 		res = get_user_pages(current, current->mm, virtp, nr_pages, 1,
 				0, &pages, NULL);

Re: [PATCH] omap_vout: find_vma() needs ->mmap_sem held

[Mauro Carvalho Chehab]

Hi Viro,

Em Sat, 15 Dec 2012 20:38:29 +0000
Al Viro <viro@ZenIV.linux.org.uk> escreveu:

> On Sat, Dec 15, 2012 at 08:12:37PM +0000, Al Viro wrote:
> > 	Walking rbtree while it's modified is a Bad Idea(tm); besides,
> > the result of find_vma() can be freed just as it's getting returned
> > to caller.  Fortunately, it's easy to fix - just take ->mmap_sem a bit
> > earlier (and don't bother with find_vma() at all if virtp >= PAGE_OFFSET -
> > in that case we don't even look at its result).
> 
> 	While we are at it, what prevents VIDIOC_PREPARE_BUF calling
> v4l_prepare_buf() -> (e.g) vb2_ioctl_prepare_buf() -> vb2_prepare_buf() ->
> __buf_prepare() -> __qbuf_userptr() -> vb2_vmalloc_get_userptr() -> find_vma(),
> AFAICS without having taken ->mmap_sem anywhere in process?  The code flow
> is bloody convoluted and depends on a bunch of things done by initialization,
> so I certainly might've missed something...

This driver is currently missing an active maintainer, as it is for an old
hardware (AFAIK, omap is now at version 4, and this is for the first one),
but I'm c/c a few developers that might help to test and analyze it.

In any case, /me is assuming that your patch is right (as nobody complained),
and I'm applying it right now on my tree. This will hopefully allow some
people to test.

Cheers,
Mauro

Re: [PATCH] omap_vout: find_vma() needs ->mmap_sem held

[Laurent Pinchart]

Hi Mauro,

On Sunday 06 January 2013 11:02:25 Mauro Carvalho Chehab wrote:
> Em Sat, 15 Dec 2012 20:38:29 +0000 Al Viro escreveu:
> > On Sat, Dec 15, 2012 at 08:12:37PM +0000, Al Viro wrote:
> > > 	Walking rbtree while it's modified is a Bad Idea(tm); besides,
> > > 
> > > the result of find_vma() can be freed just as it's getting returned
> > > to caller.  Fortunately, it's easy to fix - just take ->mmap_sem a bit
> > > earlier (and don't bother with find_vma() at all if virtp >= PAGE_OFFSET
> > > -
> > > in that case we don't even look at its result).
> > 	
> > 	While we are at it, what prevents VIDIOC_PREPARE_BUF calling
> > 
> > v4l_prepare_buf() -> (e.g) vb2_ioctl_prepare_buf() -> vb2_prepare_buf() ->
> > __buf_prepare() -> __qbuf_userptr() -> vb2_vmalloc_get_userptr() ->
> > find_vma(), AFAICS without having taken ->mmap_sem anywhere in process? 
> > The code flow is bloody convoluted and depends on a bunch of things done
> > by initialization, so I certainly might've missed something...
> 
> This driver is currently missing an active maintainer, as it is for an old
> hardware (AFAIK, omap is now at version 4, and this is for the first one),

The omap_vout driver is for OMAP2+ if I'm not mistaken. I use it with the 
OMAP3.

> but I'm c/c a few developers that might help to test and analyze it.
> 
> In any case, /me is assuming that your patch is right (as nobody
> complained), and I'm applying it right now on my tree. This will hopefully
> allow some people to test.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] omap_vout: find_vma() needs ->mmap_sem held

[Laurent Pinchart]

Hi Mauro,

On Sunday 06 January 2013 11:02:25 Mauro Carvalho Chehab wrote:
> Em Sat, 15 Dec 2012 20:38:29 +0000 Al Viro escreveu:
> > On Sat, Dec 15, 2012 at 08:12:37PM +0000, Al Viro wrote:
> > > 	Walking rbtree while it's modified is a Bad Idea(tm); besides,
> > > 
> > > the result of find_vma() can be freed just as it's getting returned
> > > to caller.  Fortunately, it's easy to fix - just take ->mmap_sem a bit
> > > earlier (and don't bother with find_vma() at all if virtp >= PAGE_OFFSET
> > > -
> > > in that case we don't even look at its result).
> > 	
> > 	While we are at it, what prevents VIDIOC_PREPARE_BUF calling
> > 
> > v4l_prepare_buf() -> (e.g) vb2_ioctl_prepare_buf() -> vb2_prepare_buf() ->
> > __buf_prepare() -> __qbuf_userptr() -> vb2_vmalloc_get_userptr() ->
> > find_vma(), AFAICS without having taken ->mmap_sem anywhere in process? 
> > The code flow is bloody convoluted and depends on a bunch of things done
> > by initialization, so I certainly might've missed something...
> 
> This driver is currently missing an active maintainer, as it is for an old
> hardware (AFAIK, omap is now at version 4, and this is for the first one),
> but I'm c/c a few developers that might help to test and analyze it.
> 
> In any case, /me is assuming that your patch is right (as nobody
> complained), and I'm applying it right now on my tree. This will hopefully
> allow some people to test.

Please make sure you apply v2 (posted as "Re: [PATCH] omap_vout: find_vma() 
needs ->mmap_sem held").

-- 
Regards,

Laurent Pinchart

Re: [PATCH] omap_vout: find_vma() needs ->mmap_sem held

[Laurent Pinchart]

On Sunday 16 December 2012 20:04:46 Al Viro wrote:
> On Sun, Dec 16, 2012 at 09:01:10PM +0100, Paul Bolle wrote:
> > > +	vma = find_vma(mm, virtp);
> > > 
> > >  	} else if (vma && (vma->vm_flags & VM_IO) && vma->vm_pgoff) {
> > 
> > Shouldn't that line become
> > 
> >     if (vma && (vma->vm_flags & VM_IO) && vma->vm_pgoff) {
> > 
> > so that this actually compiles?
> 
> *Do'h*
> 
> Yes, it should.  Mea culpa...
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> diff --git a/drivers/media/platform/omap/omap_vout.c
> b/drivers/media/platform/omap/omap_vout.c index 9935040..cb564d0 100644
> --- a/drivers/media/platform/omap/omap_vout.c
> +++ b/drivers/media/platform/omap/omap_vout.c
> @@ -207,19 +207,21 @@ static u32 omap_vout_uservirt_to_phys(u32 virtp)
>  	struct vm_area_struct *vma;
>  	struct mm_struct *mm = current->mm;
> 
> -	vma = find_vma(mm, virtp);
>  	/* For kernel direct-mapped memory, take the easy way */
> -	if (virtp >= PAGE_OFFSET) {
> -		physp = virt_to_phys((void *) virtp);
> -	} else if (vma && (vma->vm_flags & VM_IO) && vma->vm_pgoff) {
> +	if (virtp >= PAGE_OFFSET)
> +		return virt_to_phys((void *) virtp);
> +
> +	down_read(&current->mm->mmap_sem);
> +	vma = find_vma(mm, virtp);
> +	if (vma && (vma->vm_flags & VM_IO) && vma->vm_pgoff) {
>  		/* this will catch, kernel-allocated, mmaped-to-usermode
>  		   addresses */
>  		physp = (vma->vm_pgoff << PAGE_SHIFT) + (virtp - vma->vm_start);
> +		up_read(&current->mm->mmap_sem);
>  	} else {
>  		/* otherwise, use get_user_pages() for general userland pages */
>  		int res, nr_pages = 1;
>  		struct page *pages;
> -		down_read(&current->mm->mmap_sem);
> 
>  		res = get_user_pages(current, current->mm, virtp, nr_pages, 1,
>  				0, &pages, NULL);

-- 
Regards,

Laurent Pinchart