[Patch net-next] ipv6: fix a potential NULL deref

In ipv6_del_addr():

                if (rt != net->ipv6.ip6_null_entry &&
                    addrconf_is_prefix_route(rt)) {
                        if (onlink == 0) {
                                ip6_del_rt(rt);
                                rt = NULL;
                        } else if (!(rt->rt6i_flags & RTF_EXPIRES)) {
                                rt6_set_expires(rt, expires);
                        }
                }
                dst_release(&rt->dst);

obviously rt could be NULL'd before dst_release(), so
we have to check if rt is NULL before calling it.

Reported-by: Fengguang Wu <redacted>
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Cong Wang <redacted>

---

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 8f0b12a..c467dbb 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c

@@ -951,7 +951,8 @@ static void ipv6_del_addr(struct inet6_ifaddr *ifp)
 				rt6_set_expires(rt, expires);
 			}
 		}
-		dst_release(&rt->dst);
+		if (rt)
+			dst_release(&rt->dst);
 	}
 
 	/* clean up prefsrc entries */

On Mon, 2012-10-29 at 11:50 +0800, Cong Wang wrote:

dst_release() is like kfree(), it accepts a NULL argument.

On Mon, 2012-10-29 at 07:10 +0100, Eric Dumazet wrote:

quoted

-		dst_release(&rt->dst);
+		if (rt)
+			dst_release(&rt->dst);
 	}
 

dst_release() is like kfree(), it accepts a NULL argument.

'rt->dst' already dereferences 'rt', no matter dst_release() accepts
NULL or not.

On Mon, 2012-10-29 at 14:49 +0800, Cong Wang wrote:

On Mon, 2012-10-29 at 07:10 +0100, Eric Dumazet wrote:

quoted

quoted

-		dst_release(&rt->dst);
+		if (rt)
+			dst_release(&rt->dst);
 	}
 

dst_release() is like kfree(), it accepts a NULL argument.

'rt->dst' already dereferences 'rt', no matter dst_release() accepts
NULL or not.

&rt->dst doesnt dereference rt, you are quite mistaken.

if rt is NULL, &rt->dst is also NULL

On Mon, 2012-10-29 at 08:05 +0100, Eric Dumazet wrote:

On Mon, 2012-10-29 at 14:49 +0800, Cong Wang wrote:

quoted

On Mon, 2012-10-29 at 07:10 +0100, Eric Dumazet wrote:

quoted

quoted

-		dst_release(&rt->dst);
+		if (rt)
+			dst_release(&rt->dst);
 	}
 

dst_release() is like kfree(), it accepts a NULL argument.

'rt->dst' already dereferences 'rt', no matter dst_release() accepts
NULL or not.

&rt->dst doesnt dereference rt, you are quite mistaken.

if rt is NULL, &rt->dst is also NULL

Oh, yeah, gcc should be smart enough to do calculation without deref it
given it has the offset and the address. And dst happens to be first
field of rt, so offset is 0, &rt->dst should be NULL too if rt is NULL.

But this will be a problem if someone moved dst inside rt, as there is
no comment saying dst has to be the first one?

When 'rt' is NULL, '&rt->dst' is NULL too because >dst
is always the first field of 'rt'. And dst_release accepts
NULL.

Cc: Eric Dumazet <redacted>
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Cong Wang <redacted>

---

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 8f0b12a..adeb479 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c

@@ -2027,8 +2027,7 @@ void addrconf_prefix_rcv(struct net_device *dev, u8 *opt, int len, bool sllao)
 			addrconf_prefix_route(&pinfo->prefix, pinfo->prefix_len,
 					      dev, expires, flags);
 		}
-		if (rt)
-			dst_release(&rt->dst);
+		dst_release(&rt->dst);
 	}
 
 	/* Try to figure out our local address for this prefix */

On Mon, 2012-10-29 at 15:25 +0800, Cong Wang wrote:

Oh, yeah, gcc should be smart enough to do calculation without deref it
given it has the offset and the address. And dst happens to be first
field of rt, so offset is 0, &rt->dst should be NULL too if rt is NULL.

There is no dereference, even if gcc was dumb, since dst is an embedded
struct, not a pointer to a struct.

This wont change in a near future.

But this will be a problem if someone moved dst inside rt, as there is
no comment saying dst has to be the first one?

I dont think this placement will change in a near future, it would break
lot of things.

I guess we can use a BUILD_BUG_ON() instead of a comment that
could be ignored.

Note we could move dst in rtable, if we change the NULL test in
dst_release with some if ((unsigned long)dst < 4000) condition

I'll send a patch against ip_rt_put()

On Mon, 2012-10-29 at 16:16 +0800, Cong Wang wrote:

Could you instead introduce a helper for that, like we have in IPv4.

(ip_rt_put() -> ip6_rt_put() or something...)

See my followup patch for ipv4

On Mon, 2012-10-29 at 09:33 +0100, Eric Dumazet wrote:

On Mon, 2012-10-29 at 16:16 +0800, Cong Wang wrote:

quoted

When 'rt' is NULL, '&rt->dst' is NULL too because >dst
is always the first field of 'rt'. And dst_release accepts
NULL.

Cc: Eric Dumazet <redacted>
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Cong Wang <redacted>

---

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 8f0b12a..adeb479 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c

@@ -2027,8 +2027,7 @@ void addrconf_prefix_rcv(struct net_device *dev, u8 *opt, int len, bool sllao)
 			addrconf_prefix_route(&pinfo->prefix, pinfo->prefix_len,
 					      dev, expires, flags);
 		}
-		if (rt)
-			dst_release(&rt->dst);
+		dst_release(&rt->dst);
 	}
 
 	/* Try to figure out our local address for this prefix */

Could you instead introduce a helper for that, like we have in IPv4.

Sure, that is better.

From: Cong Wang <redacted>
Date: Mon, 29 Oct 2012 14:49:26 +0800

On Mon, 2012-10-29 at 07:10 +0100, Eric Dumazet wrote:

quoted

quoted

-		dst_release(&rt->dst);
+		if (rt)
+			dst_release(&rt->dst);
 	}
 

dst_release() is like kfree(), it accepts a NULL argument.

'rt->dst' already dereferences 'rt', no matter dst_release() accepts
NULL or not.

It's taking the address of a struct member, it's not a dereference.

You know what the difference is right?

From: Cong Wang <redacted>
Date: Mon, 29 Oct 2012 15:25:23 +0800

But this will be a problem if someone moved dst inside rt, as there
is no comment saying dst has to be the first one?

If we move the initial dst member, many things that depend upon it
being first will have to change.

Your change was unnecessary and inappropriate, and let's just leave
it at that.

Thanks.

On Mon, 2012-10-29 at 13:29 -0400, David Miller wrote:

From: Cong Wang <redacted>
Date: Mon, 29 Oct 2012 15:25:23 +0800

quoted

But this will be a problem if someone moved dst inside rt, as there
is no comment saying dst has to be the first one?

If we move the initial dst member, many things that depend upon it
being first will have to change.

Your change was unnecessary and inappropriate, and let's just leave
it at that.

Yeah, that is why I sent:
http://marc.info/?l=linux-netdev&m=135150561014916&w=2

:)