Re: [Qemu-devel] [PATCH] slirp: fixed potential use-after-free of a socket
Hi,
On 21.02.2013 15:33, Jan Kiszka wrote:
On 2013-02-15 12:00, Vitaly Chipounov wrote:
A socket may still have references to it in various queues
at the time it is freed, causing memory corruptions.
Did you see it in practice? Or is this patch based on code review? What
will happen if those queued mbufs find their ifq_so NULL?
I have a packet trace that triggers this problem when it is injected
into the guest's NIC.
I am still not quite sure why this happens, but suspect that it could be
caused by malformed/partial TCP/IP streams.
Setting ifq_so to NULL shouldn't be an issue because there seems to be
checks for that (e.g., in if.c).
It doesn't seem to affect normal web browsing/downloading in the guest.
Vitaly
Signed-off-by: Vitaly Chipounov vitaly.chipou...@epfl.ch
---
slirp/socket.c | 29 +
1 file changed, 29 insertions(+)
diff --git a/slirp/socket.c b/slirp/socket.c
index 77b0c98..8a7adc8 100644
--- a/slirp/socket.c
+++ b/slirp/socket.c
@@ -55,6 +55,33 @@ socreate(Slirp *slirp)
return(so);
}
+/**
+ * It may happen that a socket is still referenced in various
+ * mbufs at the time it is freed. Clear all references to the
+ * socket here.
+ */
+static void soremovefromqueues(struct socket *so)
+{
+if (!so-so_queued) {
+return;
+}
+
+Slirp *slirp = so-slirp;
+struct mbuf *ifm;
+
+for (ifm = slirp-if_fastq.ifq_next; ifm != slirp-if_fastq; ifm =
ifm-ifq_next) {
This line and the one below smells like checkpatch.pl wasn't run against
it. Granted, slirp patches easy make checkpatch upset as the input is
not properly formatted, but please don't add more violations.
+if (ifm-ifq_so == so) {
+ifm-ifq_so = NULL;
+}
+}
+
+for (ifm = slirp-if_batchq.ifq_next; ifm != slirp-if_batchq; ifm =
ifm-ifq_next) {
+if (ifm-ifq_so == so) {
+ifm-ifq_so = NULL;
+}
+}
+}
+
/*
* remque and free a socket, clobber cache
*/
@@ -79,6 +106,8 @@ sofree(struct socket *so)
if(so-so_next so-so_prev)
remque(so); /* crashes if so is not in a queue */
+ soremovefromqueues(so);
+
free(so);
}
Sorry for the late feedback,
Jan
Re: [Qemu-devel] [PATCH] slirp: fixed potential use-after-free of a socket
On 2013-02-22 10:57, Vitaly Chipounov wrote: Hi, On 21.02.2013 15:33, Jan Kiszka wrote: On 2013-02-15 12:00, Vitaly Chipounov wrote: A socket may still have references to it in various queues at the time it is freed, causing memory corruptions. Did you see it in practice? Or is this patch based on code review? What will happen if those queued mbufs find their ifq_so NULL? I have a packet trace that triggers this problem when it is injected into the guest's NIC. I am still not quite sure why this happens, but suspect that it could be caused by malformed/partial TCP/IP streams. OK. Is it shareable? We need to understand why these bufs still hang around - and exclude they leak or cause more problems elsewhere. Jan -- Siemens AG, Corporate Technology, CT RTC ITP SDP-DE Corporate Competence Center Embedded Linux
Re: [Qemu-devel] [PATCH] slirp: fixed potential use-after-free of a socket
On 2013-02-15 12:00, Vitaly Chipounov wrote:
A socket may still have references to it in various queues
at the time it is freed, causing memory corruptions.
Did you see it in practice? Or is this patch based on code review? What
will happen if those queued mbufs find their ifq_so NULL?
Signed-off-by: Vitaly Chipounov vitaly.chipou...@epfl.ch
---
slirp/socket.c | 29 +
1 file changed, 29 insertions(+)
diff --git a/slirp/socket.c b/slirp/socket.c
index 77b0c98..8a7adc8 100644
--- a/slirp/socket.c
+++ b/slirp/socket.c
@@ -55,6 +55,33 @@ socreate(Slirp *slirp)
return(so);
}
+/**
+ * It may happen that a socket is still referenced in various
+ * mbufs at the time it is freed. Clear all references to the
+ * socket here.
+ */
+static void soremovefromqueues(struct socket *so)
+{
+if (!so-so_queued) {
+return;
+}
+
+Slirp *slirp = so-slirp;
+struct mbuf *ifm;
+
+for (ifm = slirp-if_fastq.ifq_next; ifm != slirp-if_fastq; ifm =
ifm-ifq_next) {
This line and the one below smells like checkpatch.pl wasn't run against
it. Granted, slirp patches easy make checkpatch upset as the input is
not properly formatted, but please don't add more violations.
+if (ifm-ifq_so == so) {
+ifm-ifq_so = NULL;
+}
+}
+
+for (ifm = slirp-if_batchq.ifq_next; ifm != slirp-if_batchq; ifm =
ifm-ifq_next) {
+if (ifm-ifq_so == so) {
+ifm-ifq_so = NULL;
+}
+}
+}
+
/*
* remque and free a socket, clobber cache
*/
@@ -79,6 +106,8 @@ sofree(struct socket *so)
if(so-so_next so-so_prev)
remque(so); /* crashes if so is not in a queue */
+ soremovefromqueues(so);
+
free(so);
}
Sorry for the late feedback,
Jan
--
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux
Re: [Qemu-devel] [PATCH] slirp: fixed potential use-after-free of a socket
On Fri, Feb 15, 2013 at 12:00:13PM +0100, Vitaly Chipounov wrote:
A socket may still have references to it in various queues
at the time it is freed, causing memory corruptions.
Signed-off-by: Vitaly Chipounov vitaly.chipou...@epfl.ch
---
slirp/socket.c | 29 +
1 file changed, 29 insertions(+)
diff --git a/slirp/socket.c b/slirp/socket.c
index 77b0c98..8a7adc8 100644
--- a/slirp/socket.c
+++ b/slirp/socket.c
@@ -55,6 +55,33 @@ socreate(Slirp *slirp)
return(so);
}
+/**
+ * It may happen that a socket is still referenced in various
+ * mbufs at the time it is freed. Clear all references to the
+ * socket here.
+ */
+static void soremovefromqueues(struct socket *so)
+{
+if (!so-so_queued) {
+return;
+}
+
+Slirp *slirp = so-slirp;
+struct mbuf *ifm;
Declarations should come at the beginning of a block/function (see: ...
er, I could've sworn it was in HACKING or CODING_STYLE but maybe not.
That's the standard in any case)
+
+for (ifm = slirp-if_fastq.ifq_next; ifm != slirp-if_fastq; ifm =
ifm-ifq_next) {
+if (ifm-ifq_so == so) {
+ifm-ifq_so = NULL;
+}
+}
+
+for (ifm = slirp-if_batchq.ifq_next; ifm != slirp-if_batchq; ifm =
ifm-ifq_next) {
+if (ifm-ifq_so == so) {
+ifm-ifq_so = NULL;
+}
+}
Is the intention just to mark them null or actually remove? Not sure
which, but either the logic should change or the function name should
(soinvalidate() or something along that line if the former).
+}
+
/*
* remque and free a socket, clobber cache
*/
@@ -79,6 +106,8 @@ sofree(struct socket *so)
if(so-so_next so-so_prev)
remque(so); /* crashes if so is not in a queue */
+ soremovefromqueues(so);
+
free(so);
}
--
1.7.10
Re: [Qemu-devel] [PATCH] slirp: fixed potential use-after-free of a socket
On Thu, Feb 21, 2013 at 03:47:25PM -0600, mdroth wrote:
On Fri, Feb 15, 2013 at 12:00:13PM +0100, Vitaly Chipounov wrote:
A socket may still have references to it in various queues
at the time it is freed, causing memory corruptions.
Signed-off-by: Vitaly Chipounov vitaly.chipou...@epfl.ch
Meant to cc qemu-stable
---
slirp/socket.c | 29 +
1 file changed, 29 insertions(+)
diff --git a/slirp/socket.c b/slirp/socket.c
index 77b0c98..8a7adc8 100644
--- a/slirp/socket.c
+++ b/slirp/socket.c
@@ -55,6 +55,33 @@ socreate(Slirp *slirp)
return(so);
}
+/**
+ * It may happen that a socket is still referenced in various
+ * mbufs at the time it is freed. Clear all references to the
+ * socket here.
+ */
+static void soremovefromqueues(struct socket *so)
+{
+if (!so-so_queued) {
+return;
+}
+
+Slirp *slirp = so-slirp;
+struct mbuf *ifm;
Declarations should come at the beginning of a block/function (see: ...
er, I could've sworn it was in HACKING or CODING_STYLE but maybe not.
That's the standard in any case)
+
+for (ifm = slirp-if_fastq.ifq_next; ifm != slirp-if_fastq; ifm =
ifm-ifq_next) {
+if (ifm-ifq_so == so) {
+ifm-ifq_so = NULL;
+}
+}
+
+for (ifm = slirp-if_batchq.ifq_next; ifm != slirp-if_batchq; ifm =
ifm-ifq_next) {
+if (ifm-ifq_so == so) {
+ifm-ifq_so = NULL;
+}
+}
Is the intention just to mark them null or actually remove? Not sure
which, but either the logic should change or the function name should
(soinvalidate() or something along that line if the former).
+}
+
/*
* remque and free a socket, clobber cache
*/
@@ -79,6 +106,8 @@ sofree(struct socket *so)
if(so-so_next so-so_prev)
remque(so); /* crashes if so is not in a queue */
+ soremovefromqueues(so);
+
free(so);
}
--
1.7.10
[Qemu-devel] [PATCH] slirp: fixed potential use-after-free of a socket
A socket may still have references to it in various queues
at the time it is freed, causing memory corruptions.
Signed-off-by: Vitaly Chipounov vitaly.chipou...@epfl.ch
---
slirp/socket.c | 29 +
1 file changed, 29 insertions(+)
diff --git a/slirp/socket.c b/slirp/socket.c
index 77b0c98..8a7adc8 100644
--- a/slirp/socket.c
+++ b/slirp/socket.c
@@ -55,6 +55,33 @@ socreate(Slirp *slirp)
return(so);
}
+/**
+ * It may happen that a socket is still referenced in various
+ * mbufs at the time it is freed. Clear all references to the
+ * socket here.
+ */
+static void soremovefromqueues(struct socket *so)
+{
+if (!so-so_queued) {
+return;
+}
+
+Slirp *slirp = so-slirp;
+struct mbuf *ifm;
+
+for (ifm = slirp-if_fastq.ifq_next; ifm != slirp-if_fastq; ifm =
ifm-ifq_next) {
+if (ifm-ifq_so == so) {
+ifm-ifq_so = NULL;
+}
+}
+
+for (ifm = slirp-if_batchq.ifq_next; ifm != slirp-if_batchq; ifm =
ifm-ifq_next) {
+if (ifm-ifq_so == so) {
+ifm-ifq_so = NULL;
+}
+}
+}
+
/*
* remque and free a socket, clobber cache
*/
@@ -79,6 +106,8 @@ sofree(struct socket *so)
if(so-so_next so-so_prev)
remque(so); /* crashes if so is not in a queue */
+ soremovefromqueues(so);
+
free(so);
}
--
1.7.10
