fib_trie: Fix potential null pointer dereference

PATCH In review Firo Yang 5 participants 12 messages Jun 6, 2015 – Jun 7, 2015 1 message not in the archive

Subsystems (4)

Networking Network drivers Kernel janitors IPv4/IPv6

lore

Share & tools

lore

Firo Yang

author

2015-06-06 11:35 UTC #0 lore raw

PATCH 1/1 fib_trie: Fix potential null pointer dereference

A smatch warning.
When kmem_cache_alloc() failed to alloc memory, a null pointer will be returned. Redeference null pointer will generate an unnecessary oops. So, use it after check.

Signed-off-by Firo Yang <[email protected]>

---
 net/ipv4/fib_trie.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

net/ipv4/fib_trie.c +2 -1

index 01bce15..34094c7 100644
@@ -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;

Signature · 1 line

2.4.2

Eric Dumazet

Acked-by on v1 1/1

2015-06-06 12:05 UTC #1 lore raw

Quoted 3 lines · Firo Yang

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.

Quoted 16 lines

an unnecessary oops. So, use it after check.

Signed-off-by Firo Yang <[email protected]>

---
 net/ipv4/fib_trie.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

net/ipv4/fib_trie.c +0 -1

index 01bce15..34094c7 100644
@@ -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;

net/ipv4/fib_trie.c static inline void empty_child_dec(struct key_vector *n) · line 338

Quoted 10 lines

net/ipv4/fib_trie.c +2 -0

+	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 <[email protected]>

Thanks.

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

A message that is not in the archive (sent off-list or not yet received): [email protected]

Firo Yang

author

2015-06-07 04:46 UTC #4 lore raw

Quoted 59 lines · Alexander Duyck

Quoted 34 lines · Eric Dumazet
Quoted 3 lines · Firo Yang
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.
Quoted 14 lines
an unnecessary oops. So, use it after check.
Signed-off-by Firo Yang <[email protected]>
---
 net/ipv4/fib_trie.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
net/ipv4/fib_trie.c +0 -0
index 01bce15..34094c7 100644
@@ -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)
 {
net/ipv4/fib_trie.c static inline void empty_child_dec(struct key_vector *n) · line 336
Quoted 10 lines
net/ipv4/fib_trie.c +2 -1
 	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 <[email protected]>
Quoted 1 line
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.

Quoted 3 lines

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.

Thanks Alex for firguring out my false cognition!
I will update the patch with new comments.

Firo

Quoted 4 lines

- Alex

Firo Yang

author

2015-06-07 06:01 UTC #5 lore raw

In reply to Eric Dumazet

Quoted 38 lines · Eric Dumazet

Quoted 3 lines · Firo Yang
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.
Quoted 16 lines
an unnecessary oops. So, use it after check.
Signed-off-by Firo Yang <[email protected]>
---
 net/ipv4/fib_trie.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
net/ipv4/fib_trie.c +0 -1
index 01bce15..34094c7 100644
@@ -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;
net/ipv4/fib_trie.c static inline void empty_child_dec(struct key_vector *n) · line 338
Quoted 10 lines
net/ipv4/fib_trie.c +2 -0
+	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 <[email protected]>

Quoted 1 line

Thanks.

Hi Eric,
Please discard this useless patch figured out by Alexander Duyck. I will send a patch to Smatch for eliminating the negative warning.

Quoted 2 lines

Julia Lawall

2015-06-07 06:36 UTC #6 lore raw

Quoted 39 lines · Firo Yang

Quoted 38 lines · Eric Dumazet
Quoted 3 lines · Firo Yang
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.
Quoted 16 lines
an unnecessary oops. So, use it after check.
Signed-off-by Firo Yang <[email protected]>
---
 net/ipv4/fib_trie.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
net/ipv4/fib_trie.c +0 -1
index 01bce15..34094c7 100644
@@ -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;
net/ipv4/fib_trie.c static inline void empty_child_dec(struct key_vector *n) · line 338
Quoted 10 lines
net/ipv4/fib_trie.c +2 -0
+	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 <[email protected]>

Quoted 5 lines

Quoted 1 line
Thanks.
Hi Eric,
Please discard this useless patch figured out by Alexander Duyck. I will send a patch to Smatch for eliminating the negative warning.

I think that many people would make the same mistake when looking at the code. The change doesn't seem to hurt anything?

julia

Firo Yang

author

2015-06-07 13:19 UTC #7 lore raw

Quoted 43 lines · Julia Lawall

Quoted 39 lines · Firo Yang
Quoted 38 lines · Eric Dumazet
Quoted 3 lines · Firo Yang
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.
Quoted 16 lines
an unnecessary oops. So, use it after check.
Signed-off-by Firo Yang <[email protected]>
---
 net/ipv4/fib_trie.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
net/ipv4/fib_trie.c +0 -1
index 01bce15..34094c7 100644
@@ -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;
net/ipv4/fib_trie.c static inline void empty_child_dec(struct key_vector *n) · line 338
Quoted 10 lines
net/ipv4/fib_trie.c +2 -0
+	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 <[email protected]>

Quoted 8 lines

Quoted 5 lines
Quoted 1 line
Thanks.
Hi Eric,
Please discard this useless patch figured out by Alexander Duyck. I will send a patch to Smatch for eliminating the negative warning.
I think that many people would make the same mistake when looking at the code. The change doesn't seem to hurt anything?

Actually, yes. But it does imporve nothing but coding style. I think Alexander's code style make function more compact is also a good code style. So, just keep it original.

Firo

Quoted 2 lines

julia

Julia Lawall

2015-06-07 13:23 UTC #8 lore raw

Quoted 5 lines

Quoted 2 lines
I think that many people would make the same mistake when looking at the code. The change doesn't seem to hurt anything?
Actually, yes. But it does imporve nothing but coding style. I think Alexander's code style make function more compact is also a good code style. So, just keep it original.

Still it makes noise when other people look at the code and find that it looks odd. Removing the false positive in smatch won't completely help, because people could look at the code for other reasons.

But, as you like.

julia

Firo Yang

author

2015-06-07 13:42 UTC #9 lore raw

Quoted 9 lines · Julia Lawall

Quoted 5 lines
Quoted 2 lines
I think that many people would make the same mistake when looking at the code. The change doesn't seem to hurt anything?
Actually, yes. But it does imporve nothing but coding style. I think Alexander's code style make function more compact is also a good code style. So, just keep it original.
Still it makes noise when other people look at the code and find that it looks odd. Removing the false positive in smatch won't completely help, because people could look at the code for other reasons.

You said it! I will do the improvement.

Firo

Quoted 4 lines

But, as you like.
julia

Alexander Duyck

2015-06-07 16:36 UTC #10 lore raw

In reply to Julia Lawall

Quoted 9 lines · Julia Lawall

Quoted 5 lines
Quoted 2 lines
I think that many people would make the same mistake when looking at the code. The change doesn't seem to hurt anything?
Actually, yes. But it does imporve nothing but coding style. I think Alexander's code style make function more compact is also a good code style. So, just keep it original.
Still it makes noise when other people look at the code and find that it looks odd. Removing the false positive in smatch won't completely help, because people could look at the code for other reasons.

I agree. The patch is good. My only comment was about the patch description. You are fixing a coding style issue reported by smatch, not a NULL pointer dereference. This is a code clean-up rather than a fix, but still the patch is just as useful.

While you are at it you could probably also incorporate the suggestion from Walter as that helps to improve the readability further.

- Alex

walter harms

2015-06-06 13:39 UTC #11 lore raw

In reply to Firo Yang

Am 06.06.2015 13:35, schrieb Firo Yang:

Quoted 15 lines

A smatch warning.
When kmem_cache_alloc() failed to alloc memory, a null pointer will be returned. Redeference null pointer will generate an unnecessary oops. So, use it after check.
Signed-off-by Firo Yang <[email protected]>
---
 net/ipv4/fib_trie.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
net/ipv4/fib_trie.c +0 -0
index 01bce15..34094c7 100644

net/ipv4/fib_trie.c static inline void empty_child_dec(struct key_vector *n) · line 329

Quoted 5 lines

net/ipv4/fib_trie.c +1 -1

@@ -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;

It is a good custom to have action and check close together, so this may be more obvious for future readers:

	struct tnode *kv;
	struct key_vector *l;

        kv = kmem_cache_alloc(trie_leaf_kmem, GFP_KERNEL);

if (!kv)
return NULL;

re,
wh

Quoted 5 lines

net/ipv4/fib_trie.c +1 -0

 	/* initialize key vector */
+	l = kv->kv;
 	l->key = key;
 	l->pos = 0;
 	l->bits = 0;

Firo Yang

author

2015-06-07 04:52 UTC #12 lore raw

Quoted 34 lines · walter harms

Am 06.06.2015 13:35, schrieb Firo Yang:
Quoted 15 lines
A smatch warning.
When kmem_cache_alloc() failed to alloc memory, a null pointer will be returned. Redeference null pointer will generate an unnecessary oops. So, use it after check.
Signed-off-by Firo Yang <[email protected]>
---
 net/ipv4/fib_trie.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
net/ipv4/fib_trie.c +0 -0
index 01bce15..34094c7 100644
net/ipv4/fib_trie.c static inline void empty_child_dec(struct key_vector *n) · line 329
Quoted 5 lines
net/ipv4/fib_trie.c +1 -1
@@ -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;
It is a good custom to have action and check close together, so this may be more obvious for future readers:
	struct tnode *kv;
	struct key_vector *l;

       kv = kmem_cache_alloc(trie_leaf_kmem, GFP_KERNEL);
net/ipv4/fib_trie.c +0 -0
 	if (!kv)
 		return NULL;

Quoted 2 lines

re,
wh

Thanks walter harms, I will update the patch like this.

Quoted 6 lines

net/ipv4/fib_trie.c +1 -0

 	/* initialize key vector */
+	l = kv->kv;
 	l->key = key;
 	l->pos = 0;
 	l->bits = 0;