Re: [RFC PATCH v3 2/6] uretprobes/x86: hijack return address

[Ananth N Mavinakayanahalli]

On Fri, Mar 01, 2013 at 12:00:43PM +0100, Anton Arapov wrote:
> On Fri, Mar 01, 2013 at 11:15:36AM +0530, Ananth N Mavinakayanahalli wrote:
> > On Thu, Feb 28, 2013 at 12:00:11PM +0100, Anton Arapov wrote:

...

> > > +extern unsigned long arch_uretprobe_hijack_return_addr(unsigned long
> > > + rp_trampoline_vaddr, struct pt_regs *regs)
> > > +{
> > > + int rasize, ncopied;
> > > + unsigned long orig_ret_vaddr = 0; /* clear high bits for 32-bit apps */
> > > +
> > > + rasize = is_ia32_task() ? 4 : 8;
> > > + ncopied = copy_from_user(_ret_vaddr, (void __user *)regs->sp, 
> > > rasize);
> > > + if (unlikely(ncopied))
> > 
> > What if ncopied < rasize? Agreed that the upper order bits can be 0, but 
> > should
> > you not validate ncopied == rasize?
> 
>   Function returns 0 in case copy_from_user() was not able to copy
> return address entirely, and "if (ncopied)" makes sure of it. We 
> can't continue if we have no correct return address.
> 
>   copy_from_user() returns number of bytes that were *not* copied,
> thus "ncopied == rasize" means copy_from_user() was not able to copy
> *all* bytes. I don't see the point of such check here.
> 
>   Or am I missing anything?

You are right... my bad.

Ananth

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH v3 2/6] uretprobes/x86: hijack return address

[Anton Arapov]

On Fri, Mar 01, 2013 at 11:15:36AM +0530, Ananth N Mavinakayanahalli wrote:
> On Thu, Feb 28, 2013 at 12:00:11PM +0100, Anton Arapov wrote:
> >   hijack the return address and replace it with a "trampoline"
> > 
> > v2:
> >   - remove ->doomed flag, kill task immediately
> > 
> > Signed-off-by: Anton Arapov 
> > ---
> >  arch/x86/include/asm/uprobes.h |  1 +
> >  arch/x86/kernel/uprobes.c  | 29 +
> >  2 files changed, 30 insertions(+)
> > 
> > diff --git a/arch/x86/include/asm/uprobes.h b/arch/x86/include/asm/uprobes.h
> > index 8ff8be7..c353555 100644
> > --- a/arch/x86/include/asm/uprobes.h
> > +++ b/arch/x86/include/asm/uprobes.h
> > @@ -55,4 +55,5 @@ extern int  arch_uprobe_post_xol(struct arch_uprobe *aup, 
> > struct pt_regs *regs);
> >  extern bool arch_uprobe_xol_was_trapped(struct task_struct *tsk);
> >  extern int  arch_uprobe_exception_notify(struct notifier_block *self, 
> > unsigned long val, void *data);
> >  extern void arch_uprobe_abort_xol(struct arch_uprobe *aup, struct pt_regs 
> > *regs);
> > +extern unsigned long arch_uretprobe_hijack_return_addr(unsigned long 
> > rp_trampoline_vaddr, struct pt_regs *regs);
> >  #endif /* _ASM_UPROBES_H */
> > diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> > index 0ba4cfb..85e2153 100644
> > --- a/arch/x86/kernel/uprobes.c
> > +++ b/arch/x86/kernel/uprobes.c
> > @@ -697,3 +697,32 @@ bool arch_uprobe_skip_sstep(struct arch_uprobe 
> > *auprobe, struct pt_regs *regs)
> > send_sig(SIGTRAP, current, 0);
> > return ret;
> >  }
> > +
> > +extern unsigned long arch_uretprobe_hijack_return_addr(unsigned long
> > +   rp_trampoline_vaddr, struct pt_regs *regs)
> > +{
> > +   int rasize, ncopied;
> > +   unsigned long orig_ret_vaddr = 0; /* clear high bits for 32-bit apps */
> > +
> > +   rasize = is_ia32_task() ? 4 : 8;
> > +   ncopied = copy_from_user(_ret_vaddr, (void __user *)regs->sp, 
> > rasize);
> > +   if (unlikely(ncopied))
> 
> What if ncopied < rasize? Agreed that the upper order bits can be 0, but 
> should
> you not validate ncopied == rasize?

  Function returns 0 in case copy_from_user() was not able to copy
return address entirely, and "if (ncopied)" makes sure of it. We 
can't continue if we have no correct return address.

  copy_from_user() returns number of bytes that were *not* copied,
thus "ncopied == rasize" means copy_from_user() was not able to copy
*all* bytes. I don't see the point of such check here.

  Or am I missing anything?

thank you!
Anton.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH v3 2/6] uretprobes/x86: hijack return address

[Anton Arapov]

On Fri, Mar 01, 2013 at 11:15:36AM +0530, Ananth N Mavinakayanahalli wrote:
 On Thu, Feb 28, 2013 at 12:00:11PM +0100, Anton Arapov wrote:
hijack the return address and replace it with a trampoline
  
  v2:
- remove -doomed flag, kill task immediately
  
  Signed-off-by: Anton Arapov an...@redhat.com
  ---
   arch/x86/include/asm/uprobes.h |  1 +
   arch/x86/kernel/uprobes.c  | 29 +
   2 files changed, 30 insertions(+)
  
  diff --git a/arch/x86/include/asm/uprobes.h b/arch/x86/include/asm/uprobes.h
  index 8ff8be7..c353555 100644
  --- a/arch/x86/include/asm/uprobes.h
  +++ b/arch/x86/include/asm/uprobes.h
  @@ -55,4 +55,5 @@ extern int  arch_uprobe_post_xol(struct arch_uprobe *aup, 
  struct pt_regs *regs);
   extern bool arch_uprobe_xol_was_trapped(struct task_struct *tsk);
   extern int  arch_uprobe_exception_notify(struct notifier_block *self, 
  unsigned long val, void *data);
   extern void arch_uprobe_abort_xol(struct arch_uprobe *aup, struct pt_regs 
  *regs);
  +extern unsigned long arch_uretprobe_hijack_return_addr(unsigned long 
  rp_trampoline_vaddr, struct pt_regs *regs);
   #endif /* _ASM_UPROBES_H */
  diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
  index 0ba4cfb..85e2153 100644
  --- a/arch/x86/kernel/uprobes.c
  +++ b/arch/x86/kernel/uprobes.c
  @@ -697,3 +697,32 @@ bool arch_uprobe_skip_sstep(struct arch_uprobe 
  *auprobe, struct pt_regs *regs)
  send_sig(SIGTRAP, current, 0);
  return ret;
   }
  +
  +extern unsigned long arch_uretprobe_hijack_return_addr(unsigned long
  +   rp_trampoline_vaddr, struct pt_regs *regs)
  +{
  +   int rasize, ncopied;
  +   unsigned long orig_ret_vaddr = 0; /* clear high bits for 32-bit apps */
  +
  +   rasize = is_ia32_task() ? 4 : 8;
  +   ncopied = copy_from_user(orig_ret_vaddr, (void __user *)regs-sp, 
  rasize);
  +   if (unlikely(ncopied))
 
 What if ncopied  rasize? Agreed that the upper order bits can be 0, but 
 should
 you not validate ncopied == rasize?

  Function returns 0 in case copy_from_user() was not able to copy
return address entirely, and if (ncopied) makes sure of it. We 
can't continue if we have no correct return address.

  copy_from_user() returns number of bytes that were *not* copied,
thus ncopied == rasize means copy_from_user() was not able to copy
*all* bytes. I don't see the point of such check here.

  Or am I missing anything?

thank you!
Anton.

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH v3 2/6] uretprobes/x86: hijack return address

[Ananth N Mavinakayanahalli]

On Fri, Mar 01, 2013 at 12:00:43PM +0100, Anton Arapov wrote:
 On Fri, Mar 01, 2013 at 11:15:36AM +0530, Ananth N Mavinakayanahalli wrote:
  On Thu, Feb 28, 2013 at 12:00:11PM +0100, Anton Arapov wrote:

...

   +extern unsigned long arch_uretprobe_hijack_return_addr(unsigned long
   + rp_trampoline_vaddr, struct pt_regs *regs)
   +{
   + int rasize, ncopied;
   + unsigned long orig_ret_vaddr = 0; /* clear high bits for 32-bit apps */
   +
   + rasize = is_ia32_task() ? 4 : 8;
   + ncopied = copy_from_user(orig_ret_vaddr, (void __user *)regs-sp, 
   rasize);
   + if (unlikely(ncopied))
  
  What if ncopied  rasize? Agreed that the upper order bits can be 0, but 
  should
  you not validate ncopied == rasize?
 
   Function returns 0 in case copy_from_user() was not able to copy
 return address entirely, and if (ncopied) makes sure of it. We 
 can't continue if we have no correct return address.
 
   copy_from_user() returns number of bytes that were *not* copied,
 thus ncopied == rasize means copy_from_user() was not able to copy
 *all* bytes. I don't see the point of such check here.
 
   Or am I missing anything?

You are right... my bad.

Ananth

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH v3 2/6] uretprobes/x86: hijack return address

[Ananth N Mavinakayanahalli]

On Thu, Feb 28, 2013 at 12:00:11PM +0100, Anton Arapov wrote:
>   hijack the return address and replace it with a "trampoline"
> 
> v2:
>   - remove ->doomed flag, kill task immediately
> 
> Signed-off-by: Anton Arapov 
> ---
>  arch/x86/include/asm/uprobes.h |  1 +
>  arch/x86/kernel/uprobes.c  | 29 +
>  2 files changed, 30 insertions(+)
> 
> diff --git a/arch/x86/include/asm/uprobes.h b/arch/x86/include/asm/uprobes.h
> index 8ff8be7..c353555 100644
> --- a/arch/x86/include/asm/uprobes.h
> +++ b/arch/x86/include/asm/uprobes.h
> @@ -55,4 +55,5 @@ extern int  arch_uprobe_post_xol(struct arch_uprobe *aup, 
> struct pt_regs *regs);
>  extern bool arch_uprobe_xol_was_trapped(struct task_struct *tsk);
>  extern int  arch_uprobe_exception_notify(struct notifier_block *self, 
> unsigned long val, void *data);
>  extern void arch_uprobe_abort_xol(struct arch_uprobe *aup, struct pt_regs 
> *regs);
> +extern unsigned long arch_uretprobe_hijack_return_addr(unsigned long 
> rp_trampoline_vaddr, struct pt_regs *regs);
>  #endif   /* _ASM_UPROBES_H */
> diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> index 0ba4cfb..85e2153 100644
> --- a/arch/x86/kernel/uprobes.c
> +++ b/arch/x86/kernel/uprobes.c
> @@ -697,3 +697,32 @@ bool arch_uprobe_skip_sstep(struct arch_uprobe *auprobe, 
> struct pt_regs *regs)
>   send_sig(SIGTRAP, current, 0);
>   return ret;
>  }
> +
> +extern unsigned long arch_uretprobe_hijack_return_addr(unsigned long
> + rp_trampoline_vaddr, struct pt_regs *regs)
> +{
> + int rasize, ncopied;
> + unsigned long orig_ret_vaddr = 0; /* clear high bits for 32-bit apps */
> +
> + rasize = is_ia32_task() ? 4 : 8;
> + ncopied = copy_from_user(_ret_vaddr, (void __user *)regs->sp, 
> rasize);
> + if (unlikely(ncopied))

What if ncopied < rasize? Agreed that the upper order bits can be 0, but should
you not validate ncopied == rasize?

Ananth

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[RFC PATCH v3 2/6] uretprobes/x86: hijack return address

[Anton Arapov]

  hijack the return address and replace it with a "trampoline"

v2:
  - remove ->doomed flag, kill task immediately

Signed-off-by: Anton Arapov 
---
 arch/x86/include/asm/uprobes.h |  1 +
 arch/x86/kernel/uprobes.c  | 29 +
 2 files changed, 30 insertions(+)

diff --git a/arch/x86/include/asm/uprobes.h b/arch/x86/include/asm/uprobes.h
index 8ff8be7..c353555 100644
--- a/arch/x86/include/asm/uprobes.h
+++ b/arch/x86/include/asm/uprobes.h
@@ -55,4 +55,5 @@ extern int  arch_uprobe_post_xol(struct arch_uprobe *aup, 
struct pt_regs *regs);
 extern bool arch_uprobe_xol_was_trapped(struct task_struct *tsk);
 extern int  arch_uprobe_exception_notify(struct notifier_block *self, unsigned 
long val, void *data);
 extern void arch_uprobe_abort_xol(struct arch_uprobe *aup, struct pt_regs 
*regs);
+extern unsigned long arch_uretprobe_hijack_return_addr(unsigned long 
rp_trampoline_vaddr, struct pt_regs *regs);
 #endif /* _ASM_UPROBES_H */
diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
index 0ba4cfb..85e2153 100644
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -697,3 +697,32 @@ bool arch_uprobe_skip_sstep(struct arch_uprobe *auprobe, 
struct pt_regs *regs)
send_sig(SIGTRAP, current, 0);
return ret;
 }
+
+extern unsigned long arch_uretprobe_hijack_return_addr(unsigned long
+   rp_trampoline_vaddr, struct pt_regs *regs)
+{
+   int rasize, ncopied;
+   unsigned long orig_ret_vaddr = 0; /* clear high bits for 32-bit apps */
+
+   rasize = is_ia32_task() ? 4 : 8;
+   ncopied = copy_from_user(_ret_vaddr, (void __user *)regs->sp, 
rasize);
+   if (unlikely(ncopied))
+   return 0;
+
+   /* check whether address has been already hijacked */
+   if (orig_ret_vaddr == rp_trampoline_vaddr)
+   return orig_ret_vaddr;
+
+   ncopied = copy_to_user((void __user *)regs->sp, _trampoline_vaddr, 
rasize);
+   if (unlikely(ncopied)) {
+   if (ncopied != rasize) {
+   printk(KERN_ERR "uretprobe: return address clobbered: "
+   "pid=%d, %%sp=%#lx, %%ip=%#lx\n",
+   current->pid, regs->sp, regs->ip);
+   /* kill task immediately */
+   send_sig(SIGSEGV, current, 0);
+   }
+   }
+
+   return orig_ret_vaddr;
+}
-- 
1.8.1.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[RFC PATCH v3 2/6] uretprobes/x86: hijack return address

[Anton Arapov]

  hijack the return address and replace it with a trampoline

v2:
  - remove -doomed flag, kill task immediately

Signed-off-by: Anton Arapov an...@redhat.com
---
 arch/x86/include/asm/uprobes.h |  1 +
 arch/x86/kernel/uprobes.c  | 29 +
 2 files changed, 30 insertions(+)

diff --git a/arch/x86/include/asm/uprobes.h b/arch/x86/include/asm/uprobes.h
index 8ff8be7..c353555 100644
--- a/arch/x86/include/asm/uprobes.h
+++ b/arch/x86/include/asm/uprobes.h
@@ -55,4 +55,5 @@ extern int  arch_uprobe_post_xol(struct arch_uprobe *aup, 
struct pt_regs *regs);
 extern bool arch_uprobe_xol_was_trapped(struct task_struct *tsk);
 extern int  arch_uprobe_exception_notify(struct notifier_block *self, unsigned 
long val, void *data);
 extern void arch_uprobe_abort_xol(struct arch_uprobe *aup, struct pt_regs 
*regs);
+extern unsigned long arch_uretprobe_hijack_return_addr(unsigned long 
rp_trampoline_vaddr, struct pt_regs *regs);
 #endif /* _ASM_UPROBES_H */
diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
index 0ba4cfb..85e2153 100644
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -697,3 +697,32 @@ bool arch_uprobe_skip_sstep(struct arch_uprobe *auprobe, 
struct pt_regs *regs)
send_sig(SIGTRAP, current, 0);
return ret;
 }
+
+extern unsigned long arch_uretprobe_hijack_return_addr(unsigned long
+   rp_trampoline_vaddr, struct pt_regs *regs)
+{
+   int rasize, ncopied;
+   unsigned long orig_ret_vaddr = 0; /* clear high bits for 32-bit apps */
+
+   rasize = is_ia32_task() ? 4 : 8;
+   ncopied = copy_from_user(orig_ret_vaddr, (void __user *)regs-sp, 
rasize);
+   if (unlikely(ncopied))
+   return 0;
+
+   /* check whether address has been already hijacked */
+   if (orig_ret_vaddr == rp_trampoline_vaddr)
+   return orig_ret_vaddr;
+
+   ncopied = copy_to_user((void __user *)regs-sp, rp_trampoline_vaddr, 
rasize);
+   if (unlikely(ncopied)) {
+   if (ncopied != rasize) {
+   printk(KERN_ERR uretprobe: return address clobbered: 
+   pid=%d, %%sp=%#lx, %%ip=%#lx\n,
+   current-pid, regs-sp, regs-ip);
+   /* kill task immediately */
+   send_sig(SIGSEGV, current, 0);
+   }
+   }
+
+   return orig_ret_vaddr;
+}
-- 
1.8.1.2

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH v3 2/6] uretprobes/x86: hijack return address

[Ananth N Mavinakayanahalli]

On Thu, Feb 28, 2013 at 12:00:11PM +0100, Anton Arapov wrote:
   hijack the return address and replace it with a trampoline
 
 v2:
   - remove -doomed flag, kill task immediately
 
 Signed-off-by: Anton Arapov an...@redhat.com
 ---
  arch/x86/include/asm/uprobes.h |  1 +
  arch/x86/kernel/uprobes.c  | 29 +
  2 files changed, 30 insertions(+)
 
 diff --git a/arch/x86/include/asm/uprobes.h b/arch/x86/include/asm/uprobes.h
 index 8ff8be7..c353555 100644
 --- a/arch/x86/include/asm/uprobes.h
 +++ b/arch/x86/include/asm/uprobes.h
 @@ -55,4 +55,5 @@ extern int  arch_uprobe_post_xol(struct arch_uprobe *aup, 
 struct pt_regs *regs);
  extern bool arch_uprobe_xol_was_trapped(struct task_struct *tsk);
  extern int  arch_uprobe_exception_notify(struct notifier_block *self, 
 unsigned long val, void *data);
  extern void arch_uprobe_abort_xol(struct arch_uprobe *aup, struct pt_regs 
 *regs);
 +extern unsigned long arch_uretprobe_hijack_return_addr(unsigned long 
 rp_trampoline_vaddr, struct pt_regs *regs);
  #endif   /* _ASM_UPROBES_H */
 diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
 index 0ba4cfb..85e2153 100644
 --- a/arch/x86/kernel/uprobes.c
 +++ b/arch/x86/kernel/uprobes.c
 @@ -697,3 +697,32 @@ bool arch_uprobe_skip_sstep(struct arch_uprobe *auprobe, 
 struct pt_regs *regs)
   send_sig(SIGTRAP, current, 0);
   return ret;
  }
 +
 +extern unsigned long arch_uretprobe_hijack_return_addr(unsigned long
 + rp_trampoline_vaddr, struct pt_regs *regs)
 +{
 + int rasize, ncopied;
 + unsigned long orig_ret_vaddr = 0; /* clear high bits for 32-bit apps */
 +
 + rasize = is_ia32_task() ? 4 : 8;
 + ncopied = copy_from_user(orig_ret_vaddr, (void __user *)regs-sp, 
 rasize);
 + if (unlikely(ncopied))

What if ncopied  rasize? Agreed that the upper order bits can be 0, but should
you not validate ncopied == rasize?

Ananth

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/