diff mbox

Limit memory r/w length to buffer size

Message ID alpine.LFD.2.20.1510130051540.19063@wniryva
State New
Headers show

Commit Message

Prasad Pandit Oct. 12, 2015, 7:31 p.m. UTC 
Hello,

An OOB r/w access issue was reported by Mr Gerben Lubbe(CC'd here).

The GDB(1) stub protocol supports commands 'm/M' to read & write 'len' bytes 
from/to the stub memory area. In that, the 'len' parameter value supplied by 
the host gdb(1) is not validated against the local buffer size. Which in turn 
could lead to OOB r/w memory access.

Below is a proposed patch to fix this issue.

Comments

Prasad Pandit Oct. 13, 2015, 6:29 p.m. UTC | #1 
Hello,

+-- On Tue, 13 Oct 2015, P J P wrote --+
| Below is a proposed patch to fix this issue.
| 
| ===
| > From 88edb457a66f8ff96209a1603914171eade0658b Mon Sep 17 00:00:00 2001
| From: Prasad J Pandit <pjp@fedoraproject.org>
| Date: Mon, 12 Oct 2015 22:56:41 +0530
| Subject: Limit memory r/w length to buffer size
| 
| GDB(1) stub communication protocol supports commands m/M to read
| and write 'len' bytes from/to the stub memory area.
| 
|    m addr,len        : read 'len' bytes from address 'addr'
|    M addr,len:<data> : write 'len' bytes of 'data' to 'addr'
| 
| Qemu stub uses automatic buffers of size 'MAX_PACKET_LENGTH=4096'
| to process these commands. Limit 'len' parameter value supplied
| by the host gdb(1) to the maximum buffer size to avoid any OOB
| buffer access.
| 
| Reported-by: Gerben van der Lubbe <spoofedexistence@gmail.com>
| Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
| ---
|  gdbstub.c | 2 ++
|  1 file changed, 2 insertions(+)

Could someone review it please?

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
Markus Armbruster Oct. 13, 2015, 6:58 p.m. UTC | #2 
P J P <ppandit@redhat.com> writes:

>    Hello,
>
> An OOB r/w access issue was reported by Mr Gerben Lubbe(CC'd here).
>
> The GDB(1) stub protocol supports commands 'm/M' to read & write 'len'
> bytes from/to the stub memory area. In that, the 'len' parameter value
> supplied by the host gdb(1) is not validated against the local buffer
> size. Which in turn could lead to OOB r/w memory access.
>
> Below is a proposed patch to fix this issue.

How is this related to Kevin's
[PATCH] gdbstub: Fix buffer overflows in gdb_handle_packet()
Message-Id: <1444721930-5121-1-git-send-email-kwolf@redhat.com>
?
Prasad Pandit Oct. 14, 2015, 6:27 a.m. UTC | #3 
+-- On Tue, 13 Oct 2015, Markus Armbruster wrote --+
| How is this related to Kevin's
| [PATCH] gdbstub: Fix buffer overflows in gdb_handle_packet()
| Message-Id: <1444721930-5121-1-git-send-email-kwolf@redhat.com> ?

  Oh, didn't know there was already a patch. Yes it fixes the same issues; 
Also the length check is correct. In my patch I did not consider the size 
alterations that occur in memtohex() & hextomem().

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

Patch

===
>From 88edb457a66f8ff96209a1603914171eade0658b Mon Sep 17 00:00:00 2001
From: Prasad J Pandit <pjp@fedoraproject.org>
Date: Mon, 12 Oct 2015 22:56:41 +0530
Subject: Limit memory r/w length to buffer size

GDB(1) stub communication protocol supports commands m/M to read
and write 'len' bytes from/to the stub memory area.

    m addr,len        : read 'len' bytes from address 'addr'
    M addr,len:<data> : write 'len' bytes of 'data' to 'addr'

Qemu stub uses automatic buffers of size 'MAX_PACKET_LENGTH=4096'
to process these commands. Limit 'len' parameter value supplied
by the host gdb(1) to the maximum buffer size to avoid any OOB
buffer access.

Reported-by: Gerben van der Lubbe <spoofedexistence@gmail.com>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
  gdbstub.c | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/gdbstub.c b/gdbstub.c
index ffe7e6e..39da736 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -956,6 +956,7 @@  static int gdb_handle_packet(GDBState *s, const char *line_buf)
          if (*p == ',')
              p++;
          len = strtoull(p, NULL, 16);
+        len = len > MAX_PACKET_LENGTH ? MAX_PACKET_LENGTH : len;
          if (target_memory_rw_debug(s->g_cpu, addr, mem_buf, len, false) != 0) {
              put_packet (s, "E14");
          } else {
@@ -968,6 +969,7 @@  static int gdb_handle_packet(GDBState *s, const char *line_buf)
          if (*p == ',')
              p++;
          len = strtoull(p, (char **)&p, 16);
+        len = len > MAX_PACKET_LENGTH ? MAX_PACKET_LENGTH : len;
          if (*p == ':')
              p++;
          hextomem(mem_buf, p, len);