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

[Sedat Dilek <sedat.dilek@googlemail.com>]

[-- Attachment #1: Type: text/plain, Size: 5903 bytes --]

On Thu, Sep 8, 2011 at 7:59 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le mercredi 07 septembre 2011 à 14:06 -0700, Tim Chen a écrit :
>> On Thu, 2011-09-08 at 08:27 +0800, Yan, Zheng wrote:
>>
>> > >   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 <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..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 -

[-- Attachment #2: unix-stream-Fix-use-after-free-crashes-by-edumazet.patch --]
[-- Type: text/x-diff, Size: 2709 bytes --]

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