Message ID | 1315555109.2294.9.camel@edumazet-HP-Compaq-6005-Pro-SFF-PC |
---|---|
State | Not Applicable, archived |
Delegated to: | David Miller |
Headers | show |
On Fri, 2011-09-09 at 09:58 +0200, Eric Dumazet wrote: > Le vendredi 09 septembre 2011 à 08:51 +0200, Eric Dumazet a écrit : > > > Now we have to fix a bug in unix_stream_recvmsg() as well. > > > > consume_skb() call actually releases pid/cred references, and we can use > > them after their eventual freeing. > > > > Keep also in mind that receiver can provides a too short user buffer, > > and skb can be put back to head of sk_receive_queue > > > > Here is the patch to address this point. > > Apply it after (af_unix: Fix use-after-free crashes) > > I can make a combo patch once everybody agrees. > > [PATCH net-next] af_unix: fix use after free in unix_stream_recvmsg() > > Commit 0856a30409 (Scm: Remove unnecessary pid & credential references > in Unix socket's send and receive path) introduced an use-after-free > bug in unix_stream_recvmsg(). > > We should call consume_skb(skb) only after our possible use of pid/cred. > > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> > --- > net/unix/af_unix.c | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c > index c8a08ba..1bd4ecf 100644 > --- a/net/unix/af_unix.c > +++ b/net/unix/af_unix.c > @@ -1873,6 +1873,7 @@ static int unix_stream_recvmsg(struct kiocb *iocb, struct socket *sock, > int target; > int err = 0; > long timeo; > + struct sk_buff *skb; > > err = -EINVAL; > if (sk->sk_state != TCP_ESTABLISHED) > @@ -1904,7 +1905,6 @@ static int unix_stream_recvmsg(struct kiocb *iocb, struct socket *sock, > > do { > int chunk; > - struct sk_buff *skb; > > unix_state_lock(sk); > skb = skb_dequeue(&sk->sk_receive_queue); > @@ -1949,6 +1949,7 @@ static int unix_stream_recvmsg(struct kiocb *iocb, struct socket *sock, > if ((UNIXCB(skb).pid != siocb->scm->pid) || > (UNIXCB(skb).cred != siocb->scm->cred)) { > skb_queue_head(&sk->sk_receive_queue, skb); > + skb = NULL; > break; > } > } else { > @@ -1967,6 +1968,7 @@ static int unix_stream_recvmsg(struct kiocb *iocb, struct socket *sock, > chunk = min_t(unsigned int, skb->len, size); > if (memcpy_toiovec(msg->msg_iov, skb->data, chunk)) { > skb_queue_head(&sk->sk_receive_queue, skb); > + skb = NULL; > if (copied == 0) > copied = -EFAULT; > break; > @@ -1984,13 +1986,14 @@ static int unix_stream_recvmsg(struct kiocb *iocb, struct socket *sock, > /* put the skb back if we didn't use it up.. */ > if (skb->len) { > skb_queue_head(&sk->sk_receive_queue, skb); > + skb = NULL; > break; > } > > - consume_skb(skb); > - > - if (siocb->scm->fp) > + if (UNIXCB(skb).pid || siocb->scm->fp) > break; > + consume_skb(skb); > + skb = NULL; > } else { > /* It is questionable, see note in unix_dgram_recvmsg. > */ > @@ -1999,12 +2002,14 @@ static int unix_stream_recvmsg(struct kiocb *iocb, struct socket *sock, > > /* put message back and return */ > skb_queue_head(&sk->sk_receive_queue, skb); > + skb = NULL; > break; > } > } while (size); > > mutex_unlock(&u->readlock); > scm_recv(sock, msg, siocb->scm, flags); > + consume_skb(skb); > out: > return copied ? : err; > } > > Acked-by: Tim Chen <tim.c.chen@linux.intel.com> -- 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 c8a08ba..1bd4ecf 100644 --- a/net/unix/af_unix.c +++ b/net/unix/af_unix.c @@ -1873,6 +1873,7 @@ static int unix_stream_recvmsg(struct kiocb *iocb, struct socket *sock, int target; int err = 0; long timeo; + struct sk_buff *skb; err = -EINVAL; if (sk->sk_state != TCP_ESTABLISHED) @@ -1904,7 +1905,6 @@ static int unix_stream_recvmsg(struct kiocb *iocb, struct socket *sock, do { int chunk; - struct sk_buff *skb; unix_state_lock(sk); skb = skb_dequeue(&sk->sk_receive_queue); @@ -1949,6 +1949,7 @@ static int unix_stream_recvmsg(struct kiocb *iocb, struct socket *sock, if ((UNIXCB(skb).pid != siocb->scm->pid) || (UNIXCB(skb).cred != siocb->scm->cred)) { skb_queue_head(&sk->sk_receive_queue, skb); + skb = NULL; break; } } else { @@ -1967,6 +1968,7 @@ static int unix_stream_recvmsg(struct kiocb *iocb, struct socket *sock, chunk = min_t(unsigned int, skb->len, size); if (memcpy_toiovec(msg->msg_iov, skb->data, chunk)) { skb_queue_head(&sk->sk_receive_queue, skb); + skb = NULL; if (copied == 0) copied = -EFAULT; break; @@ -1984,13 +1986,14 @@ static int unix_stream_recvmsg(struct kiocb *iocb, struct socket *sock, /* put the skb back if we didn't use it up.. */ if (skb->len) { skb_queue_head(&sk->sk_receive_queue, skb); + skb = NULL; break; } - consume_skb(skb); - - if (siocb->scm->fp) + if (UNIXCB(skb).pid || siocb->scm->fp) break; + consume_skb(skb); + skb = NULL; } else { /* It is questionable, see note in unix_dgram_recvmsg. */ @@ -1999,12 +2002,14 @@ static int unix_stream_recvmsg(struct kiocb *iocb, struct socket *sock, /* put message back and return */ skb_queue_head(&sk->sk_receive_queue, skb); + skb = NULL; break; } } while (size); mutex_unlock(&u->readlock); scm_recv(sock, msg, siocb->scm, flags); + consume_skb(skb); out: return copied ? : err; }