Re: [PATCH -next v2] unix stream: Fix use-after-free crashes
flat view
From: Tim Chen <hidden>
Date: 2011-09-06 22:06:23
Subsystem:
networking [general], networking [unix sockets], the rest · Maintainers:
"David S. Miller", Eric Dumazet, Jakub Kicinski, Paolo Abeni, Kuniyuki Iwashima, Linus Torvalds
On Tue, 2011-09-06 at 22:19 +0200, Eric Dumazet wrote:
unless scm_ref really means scm_noref ? I really hate this patch. I mean it. I read it 10 times, spent 2 hours and still dont understand it.
Eric, I've tried another patch to fix my original one. I've used a boolean ref_avail to indicate if there is an outstanding ref to scm not yet encoded into the skb. Hopefully the logic is clearer in this new patch.
As I said, we should revert the buggy patch, and rewrite a performance fix from scratch, with not a single get_pid()/put_pid() in fast path. read()/write() on AF_UNIX sockets should not use a single get_pid()/put_pid(). This is a serious regression we should fix at 100%, not 50% or even 75%, adding serious bugs.
That will be ideal if there is another way to fix it 100%, other than reverting commit 7361c36c. Probably if there is some way we know beforehand that both sender and receiver share the same pid, which is quite common, a lot of these pid code can be bypassed. Tim Signed-off-by: Tim Chen <redacted> ---
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 136298c..78be921 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c@@ -1582,11 +1582,13 @@ static int unix_stream_sendmsg(struct kiocb *kiocb, struct socket *sock, struct scm_cookie tmp_scm; bool fds_sent = false; int max_level; + bool ref_avail; /* scm ref not yet used in skb */ if (NULL == siocb->scm) siocb->scm = &tmp_scm; wait_for_unix_gc(); err = scm_send(sock, msg, siocb->scm); + ref_avail = true; if (err < 0) return err;
@@ -1642,11 +1644,18 @@ 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); + /* encode scm in skb and use the scm ref */ + ref_avail = false; + 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); + ref_avail = true; + } else + err = unix_scm_to_skb(siocb->scm, skb, !fds_sent, false); if (err < 0) { kfree_skb(skb); - goto out; + goto out_err; } max_level = err + 1; fds_sent = true;
@@ -1654,7 +1663,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,10 +1680,10 @@ static int unix_stream_sendmsg(struct kiocb *kiocb, struct socket *sock, sent += size; } - if (skb) - scm_release(siocb->scm); - else + if (ref_avail) scm_destroy(siocb->scm); + else + scm_release(siocb->scm); siocb->scm = NULL; return sent;
@@ -1687,9 +1696,10 @@ pipe_err: send_sig(SIGPIPE, current, 0); err = -EPIPE; out_err: - if (skb == NULL) + if (ref_avail) scm_destroy(siocb->scm); -out: + else + scm_release(siocb->scm); siocb->scm = NULL; return sent ? : err; }