[PATCH 0/3] uprobes: mprotect fixes

[PATCH 0/3] uprobes: mprotect fixes

[Oleg Nesterov]

Hello.

_register/_unregister doesn't work correctly if the probed
application plays with mprotect.

Oleg.

 kernel/events/uprobes.c |   15 +++++----------
 1 files changed, 5 insertions(+), 10 deletions(-)

[PATCH 1/3] uprobes: Change write_opcode() to use FOLL_FORCE

[Oleg Nesterov]

write_opcode()->get_user_pages() needs FOLL_FORCE to ensure we can
read the page even if the probed task did mprotect(PROT_NONE) after
uprobe_register(). Without FOLL_WRITE, FOLL_FORCE doesn't have any
side effect but allows to read the !VM_READ memory.

Otherwiese the subsequent uprobe_unregister()->set_orig_insn() fails
and we leak "int3". If that task does mprotect(PROT_READ | EXEC) and
execute the probed insn later it will be killed.

Note: in fact this is also needed for _register, see the next patch.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/events/uprobes.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 14c2e99..fa1579a 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -221,7 +221,7 @@ static int write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm,
 
 retry:
 	/* Read the page with vaddr into memory */
-	ret = get_user_pages(NULL, mm, vaddr, 1, 0, 0, &old_page, &vma);
+	ret = get_user_pages(NULL, mm, vaddr, 1, 0, 1, &old_page, &vma);
 	if (ret <= 0)
 		return ret;
 
-- 
1.5.5.1

Re: [PATCH 1/3] uprobes: Change write_opcode() to use FOLL_FORCE

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-09-16 19:52:42]:

> write_opcode()->get_user_pages() needs FOLL_FORCE to ensure we can
> read the page even if the probed task did mprotect(PROT_NONE) after
> uprobe_register(). Without FOLL_WRITE, FOLL_FORCE doesn't have any
> side effect but allows to read the !VM_READ memory.
> 
> Otherwiese the subsequent uprobe_unregister()->set_orig_insn() fails
> and we leak "int3". If that task does mprotect(PROT_READ | EXEC) and
> execute the probed insn later it will be killed.
> 
> Note: in fact this is also needed for _register, see the next patch.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

> ---
>  kernel/events/uprobes.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 14c2e99..fa1579a 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -221,7 +221,7 @@ static int write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm,
> 
>  retry:
>  	/* Read the page with vaddr into memory */
> -	ret = get_user_pages(NULL, mm, vaddr, 1, 0, 0, &old_page, &vma);
> +	ret = get_user_pages(NULL, mm, vaddr, 1, 0, 1, &old_page, &vma);
>  	if (ret <= 0)
>  		return ret;
> 
> -- 
> 1.5.5.1
> 

[PATCH 2/3] uprobes: Change valid_vma() to demand VM_MAYEXEC rather than VM_EXEC

[Oleg Nesterov]

uprobe_register() or uprobe_mmap() requires VM_READ | VM_EXEC, this
is not right. An apllication can do mprotect(PROT_EXEC) later and
execute this code.

Change valid_vma(is_register => true) to check VM_MAYEXEC instead.
No need to check VM_MAYREAD, it is always set.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/events/uprobes.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index fa1579a..b9b50dd 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -106,8 +106,8 @@ static bool valid_vma(struct vm_area_struct *vma, bool is_register)
 	if (!is_register)
 		return true;
 
-	if ((vma->vm_flags & (VM_HUGETLB|VM_READ|VM_WRITE|VM_EXEC|VM_SHARED))
-				== (VM_READ|VM_EXEC))
+	if ((vma->vm_flags & (VM_HUGETLB | VM_WRITE | VM_MAYEXEC | VM_SHARED))
+				== VM_MAYEXEC)
 		return true;
 
 	return false;
-- 
1.5.5.1

Re: [PATCH 2/3] uprobes: Change valid_vma() to demand VM_MAYEXEC rather than VM_EXEC

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-09-16 19:52:46]:

> uprobe_register() or uprobe_mmap() requires VM_READ | VM_EXEC, this
> is not right. An apllication can do mprotect(PROT_EXEC) later and
> execute this code.
> 
> Change valid_vma(is_register => true) to check VM_MAYEXEC instead.
> No need to check VM_MAYREAD, it is always set.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

> ---
>  kernel/events/uprobes.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index fa1579a..b9b50dd 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -106,8 +106,8 @@ static bool valid_vma(struct vm_area_struct *vma, bool is_register)
>  	if (!is_register)
>  		return true;
> 
> -	if ((vma->vm_flags & (VM_HUGETLB|VM_READ|VM_WRITE|VM_EXEC|VM_SHARED))
> -				== (VM_READ|VM_EXEC))
> +	if ((vma->vm_flags & (VM_HUGETLB | VM_WRITE | VM_MAYEXEC | VM_SHARED))
> +				== VM_MAYEXEC)
>  		return true;
> 
>  	return false;
> -- 
> 1.5.5.1
> 

[PATCH 3/3] uprobes: Restrict valid_vma(false) to skip VM_SHARED

[Oleg Nesterov]

valid_vma(false) ignores ->vm_flags, this is not actually right.
We should never try to write into MAP_SHARED mapping, this can
confuse an apllication which actually writes to ->vm_file.

With this patch valid_vma(false) ignores VM_WRITE only but checks
other (immutable) bits checked by valid_vma(true). This can also
speedup uprobe_munmap() and uprobe_unregister().

Note: even after this patch _unregister can confuse the probed
application if it does mprotect(PROT_WRITE) after _register and
installs "int3", but this is hardly possible to avoid and this
doesn't differ from gdb case.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/events/uprobes.c |   13 ++++---------
 1 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index b9b50dd..78364a2 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -100,17 +100,12 @@ struct uprobe {
  */
 static bool valid_vma(struct vm_area_struct *vma, bool is_register)
 {
-	if (!vma->vm_file)
-		return false;
-
-	if (!is_register)
-		return true;
+	vm_flags_t flags = VM_HUGETLB | VM_MAYEXEC | VM_SHARED;
 
-	if ((vma->vm_flags & (VM_HUGETLB | VM_WRITE | VM_MAYEXEC | VM_SHARED))
-				== VM_MAYEXEC)
-		return true;
+	if (is_register)
+		flags |= VM_WRITE;
 
-	return false;
+	return vma->vm_file && (vma->vm_flags & flags) == VM_MAYEXEC;
 }
 
 static unsigned long offset_to_vaddr(struct vm_area_struct *vma, loff_t offset)
-- 
1.5.5.1

Re: [PATCH 3/3] uprobes: Restrict valid_vma(false) to skip VM_SHARED

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-09-16 19:52:48]:

> valid_vma(false) ignores ->vm_flags, this is not actually right.
> We should never try to write into MAP_SHARED mapping, this can
> confuse an apllication which actually writes to ->vm_file.

Agree,

> 
> With this patch valid_vma(false) ignores VM_WRITE only but checks
> other (immutable) bits checked by valid_vma(true).

Yes, checking for other immutable flags other than VM_WRITE is good.

> This can also
> speedup uprobe_munmap() and uprobe_unregister().
> 

I didnt get how it speeds up uprobe_munmap() and uprobe_unregister()?

> Note: even after this patch _unregister can confuse the probed
> application if it does mprotect(PROT_WRITE) after _register and
> installs "int3", but this is hardly possible to avoid and this
> doesn't differ from gdb case.
> 

Again I didnt quite understand how unregister can confuse the probed
application.

> Signed-off-by: Oleg Nesterov <oleg@redhat.com>


The changes look good.

Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

> ---
>  kernel/events/uprobes.c |   13 ++++---------
>  1 files changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index b9b50dd..78364a2 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -100,17 +100,12 @@ struct uprobe {
>   */
>  static bool valid_vma(struct vm_area_struct *vma, bool is_register)
>  {
> -	if (!vma->vm_file)
> -		return false;
> -
> -	if (!is_register)
> -		return true;
> +	vm_flags_t flags = VM_HUGETLB | VM_MAYEXEC | VM_SHARED;
> 
> -	if ((vma->vm_flags & (VM_HUGETLB | VM_WRITE | VM_MAYEXEC | VM_SHARED))
> -				== VM_MAYEXEC)
> -		return true;
> +	if (is_register)
> +		flags |= VM_WRITE;
> 
> -	return false;
> +	return vma->vm_file && (vma->vm_flags & flags) == VM_MAYEXEC;
>  }
> 
>  static unsigned long offset_to_vaddr(struct vm_area_struct *vma, loff_t offset)
> -- 
> 1.5.5.1
> 

Re: [PATCH 3/3] uprobes: Restrict valid_vma(false) to skip VM_SHARED

[Oleg Nesterov]

On 09/25, Srikar Dronamraju wrote:
>
> * Oleg Nesterov <oleg@redhat.com> [2012-09-16 19:52:48]:
>
> > This can also
> > speedup uprobe_munmap() and uprobe_unregister().
> >
>
> I didnt get how it speeds up uprobe_munmap() and uprobe_unregister()?

Say, uprobe_unregister()->..->build_map_info() can skip VM_SHARED vma
early and avoid the unnecessary remove_breakpoint/get_user_pages.

The same for munmap(), no need to do vma_has_uprobes/etc if we know
that this vma can't have uprobes because valid_vma(true) is not
possible.

> > Note: even after this patch _unregister can confuse the probed
> > application if it does mprotect(PROT_WRITE) after _register and
> > installs "int3", but this is hardly possible to avoid and this
> > doesn't differ from gdb case.
>
> Again I didnt quite understand how unregister can confuse the probed
> application.

Because set_orig_insn() can never know if this "int3" was set by us
(by register) or by gdb or application itself.

But I agree, the text above looks confusing, I just wanted to remind
that this patch can't solve all problems like this.

But at least with this patch it is not possible to confuse the app
which tries to _modify_ ->vm_file via mmap.


In the long term it would be nice to avoid these problems somehow,
but this is not easy. Say, perhaps we can mark the page installed
by uprobes as OWNED-BY-KERNEL-DONT-COW and offload set_swbp() to
page fault.

Or, simpler, perhaps uprobe_register() can remove VM_MAYWRITE,
but this affects the whole vma and it is not clear how _unregister
can restore this flag correctly.

But this is off-topic.

> Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

Thanks!

Oleg.