use after free bug in socket code

Hi,

Herbert Xu writes:
> Lothar Waßmann <LW@karo-electronics.de> wrote:
> >
> > So, could you point me to the place where the reference count of the
> > socket object is being incremented when a struct sock is associated
> > with it?
> 
> It's implicit.  Anyway, you should remodel your release function
> on a working protocol.
> 
OK. I checked the release functions of the can raw and bcm protocols
and found that they obviously are the culprits since they lack the
call to sock_orphan that other network protocol drivers have:



Could someone of the CAN-Folks comment on this?


Best regards,
Lothar Waßmann

Lothar Waßmann wrote:
> Herbert Xu writes:
>> Lothar Waßmann <LW@karo-electronics.de> wrote:
>>> So, could you point me to the place where the reference count of the
>>> socket object is being incremented when a struct sock is associated
>>> with it?
>> It's implicit.  Anyway, you should remodel your release function
>> on a working protocol.
>>
> OK. I checked the release functions of the can raw and bcm protocols
> and found that they obviously are the culprits since they lack the
> call to sock_orphan that other network protocol drivers have:
> 
> diff -ur linux-2.6.30/net/can/bcm.c linux-2.6.30-karo/net/can/bcm.c
> --- linux-2.6.30/net/can/bcm.c	2009-06-10 05:05:27.000000000 +0200
> +++ linux-2.6.30-karo/net/can/bcm.c	2009-07-12 20:12:38.000000000 +0200
> @@ -1469,6 +1469,9 @@
>  		bo->ifindex = 0;
>  	}
>  
> +	sock_orphan(sk);
> +	sock->sk = NULL;
> +
>  	release_sock(sk);
>  	sock_put(sk);
>  
> diff -ur linux-2.6.30/net/can/raw.c linux-2.6.30-karo/net/can/raw.c
> --- linux-2.6.30/net/can/raw.c	2009-06-10 05:05:27.000000000 +0200
> +++ linux-2.6.30-karo/net/can/raw.c	2009-07-12 20:12:29.000000000 +0200
> @@ -306,6 +306,9 @@
>  	ro->bound   = 0;
>  	ro->count   = 0;
>  
> +	sock_orphan(sk);
> +	sock->sk = NULL;
> +
>  	release_sock(sk);
>  	sock_put(sk);
>  
> 
> 
> Could someone of the CAN-Folks comment on this?

Hello Lothar,

unfortunately i did not get any answer from Urs so far, who originally created
these lines of code.

But from what i was able to get from browsing similar code in the Kernel that
at least sock_orphan() is called in the appropriate socket release functions,
which is indeed not done by the mentioned PF_CAN protocols right now.

I assume you already tested this patch (at least with CAN_RAW) successfully,
right?

If so, i would have no objections to add my Acked-by to these changes.

Would you like to prepare a proper patch and post it on netdev?

Thanks for digging that issue!

Best regards,
Oliver

ps. This code section was stable for more than three years now. Can you tell
me, how you kicked your system to run into this problem?


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

From: Oliver Hartkopp <oliver@hartkopp.net>
Date: Mon, 13 Jul 2009 19:46:51 +0200

> But from what i was able to get from browsing similar code in the Kernel that
> at least sock_orphan() is called in the appropriate socket release functions,
> which is indeed not done by the mentioned PF_CAN protocols right now.
> 
> I assume you already tested this patch (at least with CAN_RAW) successfully,
> right?
> 
> If so, i would have no objections to add my Acked-by to these changes.
> 
> Would you like to prepare a proper patch and post it on netdev?

Indeed, please do make such a formal submission once properly tested.

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