Fix buffer overrun in fetch_register_using_p

Message ID 20181010154553.11515-1-richard.henderson@linaro.org
State New
Headers show
Series
  • Fix buffer overrun in fetch_register_using_p
Related show

Commit Message

Richard Henderson Oct. 10, 2018, 3:45 p.m.
If the packet returned from the gdbserver is too long,
the stack would be clobbered and gdb would crash.

gdb/
	* remote.c (remote_target::fetch_register_using_p): Error if
	more data is received than expected in the packet.
---

I am adding SVE support to QEMU's gdbserver stub, and managed to
tickle this bug in the process.


r~

---
 gdb/remote.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

-- 
2.17.1

Comments

Simon Marchi Oct. 10, 2018, 8:38 p.m. | #1
On 2018-10-10 11:45, Richard Henderson wrote:
> If the packet returned from the gdbserver is too long,

> the stack would be clobbered and gdb would crash.

> 

> gdb/

> 	* remote.c (remote_target::fetch_register_using_p): Error if

> 	more data is received than expected in the packet.

> ---

> 

> I am adding SVE support to QEMU's gdbserver stub, and managed to

> tickle this bug in the process.

> 

> 

> r~

> 

> ---

>  gdb/remote.c | 5 ++++-

>  1 file changed, 4 insertions(+), 1 deletion(-)

> 

> diff --git a/gdb/remote.c b/gdb/remote.c

> index 724f41cf71..d68faf1046 100644

> --- a/gdb/remote.c

> +++ b/gdb/remote.c

> @@ -7958,7 +7958,8 @@ remote_target::fetch_register_using_p (struct

> regcache *regcache,

>    struct gdbarch *gdbarch = regcache->arch ();

>    struct remote_state *rs = get_remote_state ();

>    char *buf, *p;

> -  gdb_byte *regp = (gdb_byte *) alloca (register_size (gdbarch, 

> reg->regnum));

> +  int size = register_size (gdbarch, reg->regnum);

> +  gdb_byte *regp = (gdb_byte *) alloca (size);

>    int i;

> 

>    if (packet_support (PACKET_p) == PACKET_DISABLE)

> @@ -8003,6 +8004,8 @@ remote_target::fetch_register_using_p (struct

> regcache *regcache,

>      {

>        if (p[1] == 0)

>  	error (_("fetch_register_using_p: early buf termination"));

> +      if (i == size)

> +	error (_("fetch_register_using_p: late buf termination"));


Hi Richard,

As a user, I don't think I would understand "late buf termination".  I 
think it could be more straightforward, like "the received value is 
larger than the register size".

Otherwise, this is OK, thanks for this.

Simon

Patch

diff --git a/gdb/remote.c b/gdb/remote.c
index 724f41cf71..d68faf1046 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -7958,7 +7958,8 @@  remote_target::fetch_register_using_p (struct regcache *regcache,
   struct gdbarch *gdbarch = regcache->arch ();
   struct remote_state *rs = get_remote_state ();
   char *buf, *p;
-  gdb_byte *regp = (gdb_byte *) alloca (register_size (gdbarch, reg->regnum));
+  int size = register_size (gdbarch, reg->regnum);
+  gdb_byte *regp = (gdb_byte *) alloca (size);
   int i;
 
   if (packet_support (PACKET_p) == PACKET_DISABLE)
@@ -8003,6 +8004,8 @@  remote_target::fetch_register_using_p (struct regcache *regcache,
     {
       if (p[1] == 0)
 	error (_("fetch_register_using_p: early buf termination"));
+      if (i == size)
+	error (_("fetch_register_using_p: late buf termination"));
 
       regp[i++] = fromhex (p[0]) * 16 + fromhex (p[1]);
       p += 2;