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

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

[Cong Wang]

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 <fengguang.wu@intel.com>
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Cong Wang <amwang@redhat.com>

---
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 */

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

[Eric Dumazet]

On Mon, 2012-10-29 at 11:50 +0800, Cong Wang wrote:
> 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 <fengguang.wu@intel.com>
> Cc: David S. Miller <davem@davemloft.net>
> Signed-off-by: Cong Wang <amwang@redhat.com>
> 
> ---
> 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);
>  	}
>  

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

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

[Cong Wang]

On Mon, 2012-10-29 at 07:10 +0100, Eric Dumazet wrote:
> > -		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.

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

[Eric Dumazet]

On Mon, 2012-10-29 at 14:49 +0800, Cong Wang wrote:
> On Mon, 2012-10-29 at 07:10 +0100, Eric Dumazet wrote:
> > > -		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

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

[Cong Wang]

On Mon, 2012-10-29 at 08:05 +0100, Eric Dumazet wrote:
> On Mon, 2012-10-29 at 14:49 +0800, Cong Wang wrote:
> > On Mon, 2012-10-29 at 07:10 +0100, Eric Dumazet wrote:
> > > > -		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?

[Patch net-next] ipv6: remove another useless NULL check

[Cong Wang]

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 <eric.dumazet@gmail.com>
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Cong Wang <amwang@redhat.com>

---
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 */

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

[Eric Dumazet]

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()

Re: [Patch net-next] ipv6: remove another useless NULL check

[Eric Dumazet]

On Mon, 2012-10-29 at 16:16 +0800, Cong Wang wrote:
> 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 <eric.dumazet@gmail.com>
> Cc: David S. Miller <davem@davemloft.net>
> Signed-off-by: Cong Wang <amwang@redhat.com>
> 
> ---
> 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.

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

See my followup patch for ipv4

Re: [Patch net-next] ipv6: remove another useless NULL check

[Cong Wang]

On Mon, 2012-10-29 at 09:33 +0100, Eric Dumazet wrote:
> On Mon, 2012-10-29 at 16:16 +0800, Cong Wang wrote:
> > 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 <eric.dumazet@gmail.com>
> > Cc: David S. Miller <davem@davemloft.net>
> > Signed-off-by: Cong Wang <amwang@redhat.com>
> > 
> > ---
> > 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.

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

[David Miller]

From: Cong Wang <amwang@redhat.com>
Date: Mon, 29 Oct 2012 14:49:26 +0800

> On Mon, 2012-10-29 at 07:10 +0100, Eric Dumazet wrote:
>> > -		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?

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

[David Miller]

From: Cong Wang <amwang@redhat.com>
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.

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

[Cong Wang]

On Mon, 2012-10-29 at 13:29 -0400, David Miller wrote:
> From: Cong Wang <amwang@redhat.com>
> 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.

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

:)