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

login
register
mail settings
Submitter Amerigo Wang
Date Oct. 29, 2012, 3:50 a.m.
Message ID <1351482620-11008-1-git-send-email-amwang@redhat.com>
Download mbox | patch
Permalink /patch/194788/
State Rejected
Delegated to: David Miller
Headers show

Comments

Amerigo Wang - Oct. 29, 2012, 3:50 a.m.
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>

---
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Dumazet - Oct. 29, 2012, 6:10 a.m.
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.




--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Amerigo Wang - Oct. 29, 2012, 6:49 a.m.
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.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Dumazet - Oct. 29, 2012, 7:05 a.m.
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




--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Amerigo Wang - Oct. 29, 2012, 7:25 a.m.
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?


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Dumazet - Oct. 29, 2012, 8:25 a.m.
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()



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Oct. 29, 2012, 5:22 p.m.
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?
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Oct. 29, 2012, 5:29 p.m.
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.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Amerigo Wang - Oct. 30, 2012, 1:44 a.m.
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

:)

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

Patch

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