From patchwork Wed Sep 7 14:37:36 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Tim Chen X-Patchwork-Id: 113839 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 53526B6F84 for ; Thu, 8 Sep 2011 07:37:45 +1000 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757169Ab1IGVhj (ORCPT ); Wed, 7 Sep 2011 17:37:39 -0400 Received: from mga11.intel.com ([192.55.52.93]:26546 "EHLO mga11.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757138Ab1IGVhi (ORCPT ); Wed, 7 Sep 2011 17:37:38 -0400 Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga102.fm.intel.com with ESMTP; 07 Sep 2011 14:37:38 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="4.68,347,1312182000"; d="scan'208";a="49052540" Received: from unknown (HELO [10.255.14.207]) ([10.255.14.207]) by fmsmga001.fm.intel.com with ESMTP; 07 Sep 2011 14:37:37 -0700 Subject: Re: [PATCH -next v2] unix stream: Fix use-after-free crashes From: Tim Chen To: sedat.dilek@gmail.com Cc: Eric Dumazet , "Yan, Zheng" , "Yan, Zheng" , "netdev@vger.kernel.org" , "davem@davemloft.net" , "sfr@canb.auug.org.au" , "jirislaby@gmail.com" , "Shi, Alex" , Valdis Kletnieks In-Reply-To: References: <4E631032.6050606@intel.com> <1315326326.2576.2980.camel@schen9-DESK> <1315330805.2899.16.camel@edumazet-HP-Compaq-6005-Pro-SFF-PC> <1315335019.2576.3048.camel@schen9-DESK> <1315335660.3400.7.camel@edumazet-laptop> <1315337580.2576.3066.camel@schen9-DESK> <1315338186.3400.20.camel@edumazet-laptop> <1315339157.2576.3079.camel@schen9-DESK> <1315340388.3400.28.camel@edumazet-laptop> <1315372100.3400.76.camel@edumazet-laptop> <4E66FF38.9000107@intel.com> <1315381503.3400.85.camel@edumazet-laptop> <1315396903.2364.23.camel@schen9-mobl> Date: Wed, 07 Sep 2011 07:37:36 -0700 Message-ID: <1315406256.6287.7.camel@schen9-mobl> Mime-Version: 1.0 X-Mailer: Evolution 2.28.3 (2.28.3-1.fc12) Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Wed, 2011-09-07 at 22:30 +0200, Sedat Dilek wrote: > > > > Replaced v2 with this patch (against next-20110831), I see now some > > different call-traces which I did not see with v1 or v2. > > Can't say if it's related to the new patch or not. > > ( dmesg attached. ) > > > > - Sedat - > > > > Call-traces seem to go away when adding "irqpoll" to Kernel command line. > ( See dmesg_irqpoll.txt ) > > - Sedat - Sedat, The previous patch should use the new steal_refs to check for the release of scm references in the error handling at the end. I've updated the patch to take care of it. Hopefully the traces you see will go away. Can you verify? Thanks. Tim ---- Commit 0856a30409 (Scm: Remove unnecessary pid & credential references in Unix socket's send and receive path) introduced a use-after-free bug. The sent skbs from unix_stream_sendmsg could be consumed and destructed by the receive side, removing all referentials to the credentials, before the send side has finished sending out all packets. However, send side could continue to consturct new packets in the stream, using credentials that have lost its last reference and been freed. In this fix, we don't steal the reference to credentials we have obtained in scm_send at beginning of unix_stream_sendmsg, till we've reached the last packet. This fixes the problem in commit 0856a30409. Signed-off-by: Tim Chen Signed-off-by: Eric Dumazet Reported-by: Jiri Slaby Tested-by: Sedat Dilek Tested-by: Valdis Kletnieks --- -- 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 136298c..be712ae 100644 --- a/net/unix/af_unix.c +++ b/net/unix/af_unix.c @@ -1383,10 +1383,11 @@ static int unix_attach_fds(struct scm_cookie *scm, struct sk_buff *skb) } static int unix_scm_to_skb(struct scm_cookie *scm, struct sk_buff *skb, - bool send_fds, bool ref) + bool send_fds, bool steal_refs) { int err = 0; - if (ref) { + + if (!steal_refs) { UNIXCB(skb).pid = get_pid(scm->pid); UNIXCB(skb).cred = get_cred(scm->cred); } else { @@ -1458,7 +1459,7 @@ static int unix_dgram_sendmsg(struct kiocb *kiocb, struct socket *sock, if (skb == NULL) goto out; - err = unix_scm_to_skb(siocb->scm, skb, true, false); + err = unix_scm_to_skb(siocb->scm, skb, true, true); if (err < 0) goto out_free; max_level = err + 1; @@ -1581,6 +1582,7 @@ static int unix_stream_sendmsg(struct kiocb *kiocb, struct socket *sock, int sent = 0; struct scm_cookie tmp_scm; bool fds_sent = false; + bool steal_refs = false; int max_level; if (NULL == siocb->scm) @@ -1642,11 +1644,14 @@ static int unix_stream_sendmsg(struct kiocb *kiocb, struct socket *sock, size = min_t(int, size, skb_tailroom(skb)); - /* Only send the fds and no ref to pid in the first buffer */ - err = unix_scm_to_skb(siocb->scm, skb, !fds_sent, fds_sent); + /* Only send the fds in first buffer + * Last buffer can steal our references to pid/cred + */ + steal_refs = (sent + size >= len); + err = unix_scm_to_skb(siocb->scm, skb, !fds_sent, steal_refs); if (err < 0) { kfree_skb(skb); - goto out; + goto out_err; } max_level = err + 1; fds_sent = true; @@ -1654,7 +1659,7 @@ static int unix_stream_sendmsg(struct kiocb *kiocb, struct socket *sock, err = memcpy_fromiovec(skb_put(skb, size), msg->msg_iov, size); if (err) { kfree_skb(skb); - goto out; + goto out_err; } unix_state_lock(other); @@ -1671,7 +1676,7 @@ static int unix_stream_sendmsg(struct kiocb *kiocb, struct socket *sock, sent += size; } - if (skb) + if (steal_refs) scm_release(siocb->scm); else scm_destroy(siocb->scm); @@ -1687,9 +1692,8 @@ pipe_err: send_sig(SIGPIPE, current, 0); err = -EPIPE; out_err: - if (skb == NULL) + if (!steal_refs) scm_destroy(siocb->scm); -out: siocb->scm = NULL; return sent ? : err; }