linux-kernel - Re: [PATCH 10/10] cleanup the code between tmem_obj_init and tmem_obj

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 19 Jun 2012 11:49:13 -0500
From:	Seth Jennings <sjenning@...ux.vnet.ibm.com>
To:	Xiao Guangrong <xiaoguangrong@...ux.vnet.ibm.com>
CC:	Andrew Morton <akpm@...ux-foundation.org>,
	Dan Magenheimer <dan.magenheimer@...cle.com>,
	LKML <linux-kernel@...r.kernel.org>, linux-mm@...ck.org
Subject: Re: [PATCH 10/10] cleanup the code between tmem_obj_init and tmem_obj_find

This patch causes a crash, details below.

On 06/19/2012 03:37 AM, Xiao Guangrong wrote:

> tmem_obj_find and insertion tmem-obj have the some logic, we can integrate
> the code
> 
> Signed-off-by: Xiao Guangrong <xiaoguangrong@...ux.vnet.ibm.com>
> ---
>  drivers/staging/zcache/tmem.c |   58 +++++++++++++++++++++-------------------
>  1 files changed, 30 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/staging/zcache/tmem.c b/drivers/staging/zcache/tmem.c
> index 1ca66ea..cdf2d3c 100644
> --- a/drivers/staging/zcache/tmem.c
> +++ b/drivers/staging/zcache/tmem.c
> @@ -72,33 +72,48 @@ void tmem_register_pamops(struct tmem_pamops *m)
>   * the hashbucket lock must be held.
>   */
> 
> -/* searches for object==oid in pool, returns locked object if found */
> -static struct tmem_obj *tmem_obj_find(struct tmem_hashbucket *hb,
> -					struct tmem_oid *oidp)
> +static struct tmem_obj
> +*__tmem_obj_find(struct tmem_hashbucket*hb, struct tmem_oid *oidp,
> +		 struct rb_node *parent, struct rb_node **link)
>  {
> -	struct rb_node *rbnode;
> +	struct rb_node **rbnode;
>  	struct tmem_obj *obj;
> 
> -	rbnode = hb->obj_rb_root.rb_node;
> -	while (rbnode) {
> -		BUG_ON(RB_EMPTY_NODE(rbnode));
> -		obj = rb_entry(rbnode, struct tmem_obj, rb_tree_node);
> +	rbnode = &hb->obj_rb_root.rb_node;
> +	while (*rbnode) {
> +		BUG_ON(RB_EMPTY_NODE(*rbnode));
> +		obj = rb_entry(*rbnode, struct tmem_obj,
> +			       rb_tree_node);
>  		switch (tmem_oid_compare(oidp, &obj->oid)) {
>  		case 0: /* equal */
>  			goto out;
>  		case -1:
> -			rbnode = rbnode->rb_left;
> +			rbnode = &(*rbnode)->rb_left;
>  			break;
>  		case 1:
> -			rbnode = rbnode->rb_right;
> +			rbnode = &(*rbnode)->rb_right;
>  			break;
>  		}
>  	}
> +
> +	if (parent)
> +		parent = &obj->rb_tree_node;
> +	if (link)
> +		link = rbnode;
> +
>  	obj = NULL;
>  out:
>  	return obj;
>  }
> 
> +
> +/* searches for object==oid in pool, returns locked object if found */
> +static struct tmem_obj *tmem_obj_find(struct tmem_hashbucket *hb,
> +					struct tmem_oid *oidp)
> +{
> +	return __tmem_obj_find(hb, oidp, NULL, NULL);
> +}
> +
>  static void tmem_pampd_destroy_all_in_obj(struct tmem_obj *);
> 
>  /* free an object that has no more pampds in it */
> @@ -131,8 +146,7 @@ static void tmem_obj_init(struct tmem_obj *obj, struct tmem_hashbucket *hb,
>  					struct tmem_oid *oidp)
>  {
>  	struct rb_root *root = &hb->obj_rb_root;
> -	struct rb_node **new = &(root->rb_node), *parent = NULL;
> -	struct tmem_obj *this;
> +	struct rb_node **new = NULL, *parent = NULL;
> 
>  	BUG_ON(pool == NULL);
>  	atomic_inc(&pool->obj_count);
> @@ -144,22 +158,10 @@ static void tmem_obj_init(struct tmem_obj *obj, struct tmem_hashbucket *hb,
>  	obj->pampd_count = 0;
>  	(*tmem_pamops.new_obj)(obj);
>  	SET_SENTINEL(obj, OBJ);
> -	while (*new) {
> -		BUG_ON(RB_EMPTY_NODE(*new));
> -		this = rb_entry(*new, struct tmem_obj, rb_tree_node);
> -		parent = *new;
> -		switch (tmem_oid_compare(oidp, &this->oid)) {
> -		case 0:
> -			BUG(); /* already present; should never happen! */
> -			break;
> -		case -1:
> -			new = &(*new)->rb_left;
> -			break;
> -		case 1:
> -			new = &(*new)->rb_right;
> -			break;
> -		}
> -	}
> +
> +	if (__tmem_obj_find(hb, oidp, parent, new))
> +		BUG();
> +
>  	rb_link_node(&obj->rb_tree_node, parent, new);


Getting a NULL deref crash here because new is NULL

[   56.422031] BUG: unable to handle kernel NULL pointer dereference at           (null)
[   56.423008] IP: [<ffffffff812b8ba4>] tmem_put+0x3a4/0x3d0

static inline void rb_link_node(struct rb_node * node, struct rb_node * parent,
				struct rb_node ** rb_link)
{
...
	*rb_link = node;
ffffffff812b8ba4:	48 89 38             	mov    %rdi,(%rax) <--- here
ffffffff812b8ba7:	e8 00 00 00 00       	callq  ffffffff812b8bac <tmem_put+0x3ac>

--
Seth

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