Re: [RFC: 2.6.25 patch] ipv4/fib_hash.c: fix NULL dereference

[Eric Dumazet <dada1@cosmosbay.com>]

Adrian Bunk a écrit :
> Unless I miss a guaranteed relation between between "f" and 
> "new_fa->fa_info" this patch is required for fixing a NULL dereference
> introduced by commit a6501e080c318f8d4467679d17807f42b3a33cd5 and 
> spotted by the Coverity checker.
> 
> Signed-off-by: Adrian Bunk <bunk@kernel.org>
> 
> ---
> 
>  net/ipv4/fib_hash.c |   10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> --- linux-2.6/net/ipv4/fib_hash.c.old	2008-02-19 23:23:14.000000000 +0200
> +++ linux-2.6/net/ipv4/fib_hash.c	2008-02-19 23:38:28.000000000 +0200
> @@ -367,17 +367,18 @@ static struct fib_node *fib_find_node(st
>  	}
>  
>  	return NULL;
>  }
>  
>  static int fn_hash_insert(struct fib_table *tb, struct fib_config *cfg)
>  {
>  	struct fn_hash *table = (struct fn_hash *) tb->tb_data;
> -	struct fib_node *new_f, *f;
> +	struct fib_node *new_f = NULL;
> +	struct fib_node *f;
>  	struct fib_alias *fa, *new_fa;
>  	struct fn_zone *fz;
>  	struct fib_info *fi;
>  	u8 tos = cfg->fc_tos;
>  	__be32 key;
>  	int err;
>  
>  	if (cfg->fc_dst_len > 32)
> @@ -491,33 +492,32 @@ static int fn_hash_insert(struct fib_tab
>  	}
>  
>  	err = -ENOENT;
>  	if (!(cfg->fc_nlflags & NLM_F_CREATE))
>  		goto out;
>  
>  	err = -ENOBUFS;
>  
> -	new_f = NULL;
>  	if (!f) {
>  		new_f = kmem_cache_zalloc(fn_hash_kmem, GFP_KERNEL);
>  		if (new_f == NULL)
>  			goto out;
>  
>  		INIT_HLIST_NODE(&new_f->fn_hash);
>  		INIT_LIST_HEAD(&new_f->fn_alias);
>  		new_f->fn_key = key;
>  		f = new_f;
>  	}
>  
>  	new_fa = &f->fn_embedded_alias;
>  	if (new_fa->fa_info != NULL) {
>  		new_fa = kmem_cache_alloc(fn_alias_kmem, GFP_KERNEL);
>  		if (new_fa == NULL)
> -			goto out_free_new_f;
> +			goto out;
>  	}
>  	new_fa->fa_info = fi;
>  	new_fa->fa_tos = tos;
>  	new_fa->fa_type = cfg->fc_type;
>  	new_fa->fa_scope = cfg->fc_scope;
>  	new_fa->fa_state = 0;
>  
>  	/*
> @@ -535,19 +535,19 @@ static int fn_hash_insert(struct fib_tab
>  	if (new_f)
>  		fz->fz_nent++;
>  	rt_cache_flush(-1);
>  
>  	rtmsg_fib(RTM_NEWROUTE, key, new_fa, cfg->fc_dst_len, tb->tb_id,
>  		  &cfg->fc_nlinfo, 0);
>  	return 0;
>  
> -out_free_new_f:
> -	kmem_cache_free(fn_hash_kmem, new_f);
>  out:
> +	if (new_f)
> +		kmem_cache_free(fn_hash_kmem, new_f);
>  	fib_release_info(fi);
>  	return err;
>  }
>  
>  
>  static int fn_hash_delete(struct fib_table *tb, struct fib_config *cfg)
>  {
>  	struct fn_hash *table = (struct fn_hash*)tb->tb_data;
> 

Hum, you are right, kmem_cache_free() doesnt allow a NULL object, like kfree() 
does.