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

On Wed, 2011-09-07 at 09:45 +0200, Eric Dumazet wrote:

Le mercredi 07 septembre 2011 à 13:20 +0800, Yan, Zheng a écrit :

quoted

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 <redacted>
Reported-by: Jiri Slaby <redacted>
Tested-by: Sedat Dilek <redacted>
Tested-by: Valdis Kletnieks <redacted>
---

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);

On Wed, Sep 7, 2011 at 2:01 PM, Tim Chen [off-list ref] 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 -

On Wed, Sep 7, 2011 at 10:12 PM, Sedat Dilek [off-list ref] wrote:

On Wed, Sep 7, 2011 at 2:01 PM, Tim Chen [off-list ref] wrote:

quoted

On Wed, 2011-09-07 at 09:45 +0200, Eric Dumazet wrote:

quoted

Le mercredi 07 septembre 2011 à 13:20 +0800, Yan, Zheng a écrit :

quoted

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 <redacted>
Reported-by: Jiri Slaby <redacted>
Tested-by: Sedat Dilek <redacted>
Tested-by: Valdis Kletnieks <redacted>
---

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);

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 -

Le mercredi 07 septembre 2011 à 05:01 -0700, Tim Chen a écrit :

On Wed, 2011-09-07 at 09:45 +0200, Eric Dumazet wrote:

quoted

Le mercredi 07 septembre 2011 à 13:20 +0800, Yan, Zheng a écrit :

quoted

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?

Yes, I'll try to cook a patch.

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 <redacted>
Reported-by: Jiri Slaby <redacted>
Tested-by: Sedat Dilek <redacted>
Tested-by: Valdis Kletnieks <redacted>
---

Signed-off-by: Eric Dumazet <redacted>

Thanks !

On Wed, 2011-09-07 at 22:30 +0200, Sedat Dilek wrote:

quoted

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 <redacted>
Signed-off-by: Eric Dumazet <redacted>
Reported-by: Jiri Slaby <redacted>
Tested-by: Sedat Dilek <redacted>
Tested-by: Valdis Kletnieks <redacted>
---

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;
 }

On 09/07/2011 10:37 PM, Tim Chen wrote:

I think we should call scm_release() here in the case of
steal_refs == true. Otherwise siocb->scm->fp may leak.

On Thu, 2011-09-08 at 08:27 +0800, Yan, Zheng wrote:

quoted

 	err = -EPIPE;
 out_err:
-	if (skb == NULL)
+	if (!steal_refs)
 		scm_destroy(siocb->scm);

I think we should call scm_release() here in the case of
steal_refs == true. Otherwise siocb->scm->fp may leak.

Yan Zheng,

I've updated the patch.  If it looks good to you now, can you add your
Signed-off-by to the patch.  Pending Sedat's testing on this patch,
I think it is good to go.

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 <redacted>
Reported-by: Jiri Slaby <redacted>
Tested-by: Sedat Dilek <redacted>
Tested-by: Valdis Kletnieks <redacted>
---

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 136298c..47780dc 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,10 @@ pipe_err:
 		send_sig(SIGPIPE, current, 0);
 	err = -EPIPE;
 out_err:
-	if (skb == NULL)
+	if (steal_refs)
+		scm_release(siocb->scm);
+	else
 		scm_destroy(siocb->scm);
-out:
 	siocb->scm = NULL;
 	return sent ? : err;
 }

On Wed, 2011-09-07 at 14:06 -0700, Tim Chen wrote:

On Thu, 2011-09-08 at 08:27 +0800, Yan, Zheng wrote:

quoted

quoted

 	err = -EPIPE;
 out_err:
-	if (skb == NULL)
+	if (!steal_refs)
 		scm_destroy(siocb->scm);

I think we should call scm_release() here in the case of
steal_refs == true. Otherwise siocb->scm->fp may leak.

Yan Zheng,

I've updated the patch.  If it looks good to you now, can you add your
Signed-off-by to the patch.  Pending Sedat's testing on this patch,
I think it is good to go.

Tim

Oops, I've forgotten to add Eric's previous Signed-off-by in my latest
patch log.  David, please add that when you pick up the patch.  
Once Yan Zheng added his sign off and Sedat tested the patch, I think it
will be good to go.

Thanks.

Tim 

On 09/08/2011 05:06 AM, Tim Chen wrote:

Signed-off-by: Zheng Yan <redacted>

Le mercredi 07 septembre 2011 à 14:06 -0700, Tim Chen a écrit :

I dont think this patch is good.

Sedat traces have nothing to do with af_unix.

Once unix_scm_to_skb() was called and successful, and steal_refs is true
our refs are attached to this skb. They will be released by
skb_free(skb). Same for fp : They either were sent in a previous skb or
this one.

This is why the "goto out;" was OK.

Le mercredi 07 septembre 2011 à 14:15 -0700, Tim Chen a écrit :

Oops, I've forgotten to add Eric's previous Signed-off-by in my latest
patch log.  David, please add that when you pick up the patch.  
Once Yan Zheng added his sign off and Sedat tested the patch, I think it
will be good to go.

Tim, as soon as you post another patch version, you must remove all
prior Signed-off-by, and only add yours.

So it was fine to do so.

By the way your last version introduce a new bug, so I would rather NACK
it ;)

On 09/08/2011 01:59 PM, Eric Dumazet wrote:

Le mercredi 07 septembre 2011 à 14:06 -0700, Tim Chen a écrit :

quoted

On Thu, 2011-09-08 at 08:27 +0800, Yan, Zheng wrote:

quoted

quoted

 	err = -EPIPE;
 out_err:
-	if (skb == NULL)
+	if (!steal_refs)
 		scm_destroy(siocb->scm);

I think we should call scm_release() here in the case of
steal_refs == true. Otherwise siocb->scm->fp may leak.

Yan Zheng,

I've updated the patch.  If it looks good to you now, can you add your
Signed-off-by to the patch.  Pending Sedat's testing on this patch,
I think it is good to go.

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 <redacted>
Reported-by: Jiri Slaby <redacted>
Tested-by: Sedat Dilek <redacted>
Tested-by: Valdis Kletnieks <redacted>
---

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 136298c..47780dc 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,10 @@ pipe_err:
 		send_sig(SIGPIPE, current, 0);
 	err = -EPIPE;
 out_err:
-	if (skb == NULL)
+	if (steal_refs)
+		scm_release(siocb->scm);
+	else
 		scm_destroy(siocb->scm);
-out:
 	siocb->scm = NULL;
 	return sent ? : err;
 }

I dont think this patch is good.

Sedat traces have nothing to do with af_unix.

Once unix_scm_to_skb() was called and successful, and steal_refs is true
our refs are attached to this skb. They will be released by
skb_free(skb). Same for fp : They either were sent in a previous skb or
this one.

This is why the "goto out;" was OK.

I don't think so. unix_scm_to_skb() calls unix_attach_fds(), it
always duplicates scm->fp. 

Regards

On Thu, Sep 8, 2011 at 7:59 AM, Eric Dumazet [off-list ref] wrote:

Le mercredi 07 septembre 2011 à 14:06 -0700, Tim Chen a écrit :

quoted

On Thu, 2011-09-08 at 08:27 +0800, Yan, Zheng wrote:

quoted

quoted

  err = -EPIPE;
 out_err:
- if (skb == NULL)
+ if (!steal_refs)
          scm_destroy(siocb->scm);

I think we should call scm_release() here in the case of
steal_refs == true. Otherwise siocb->scm->fp may leak.

Yan Zheng,

I've updated the patch.  If it looks good to you now, can you add your
Signed-off-by to the patch.  Pending Sedat's testing on this patch,
I think it is good to go.

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 <redacted>
Reported-by: Jiri Slaby <redacted>
Tested-by: Sedat Dilek <redacted>
Tested-by: Valdis Kletnieks <redacted>
---

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 136298c..47780dc 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,10 @@ pipe_err:

              send_sig(SIGPIPE, current, 0);
      err = -EPIPE;
 out_err:
-     if (skb == NULL)
+     if (steal_refs)
+             scm_release(siocb->scm);
+     else
              scm_destroy(siocb->scm);
-out:
      siocb->scm = NULL;
      return sent ? : err;
 }

I dont think this patch is good.

Sedat traces have nothing to do with af_unix.

Once unix_scm_to_skb() was called and successful, and steal_refs is true
our refs are attached to this skb. They will be released by
skb_free(skb). Same for fp : They either were sent in a previous skb or
this one.

This is why the "goto out;" was OK.

Good morning,

/me sees so many patches :-).
Yes, I guess the patch by Eric has nothing to do with seen
call-traces, adding "irqpoll" to Kernel command line seems to fix them
(my uptime: approx. 10:30, Eric's proposal patch in my last
patch-series is attached).

- Sedat -

Le jeudi 08 septembre 2011 à 14:22 +0800, Yan, Zheng a écrit :

I don't think so. unix_scm_to_skb() calls unix_attach_fds(), it
always duplicates scm->fp. 

What a mess. This code is a nightmare.

Part of the mess comes from scm_destroy() and scm_release() duplication.

We should have scm_destroy() only, as before, and NULLify scm->pid/cred
in unix_scm_to_skb() when we steal references.

It makes more sense and keeps things clearer.


 include/net/scm.h  |    9 ---------
 net/unix/af_unix.c |   27 +++++++++++++++------------
 2 files changed, 15 insertions(+), 21 deletions(-)

diff --git a/include/net/scm.h b/include/net/scm.h
index 68e1e48..2a5b42f 100644
--- a/include/net/scm.h
+++ b/include/net/scm.h

@@ -78,15 +78,6 @@ static __inline__ void scm_destroy(struct scm_cookie *scm)
 		__scm_destroy(scm);
 }
 
-static __inline__ void scm_release(struct scm_cookie *scm)
-{
-	/* keep ref on pid and cred */
-	scm->pid = NULL;
-	scm->cred = NULL;
-	if (scm->fp)
-		__scm_destroy(scm);
-}
-
 static __inline__ int scm_send(struct socket *sock, struct msghdr *msg,
 			       struct scm_cookie *scm)
 {

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index e6d9d10..1fa270a 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c

@@ -1379,15 +1379,18 @@ 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 {
 		UNIXCB(skb).pid  = scm->pid;
 		UNIXCB(skb).cred = scm->cred;
+		scm->pid = NULL;
+		scm->cred = NULL;
 	}
 	UNIXCB(skb).fp = NULL;
 	if (scm->fp && send_fds)

@@ -1454,7 +1457,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;

@@ -1550,7 +1553,7 @@ restart:
 	unix_state_unlock(other);
 	other->sk_data_ready(other, len);
 	sock_put(other);
-	scm_release(siocb->scm);
+	scm_destroy(siocb->scm);
 	return len;
 
 out_unlock:

@@ -1577,6 +1580,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)

@@ -1638,8 +1642,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;

@@ -1667,10 +1674,7 @@ static int unix_stream_sendmsg(struct kiocb *kiocb, struct socket *sock,
 		sent += size;
 	}
 
-	if (skb)
-		scm_release(siocb->scm);
-	else
-		scm_destroy(siocb->scm);
+	scm_destroy(siocb->scm);
 	siocb->scm = NULL;
 
 	return sent;

@@ -1683,8 +1687,7 @@ pipe_err:
 		send_sig(SIGPIPE, current, 0);
 	err = -EPIPE;
 out_err:
-	if (skb == NULL)
-		scm_destroy(siocb->scm);
+	scm_destroy(siocb->scm);
 out:
 	siocb->scm = NULL;
 	return sent ? : err;

On 09/08/2011 03:11 PM, Eric Dumazet wrote:

This code looks great, except "goto out;" is still there. I think we should replace it with "goto out_err;" :)

Regards

Le jeudi 08 septembre 2011 à 15:23 +0800, Yan, Zheng a écrit :

This code looks great, except "goto out;" is still there. I think we should replace it with "goto out_err;" :)

Indeed, you're right, thanks

 include/net/scm.h  |    9 ---------
 net/unix/af_unix.c |   32 +++++++++++++++++---------------
 2 files changed, 17 insertions(+), 24 deletions(-)

diff --git a/include/net/scm.h b/include/net/scm.h
index 68e1e48..2a5b42f 100644
--- a/include/net/scm.h
+++ b/include/net/scm.h

@@ -78,15 +78,6 @@ static __inline__ void scm_destroy(struct scm_cookie *scm)
 		__scm_destroy(scm);
 }
 
-static __inline__ void scm_release(struct scm_cookie *scm)
-{
-	/* keep ref on pid and cred */
-	scm->pid = NULL;
-	scm->cred = NULL;
-	if (scm->fp)
-		__scm_destroy(scm);
-}
-
 static __inline__ int scm_send(struct socket *sock, struct msghdr *msg,
 			       struct scm_cookie *scm)
 {

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index e6d9d10..c8a08ba 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c

@@ -1379,15 +1379,18 @@ 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 {
 		UNIXCB(skb).pid  = scm->pid;
 		UNIXCB(skb).cred = scm->cred;
+		scm->pid = NULL;
+		scm->cred = NULL;
 	}
 	UNIXCB(skb).fp = NULL;
 	if (scm->fp && send_fds)

@@ -1454,7 +1457,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;

@@ -1550,7 +1553,7 @@ restart:
 	unix_state_unlock(other);
 	other->sk_data_ready(other, len);
 	sock_put(other);
-	scm_release(siocb->scm);
+	scm_destroy(siocb->scm);
 	return len;
 
 out_unlock:

@@ -1577,6 +1580,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)

@@ -1638,11 +1642,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;

@@ -1650,7 +1657,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);

@@ -1667,10 +1674,7 @@ static int unix_stream_sendmsg(struct kiocb *kiocb, struct socket *sock,
 		sent += size;
 	}
 
-	if (skb)
-		scm_release(siocb->scm);
-	else
-		scm_destroy(siocb->scm);
+	scm_destroy(siocb->scm);
 	siocb->scm = NULL;
 
 	return sent;

@@ -1683,9 +1687,7 @@ pipe_err:
 		send_sig(SIGPIPE, current, 0);
 	err = -EPIPE;
 out_err:
-	if (skb == NULL)
-		scm_destroy(siocb->scm);
-out:
+	scm_destroy(siocb->scm);
 	siocb->scm = NULL;
 	return sent ? : err;
 }

On 09/08/2011 09:11 AM, Eric Dumazet wrote:

Le jeudi 08 septembre 2011 à 14:22 +0800, Yan, Zheng a écrit :

quoted

I don't think so. unix_scm_to_skb() calls unix_attach_fds(), it
always duplicates scm->fp. 

What a mess. This code is a nightmare.

Part of the mess comes from scm_destroy() and scm_release() duplication.

We should have scm_destroy() only, as before, and NULLify scm->pid/cred
in unix_scm_to_skb() when we steal references.

This patch works for me. I haven't tried the out_err fixup from the
followup, but I assume I won't spot a difference as those are fail paths
anyway...

thanks,
-- 
js

On Thu, Sep 8, 2011 at 9:56 AM, Jiri Slaby [off-list ref] wrote:

On 09/08/2011 09:11 AM, Eric Dumazet wrote:

quoted

Le jeudi 08 septembre 2011 à 14:22 +0800, Yan, Zheng a écrit :

quoted

I don't think so. unix_scm_to_skb() calls unix_attach_fds(), it
always duplicates scm->fp.

What a mess. This code is a nightmare.

Part of the mess comes from scm_destroy() and scm_release() duplication.

We should have scm_destroy() only, as before, and NULLify scm->pid/cred
in unix_scm_to_skb() when we steal references.

This patch works for me. I haven't tried the out_err fixup from the
followup, but I assume I won't spot a difference as those are fail paths
anyway...

thanks,
--
js

I have tested the same patch here (before shopping, but can test the
"final" patch, too.).

- Sedat -

On Thu, Sep 8, 2011 at 9:33 AM, Eric Dumazet [off-list ref] wrote:

I have tested this fixup patch on i386.
Can we have a separate patch with corrected descriptive text?

Thanks to all involved people.

- Sedat -

Le jeudi 08 septembre 2011 à 11:59 +0200, Sedat Dilek a écrit :

I have tested this fixup patch on i386.
Can we have a separate patch with corrected descriptive text?

Thanks to all involved people.

Here it is :

[PATCH net-next v3] af_unix: Fix use-after-free crashes

Commit 0856a30409 (Scm: Remove unnecessary pid & credential references
in Unix socket's send and receive path) introduced an use-after-free
bug.

We are allowed to steal the references to pid/cred only in the last skb
sent from unix_stream_sendmsg(), because first skbs might be consumed by
the receiver before we finish our sendmsg() call.

Remove scm_release() helper, since its cleaner to clear pid/cred fields
in unix_scm_to_skb() when we steal them.

Based on prior patches from Yan Zheng and Tim Chen

Signed-off-by: Eric Dumazet <redacted>
Reported-by: Jiri Slaby <redacted>
Tested-by: Sedat Dilek <redacted>
Tested-by: Valdis Kletnieks <redacted>
---
 include/net/scm.h  |    9 ---------
 net/unix/af_unix.c |   32 +++++++++++++++++---------------
 2 files changed, 17 insertions(+), 24 deletions(-)

diff --git a/include/net/scm.h b/include/net/scm.h
index 68e1e48..2a5b42f 100644
--- a/include/net/scm.h
+++ b/include/net/scm.h

@@ -78,15 +78,6 @@ static __inline__ void scm_destroy(struct scm_cookie *scm)
 		__scm_destroy(scm);
 }
 
-static __inline__ void scm_release(struct scm_cookie *scm)
-{
-	/* keep ref on pid and cred */
-	scm->pid = NULL;
-	scm->cred = NULL;
-	if (scm->fp)
-		__scm_destroy(scm);
-}
-
 static __inline__ int scm_send(struct socket *sock, struct msghdr *msg,
 			       struct scm_cookie *scm)
 {

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index e6d9d10..c8a08ba 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c

@@ -1379,15 +1379,18 @@ 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 {
 		UNIXCB(skb).pid  = scm->pid;
 		UNIXCB(skb).cred = scm->cred;
+		scm->pid = NULL;
+		scm->cred = NULL;
 	}
 	UNIXCB(skb).fp = NULL;
 	if (scm->fp && send_fds)