Re: Check for truncated registers in process_g_packet

[Simon Marchi <simon.marchi@ericsson.com>]

On 16-10-18 07:10 AM, Lionel Flandrin wrote:
> Hello,
> 
> While investigating an unrelated issue in remote.c I noticed that the
> bound checking for 'g' packets was bogus:
> 
> The previous code would only check that the first byte of the register
> was within bounds before passing the buffer to regcache_raw_supply. If
> it turned out that the register in the 'g' packet was incomplete then
> regcache_raw_supply would proceed to memcpy out-of-bounds.
> 
> Since the buffer is allocated with alloca it's relatively unlikely to
> crash (you just end up dumping gdb's stack into the cache) but it's
> still a bit messy.
> 
> I changed this logic to check for truncated registers and raise an
> error if one is encountered. Hopefully it should make debugging remote
> stubs a bit easier.

Hi Lionel,

This patch looks good to me, a few minor comments below about formatting.
Someone else with the approval stamp must look at it, but hopefully it will
save them a bit of work.

> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 4b642b8..73b9b9e 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,9 @@
> +2016-10-18  Lionel Flandrin <lionel@svkt.org>

Missing space between name and email.

> +
> +	* remote.c (process_g_packet): Detect truncated registers in 'g'
> +	packets and raise an error. Fixes a potential out-of-bounds buffer
> +	access if the remote sent a truncated 'g' packet.
> +

The ChangeLog should only contain "what has changed", and not the "why".  In
your case, I think the first sentence would be sufficient:

+	* remote.c (process_g_packet): Detect truncated registers in 'g'
+	packets and raise an error.

If somebody wants to know why that was changed, they would go look at the commit
message, where you explained the issue in details.  It's easy to go from the
ChangeLog entry to the commit using git blame.

> @@ -7163,18 +7163,31 @@ process_g_packet (struct regcache *regcache)
>       the 'p' packet must be used.  */
>    if (buf_len < 2 * rsa->sizeof_g_packet)
>      {
> -      rsa->sizeof_g_packet = buf_len / 2;
> +      long sizeof_g_packet = buf_len / 2;
>  
>        for (i = 0; i < gdbarch_num_regs (gdbarch); i++)
>  	{
> +	  long offset = rsa->regs[i].offset;
> +	  long reg_size = register_size(gdbarch, i);

Missing space after "register_size".

> +      /* Looks valid enough, we can assume this is the correct length
> +         for a 'g' packet. It's important not to adjust
> +         rsa.sizeof_g_packet if we have truncated registers otherwise

rsa->sizeof_g_packet ?

> +         this "if" won't be run the next time the method is called
> +         with a packet of the same size and one of the internal errors
> +         below will trigger instead. */

Use two spaces after each period (including the final one, before the */.

> +      rsa->sizeof_g_packet = sizeof_g_packet;
>      }
>  
>    regs = (char *) alloca (rsa->sizeof_g_packet);
> @@ -7204,10 +7217,11 @@ process_g_packet (struct regcache *regcache)
>    for (i = 0; i < gdbarch_num_regs (gdbarch); i++)
>      {
>        struct packet_reg *r = &rsa->regs[i];
> +      long reg_size = register_size(gdbarch, i);

Space after register_size.

Thanks!

Simon