[PATCH] Fix buffer overrun in fetch_register_using_p

[PATCH] Fix buffer overrun in fetch_register_using_p

[Richard Henderson]

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"));
 
       regp[i++] = fromhex (p[0]) * 16 + fromhex (p[1]);
       p += 2;
-- 
2.17.1

Re: [PATCH] Fix buffer overrun in fetch_register_using_p

[Simon Marchi]

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