netdev - Re: [PATCH] fib_trie: Fix potential null pointer dereference

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:	Sat, 06 Jun 2015 20:20:38 -0700
From:	Alexander Duyck <alexander.duyck@...il.com>
To:	Eric Dumazet <eric.dumazet@...il.com>, Firo Yang <firogm@...il.com>
CC:	davem@...emloft.net, kuznet@....inr.ac.ru, jmorris@...ei.org,
	yoshfuji@...ux-ipv6.org, kaber@...sh.net, netdev@...r.kernel.org,
	kernel-janitors@...r.kernel.org
Subject: Re: [PATCH] fib_trie: Fix potential null pointer dereference

On 06/06/2015 05:05 AM, Eric Dumazet wrote:
> On Sat, 2015-06-06 at 19:35 +0800, Firo Yang wrote:
>> A smatch warning.
>> When kmem_cache_alloc() failed to alloc memory, a null pointer
>> will be returned. Redeference null pointer will generate
>
>
> Dereferencing a null pointer will crash.
>
>> an unnecessary oops. So, use it after check.
>>
>> Signed-off-by: Firo Yang <firogm@...il.com>
>> ---
>>   net/ipv4/fib_trie.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c
>> index 01bce15..34094c7 100644
>> --- a/net/ipv4/fib_trie.c
>> +++ b/net/ipv4/fib_trie.c
>> @@ -326,12 +326,13 @@ static inline void empty_child_dec(struct key_vector *n)
>>   static struct key_vector *leaf_new(t_key key, struct fib_alias *fa)
>>   {
>>   	struct tnode *kv = kmem_cache_alloc(trie_leaf_kmem, GFP_KERNEL);
>> -	struct key_vector *l = kv->kv;
>> +	struct key_vector *l;
>>
>>   	if (!kv)
>>   		return NULL;
>>
>>   	/* initialize key vector */
>> +	l = kv->kv;
>>   	l->key = key;
>>   	l->pos = 0;
>>   	l->bits = 0;
>
> Fixes: dc35dbeda3e0 ("fib_trie: Add tnode struct as a container for fields not needed in key_vector")
> Acked-by: Eric Dumazet <edumazet@...gle.com>
>
> Thanks.

kv->kv isn't a dereference.  kv is an array contained within the tnode. 
  Below is the layout of struct tnode:


struct tnode {
	struct rcu_head rcu;
	t_key empty_children;		/* KEYLENGTH bits needed */
	t_key full_children;		/* KEYLENGTH bits needed */
	struct key_vector __rcu *parent;
	struct key_vector kv[1];
#define tn_bits kv[0].bits
};


As such kv->kv doesn't actually dereference anything, it is simply a 
means for getting the offset to the array from the pointer kv.  It 
should be equivalent to &kv->kv[0] which last I knew was how the 
offsetof macro basically worked and it uses a NULL pointer.  The first 
actual derference occurs at "l->key = key" which should be safe since 
writing to a memory location has side effects so the compiler cannot 
reorder the writes before the !kv check.

The patch itself is fine.  However what it is fixing is the smatch false 
positive, not a null pointer deference.  As such you may need to update 
the comments to reflect that.

- Alex



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html