All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH 0/2] Memory fixes for slirp tcp_emu()
@ 2019-04-18 12:59 ` Marc-André Lureau
  0 siblings, 0 replies; 8+ messages in thread
From: Marc-André Lureau @ 2019-04-18 12:59 UTC (permalink / raw)
  To: qemu-devel; +Cc: Jan Kiszka, Samuel Thibault, Marc-André Lureau

Hi,

See the following 2 patches fixing some memory issues in
tcp_emu(). Those are not regressions, consider it for 4.1.

Marc-André Lureau (2):
  slirp: ensure there is enough space in mbuf to null-terminate
  slirp: don't manipulate so_rcv in tcp_emu()

 slirp/src/tcp_subr.c | 27 +++++++--------------------
 1 file changed, 7 insertions(+), 20 deletions(-)

-- 
2.21.0.313.ge35b8cb8e2

^ permalink raw reply	[flat|nested] 8+ messages in thread
* [Qemu-devel] [PATCH 0/2] Memory fixes for slirp tcp_emu()
@ 2019-04-18 12:59 ` Marc-André Lureau
  0 siblings, 0 replies; 8+ messages in thread
From: Marc-André Lureau @ 2019-04-18 12:59 UTC (permalink / raw)
  To: qemu-devel; +Cc: Jan Kiszka, Marc-André Lureau, Samuel Thibault

Hi,

See the following 2 patches fixing some memory issues in
tcp_emu(). Those are not regressions, consider it for 4.1.

Marc-André Lureau (2):
  slirp: ensure there is enough space in mbuf to null-terminate
  slirp: don't manipulate so_rcv in tcp_emu()

 slirp/src/tcp_subr.c | 27 +++++++--------------------
 1 file changed, 7 insertions(+), 20 deletions(-)

-- 
2.21.0.313.ge35b8cb8e2



^ permalink raw reply	[flat|nested] 8+ messages in thread
* [Qemu-devel] [PATCH 1/2] slirp: ensure there is enough space in mbuf to null-terminate
@ 2019-04-18 12:59   ` Marc-André Lureau
  0 siblings, 0 replies; 8+ messages in thread
From: Marc-André Lureau @ 2019-04-18 12:59 UTC (permalink / raw)
  To: qemu-devel
  Cc: Jan Kiszka, Samuel Thibault, Marc-André Lureau, Prasad J Pandit

Prevents from buffer overflows.
Related to: https://bugzilla.redhat.com/show_bug.cgi?id=1664205

Cc: Prasad J Pandit <pjp@fedoraproject.org>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 slirp/src/tcp_subr.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/slirp/src/tcp_subr.c b/slirp/src/tcp_subr.c
index fde9207b0c..c43598de48 100644
--- a/slirp/src/tcp_subr.c
+++ b/slirp/src/tcp_subr.c
@@ -644,6 +644,7 @@ tcp_emu(struct socket *so, struct mbuf *m)
 			memcpy(so_rcv->sb_wptr, m->m_data, m->m_len);
 			so_rcv->sb_wptr += m->m_len;
 			so_rcv->sb_rptr += m->m_len;
+			m_inc(m, m->m_len + 1);
 			m->m_data[m->m_len] = 0; /* NULL terminate */
 			if (strchr(m->m_data, '\r') || strchr(m->m_data, '\n')) {
 				if (sscanf(so_rcv->sb_data, "%u%*[ ,]%u", &n1, &n2) == 2) {
@@ -677,6 +678,7 @@ tcp_emu(struct socket *so, struct mbuf *m)
 		}
 
         case EMU_FTP: /* ftp */
+                m_inc(m, m->m_len + 1);
                 *(m->m_data+m->m_len) = 0; /* NUL terminate for strstr */
 		if ((bptr = (char *)strstr(m->m_data, "ORT")) != NULL) {
 			/*
@@ -774,6 +776,7 @@ tcp_emu(struct socket *so, struct mbuf *m)
 		/*
 		 * Need to emulate DCC CHAT, DCC SEND and DCC MOVE
 		 */
+		m_inc(m, m->m_len + 1);
 		*(m->m_data+m->m_len) = 0; /* NULL terminate the string for strstr */
 		if ((bptr = (char *)strstr(m->m_data, "DCC")) == NULL)
 			 return 1;
-- 
2.21.0.313.ge35b8cb8e2

^ permalink raw reply related	[flat|nested] 8+ messages in thread
* [Qemu-devel] [PATCH 1/2] slirp: ensure there is enough space in mbuf to null-terminate
@ 2019-04-18 12:59   ` Marc-André Lureau
  0 siblings, 0 replies; 8+ messages in thread
From: Marc-André Lureau @ 2019-04-18 12:59 UTC (permalink / raw)
  To: qemu-devel
  Cc: Jan Kiszka, Prasad J Pandit, Marc-André Lureau, Samuel Thibault

Prevents from buffer overflows.
Related to: https://bugzilla.redhat.com/show_bug.cgi?id=1664205

Cc: Prasad J Pandit <pjp@fedoraproject.org>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 slirp/src/tcp_subr.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/slirp/src/tcp_subr.c b/slirp/src/tcp_subr.c
index fde9207b0c..c43598de48 100644
--- a/slirp/src/tcp_subr.c
+++ b/slirp/src/tcp_subr.c
@@ -644,6 +644,7 @@ tcp_emu(struct socket *so, struct mbuf *m)
 			memcpy(so_rcv->sb_wptr, m->m_data, m->m_len);
 			so_rcv->sb_wptr += m->m_len;
 			so_rcv->sb_rptr += m->m_len;
+			m_inc(m, m->m_len + 1);
 			m->m_data[m->m_len] = 0; /* NULL terminate */
 			if (strchr(m->m_data, '\r') || strchr(m->m_data, '\n')) {
 				if (sscanf(so_rcv->sb_data, "%u%*[ ,]%u", &n1, &n2) == 2) {
@@ -677,6 +678,7 @@ tcp_emu(struct socket *so, struct mbuf *m)
 		}
 
         case EMU_FTP: /* ftp */
+                m_inc(m, m->m_len + 1);
                 *(m->m_data+m->m_len) = 0; /* NUL terminate for strstr */
 		if ((bptr = (char *)strstr(m->m_data, "ORT")) != NULL) {
 			/*
@@ -774,6 +776,7 @@ tcp_emu(struct socket *so, struct mbuf *m)
 		/*
 		 * Need to emulate DCC CHAT, DCC SEND and DCC MOVE
 		 */
+		m_inc(m, m->m_len + 1);
 		*(m->m_data+m->m_len) = 0; /* NULL terminate the string for strstr */
 		if ((bptr = (char *)strstr(m->m_data, "DCC")) == NULL)
 			 return 1;
-- 
2.21.0.313.ge35b8cb8e2



^ permalink raw reply related	[flat|nested] 8+ messages in thread
* [Qemu-devel] [PATCH 2/2] slirp: don't manipulate so_rcv in tcp_emu()
@ 2019-04-18 12:59   ` Marc-André Lureau
  0 siblings, 0 replies; 8+ messages in thread
From: Marc-André Lureau @ 2019-04-18 12:59 UTC (permalink / raw)
  To: qemu-devel
  Cc: Jan Kiszka, Samuel Thibault, Marc-André Lureau, Prasad J Pandit

For some reason, EMU_IDENT is not like other "emulated" protocols and
tries to reconstitute the original buffer, if it came in multiple
packets. Unfortunately, it does so wrongly, as it doesn't respect the
sbuf circular buffer appending rules, nor does it maintain some of the
invariants (rptr is incremented without bounds, etc): this leads to
further memory corruption revealed by ASAN or various malloc
errors. Furthermore, the so_rcv buffer is regularly flushed, so there
is no guarantee that buffer reconstruction will do what is expected.

Instead, do what the function comment says: "XXX Assumes the whole
command came in one packet", and don't touch so_rcv.

Related to: https://bugzilla.redhat.com/show_bug.cgi?id=1664205

Cc: Prasad J Pandit <pjp@fedoraproject.org>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 slirp/src/tcp_subr.c | 26 +++++---------------------
 1 file changed, 5 insertions(+), 21 deletions(-)

diff --git a/slirp/src/tcp_subr.c b/slirp/src/tcp_subr.c
index c43598de48..f4379ae012 100644
--- a/slirp/src/tcp_subr.c
+++ b/slirp/src/tcp_subr.c
@@ -634,20 +634,9 @@ tcp_emu(struct socket *so, struct mbuf *m)
 			struct socket *tmpso;
 			struct sockaddr_in addr;
 			socklen_t addrlen = sizeof(struct sockaddr_in);
-			struct sbuf *so_rcv = &so->so_rcv;
 
-			if (m->m_len > so_rcv->sb_datalen
-					- (so_rcv->sb_wptr - so_rcv->sb_data)) {
-			    return 1;
-			}
-
-			memcpy(so_rcv->sb_wptr, m->m_data, m->m_len);
-			so_rcv->sb_wptr += m->m_len;
-			so_rcv->sb_rptr += m->m_len;
-			m_inc(m, m->m_len + 1);
-			m->m_data[m->m_len] = 0; /* NULL terminate */
-			if (strchr(m->m_data, '\r') || strchr(m->m_data, '\n')) {
-				if (sscanf(so_rcv->sb_data, "%u%*[ ,]%u", &n1, &n2) == 2) {
+			if (g_strstr_len(m->m_data, m->m_len, "\r\n")
+				&& sscanf(m->m_data, "%u%*[ ,]%u\r\n", &n1, &n2) == 2) {
 					HTONS(n1);
 					HTONS(n2);
 					/* n2 is the one on our host */
@@ -666,15 +655,10 @@ tcp_emu(struct socket *so, struct mbuf *m)
 					}
 					NTOHS(n1);
 					NTOHS(n2);
-					so_rcv->sb_cc = snprintf(so_rcv->sb_data,
-								 so_rcv->sb_datalen,
-								 "%d,%d\r\n", n1, n2);
-					so_rcv->sb_rptr = so_rcv->sb_data;
-					so_rcv->sb_wptr = so_rcv->sb_data + so_rcv->sb_cc;
+					m->m_len = snprintf(m->m_data, m->m_size,
+										"%d,%d\r\n", n1, n2);
 				}
-			}
-			m_free(m);
-			return 0;
+			return 1;
 		}
 
         case EMU_FTP: /* ftp */
-- 
2.21.0.313.ge35b8cb8e2

^ permalink raw reply related	[flat|nested] 8+ messages in thread
* [Qemu-devel] [PATCH 2/2] slirp: don't manipulate so_rcv in tcp_emu()
@ 2019-04-18 12:59   ` Marc-André Lureau
  0 siblings, 0 replies; 8+ messages in thread
From: Marc-André Lureau @ 2019-04-18 12:59 UTC (permalink / raw)
  To: qemu-devel
  Cc: Jan Kiszka, Prasad J Pandit, Marc-André Lureau, Samuel Thibault

For some reason, EMU_IDENT is not like other "emulated" protocols and
tries to reconstitute the original buffer, if it came in multiple
packets. Unfortunately, it does so wrongly, as it doesn't respect the
sbuf circular buffer appending rules, nor does it maintain some of the
invariants (rptr is incremented without bounds, etc): this leads to
further memory corruption revealed by ASAN or various malloc
errors. Furthermore, the so_rcv buffer is regularly flushed, so there
is no guarantee that buffer reconstruction will do what is expected.

Instead, do what the function comment says: "XXX Assumes the whole
command came in one packet", and don't touch so_rcv.

Related to: https://bugzilla.redhat.com/show_bug.cgi?id=1664205

Cc: Prasad J Pandit <pjp@fedoraproject.org>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 slirp/src/tcp_subr.c | 26 +++++---------------------
 1 file changed, 5 insertions(+), 21 deletions(-)

diff --git a/slirp/src/tcp_subr.c b/slirp/src/tcp_subr.c
index c43598de48..f4379ae012 100644
--- a/slirp/src/tcp_subr.c
+++ b/slirp/src/tcp_subr.c
@@ -634,20 +634,9 @@ tcp_emu(struct socket *so, struct mbuf *m)
 			struct socket *tmpso;
 			struct sockaddr_in addr;
 			socklen_t addrlen = sizeof(struct sockaddr_in);
-			struct sbuf *so_rcv = &so->so_rcv;
 
-			if (m->m_len > so_rcv->sb_datalen
-					- (so_rcv->sb_wptr - so_rcv->sb_data)) {
-			    return 1;
-			}
-
-			memcpy(so_rcv->sb_wptr, m->m_data, m->m_len);
-			so_rcv->sb_wptr += m->m_len;
-			so_rcv->sb_rptr += m->m_len;
-			m_inc(m, m->m_len + 1);
-			m->m_data[m->m_len] = 0; /* NULL terminate */
-			if (strchr(m->m_data, '\r') || strchr(m->m_data, '\n')) {
-				if (sscanf(so_rcv->sb_data, "%u%*[ ,]%u", &n1, &n2) == 2) {
+			if (g_strstr_len(m->m_data, m->m_len, "\r\n")
+				&& sscanf(m->m_data, "%u%*[ ,]%u\r\n", &n1, &n2) == 2) {
 					HTONS(n1);
 					HTONS(n2);
 					/* n2 is the one on our host */
@@ -666,15 +655,10 @@ tcp_emu(struct socket *so, struct mbuf *m)
 					}
 					NTOHS(n1);
 					NTOHS(n2);
-					so_rcv->sb_cc = snprintf(so_rcv->sb_data,
-								 so_rcv->sb_datalen,
-								 "%d,%d\r\n", n1, n2);
-					so_rcv->sb_rptr = so_rcv->sb_data;
-					so_rcv->sb_wptr = so_rcv->sb_data + so_rcv->sb_cc;
+					m->m_len = snprintf(m->m_data, m->m_size,
+										"%d,%d\r\n", n1, n2);
 				}
-			}
-			m_free(m);
-			return 0;
+			return 1;
 		}
 
         case EMU_FTP: /* ftp */
-- 
2.21.0.313.ge35b8cb8e2



^ permalink raw reply related	[flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH 0/2] Memory fixes for slirp tcp_emu()
@ 2019-04-18 13:20   ` no-reply
  0 siblings, 0 replies; 8+ messages in thread
From: no-reply @ 2019-04-18 13:20 UTC (permalink / raw)
  To: marcandre.lureau; +Cc: fam, qemu-devel, jan.kiszka, samuel.thibault

Patchew URL: https://patchew.org/QEMU/20190418125908.11928-1-marcandre.lureau@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [Qemu-devel] [PATCH 0/2] Memory fixes for slirp tcp_emu()
Type: series
Message-id: 20190418125908.11928-1-marcandre.lureau@redhat.com

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
7c6d6d0 slirp: don't manipulate so_rcv in tcp_emu()
84d8e3e slirp: ensure there is enough space in mbuf to null-terminate

=== OUTPUT BEGIN ===
1/2 Checking commit 84d8e3ea687d (slirp: ensure there is enough space in mbuf to null-terminate)
ERROR: code indent should never use tabs
#22: FILE: slirp/src/tcp_subr.c:647:
+^I^I^Im_inc(m, m->m_len + 1);$

ERROR: code indent should never use tabs
#38: FILE: slirp/src/tcp_subr.c:779:
+^I^Im_inc(m, m->m_len + 1);$

total: 2 errors, 0 warnings, 21 lines checked

Patch 1/2 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

2/2 Checking commit 7c6d6d0e8c68 (slirp: don't manipulate so_rcv in tcp_emu())
ERROR: code indent should never use tabs
#47: FILE: slirp/src/tcp_subr.c:638:
+^I^I^Iif (g_strstr_len(m->m_data, m->m_len, "\r\n")$

WARNING: line over 80 characters
#48: FILE: slirp/src/tcp_subr.c:639:
+                               && sscanf(m->m_data, "%u%*[ ,]%u\r\n", &n1, &n2) == 2) {

ERROR: code indent should never use tabs
#48: FILE: slirp/src/tcp_subr.c:639:
+^I^I^I^I&& sscanf(m->m_data, "%u%*[ ,]%u\r\n", &n1, &n2) == 2) {$

WARNING: line over 80 characters
#61: FILE: slirp/src/tcp_subr.c:658:
+                                       m->m_len = snprintf(m->m_data, m->m_size,

ERROR: code indent should never use tabs
#61: FILE: slirp/src/tcp_subr.c:658:
+^I^I^I^I^Im->m_len = snprintf(m->m_data, m->m_size,$

ERROR: line over 90 characters
#62: FILE: slirp/src/tcp_subr.c:659:
+                                                                               "%d,%d\r\n", n1, n2);

ERROR: code indent should never use tabs
#62: FILE: slirp/src/tcp_subr.c:659:
+^I^I^I^I^I^I^I^I^I^I"%d,%d\r\n", n1, n2);$

ERROR: code indent should never use tabs
#67: FILE: slirp/src/tcp_subr.c:661:
+^I^I^Ireturn 1;$

total: 6 errors, 2 warnings, 40 lines checked

Patch 2/2 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20190418125908.11928-1-marcandre.lureau@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH 0/2] Memory fixes for slirp tcp_emu()
@ 2019-04-18 13:20   ` no-reply
  0 siblings, 0 replies; 8+ messages in thread
From: no-reply @ 2019-04-18 13:20 UTC (permalink / raw)
  To: marcandre.lureau
  Cc: fam, jan.kiszka, qemu-devel, samuel.thibault, marcandre.lureau

Patchew URL: https://patchew.org/QEMU/20190418125908.11928-1-marcandre.lureau@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [Qemu-devel] [PATCH 0/2] Memory fixes for slirp tcp_emu()
Type: series
Message-id: 20190418125908.11928-1-marcandre.lureau@redhat.com

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
7c6d6d0 slirp: don't manipulate so_rcv in tcp_emu()
84d8e3e slirp: ensure there is enough space in mbuf to null-terminate

=== OUTPUT BEGIN ===
1/2 Checking commit 84d8e3ea687d (slirp: ensure there is enough space in mbuf to null-terminate)
ERROR: code indent should never use tabs
#22: FILE: slirp/src/tcp_subr.c:647:
+^I^I^Im_inc(m, m->m_len + 1);$

ERROR: code indent should never use tabs
#38: FILE: slirp/src/tcp_subr.c:779:
+^I^Im_inc(m, m->m_len + 1);$

total: 2 errors, 0 warnings, 21 lines checked

Patch 1/2 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

2/2 Checking commit 7c6d6d0e8c68 (slirp: don't manipulate so_rcv in tcp_emu())
ERROR: code indent should never use tabs
#47: FILE: slirp/src/tcp_subr.c:638:
+^I^I^Iif (g_strstr_len(m->m_data, m->m_len, "\r\n")$

WARNING: line over 80 characters
#48: FILE: slirp/src/tcp_subr.c:639:
+                               && sscanf(m->m_data, "%u%*[ ,]%u\r\n", &n1, &n2) == 2) {

ERROR: code indent should never use tabs
#48: FILE: slirp/src/tcp_subr.c:639:
+^I^I^I^I&& sscanf(m->m_data, "%u%*[ ,]%u\r\n", &n1, &n2) == 2) {$

WARNING: line over 80 characters
#61: FILE: slirp/src/tcp_subr.c:658:
+                                       m->m_len = snprintf(m->m_data, m->m_size,

ERROR: code indent should never use tabs
#61: FILE: slirp/src/tcp_subr.c:658:
+^I^I^I^I^Im->m_len = snprintf(m->m_data, m->m_size,$

ERROR: line over 90 characters
#62: FILE: slirp/src/tcp_subr.c:659:
+                                                                               "%d,%d\r\n", n1, n2);

ERROR: code indent should never use tabs
#62: FILE: slirp/src/tcp_subr.c:659:
+^I^I^I^I^I^I^I^I^I^I"%d,%d\r\n", n1, n2);$

ERROR: code indent should never use tabs
#67: FILE: slirp/src/tcp_subr.c:661:
+^I^I^Ireturn 1;$

total: 6 errors, 2 warnings, 40 lines checked

Patch 2/2 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20190418125908.11928-1-marcandre.lureau@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

^ permalink raw reply	[flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-04-18 13:22 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-18 12:59 [Qemu-devel] [PATCH 0/2] Memory fixes for slirp tcp_emu() Marc-André Lureau
2019-04-18 12:59 ` Marc-André Lureau
2019-04-18 12:59 ` [Qemu-devel] [PATCH 1/2] slirp: ensure there is enough space in mbuf to null-terminate Marc-André Lureau
2019-04-18 12:59   ` Marc-André Lureau
2019-04-18 12:59 ` [Qemu-devel] [PATCH 2/2] slirp: don't manipulate so_rcv in tcp_emu() Marc-André Lureau
2019-04-18 12:59   ` Marc-André Lureau
2019-04-18 13:20 ` [Qemu-devel] [PATCH 0/2] Memory fixes for slirp tcp_emu() no-reply
2019-04-18 13:20   ` no-reply

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.