[PATCH] uprobes: register_uprobe() crashes when bailing out.

[PATCH] uprobes: register_uprobe() crashes when bailing out.

[Torsten Polle]

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

Hi,

I had a problem with register_uprobe() when it did not return
successfully. I've not checked whether any other uses of hlist_del() may
cause similar symptoms.

Kind Regards,
Torsten


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-uprobes-register_uprobe-crashes-when-bailing-out.patch --]
[-- Type: text/x-patch, Size: 1999 bytes --]

From 9ea411a8619d2fe7d927e8068c66059c7fd002a6 Mon Sep 17 00:00:00 2001
Message-Id: <9ea411a8619d2fe7d927e8068c66059c7fd002a6.1365626073.git.Torsten.Polle@gmx.de>
From: Torsten Polle <Torsten.Polle@gmx.de>
Date: Wed, 10 Apr 2013 22:33:47 +0200
Subject: [PATCH] uprobes: register_uprobe() crashes when bailing out.

uprobe_mk_process() initialises uproc->hlist, but does not put uproc on any
list, i.e. uproc_table. If register_uprobe() now bails out before uproc is put
on a list, uprobe_free_process() still tries to remove uproc from a list. But
hlist_del() only works, if the element is already on list. hlist_del_init()
first checks if the element is on any list, before it removes the element
(uproc) from the list.

Signed-off-by: Torsten Polle <Torsten.Polle@gmx.de>
---
 runtime/linux/uprobes/uprobes.c  |    2 +-
 runtime/linux/uprobes2/uprobes.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/runtime/linux/uprobes/uprobes.c b/runtime/linux/uprobes/uprobes.c
index 01e2652..100dee9 100644
--- a/runtime/linux/uprobes/uprobes.c
+++ b/runtime/linux/uprobes/uprobes.c
@@ -519,7 +519,7 @@ static void uprobe_free_process(struct uprobe_process *uproc)
 		uprobe_release_ssol_vma(uproc);
 	if (area->slots)
 		kfree(area->slots);
-	hlist_del(&uproc->hlist);
+	hlist_del_init(&uproc->hlist);
 	list_for_each_entry_safe(utask, tmp, &uproc->thread_list, list) {
 		/*
 		 * utrace_detach() is OK here (required, it seems) even if
diff --git a/runtime/linux/uprobes2/uprobes.c b/runtime/linux/uprobes2/uprobes.c
index bb997f2..b8003f9 100644
--- a/runtime/linux/uprobes2/uprobes.c
+++ b/runtime/linux/uprobes2/uprobes.c
@@ -611,7 +611,7 @@ static void uprobe_free_process(struct uprobe_process *uproc, int in_callback)
 
 	if (area->slots)
 		kfree(area->slots);
-	hlist_del(&uproc->hlist);
+	hlist_del_init(&uproc->hlist);
 	list_for_each_entry_safe(utask, tmp, &uproc->thread_list, list)
 		uprobe_free_task(utask, in_callback);
 	put_pid(uproc->tg_leader);
-- 
1.7.4.1

Re: [PATCH] uprobes: register_uprobe() crashes when bailing out.

[Frank Ch. Eigler]

Hi -

Torsten.Polle wrote:

> [...]
> Subject: [PATCH] uprobes: register_uprobe() crashes when bailing out.
>
> uprobe_mk_process() initialises uproc->hlist, but does not put uproc on any
> list, i.e. uproc_table. If register_uprobe() now bails out before uproc is put
> on a list, uprobe_free_process() still tries to remove uproc from a list. But
> hlist_del() only works, if the element is already on list. hlist_del_init()
> first checks if the element is on any list, before it removes the element
> (uproc) from the list.

That description doesn't sound quite right to me.  I could be
mistaken, but I thought the hlist_del_init() variant was available so
that the hlist pointers are cleared after deletion (for purposes of
reuse in a different list perhaps), not in order to be checked for
list-membership before deletion.  This The change may still be right,
but perhaps for a different reason.

- FChE

Re: [PATCH] uprobes: register_uprobe() crashes when bailing out.

[Torsten Polle]

Hi Frank,

Frank Ch Eigler writes:

 > Hi -

 > Torsten.Polle wrote:

 >> [...]
 >> Subject: [PATCH] uprobes: register_uprobe() crashes when bailing out.
 >> 
 >> uprobe_mk_process() initialises uproc->hlist, but does not put uproc
 >> on any list, i.e. uproc_table. If register_uprobe() now bails out
 >> before uproc is put on a list, uprobe_free_process() still tries to
 >> remove uproc from a list. But hlist_del() only works, if the element
 >> is already on list. hlist_del_init() first checks if the element is
 >> on any list, before it removes the element (uproc) from the list.

 > That description doesn't sound quite right to me.  I could be
 > mistaken, but I thought the hlist_del_init() variant was available so
 > that the hlist pointers are cleared after deletion (for purposes of
 > reuse in a different list perhaps), not in order to be checked for
 > list-membership before deletion.  This The change may still be right,
 > but perhaps for a different reason.

below is the flow of events in this case. I have an issue with a probe
definition which makes register_uprobe() bail out. (I will follow up on
the that issue after I'm through with the analysis.)

register_uprobe() is called.
	...
	uproc = uprobe_mk_process(p, 0);
		INIT_HLIST_NODE(&uproc->hlist);
			h->next = NULL;
			h->pprev = NULL;
	uproc_is_new = 1;

Now uproc->hlist.pprev is NULL.

I know why register_uprobe() bails out but not where. I only know that 
	goto fail_uproc;
is executed.

Therefore uprobe_free_process() is called. This in turn does the following.

hlist_del(&uproc->hlist);
	__hlist_del(n);
		struct hlist_node *next = n->next;
		struct hlist_node **pprev = n->pprev;
		*pprev = next;
As n->pprev is still NULL, *pprev leads to a NULL pointer exception.

The reason why I chose hlist_del_init() is that it first checks for
pprev to be not NULL by calling hlist_unhashed().

static inline void hlist_del_init(struct hlist_node *n)
{
	if (!hlist_unhashed(n)) {
		__hlist_del(n);
		INIT_HLIST_NODE(n);
	}
}

My idea was to either call hlist_unhashed() in uprobe_free_process() or
let hlist_del_init() do the job.

Kind Regards,
Torsten

Re: [PATCH] uprobes: register_uprobe() crashes when bailing out.

[David Smith]

On 04/13/2013 06:18 AM, Torsten Polle wrote:
> Hi Frank,
> 
> Frank Ch Eigler writes:
> 
>  > Hi -
> 
>  > Torsten.Polle wrote:
> 
>  >> [...]
>  >> Subject: [PATCH] uprobes: register_uprobe() crashes when bailing out.
>  >> 
>  >> uprobe_mk_process() initialises uproc->hlist, but does not put uproc
>  >> on any list, i.e. uproc_table. If register_uprobe() now bails out
>  >> before uproc is put on a list, uprobe_free_process() still tries to
>  >> remove uproc from a list. But hlist_del() only works, if the element
>  >> is already on list. hlist_del_init() first checks if the element is
>  >> on any list, before it removes the element (uproc) from the list.
> 
>  > That description doesn't sound quite right to me.  I could be
>  > mistaken, but I thought the hlist_del_init() variant was available so
>  > that the hlist pointers are cleared after deletion (for purposes of
>  > reuse in a different list perhaps), not in order to be checked for
>  > list-membership before deletion.  This The change may still be right,
>  > but perhaps for a different reason.
> 
> below is the flow of events in this case. I have an issue with a probe
> definition which makes register_uprobe() bail out. (I will follow up on
> the that issue after I'm through with the analysis.)
> 
> register_uprobe() is called.
> 	...
> 	uproc = uprobe_mk_process(p, 0);
> 		INIT_HLIST_NODE(&uproc->hlist);
> 			h->next = NULL;
> 			h->pprev = NULL;
> 	uproc_is_new = 1;
> 
> Now uproc->hlist.pprev is NULL.
> 
> I know why register_uprobe() bails out but not where. I only know that 
> 	goto fail_uproc;
> is executed.
> 
> Therefore uprobe_free_process() is called. This in turn does the following.
> 
> hlist_del(&uproc->hlist);
> 	__hlist_del(n);
> 		struct hlist_node *next = n->next;
> 		struct hlist_node **pprev = n->pprev;
> 		*pprev = next;
> As n->pprev is still NULL, *pprev leads to a NULL pointer exception.
> 
> The reason why I chose hlist_del_init() is that it first checks for
> pprev to be not NULL by calling hlist_unhashed().
> 
> static inline void hlist_del_init(struct hlist_node *n)
> {
> 	if (!hlist_unhashed(n)) {
> 		__hlist_del(n);
> 		INIT_HLIST_NODE(n);
> 	}
> }
> 
> My idea was to either call hlist_unhashed() in uprobe_free_process() or
> let hlist_del_init() do the job.

I took a look at this one and decided this change made sense. I decided
to call hlist_unhashed() in uprobe_free_process() instead of
hlist_del_init(), since that seemed to make a bit more sense.

Here's the commit:

====
commit 5bb23177feb7974ef726c6b34bf3461729aeadd2
Author: Torsten Polle <Torsten.Polle@gmx.de>
Date:   Wed Jun 19 16:05:48 2013 -0500

    Avoid uprobes crash on error.

    * runtime/linux/uprobes/uprobes.c (uprobe_free_process): Make sure the
      uprobe_proceess' hlist field is initialized before trying to
deleted it,
      to avoid a NULL pointer dereference.
    * runtime/linux/uprobes2/uprobes.c (uprobe_free_process): Ditto.
====

Thanks for the patch. Sorry it took so long to resolve.

-- 
David Smith
dsmith@redhat.com
Red Hat
http://www.redhat.com
256.217.0141 (direct)
256.837.0057 (fax)