Re: [PATCH -next v2] unix stream: Fix use-after-free crashes

[Tim Chen <tim.c.chen@linux.intel.com>]

On Wed, 2011-09-07 at 09:45 +0200, Eric Dumazet wrote:
> Le mercredi 07 septembre 2011 à 13:20 +0800, Yan, Zheng a écrit :
> 
> > Is code like this OK? Thanks
> > ---
> > 	if (sent + size < len) { 
> > 		/* Only send the fds in the first buffer */
> > 		/* get additional ref if more skbs will be created */
> > 		err = unix_scm_to_skb(siocb->scm, skb, !fds_sent, true);
> > 	} else {
> > 		err = unix_scm_to_skb(siocb->scm, skb, !fds_sent, false);
> > 		ref_avail = false;
> > 	}
> > 
> > 
> 
> Whats wrong with using ref_avail in the unix_scm_to_skb() call itself ?
> 
> something like :
> 

Eric,

Your updated patch looks good when I tested it on my side.  It makes the
patch much more readable.  If this patch looks good with you and Yan
Zheng, can you and Yan Zheng add your Signed-off-by to the patch?

Jiri, Sedat or Valdis, if you can verify that the patch fixed commit
0856a30409, that will be appreciated.

Eric, are you planning to do a fast path patch that doesn't do pid ref
for the case where CONFIG_PID_NS is not set?

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 references 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 <tim.c.chen@linux.intel.com>
Reported-by: Jiri Slaby <jirislaby@gmail.com>
Tested-by: Sedat Dilek <sedat.dilek@googlemail.com>
Tested-by: Valdis Kletnieks <Valdis.Kletnieks@vt.edu>
---

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 136298c..4a324a0 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,8 +1644,11 @@ 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;
@@ -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);