| Message ID | 1447168995-25820-1-git-send-email-hannes@stressinduktion.org |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On Tue, 2015-11-10 at 16:23 +0100, Hannes Frederic Sowa wrote: > During splicing an af-unix socket to a pipe we have to drop all > af-unix socket locks. While doing so we allow another reader to enter > unix_stream_read_generic which can read, copy and finally free another > skb. If exactly this skb is just in process of being spliced we get a > use-after-free report by kasan. > > First, we must make sure to not have a free while the skb is used during > the splice operation. We simply increment its use counter before unlocking > the reader lock. > > Stream sockets have the nice characteristic that we don't care about > zero length writes and they never reach the peer socket's queue. That > said, we can take the UNIXCB.consumed field as the indicator if the > skb was already freed from the socket's receive queue. If the skb was > fully consumed after we locked the reader side again we know it has been > dropped by a second reader. We indicate a short read to user space and > abort the current splice operation. > > This bug has been found with syzkaller > (http://github.com/google/syzkaller) by Dmitry Vyukov. > > Fixes: 2b514574f7e8 ("net: af_unix: implement splice for stream af_unix sockets") > Reported-by: Dmitry Vyukov <dvyukov@google.com> > Cc: Dmitry Vyukov <dvyukov@google.com> > Cc: Eric Dumazet <eric.dumazet@gmail.com> > Acked-by: Eric Dumazet <edumazet@google.com> > Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org> > --- > v2: add missing consume_skb in error path of recv_actor > v3: move skb_get to separate line as proposed by Eric Dumazet (thanks!) > I believe there is another bug in unix_stream_sendpage() skb = skb_peek_tail(&other->sk_receive_queue); if (tail && tail == skb) { Is clearly not safe ? -- 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
On Wed, Nov 11, 2015, at 19:58, Eric Dumazet wrote: > On Tue, 2015-11-10 at 16:23 +0100, Hannes Frederic Sowa wrote: > > During splicing an af-unix socket to a pipe we have to drop all > > af-unix socket locks. While doing so we allow another reader to enter > > unix_stream_read_generic which can read, copy and finally free another > > skb. If exactly this skb is just in process of being spliced we get a > > use-after-free report by kasan. > > > > First, we must make sure to not have a free while the skb is used during > > the splice operation. We simply increment its use counter before unlocking > > the reader lock. > > > > Stream sockets have the nice characteristic that we don't care about > > zero length writes and they never reach the peer socket's queue. That > > said, we can take the UNIXCB.consumed field as the indicator if the > > skb was already freed from the socket's receive queue. If the skb was > > fully consumed after we locked the reader side again we know it has been > > dropped by a second reader. We indicate a short read to user space and > > abort the current splice operation. > > > > This bug has been found with syzkaller > > (http://github.com/google/syzkaller) by Dmitry Vyukov. > > > > Fixes: 2b514574f7e8 ("net: af_unix: implement splice for stream af_unix sockets") > > Reported-by: Dmitry Vyukov <dvyukov@google.com> > > Cc: Dmitry Vyukov <dvyukov@google.com> > > Cc: Eric Dumazet <eric.dumazet@gmail.com> > > Acked-by: Eric Dumazet <edumazet@google.com> > > Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org> > > --- > > v2: add missing consume_skb in error path of recv_actor > > v3: move skb_get to separate line as proposed by Eric Dumazet (thanks!) > > > > I believe there is another bug in unix_stream_sendpage() > > skb = skb_peek_tail(&other->sk_receive_queue); > if (tail && tail == skb) { > > Is clearly not safe ? Can you elaborate? I use tail as a cookie and check if we already tried to append to the same tail skb with skb_append_pagefrags. If during allocation, which we do outside of the locks, a new skb arrives, we take that and try to append again (and free the old skb), to correctly not create any reordering in the data stream. You think that tail could be reused in the meanwhile? Bye, Hannes -- 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
On Wed, Nov 11, 2015, at 20:14, Hannes Frederic Sowa wrote: > On Wed, Nov 11, 2015, at 19:58, Eric Dumazet wrote: > > On Tue, 2015-11-10 at 16:23 +0100, Hannes Frederic Sowa wrote: > > > During splicing an af-unix socket to a pipe we have to drop all > > > af-unix socket locks. While doing so we allow another reader to enter > > > unix_stream_read_generic which can read, copy and finally free another > > > skb. If exactly this skb is just in process of being spliced we get a > > > use-after-free report by kasan. > > > > > > First, we must make sure to not have a free while the skb is used during > > > the splice operation. We simply increment its use counter before unlocking > > > the reader lock. > > > > > > Stream sockets have the nice characteristic that we don't care about > > > zero length writes and they never reach the peer socket's queue. That > > > said, we can take the UNIXCB.consumed field as the indicator if the > > > skb was already freed from the socket's receive queue. If the skb was > > > fully consumed after we locked the reader side again we know it has been > > > dropped by a second reader. We indicate a short read to user space and > > > abort the current splice operation. > > > > > > This bug has been found with syzkaller > > > (http://github.com/google/syzkaller) by Dmitry Vyukov. > > > > > > Fixes: 2b514574f7e8 ("net: af_unix: implement splice for stream af_unix sockets") > > > Reported-by: Dmitry Vyukov <dvyukov@google.com> > > > Cc: Dmitry Vyukov <dvyukov@google.com> > > > Cc: Eric Dumazet <eric.dumazet@gmail.com> > > > Acked-by: Eric Dumazet <edumazet@google.com> > > > Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org> > > > --- > > > v2: add missing consume_skb in error path of recv_actor > > > v3: move skb_get to separate line as proposed by Eric Dumazet (thanks!) > > > > > > > I believe there is another bug in unix_stream_sendpage() > > > > skb = skb_peek_tail(&other->sk_receive_queue); > > if (tail && tail == skb) { > > > > Is clearly not safe ? > > Can you elaborate? > > I use tail as a cookie and check if we already tried to append to the > same tail skb with skb_append_pagefrags. If during allocation, which we > do outside of the locks, a new skb arrives, we take that and try to > append again (and free the old skb), to correctly not create any > reordering in the data stream. > > You think that tail could be reused in the meanwhile? Also note, I never use the content of the tail pointer and we don't trim the skb from the end when it is still alive in the socket queue. Bye, Hannes -- 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
On Wed, 2015-11-11 at 20:14 +0100, Hannes Frederic Sowa wrote: > On Wed, Nov 11, 2015, at 19:58, Eric Dumazet wrote: > Can you elaborate? > > I use tail as a cookie and check if we already tried to append to the > same tail skb with skb_append_pagefrags. If during allocation, which we > do outside of the locks, a new skb arrives, we take that and try to > append again (and free the old skb), to correctly not create any > reordering in the data stream. > > You think that tail could be reused in the meanwhile? Hmmm, there is some funky stuff at least. Are you sure the __skb_queue_tail(&other->sk_receive_queue, newskb) is appropriate ? (Why not locking sk_receive_queue is safe ?) -- 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
On Wed, Nov 11, 2015, at 20:28, Eric Dumazet wrote: > On Wed, 2015-11-11 at 20:14 +0100, Hannes Frederic Sowa wrote: > > On Wed, Nov 11, 2015, at 19:58, Eric Dumazet wrote: > > > Can you elaborate? > > > > I use tail as a cookie and check if we already tried to append to the > > same tail skb with skb_append_pagefrags. If during allocation, which we > > do outside of the locks, a new skb arrives, we take that and try to > > append again (and free the old skb), to correctly not create any > > reordering in the data stream. > > > > You think that tail could be reused in the meanwhile? > > Hmmm, there is some funky stuff at least. > > Are you sure the __skb_queue_tail(&other->sk_receive_queue, newskb) > is appropriate ? > > (Why not locking sk_receive_queue is safe ?) We hold the other's state lock at that time. But I see another problem in unix_stream_read_generic: /* Mark read part of skb as used */ if (!(flags & MSG_PEEK)) { UNIXCB(skb).consumed += chunk; sk_peek_offset_bwd(sk, chunk); if (UNIXCB(skb).fp) unix_detach_fds(&scm, skb); if (unix_skb_len(skb)) break; skb_unlink(skb, &sk->sk_receive_queue); consume_skb(skb); if (scm.fp) break; The skb_unlink happens solely on the list lock and not on the state lock of the sk, so it is not synchronized to the other locks. Bye, Hannes -- 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
On Wed, 2015-11-11 at 20:35 +0100, Hannes Frederic Sowa wrote: > > On Wed, Nov 11, 2015, at 20:28, Eric Dumazet wrote: > > On Wed, 2015-11-11 at 20:14 +0100, Hannes Frederic Sowa wrote: > > > On Wed, Nov 11, 2015, at 19:58, Eric Dumazet wrote: > > > > > Can you elaborate? > > > > > > I use tail as a cookie and check if we already tried to append to the > > > same tail skb with skb_append_pagefrags. If during allocation, which we > > > do outside of the locks, a new skb arrives, we take that and try to > > > append again (and free the old skb), to correctly not create any > > > reordering in the data stream. > > > > > > You think that tail could be reused in the meanwhile? > > > > Hmmm, there is some funky stuff at least. > > > > Are you sure the __skb_queue_tail(&other->sk_receive_queue, newskb) > > is appropriate ? > > > > (Why not locking sk_receive_queue is safe ?) > > We hold the other's state lock at that time. Well, this is not safe enough :( Look at unix_stream_sendmsg() : It uses skb_queue_tail(), not __skb_queue_tail() Think of concurrent splice() (or sendfile()) and sendmsg() on the same af_unix socket. -- 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
On Wed, Nov 11, 2015, at 20:42, Eric Dumazet wrote: > On Wed, 2015-11-11 at 20:35 +0100, Hannes Frederic Sowa wrote: > > > > On Wed, Nov 11, 2015, at 20:28, Eric Dumazet wrote: > > > On Wed, 2015-11-11 at 20:14 +0100, Hannes Frederic Sowa wrote: > > > > On Wed, Nov 11, 2015, at 19:58, Eric Dumazet wrote: > > > > > > > Can you elaborate? > > > > > > > > I use tail as a cookie and check if we already tried to append to the > > > > same tail skb with skb_append_pagefrags. If during allocation, which we > > > > do outside of the locks, a new skb arrives, we take that and try to > > > > append again (and free the old skb), to correctly not create any > > > > reordering in the data stream. > > > > > > > > You think that tail could be reused in the meanwhile? > > > > > > Hmmm, there is some funky stuff at least. > > > > > > Are you sure the __skb_queue_tail(&other->sk_receive_queue, newskb) > > > is appropriate ? > > > > > > (Why not locking sk_receive_queue is safe ?) > > > > We hold the other's state lock at that time. > > Well, this is not safe enough :( > > Look at unix_stream_sendmsg() : It uses skb_queue_tail(), not > __skb_queue_tail() > > Think of concurrent splice() (or sendfile()) and sendmsg() on the same > af_unix socket. Well, unix_stream_sendmsg: unix_state_lock(other); skb_queue_tail(&other->sk_receive_queue); unix_state_unlock(other); unix_stream_sendpage: unix_state_lock(other); __skb_queue_tail(&other->sk_receive_queue, skb); unix_state_unlock(other); unix_stream_read_generic: I only see the skb_unlink as a dangerous operation because outside of other lock and solely taking the sk_receivie_queue lock. Actually I think skb_queue_tail can be converted to __skb_queue_tail. But then the logic in unix_stream_connect() does not match, but it also holds the other's socket lock durign sk_receive_queue modification. Hmm, something is wrong here, that is clear. :/ Bye, Hannes -- 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
On Wed, 2015-11-11 at 20:58 +0100, Hannes Frederic Sowa wrote: > > On Wed, Nov 11, 2015, at 20:42, Eric Dumazet wrote: > > On Wed, 2015-11-11 at 20:35 +0100, Hannes Frederic Sowa wrote: > > > > > > On Wed, Nov 11, 2015, at 20:28, Eric Dumazet wrote: > > > > On Wed, 2015-11-11 at 20:14 +0100, Hannes Frederic Sowa wrote: > > > > > On Wed, Nov 11, 2015, at 19:58, Eric Dumazet wrote: > > > > > > > > > Can you elaborate? > > > > > > > > > > I use tail as a cookie and check if we already tried to append to the > > > > > same tail skb with skb_append_pagefrags. If during allocation, which we > > > > > do outside of the locks, a new skb arrives, we take that and try to > > > > > append again (and free the old skb), to correctly not create any > > > > > reordering in the data stream. > > > > > > > > > > You think that tail could be reused in the meanwhile? > > > > > > > > Hmmm, there is some funky stuff at least. > > > > > > > > Are you sure the __skb_queue_tail(&other->sk_receive_queue, newskb) > > > > is appropriate ? > > > > > > > > (Why not locking sk_receive_queue is safe ?) > > > > > > We hold the other's state lock at that time. > > > > Well, this is not safe enough :( > > > > Look at unix_stream_sendmsg() : It uses skb_queue_tail(), not > > __skb_queue_tail() > > > > Think of concurrent splice() (or sendfile()) and sendmsg() on the same > > af_unix socket. > > Well, > > unix_stream_sendmsg: > > unix_state_lock(other); > skb_queue_tail(&other->sk_receive_queue); > unix_state_unlock(other); > > unix_stream_sendpage: > > unix_state_lock(other); > __skb_queue_tail(&other->sk_receive_queue, skb); > unix_state_unlock(other); > > unix_stream_read_generic: > > I only see the skb_unlink as a dangerous operation because outside of > other lock and solely taking the sk_receivie_queue lock. Actually I > think skb_queue_tail can be converted to __skb_queue_tail. Nope. See unix_inq_len() you definitely need to take the list lock, or make a full audit. -- 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
On Wed, Nov 11, 2015, at 21:09, Eric Dumazet wrote: > On Wed, 2015-11-11 at 20:58 +0100, Hannes Frederic Sowa wrote: > > > > On Wed, Nov 11, 2015, at 20:42, Eric Dumazet wrote: > > > On Wed, 2015-11-11 at 20:35 +0100, Hannes Frederic Sowa wrote: > > > > > > > > On Wed, Nov 11, 2015, at 20:28, Eric Dumazet wrote: > > > > > On Wed, 2015-11-11 at 20:14 +0100, Hannes Frederic Sowa wrote: > > > > > > On Wed, Nov 11, 2015, at 19:58, Eric Dumazet wrote: > > > > > > > > > > > Can you elaborate? > > > > > > > > > > > > I use tail as a cookie and check if we already tried to append to the > > > > > > same tail skb with skb_append_pagefrags. If during allocation, which we > > > > > > do outside of the locks, a new skb arrives, we take that and try to > > > > > > append again (and free the old skb), to correctly not create any > > > > > > reordering in the data stream. > > > > > > > > > > > > You think that tail could be reused in the meanwhile? > > > > > > > > > > Hmmm, there is some funky stuff at least. > > > > > > > > > > Are you sure the __skb_queue_tail(&other->sk_receive_queue, newskb) > > > > > is appropriate ? > > > > > > > > > > (Why not locking sk_receive_queue is safe ?) > > > > > > > > We hold the other's state lock at that time. > > > > > > Well, this is not safe enough :( > > > > > > Look at unix_stream_sendmsg() : It uses skb_queue_tail(), not > > > __skb_queue_tail() > > > > > > Think of concurrent splice() (or sendfile()) and sendmsg() on the same > > > af_unix socket. > > > > Well, > > > > unix_stream_sendmsg: > > > > unix_state_lock(other); > > skb_queue_tail(&other->sk_receive_queue); > > unix_state_unlock(other); > > > > unix_stream_sendpage: > > > > unix_state_lock(other); > > __skb_queue_tail(&other->sk_receive_queue, skb); > > unix_state_unlock(other); > > > > unix_stream_read_generic: > > > > I only see the skb_unlink as a dangerous operation because outside of > > other lock and solely taking the sk_receivie_queue lock. Actually I > > think skb_queue_tail can be converted to __skb_queue_tail. > > Nope. See unix_inq_len() you definitely need to take the list lock, > or make a full audit. Found it right after sending the last mail. :) I do a full audit of all sk_receive_queue accesses. Bye, Hannes -- 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: Hannes Frederic Sowa <hannes@stressinduktion.org> Date: Tue, 10 Nov 2015 16:23:15 +0100 > During splicing an af-unix socket to a pipe we have to drop all > af-unix socket locks. While doing so we allow another reader to enter > unix_stream_read_generic which can read, copy and finally free another > skb. If exactly this skb is just in process of being spliced we get a > use-after-free report by kasan. > > First, we must make sure to not have a free while the skb is used during > the splice operation. We simply increment its use counter before unlocking > the reader lock. > > Stream sockets have the nice characteristic that we don't care about > zero length writes and they never reach the peer socket's queue. That > said, we can take the UNIXCB.consumed field as the indicator if the > skb was already freed from the socket's receive queue. If the skb was > fully consumed after we locked the reader side again we know it has been > dropped by a second reader. We indicate a short read to user space and > abort the current splice operation. > > This bug has been found with syzkaller > (http://github.com/google/syzkaller) by Dmitry Vyukov. > > Fixes: 2b514574f7e8 ("net: af_unix: implement splice for stream af_unix sockets") > Reported-by: Dmitry Vyukov <dvyukov@google.com> > Cc: Dmitry Vyukov <dvyukov@google.com> > Cc: Eric Dumazet <eric.dumazet@gmail.com> > Acked-by: Eric Dumazet <edumazet@google.com> > Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org> Applied and queued up for -stable, 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
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c index aaa0b58..12b886f 100644 --- a/net/unix/af_unix.c +++ b/net/unix/af_unix.c @@ -441,6 +441,7 @@ static void unix_release_sock(struct sock *sk, int embrion) if (state == TCP_LISTEN) unix_release_sock(skb->sk, 1); /* passed fds are erased in the kfree_skb hook */ + UNIXCB(skb).consumed = skb->len; kfree_skb(skb); } @@ -2072,6 +2073,7 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state) do { int chunk; + bool drop_skb; struct sk_buff *skb, *last; unix_state_lock(sk); @@ -2152,7 +2154,11 @@ unlock: } chunk = min_t(unsigned int, unix_skb_len(skb) - skip, size); + skb_get(skb); chunk = state->recv_actor(skb, skip, chunk, state); + drop_skb = !unix_skb_len(skb); + /* skb is only safe to use if !drop_skb */ + consume_skb(skb); if (chunk < 0) { if (copied == 0) copied = -EFAULT; @@ -2161,6 +2167,18 @@ unlock: copied += chunk; size -= chunk; + if (drop_skb) { + /* the skb was touched by a concurrent reader; + * we should not expect anything from this skb + * anymore and assume it invalid - we can be + * sure it was dropped from the socket queue + * + * let's report a short read + */ + err = 0; + break; + } + /* Mark read part of skb as used */ if (!(flags & MSG_PEEK)) { UNIXCB(skb).consumed += chunk;
During splicing an af-unix socket to a pipe we have to drop all af-unix socket locks. While doing so we allow another reader to enter unix_stream_read_generic which can read, copy and finally free another skb. If exactly this skb is just in process of being spliced we get a use-after-free report by kasan. First, we must make sure to not have a free while the skb is used during the splice operation. We simply increment its use counter before unlocking the reader lock. Stream sockets have the nice characteristic that we don't care about zero length writes and they never reach the peer socket's queue. That said, we can take the UNIXCB.consumed field as the indicator if the skb was already freed from the socket's receive queue. If the skb was fully consumed after we locked the reader side again we know it has been dropped by a second reader. We indicate a short read to user space and abort the current splice operation. This bug has been found with syzkaller (http://github.com/google/syzkaller) by Dmitry Vyukov. Fixes: 2b514574f7e8 ("net: af_unix: implement splice for stream af_unix sockets") Reported-by: Dmitry Vyukov <dvyukov@google.com> Cc: Dmitry Vyukov <dvyukov@google.com> Cc: Eric Dumazet <eric.dumazet@gmail.com> Acked-by: Eric Dumazet <edumazet@google.com> Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org> --- v2: add missing consume_skb in error path of recv_actor v3: move skb_get to separate line as proposed by Eric Dumazet (thanks!) net/unix/af_unix.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)