| Message ID | 19020.39270.408816.360526@ipc1.ka-ro |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Lothar Waßmann <LW@KARO-electronics.de> Date: Thu, 2 Jul 2009 13:26:30 +0200 > Hi, > > while developing a canbus driver (with kernel 2.6.30-rc4) I > encountered a use-after-free bug that led to the following crash (due > to CONFIG_DEBUG_SLAB being enabled): ... > With the following patch I could alleviate the problem and did not > find any negative side effects, but I'm not sure, whether this is the > Right Thing(TM), since I'm not too familiar with the networking code: ... > Any comments on this? A patch like this shouldn't be needed. Can one of the CAN folks look into this? -- 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
Hi, David Miller writes: > From: Lothar Waßmann <LW@KARO-electronics.de> > Date: Thu, 2 Jul 2009 13:26:30 +0200 > > > Hi, > > > > while developing a canbus driver (with kernel 2.6.30-rc4) I > > encountered a use-after-free bug that led to the following crash (due > > to CONFIG_DEBUG_SLAB being enabled): > ... > > With the following patch I could alleviate the problem and did not > > find any negative side effects, but I'm not sure, whether this is the > > Right Thing(TM), since I'm not too familiar with the networking code: > ... > > Any comments on this? > > A patch like this shouldn't be needed. > Could you explain why it shouldn't be needed? To me it seems much more logical to invalidate any references from some object 'B' (struct sock) to some object 'A' (struct socket) when object 'A' is being released rather than invalidating them when object 'B' is being released. As far as I understand the code the 'struct socket' can vanish any time after sock_release() has been called. Thus the pointers in the 'struct sock' that point to the 'struct socket' should be invalidated at that point and not when the 'struct sock' itself is being released. Also, the messages I had added showed that sock_release() is being called before sk_common_release() (from standard networking code that has nothing to do with my can driver) leaving the 'struct sock' object with dangling 'sk_sleep' and 'sk_socket' pointers for the time between those two function calls. And I don't see anything preventing those pointers being dereferenced during this time. Lothar Waßmann
David Miller wrote: > From: Lothar Waßmann <LW@KARO-electronics.de> > Date: Thu, 2 Jul 2009 13:26:30 +0200 > >> Hi, >> >> while developing a canbus driver (with kernel 2.6.30-rc4) I >> encountered a use-after-free bug that led to the following crash (due >> to CONFIG_DEBUG_SLAB being enabled): > ... >> With the following patch I could alleviate the problem and did not >> find any negative side effects, but I'm not sure, whether this is the >> Right Thing(TM), since I'm not too familiar with the networking code: > ... >> Any comments on this? > > A patch like this shouldn't be needed. > > Can one of the CAN folks look into this? Hi Dave, i did - but i had no concerns that Lothars remark was an appropriate request. I'm not the socket layer expert but IMO this looks like something to be fixed in standard networking code. The only thing we do in out private sk->sk_destruct function is: skb_queue_purge(&sk->sk_receive_queue); As Urs is currently out of office, i added his private mail address ... Regards, Oliver -- 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: Lothar Waßmann <LW@KARO-electronics.de> Date: Tue, 7 Jul 2009 08:59:59 +0200 > To me it seems much more logical to invalidate any references from > some object 'B' (struct sock) to some object 'A' (struct socket) when > object 'A' is being released rather than invalidating them when object > 'B' is being released. Because B should hold a reference to A in that situation. If one object refers to another, it should grab and hold a reference to that object. -- 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
Hi, David Miller writes: > From: Lothar Waßmann <LW@KARO-electronics.de> > Date: Tue, 7 Jul 2009 08:59:59 +0200 > > > To me it seems much more logical to invalidate any references from > > some object 'B' (struct sock) to some object 'A' (struct socket) when > > object 'A' is being released rather than invalidating them when object > > 'B' is being released. > > Because B should hold a reference to A in that situation. > > If one object refers to another, it should grab and hold a reference > to that object. > 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? In sock_init_data() where the pointers in question are being initialized I don't see anything like that: | sk_set_socket(sk, sock); | | sock_set_flag(sk, SOCK_ZAPPED); | | if (sock) { | sk->sk_type = sock->type; | sk->sk_sleep = &sock->wait; | sock->sk = sk; | } else | sk->sk_sleep = NULL; with sk_set_socket() being: |static inline void sk_set_socket(struct sock *sk, struct socket *sock) |{ | sk->sk_socket = sock; |} Lothar Waßmann
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. Cheers,
--- net/socket.c 1 Jul 2009 09:30:42 -0000 +++ net/socket.c 2 Jul 2009 10:25:27 -0000 @@ -524,7 +524,9 @@ void sock_release(struct socket *sock) if (sock->ops) { struct module *owner = sock->ops->owner; + if (sock->sk) + sock_orphan(sock->sk); sock->ops->release(sock); sock->ops = NULL; module_put(owner); a/net/core/sock.c b/net/core/sock.c --- net/core/sock.c 2 Jun 2009 15:37:50 -0000 +++ net/core/sock.c 2 Jul 2009 10:42:33 -0000 @@ -1982,7 +1983,9 @@ void sk_common_release(struct sock *sk) * until the last reference will be released. */ - sock_orphan(sk); + //sock_orphan(sk); + if (WARN_ON(sk->sk_sleep || sk->sk_socket)) + sock_orphan(sk); xfrm_sk_free_policy(sk);