[PATCH] mm: Check we have the right vma in __access_remote_vm()

[PATCH] mm: Check we have the right vma in __access_remote_vm()

[Michael Ellerman]

In __access_remote_vm() we need to check that we have found the right
vma, not the following vma, before we try to access it. Otherwise we
might call the vma's access routine with an address which does not
fall inside the vma.

Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
---
 mm/memory.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/mm/memory.c b/mm/memory.c
index 9da8cab..ce999ca 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -3678,7 +3678,7 @@ static int __access_remote_vm(struct task_struct *tsk, struct mm_struct *mm,
 			 */
 #ifdef CONFIG_HAVE_IOREMAP_PROT
 			vma = find_vma(mm, addr);
-			if (!vma)
+			if (!vma || vma->vm_start > addr)
 				break;
 			if (vma->vm_ops && vma->vm_ops->access)
 				ret = vma->vm_ops->access(vma, addr, buf,
-- 
1.7.1

Re: [PATCH] mm: Check we have the right vma in __access_remote_vm()

[KOSAKI Motohiro]

> In __access_remote_vm() we need to check that we have found the right
> vma, not the following vma, before we try to access it. Otherwise we
> might call the vma's access routine with an address which does not
> fall inside the vma.
> 
> Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
> ---
>  mm/memory.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/mm/memory.c b/mm/memory.c
> index 9da8cab..ce999ca 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -3678,7 +3678,7 @@ static int __access_remote_vm(struct task_struct *tsk, struct mm_struct *mm,
>  			 */
>  #ifdef CONFIG_HAVE_IOREMAP_PROT
>  			vma = find_vma(mm, addr);
> -			if (!vma)
> +			if (!vma || vma->vm_start > addr)
>  				break;
>  			if (vma->vm_ops && vma->vm_ops->access)
>  				ret = vma->vm_ops->access(vma, addr, buf,

Looks good to me.
	Reviewed-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>

Re: [PATCH] mm: Check we have the right vma in __access_remote_vm()

[Andrew Morton]

On Fri,  8 Apr 2011 17:24:01 +1000 (EST)
Michael Ellerman <michael@ellerman.id.au> wrote:

> In __access_remote_vm() we need to check that we have found the right
> vma, not the following vma, before we try to access it. Otherwise we
> might call the vma's access routine with an address which does not
> fall inside the vma.
> 

hm, mysteries.  Does this patch fix any known problem in any known
kernel, or was the problem discovered by inspection, or what?

> 
> diff --git a/mm/memory.c b/mm/memory.c
> index 9da8cab..ce999ca 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -3678,7 +3678,7 @@ static int __access_remote_vm(struct task_struct *tsk, struct mm_struct *mm,
>  			 */
>  #ifdef CONFIG_HAVE_IOREMAP_PROT
>  			vma = find_vma(mm, addr);
> -			if (!vma)
> +			if (!vma || vma->vm_start > addr)
>  				break;
>  			if (vma->vm_ops && vma->vm_ops->access)
>  				ret = vma->vm_ops->access(vma, addr, buf,

Re: [PATCH] mm: Check we have the right vma in __access_remote_vm()

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 1621 bytes --]

On Mon, 2011-04-11 at 16:50 -0700, Andrew Morton wrote:
> On Fri,  8 Apr 2011 17:24:01 +1000 (EST)
> Michael Ellerman <michael@ellerman.id.au> wrote:
> 
> > In __access_remote_vm() we need to check that we have found the right
> > vma, not the following vma, before we try to access it. Otherwise we
> > might call the vma's access routine with an address which does not
> > fall inside the vma.
> > 
> 
> hm, mysteries.  Does this patch fix any known problem in any known
> kernel, or was the problem discovered by inspection, or what?

Sorry I meant to add that explanation but forgot.

It was discovered on a current kernel but with an unreleased driver,
from memory it was strace leading to a kernel bad access, but it
obviously depends on what the access implementation does. 

Looking at other access implementations I only see:

$ git grep -A 5 vm_operations|grep access
arch/powerpc/platforms/cell/spufs/file.c-	.access = spufs_mem_mmap_access,
arch/x86/pci/i386.c-	.access = generic_access_phys,
drivers/char/mem.c-	.access = generic_access_phys
fs/sysfs/bin.c-	.access		= bin_access,


The spufs one looks like it might behave badly given the wrong vma, it
assumes vma->vm_file->private_data is a spu_context, and looks like it
would probably blow up pretty quickly if it wasn't.

generic_access_phys() only uses the vma to check vm_flags and get the
mm, and then walks page tables using the address. So it should bail on
the vm_flags check, or at worst let you access some other VM_IO mapping.

And bin_access() just proxies to another access implementation.

cheers



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]