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

[David Smith <dsmith at redhat dot com>]

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)