All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH] gdbstub: Fix buffer overflows in gdb_handle_packet()
@ 2015-10-13  7:38 Kevin Wolf
  2015-10-14  6:53 ` P J P
  2015-10-29  7:30 ` Michael Tokarev
  0 siblings, 2 replies; 5+ messages in thread
From: Kevin Wolf @ 2015-10-13  7:38 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, qemu-trivial, qemu-stable

Some places in gdb_handle_packet() can get an arbitrary length (most
times directly from the client) and either didn't check it at all or
checked against the wrong value, potentially causing buffer overflows.

Cc: qemu-stable@nongnu.org
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 gdbstub.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index d2c95b5..9c29aa0 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -956,6 +956,13 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
         if (*p == ',')
             p++;
         len = strtoull(p, NULL, 16);
+
+        /* memtohex() doubles the required space */
+        if (len > MAX_PACKET_LENGTH / 2) {
+            put_packet (s, "E22");
+            break;
+        }
+
         if (target_memory_rw_debug(s->g_cpu, addr, mem_buf, len, false) != 0) {
             put_packet (s, "E14");
         } else {
@@ -970,6 +977,12 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
         len = strtoull(p, (char **)&p, 16);
         if (*p == ':')
             p++;
+
+        /* hextomem() reads 2*len bytes */
+        if (len > strlen(p) / 2) {
+            put_packet (s, "E22");
+            break;
+        }
         hextomem(mem_buf, p, len);
         if (target_memory_rw_debug(s->g_cpu, addr, mem_buf, len,
                                    true) != 0) {
@@ -1107,7 +1120,8 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
             cpu = find_cpu(thread);
             if (cpu != NULL) {
                 cpu_synchronize_state(cpu);
-                len = snprintf((char *)mem_buf, sizeof(mem_buf),
+                /* memtohex() doubles the required space */
+                len = snprintf((char *)mem_buf, sizeof(buf) / 2,
                                "CPU#%d [%s]", cpu->cpu_index,
                                cpu->halted ? "halted " : "running");
                 memtohex(buf, mem_buf, len);
@@ -1136,8 +1150,8 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
                 put_packet(s, "E01");
                 break;
             }
-            hextomem(mem_buf, p + 5, len);
             len = len / 2;
+            hextomem(mem_buf, p + 5, len);
             mem_buf[len++] = 0;
             qemu_chr_be_write(s->mon_chr, mem_buf, len);
             put_packet(s, "OK");
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] gdbstub: Fix buffer overflows in gdb_handle_packet()
  2015-10-13  7:38 [Qemu-devel] [PATCH] gdbstub: Fix buffer overflows in gdb_handle_packet() Kevin Wolf
@ 2015-10-14  6:53 ` P J P
  2015-10-14  8:09   ` Kevin Wolf
  2015-10-29  7:30 ` Michael Tokarev
  1 sibling, 1 reply; 5+ messages in thread
From: P J P @ 2015-10-14  6:53 UTC (permalink / raw)
  To: Kevin Wolf; +Cc: qemu-trivial, qemu-devel, qemu-stable

+-- On Tue, 13 Oct 2015, Kevin Wolf wrote --+
| diff --git a/gdbstub.c b/gdbstub.c
| index d2c95b5..9c29aa0 100644
| --- a/gdbstub.c
| +++ b/gdbstub.c
| @@ -956,6 +956,13 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
|          if (*p == ',')
|              p++;
|          len = strtoull(p, NULL, 16);
| +
| +        /* memtohex() doubles the required space */
| +        if (len > MAX_PACKET_LENGTH / 2) {
| +            put_packet (s, "E22");
| +            break;
| +        }
| +
|          if (target_memory_rw_debug(s->g_cpu, addr, mem_buf, len, false) != 0) {
|              put_packet (s, "E14");
|          } else {
| @@ -970,6 +977,12 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
|          len = strtoull(p, (char **)&p, 16);
|          if (*p == ':')
|              p++;
| +
| +        /* hextomem() reads 2*len bytes */
| +        if (len > strlen(p) / 2) {
| +            put_packet (s, "E22");
| +            break;
| +        }
|          hextomem(mem_buf, p, len);
|          if (target_memory_rw_debug(s->g_cpu, addr, mem_buf, len,
|                                     true) != 0) {
| @@ -1107,7 +1120,8 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
|              cpu = find_cpu(thread);
|              if (cpu != NULL) {
|                  cpu_synchronize_state(cpu);
| -                len = snprintf((char *)mem_buf, sizeof(mem_buf),
| +                /* memtohex() doubles the required space */
| +                len = snprintf((char *)mem_buf, sizeof(buf) / 2,
|                                 "CPU#%d [%s]", cpu->cpu_index,
|                                 cpu->halted ? "halted " : "running");
|                  memtohex(buf, mem_buf, len);
| @@ -1136,8 +1150,8 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
|                  put_packet(s, "E01");
|                  break;
|              }
| -            hextomem(mem_buf, p + 5, len);
|              len = len / 2;
| +            hextomem(mem_buf, p + 5, len);
|              mem_buf[len++] = 0;
|              qemu_chr_be_write(s->mon_chr, mem_buf, len);
|              put_packet(s, "OK");
| 

Yes, patch looks good to fix the said buffer overflows.

Nevertheless, I wonder how hextomem() & memtohex() routines help? Because IIUC 
they seem to be changing command semantics. Ie host gdb(1) user would need to 
supply len/2 value to read/write 'len' bytes.

--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] gdbstub: Fix buffer overflows in gdb_handle_packet()
  2015-10-14  6:53 ` P J P
@ 2015-10-14  8:09   ` Kevin Wolf
  2015-10-14  9:24     ` P J P
  0 siblings, 1 reply; 5+ messages in thread
From: Kevin Wolf @ 2015-10-14  8:09 UTC (permalink / raw)
  To: P J P; +Cc: qemu-trivial, qemu-devel, qemu-stable

Am 14.10.2015 um 08:53 hat P J P geschrieben:
> +-- On Tue, 13 Oct 2015, Kevin Wolf wrote --+
> | diff --git a/gdbstub.c b/gdbstub.c
> | index d2c95b5..9c29aa0 100644
> | --- a/gdbstub.c
> | +++ b/gdbstub.c
> | @@ -956,6 +956,13 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
> |          if (*p == ',')
> |              p++;
> |          len = strtoull(p, NULL, 16);
> | +
> | +        /* memtohex() doubles the required space */
> | +        if (len > MAX_PACKET_LENGTH / 2) {
> | +            put_packet (s, "E22");
> | +            break;
> | +        }
> | +
> |          if (target_memory_rw_debug(s->g_cpu, addr, mem_buf, len, false) != 0) {
> |              put_packet (s, "E14");
> |          } else {
> | @@ -970,6 +977,12 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
> |          len = strtoull(p, (char **)&p, 16);
> |          if (*p == ':')
> |              p++;
> | +
> | +        /* hextomem() reads 2*len bytes */
> | +        if (len > strlen(p) / 2) {
> | +            put_packet (s, "E22");
> | +            break;
> | +        }
> |          hextomem(mem_buf, p, len);
> |          if (target_memory_rw_debug(s->g_cpu, addr, mem_buf, len,
> |                                     true) != 0) {
> | @@ -1107,7 +1120,8 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
> |              cpu = find_cpu(thread);
> |              if (cpu != NULL) {
> |                  cpu_synchronize_state(cpu);
> | -                len = snprintf((char *)mem_buf, sizeof(mem_buf),
> | +                /* memtohex() doubles the required space */
> | +                len = snprintf((char *)mem_buf, sizeof(buf) / 2,
> |                                 "CPU#%d [%s]", cpu->cpu_index,
> |                                 cpu->halted ? "halted " : "running");
> |                  memtohex(buf, mem_buf, len);
> | @@ -1136,8 +1150,8 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
> |                  put_packet(s, "E01");
> |                  break;
> |              }
> | -            hextomem(mem_buf, p + 5, len);
> |              len = len / 2;
> | +            hextomem(mem_buf, p + 5, len);
> |              mem_buf[len++] = 0;
> |              qemu_chr_be_write(s->mon_chr, mem_buf, len);
> |              put_packet(s, "OK");
> | 
> 
> Yes, patch looks good to fix the said buffer overflows.
> 
> Nevertheless, I wonder how hextomem() & memtohex() routines help? Because IIUC 
> they seem to be changing command semantics. Ie host gdb(1) user would need to 
> supply len/2 value to read/write 'len' bytes.

That's just how the gdb protocol works. Binary data is transferred as a
string of hex digits, with every byte being represented by two digits.
The requested length is in bytes of binary data, so the length of the
actually transferred data on the wire is indeed double that length.

Kevin

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] gdbstub: Fix buffer overflows in gdb_handle_packet()
  2015-10-14  8:09   ` Kevin Wolf
@ 2015-10-14  9:24     ` P J P
  0 siblings, 0 replies; 5+ messages in thread
From: P J P @ 2015-10-14  9:24 UTC (permalink / raw)
  To: Kevin Wolf; +Cc: qemu-trivial, qemu-devel, qemu-stable

+-- On Wed, 14 Oct 2015, Kevin Wolf wrote --+
| >  Ie host gdb(1) user would need to 
| > supply len/2 value to read/write 'len' bytes.
| 
| That's just how the gdb protocol works. Binary data is transferred as a
| string of hex digits, with every byte being represented by two digits.
| The requested length is in bytes of binary data, so the length of the
| actually transferred data on the wire is indeed double that length.

  I see, got it.

Thank you.
--
 - P J P
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] gdbstub: Fix buffer overflows in gdb_handle_packet()
  2015-10-13  7:38 [Qemu-devel] [PATCH] gdbstub: Fix buffer overflows in gdb_handle_packet() Kevin Wolf
  2015-10-14  6:53 ` P J P
@ 2015-10-29  7:30 ` Michael Tokarev
  1 sibling, 0 replies; 5+ messages in thread
From: Michael Tokarev @ 2015-10-29  7:30 UTC (permalink / raw)
  To: Kevin Wolf, qemu-devel; +Cc: qemu-trivial, qemu-stable

13.10.2015 10:38, Kevin Wolf wrote:
> Some places in gdb_handle_packet() can get an arbitrary length (most
> times directly from the client) and either didn't check it at all or
> checked against the wrong value, potentially causing buffer overflows.

Applied to -trivial, thank you!

/mjt

^ permalink raw reply	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-10-29  7:30 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-10-13  7:38 [Qemu-devel] [PATCH] gdbstub: Fix buffer overflows in gdb_handle_packet() Kevin Wolf
2015-10-14  6:53 ` P J P
2015-10-14  8:09   ` Kevin Wolf
2015-10-14  9:24     ` P J P
2015-10-29  7:30 ` Michael Tokarev

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.