[PATCH] IPv6: Fix NULL dereference in ipv6_del_addr()

[PATCH] IPv6: Fix NULL dereference in ipv6_del_addr()

[Ville Nuorvala]

[-- Attachment #1: Type: text/plain, Size: 0 bytes --]



[-- Attachment #2: 0001-IPv6-Fix-NULL-dereference-in-ipv6_del_addr.txt --]
[-- Type: text/plain, Size: 709 bytes --]

>From 07ed0369cca6ef51013a63664b09ef402e79af9e Mon Sep 17 00:00:00 2001
From: Ville Nuorvala <vnuorval@tcs.hut.fi>
Date: Fri, 17 Nov 2006 14:05:45 +0200
Subject: [PATCH] IPv6: Fix NULL dereference in ipv6_del_addr()


Signed-off-by: Ville Nuorvala <vnuorval@tcs.hut.fi>
---
 net/ipv6/addrconf.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index b312a5f..5a88378 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -749,7 +749,8 @@ static void ipv6_del_addr(struct inet6_i
 				rt->rt6i_flags |= RTF_EXPIRES;
 			}
 		}
-		dst_release(&rt->u.dst);
+		if (rt)
+			dst_release(&rt->u.dst);
 	}
 
 	in6_ifa_put(ifp);
-- 
1.4.3.5

Re: [PATCH] IPv6: Fix NULL dereference in ipv6_del_addr()

[YOSHIFUJI Hideaki / 吉藤英明]

In article <455DB884.3050203@tcs.hut.fi> (at Fri, 17 Nov 2006 15:26:28 +0200), Ville Nuorvala <vnuorval@tcs.hut.fi> says:


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

I disagree.  This does NOT fix any bugs.

(void *)&rt->u.dst is ever equal to (void*)rt, and
dst_release() checks if the argument is NULL.

--yoshfuji

Re: [PATCH] IPv6: Fix NULL dereference in ipv6_del_addr()

[Ville Nuorvala]

YOSHIFUJI Hideaki wrote:
> In article <455DB884.3050203@tcs.hut.fi> (at Fri, 17 Nov 2006 15:26:28 +0200), Ville Nuorvala <vnuorval@tcs.hut.fi> says:
> 
> 
>> -		dst_release(&rt->u.dst);
>> +		if (rt)
>> +			dst_release(&rt->u.dst);
>>  	}
> 
> I disagree.  This does NOT fix any bugs.
> 
> (void *)&rt->u.dst is ever equal to (void*)rt, and
> dst_release() checks if the argument is NULL.

As the check is unnecessary you probably want to clean up the other 
places where rt is checked before &rt->u.dst is passed, as well ;-)
This is done at least in addrconf.c, ndisc.c and route.c...

Seriously though, you are probably right about the pointer being equal 
to NULL in this case,  but does the C language actually guarantee that 
the pointer to the structure and its first element are equal, or is it 
implementation dependent? I don't have my K&R here, so I can't check.

Regards,
Ville

Re: [PATCH] IPv6: Fix NULL dereference in ipv6_del_addr()

[David Miller]

From: Ville Nuorvala <vnuorval@tcs.hut.fi>
Date: Fri, 17 Nov 2006 21:27:21 +0200

> YOSHIFUJI Hideaki wrote:
> > In article <455DB884.3050203@tcs.hut.fi> (at Fri, 17 Nov 2006 15:26:28 +0200), Ville Nuorvala <vnuorval@tcs.hut.fi> says:
> > 
> > 
> >> -		dst_release(&rt->u.dst);
> >> +		if (rt)
> >> +			dst_release(&rt->u.dst);
> >>  	}
> > 
> > I disagree.  This does NOT fix any bugs.
> > 
> > (void *)&rt->u.dst is ever equal to (void*)rt, and
> > dst_release() checks if the argument is NULL.
> 
> As the check is unnecessary you probably want to clean up the other 
> places where rt is checked before &rt->u.dst is passed, as well ;-)
> This is done at least in addrconf.c, ndisc.c and route.c...
> 
> Seriously though, you are probably right about the pointer being equal 
> to NULL in this case,  but does the C language actually guarantee that 
> the pointer to the structure and its first element are equal, or is it 
> implementation dependent? I don't have my K&R here, so I can't check.

I would imagine that it does.  We rely on similar struct layout
semantics in other areas of the networking.

Also, in the past I've been told by GCC folks that the only way to
guarentee that two objects appear together, one after another, in
the .data segment is to place them into a structure :)

I don't think, therefore, that this will ever break.