From patchwork Tue Nov 10 15:23:15 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Hannes Frederic Sowa X-Patchwork-Id: 542476 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id A1706141408 for ; Wed, 11 Nov 2015 02:23:29 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=stressinduktion.org header.i=@stressinduktion.org header.b=QeOyA/IA; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=messagingengine.com header.i=@messagingengine.com header.b=Zk0/uIc2; dkim-atps=neutral Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752815AbbKJPXZ (ORCPT ); Tue, 10 Nov 2015 10:23:25 -0500 Received: from out4-smtp.messagingengine.com ([66.111.4.28]:33627 "EHLO out4-smtp.messagingengine.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752707AbbKJPXY (ORCPT ); Tue, 10 Nov 2015 10:23:24 -0500 Received: from compute3.internal (compute3.nyi.internal [10.202.2.43]) by mailout.nyi.internal (Postfix) with ESMTP id B132620FC0 for ; Tue, 10 Nov 2015 10:23:23 -0500 (EST) Received: from frontend1 ([10.202.2.160]) by compute3.internal (MEProxy); Tue, 10 Nov 2015 10:23:23 -0500 DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d= stressinduktion.org; h=cc:date:from:message-id:subject:to :x-sasl-enc:x-sasl-enc; s=mesmtp; bh=auVFn8SlHeKh5aQszO9bAMVpHwc =; b=QeOyA/IA1Rh3MfJ1HH9kXz1JPket/nNxN2JshE/fkRt886IdUPSkFd6j4ca dDxN365iVyhRc3RY9+CsyHigG1eshkV0dS1SN764OTylw/IFPtDkI6MyEUDXcKmc vi3dDXInyp0O44zoDOKq2c5n5UX/z2zkjR9t/NvKBNCRvgac= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d= messagingengine.com; h=cc:date:from:message-id:subject:to :x-sasl-enc:x-sasl-enc; s=smtpout; bh=auVFn8SlHeKh5aQszO9bAMVpHw c=; b=Zk0/uIc2zY/Bsk0L/KTGLRX5Z33zVCLAkA6J6L3G7WD00GywXggR6BxQUI /4aG3tcvOMeasnPHhPvHvRBXqQHESPvXd3F9/QTvYWdfaTXxTWeSaOqoZ4iwCCuA zaYN7k2GyYJ4loyULqKbD8yDUzAyPMyludjJnKHyBvf/HtSLA= X-Sasl-enc: U/o2bRfAI6em5BEvt0+p6hfOvWxjbPuDA/sHyZuEIIUf 1447169003 Received: from z.com (unknown [217.192.177.51]) by mail.messagingengine.com (Postfix) with ESMTPA id CC4FEC016DA; Tue, 10 Nov 2015 10:23:22 -0500 (EST) From: Hannes Frederic Sowa To: netdev@vger.kernel.org Cc: Hannes Frederic Sowa , Dmitry Vyukov , Eric Dumazet Subject: [PATCH net v3] af-unix: fix use-after-free with concurrent readers while splicing Date: Tue, 10 Nov 2015 16:23:15 +0100 Message-Id: <1447168995-25820-1-git-send-email-hannes@stressinduktion.org> X-Mailer: git-send-email 2.5.0 Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org 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 Cc: Dmitry Vyukov Cc: Eric Dumazet Acked-by: Eric Dumazet Signed-off-by: Hannes Frederic Sowa --- 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(+) 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;