[Qemu-devel] [PATCH v2 0/4] slirp: deliver received TCP RSTs to the guest

[Qemu-devel] [PATCH v2 0/4] slirp: deliver received TCP RSTs to the guest

[Steven Luo]

Changes from v1:

* added patch 2, a fix for an use-after-free exposed by this series
  (thanks Samuel Thibault)
* incorporated Reviewed-by lines
* attributed patches correctly

-Steven Luo

===

QEMU's user-mode networking does not currently pass received TCP RSTs to
guests, meaning that applications in guests hang if the remote server
rejects their network connections.  This is particularly noticeable when
IPv6 is enabled, the guest is configured to prefer IPv6 and the remote
server rejects IPv6 connections (segment-data.zqtk.net is one example),
but the bug appears to be longstanding and affects TCP over IPv4 as
well.

There are four short patches in this series.  The first and second fix
crashes which would be exposed by the rest of the series.  The third,
which fixes delivery of an RST interrupting an already-established TCP
connection, was submitted by Edgar Iglesias in 2008 and appears to have
been missed then.  The last patch fixes the case where the remote end
sends RST in reply to our SYN (rejects our incoming connection attempt).

Lightly tested on a Linux host with Linux and Windows 7 guests.

Edgar E. Iglesias (1):
  slirp: Propagate host TCP RST to the guest.

Steven Luo (3):
  slirp: don't crash when tcp_sockclosed() is called with a NULL tp
  slirp: avoid use-after-free in slirp_pollfds_poll() if soread()
    returns an error
  slirp: handle deferred ECONNREFUSED on non-blocking TCP sockets

 slirp/slirp.c     | 12 +++++++++++-
 slirp/socket.c    | 34 +++++++++++++++++++++++++++-------
 slirp/socket.h    |  2 +-
 slirp/tcp_input.c |  6 ++++++
 slirp/tcp_subr.c  |  7 +++++--
 5 files changed, 50 insertions(+), 11 deletions(-)

-- 
2.1.4

[Qemu-devel] [PATCH v2 1/4] slirp: don't crash when tcp_sockclosed() is called with a NULL tp

[Steven Luo]

From: Steven Luo <steven+qemu@steven676.net>

Signed-off-by: Steven Luo <steven+qemu@steven676.net>
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
v1->v2:
* added Reviewed-by line

 slirp/tcp_subr.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/slirp/tcp_subr.c b/slirp/tcp_subr.c
index dbfd2c6..32ff452 100644
--- a/slirp/tcp_subr.c
+++ b/slirp/tcp_subr.c
@@ -356,6 +356,10 @@ tcp_sockclosed(struct tcpcb *tp)
 	DEBUG_CALL("tcp_sockclosed");
 	DEBUG_ARG("tp = %p", tp);
 
+	if (!tp) {
+		return;
+	}
+
 	switch (tp->t_state) {
 
 	case TCPS_CLOSED:
@@ -374,8 +378,7 @@ tcp_sockclosed(struct tcpcb *tp)
 		tp->t_state = TCPS_LAST_ACK;
 		break;
 	}
-	if (tp)
-		tcp_output(tp);
+	tcp_output(tp);
 }
 
 /*
-- 
2.1.4

[Qemu-devel] [PATCH v2 2/4] slirp: avoid use-after-free in slirp_pollfds_poll() if soread() returns an error

[Steven Luo]

From: Steven Luo <steven+qemu@steven676.net>

Samuel Thibault pointed out that it's possible that slirp_pollfds_poll()
will try to use a socket even after soread() returns an error, resulting
in an use-after-free if the socket was removed while handling the error.
Avoid this by refusing to continue to work with the socket in this case.

Signed-off-by: Steven Luo <steven+qemu@steven676.net>
---
It seems to me that it might be possible to trigger this use-after-free
even without the following patches, as tcp_sockclosed() (which soread()
currently calls when it gets an error in recv()) frees the socket in
certain cases.  The remaining patches in this series probably make this
much easier to trigger, though.

 slirp/slirp.c  | 12 +++++++++++-
 slirp/socket.c | 17 +++++++++++------
 slirp/socket.h |  2 +-
 3 files changed, 23 insertions(+), 8 deletions(-)

diff --git a/slirp/slirp.c b/slirp/slirp.c
index fef526c..9f4bea3 100644
--- a/slirp/slirp.c
+++ b/slirp/slirp.c
@@ -534,7 +534,12 @@ void slirp_pollfds_poll(GArray *pollfds, int select_error)
                  * test for G_IO_IN below if this succeeds
                  */
                 if (revents & G_IO_PRI) {
-                    sorecvoob(so);
+                    ret = sorecvoob(so);
+                    if (ret < 0) {
+                        /* Socket error might have resulted in the socket being
+                         * removed, do not try to do anything more with it. */
+                        continue;
+                    }
                 }
                 /*
                  * Check sockets for reading
@@ -553,6 +558,11 @@ void slirp_pollfds_poll(GArray *pollfds, int select_error)
                     if (ret > 0) {
                         tcp_output(sototcpcb(so));
                     }
+                    if (ret < 0) {
+                        /* Socket error might have resulted in the socket being
+                         * removed, do not try to do anything more with it. */
+                        continue;
+                    }
                 }
 
                 /*
diff --git a/slirp/socket.c b/slirp/socket.c
index b836c42..7f022a6 100644
--- a/slirp/socket.c
+++ b/slirp/socket.c
@@ -260,10 +260,11 @@ err:
  * so when OOB data arrives, we soread() it and everything
  * in the send buffer is sent as urgent data
  */
-void
+int
 sorecvoob(struct socket *so)
 {
 	struct tcpcb *tp = sototcpcb(so);
+	int ret;
 
 	DEBUG_CALL("sorecvoob");
 	DEBUG_ARG("so = %p", so);
@@ -276,11 +277,15 @@ sorecvoob(struct socket *so)
 	 * urgent data, or the read() doesn't return all the
 	 * urgent data.
 	 */
-	soread(so);
-	tp->snd_up = tp->snd_una + so->so_snd.sb_cc;
-	tp->t_force = 1;
-	tcp_output(tp);
-	tp->t_force = 0;
+	ret = soread(so);
+	if (ret > 0) {
+	    tp->snd_up = tp->snd_una + so->so_snd.sb_cc;
+	    tp->t_force = 1;
+	    tcp_output(tp);
+	    tp->t_force = 0;
+	}
+
+	return ret;
 }
 
 /*
diff --git a/slirp/socket.h b/slirp/socket.h
index e9c9b05..7dca506 100644
--- a/slirp/socket.h
+++ b/slirp/socket.h
@@ -127,7 +127,7 @@ struct socket *solookup(struct socket **, struct socket *,
 struct socket *socreate(Slirp *);
 void sofree(struct socket *);
 int soread(struct socket *);
-void sorecvoob(struct socket *);
+int sorecvoob(struct socket *);
 int sosendoob(struct socket *);
 int sowrite(struct socket *);
 void sorecvfrom(struct socket *);
-- 
2.1.4

[Qemu-devel] [PATCH v2 3/4] slirp: Propagate host TCP RST to the guest.

[Steven Luo]

From: Edgar E. Iglesias <edgar.iglesias@gmail.com>

When the host aborts (RST) it's side of a TCP connection we need to
propagate that RST to the guest. The current code can leave such guest
connections dangling forever. Spotted by Jason Wessel.

[steven@steven676.net: coding style adjustments]
Signed-off-by: Steven Luo <steven+qemu@steven676.net>
---
v1->v2:
* fix attribution

 slirp/socket.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/slirp/socket.c b/slirp/socket.c
index 7f022a6..0d67b12 100644
--- a/slirp/socket.c
+++ b/slirp/socket.c
@@ -176,9 +176,24 @@ soread(struct socket *so)
 		if (nn < 0 && (errno == EINTR || errno == EAGAIN))
 			return 0;
 		else {
+			int err;
+			socklen_t slen = sizeof err;
+
+			err = errno;
+			if (nn == 0) {
+				getsockopt(so->s, SOL_SOCKET, SO_ERROR,
+					   &err, &slen);
+			}
+
 			DEBUG_MISC((dfd, " --- soread() disconnected, nn = %d, errno = %d-%s\n", nn, errno,strerror(errno)));
 			sofcantrcvmore(so);
-			tcp_sockclosed(sototcpcb(so));
+
+			if (err == ECONNRESET
+			    || err == ENOTCONN || err == EPIPE) {
+				tcp_drop(sototcpcb(so), err);
+			} else {
+				tcp_sockclosed(sototcpcb(so));
+			}
 			return -1;
 		}
 	}
-- 
2.1.4

[Qemu-devel] [PATCH v2 4/4] slirp: handle deferred ECONNREFUSED on non-blocking TCP sockets

[Steven Luo]

From: Steven Luo <steven+qemu@steven676.net>

slirp currently only handles ECONNREFUSED in the case where connect()
returns immediately with that error; since we use non-blocking sockets,
most of the time we won't receive the error until we later try to read
from the socket.  Ensure that we deliver the appropriate RST to the
guest in this case.

Signed-off-by: Steven Luo <steven+qemu@steven676.net>
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
v1->v2:
* added Reviewed-by line

 slirp/socket.c    | 2 +-
 slirp/tcp_input.c | 6 ++++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/slirp/socket.c b/slirp/socket.c
index 0d67b12..bd97b2d 100644
--- a/slirp/socket.c
+++ b/slirp/socket.c
@@ -188,7 +188,7 @@ soread(struct socket *so)
 			DEBUG_MISC((dfd, " --- soread() disconnected, nn = %d, errno = %d-%s\n", nn, errno,strerror(errno)));
 			sofcantrcvmore(so);
 
-			if (err == ECONNRESET
+			if (err == ECONNRESET || err == ECONNREFUSED
 			    || err == ENOTCONN || err == EPIPE) {
 				tcp_drop(sototcpcb(so), err);
 			} else {
diff --git a/slirp/tcp_input.c b/slirp/tcp_input.c
index 1fcca30..5433e7f 100644
--- a/slirp/tcp_input.c
+++ b/slirp/tcp_input.c
@@ -725,6 +725,12 @@ findso:
 	    so->so_ti = ti;
 	    tp->t_timer[TCPT_KEEP] = TCPTV_KEEP_INIT;
 	    tp->t_state = TCPS_SYN_RECEIVED;
+	    /*
+	     * Initialize receive sequence numbers now so that we can send a
+	     * valid RST if the remote end rejects our connection.
+	     */
+	    tp->irs = ti->ti_seq;
+	    tcp_rcvseqinit(tp);
 	    tcp_template(tp);
 	  }
 	  return;
-- 
2.1.4