[PATCH PR gdb/15236] gdbserver write to linux memory with zero length corrupts stack

[PATCH PR gdb/15236] gdbserver write to linux memory with zero length corrupts stack

[Jeremy Bennett]

PROBLEM:

The function linux_write_memory () in linux-low.c allocates a buffer on
the stack to hold a copy of the data to be written.

  register PTRACE_XFER_TYPE *buffer = (PTRACE_XFER_TYPE *)
    alloca (count * sizeof (PTRACE_XFER_TYPE));

"count" is the number of bytes to be written, rounded up to the nearest
multiple of sizeof (PTRACE_XFER_TYPE) and allowing for not being an
aligned address. The function later uses 

  buffer[0] = ptrace (PTRACE_PEEKTEXT, pid,
                      (PTRACE_ARG3_TYPE) (uintptr_t) addr, 0);

The problem is that this function can be called to write zero bytes on
an aligned address, for example when receiving an X packet of length 0
(used to test if 8-bit write is supported). Under these circumstances,
count can be zero.

Since in this case, buffer[0] may never have been allocated, the stack
is corrupted and gdbserver may crash.

Demonstrated with the port of GDB 7.5.1 for the Synopsys
arc-linux-uclibc- target, currently under development at:

        https://github.com/foss-for-synopsys-dwc-arc-processors/gdb

(to be submitted to the FSF in due course).

SOLUTION:

Writing zero bytes should always succeed. The patch below returns
successfully early if the length is zero, so avoiding the stack
corruption.

Verified on the ARC GDB 7.5.1 port.

CHANGELOG ENTRY:

2013-03-06  Jeremy Bennett  <jeremy.bennett@embecosm.com> 

	PR gdb/15236
	* linux-low.c (linux_write_memory): Return early success if len is
	zero.

PATCH:

I'm working from a git mirror and have produced this using git diff.
Advice on how to do this properly appreciated!

diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
index 7b79cd1..450e2db 100644
--- a/gdb/gdbserver/ChangeLog
+++ b/gdb/gdbserver/ChangeLog
@@ -1,3 +1,9 @@
+2013-03-06  Jeremy Bennett  <jeremy.bennett@embecosm.com>
+
+	PR gdb/15236
+	* linux-low.c (linux_write_memory): Return early success if len is
+	zero.
+
 2012-04-29  Yao Qi  <yao@codesourcery.com>
 
 	* server.h: Move some code to ...
diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index bbb0693..8e576bd 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -4421,7 +4421,14 @@ linux_read_memory (CORE_ADDR memaddr, unsigned char *myaddr, int len)
 
 /* Copy LEN bytes of data from debugger memory at MYADDR to inferior's
    memory at MEMADDR.  On failure (cannot write to the inferior)
-   returns the value of errno.  */
+   returns the value of errno.
+
+   6-Mar-13, Jeremy Bennett: [PR gdb/15236] This function can be called with
+   length 0 (for example with a zero length X packet). If memaddr is aligned
+   to sizeof (PTRACE_XFER_TYPE), then count will be zero and nothing may be
+   allocated for buffer (architecture dependent). The function must return
+   early in this circumstance, to avoid stack corruption when assigning
+   to buffer[0]. */
 
 static int
 linux_write_memory (CORE_ADDR memaddr, const unsigned char *myaddr, int len)
@@ -4440,6 +4447,10 @@ linux_write_memory (CORE_ADDR memaddr, const unsigned char *myaddr, int len)
 
   int pid = lwpid_of (get_thread_lwp (current_inferior));
 
+  if (0 == len) {
+    return 0;			/* Zero length write always succeeds. */
+  }
+
   if (debug_threads)
     {
       /* Dump up to four bytes.  */


Best wishes,

Jeremy

-- 
Tel:      +44 (1590) 610184
Cell:     +44 (7970) 676050
SkypeID: jeremybennett
Email:   jeremy.bennett@embecosm.com
Web:     www.embecosm.com

Re: [PATCH PR gdb/15236] gdbserver write to linux memory with zero length corrupts stack

[Pedro Alves]

Hi Jeremy,

Thanks for the diagnosis and the patch.

On 03/06/2013 06:03 PM, Jeremy Bennett wrote:
> PROBLEM:
> 
> The function linux_write_memory () in linux-low.c allocates a buffer on
> the stack to hold a copy of the data to be written.
> 
>   register PTRACE_XFER_TYPE *buffer = (PTRACE_XFER_TYPE *)
>     alloca (count * sizeof (PTRACE_XFER_TYPE));
> 
> "count" is the number of bytes to be written, rounded up to the nearest
> multiple of sizeof (PTRACE_XFER_TYPE) and allowing for not being an
> aligned address. The function later uses 
> 
>   buffer[0] = ptrace (PTRACE_PEEKTEXT, pid,
>                       (PTRACE_ARG3_TYPE) (uintptr_t) addr, 0);
> 
> The problem is that this function can be called to write zero bytes on
> an aligned address, for example when receiving an X packet of length 0
> (used to test if 8-bit write is supported). Under these circumstances,
> count can be zero.
> 
> Since in this case, buffer[0] may never have been allocated, the stack
> is corrupted and gdbserver may crash.
> 
> Demonstrated with the port of GDB 7.5.1 for the Synopsys
> arc-linux-uclibc- target, currently under development at:
> 
>         https://github.com/foss-for-synopsys-dwc-arc-processors/gdb
> 
> (to be submitted to the FSF in due course).
> 
> SOLUTION:
> 
> Writing zero bytes should always succeed. The patch below returns
> successfully early if the length is zero, so avoiding the stack
> corruption.
> 
> Verified on the ARC GDB 7.5.1 port.
> 
> CHANGELOG ENTRY:
> 
> 2013-03-06  Jeremy Bennett  <jeremy.bennett@embecosm.com> 
> 
> 	PR gdb/15236
> 	* linux-low.c (linux_write_memory): Return early success if len is
> 	zero.
> 

> +2013-03-06  Jeremy Bennett  <jeremy.bennett@embecosm.com>
> +
> +	PR gdb/15236
> +	* linux-low.c (linux_write_memory): Return early success if len is
> +	zero.

All caps when talking about the value of a variable.  So, if LEN is zero.

> +
>  2012-04-29  Yao Qi  <yao@codesourcery.com>
>  
>  	* server.h: Move some code to ...
> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
> index bbb0693..8e576bd 100644
> --- a/gdb/gdbserver/linux-low.c
> +++ b/gdb/gdbserver/linux-low.c
> @@ -4421,7 +4421,14 @@ linux_read_memory (CORE_ADDR memaddr, unsigned char *myaddr, int len)
>  
>  /* Copy LEN bytes of data from debugger memory at MYADDR to inferior's
>     memory at MEMADDR.  On failure (cannot write to the inferior)
> -   returns the value of errno.  */
> +   returns the value of errno.
> +
> +   6-Mar-13, Jeremy Bennett: [PR gdb/15236] This function can be called with
> +   length 0 (for example with a zero length X packet). If memaddr is aligned
> +   to sizeof (PTRACE_XFER_TYPE), then count will be zero and nothing may be
> +   allocated for buffer (architecture dependent). The function must return
> +   early in this circumstance, to avoid stack corruption when assigning
> +   to buffer[0]. */

I appreciate the thorough description of the issue.

But, that's really a too long comment in the function header, the place
people look at the learn about the function's _interface_, on a subject that
really is not an detail of the function's external interface.  Comments
on implementation details should go within the function body.  The date/name/PR
number are just unnecessary.  In fact, I'd rather just remove the whole
comment -- it's useful for the patch description, but otherwise, in the
code, to the reader, I think it distracts more than it adds value.

Also, double-space after periods.

>  
>  static int
>  linux_write_memory (CORE_ADDR memaddr, const unsigned char *myaddr, int len)
> @@ -4440,6 +4447,10 @@ linux_write_memory (CORE_ADDR memaddr, const unsigned char *myaddr, int len)
>  
>    int pid = lwpid_of (get_thread_lwp (current_inferior));
>  
> +  if (0 == len) {

We don't use that style of putting the constant on the lhs
in GDB.

Opening { goes on new line.

> +    return 0;			/* Zero length write always succeeds. */
> +  }
> +

Single-line statements in if blocks don't get wrapped with {}'s.

Comment is put above the return instead of on the side (we do
have a few back sheep).  The comment then makes the if block more
than one line, so, the {}'s remain.  IOW, write as:

  if (len == 0)
    {
      /* Zero length write always succeeds. */
      return 0;			
    }

OK with those changes.

Thanks,
-- 
Pedro Alves

Re: [PATCH PR gdb/15236] gdbserver write to linux memory with zero length corrupts stack

[Jeremy Bennett]

On Wed, 2013-03-06 at 19:06 +0000, Pedro Alves wrote: 
> Hi Jeremy,
> 
> Thanks for the diagnosis and the patch.

Pedro, thanks for the comments. I've revised the patch accordingly.
Since I don't have write permission, would you commit it for me. Here is
the corrected ChangeLog entry:

2013-03-07  Jeremy Bennett  <jeremy.bennett@embecosm.com>

	PR gdb/15236
	* linux-low.c (linux_write_memory): Return early success if LEN is
	zero.

Here is the corrected patch:

diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
index a8cf78c..d7202fd 100644
--- a/gdb/gdbserver/ChangeLog
+++ b/gdb/gdbserver/ChangeLog
@@ -1,3 +1,9 @@
+2013-03-07  Jeremy Bennett  <jeremy.bennett@embecosm.com>
+
+	PR gdb/15236
+	* linux-low.c (linux_write_memory): Return early success if LEN is
+	zero.
+
 2013-03-05  Corinna Vinschen  <vinschen@redhat.de>
 
 	* configure.srv: Add x86_64-*-cygwin* as target.
diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index c52cd2e..8eb5dac 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -4481,7 +4481,7 @@ linux_read_memory (CORE_ADDR memaddr, unsigned char *myaddr, int len)
 
 /* Copy LEN bytes of data from debugger memory at MYADDR to inferior's
    memory at MEMADDR.  On failure (cannot write to the inferior)
-   returns the value of errno.  */
+   returns the value of errno.  Succeeds immediately if LEN is zero. */
 
 static int
 linux_write_memory (CORE_ADDR memaddr, const unsigned char *myaddr, int len)
@@ -4500,6 +4500,12 @@ linux_write_memory (CORE_ADDR memaddr, const unsigned char *myaddr, int len)
 
   int pid = lwpid_of (get_thread_lwp (current_inferior));
 
+  if (len == 0)
+    {
+      /* Zero length write always succeeds. */
+      return 0;
+    }
+
   if (debug_threads)
     {
       /* Dump up to four bytes.  */

Best wishes,


Jeremy

-- 
Tel:      +44 (1590) 610184
Cell:     +44 (7970) 676050
SkypeID: jeremybennett
Email:   jeremy.bennett@embecosm.com
Web:     www.embecosm.com

Re: [PATCH PR gdb/15236] gdbserver write to linux memory with zero length corrupts stack

[Yao Qi]

On 03/07/2013 04:52 PM, Jeremy Bennett wrote:
> 2013-03-07  Jeremy Bennett<jeremy.bennett@embecosm.com>
>
> 	PR gdb/15236
            ^^^ It should be "server".
> 	* linux-low.c (linux_write_memory): Return early success if LEN is
> 	zero.


-- 
Yao (齐尧)

Re: [PATCH PR gdb/15236] gdbserver write to linux memory with zero length corrupts stack

[Jeremy Bennett]

On Thu, 2013-03-07 at 17:06 +0800, Yao Qi wrote: 
> On 03/07/2013 04:52 PM, Jeremy Bennett wrote:
> > 2013-03-07  Jeremy Bennett<jeremy.bennett@embecosm.com>
> >
> > 	PR gdb/15236
>             ^^^ It should be "server".
> > 	* linux-low.c (linux_write_memory): Return early success if LEN is
> > 	zero.

Hi Yao,

My misunderstanding. Revised ChangeLog entry:

2013-03-07  Jeremy Bennett  <jeremy.bennett@embecosm.com>

	PR server/15236
	* linux-low.c (linux_write_memory): Return early success if LEN is
	zero.

and patch:

diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
index a8cf78c..67bc149 100644
--- a/gdb/gdbserver/ChangeLog
+++ b/gdb/gdbserver/ChangeLog
@@ -1,3 +1,9 @@
+2013-03-07  Jeremy Bennett  <jeremy.bennett@embecosm.com>
+
+	PR server/15236
+	* linux-low.c (linux_write_memory): Return early success if LEN is
+	zero.
+
 2013-03-05  Corinna Vinschen  <vinschen@redhat.de>
 
 	* configure.srv: Add x86_64-*-cygwin* as target.
diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index c52cd2e..8eb5dac 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -4481,7 +4481,7 @@ linux_read_memory (CORE_ADDR memaddr, unsigned char *myaddr, int len)
 
 /* Copy LEN bytes of data from debugger memory at MYADDR to inferior's
    memory at MEMADDR.  On failure (cannot write to the inferior)
-   returns the value of errno.  */
+   returns the value of errno.  Succeeds immediately if LEN is zero. */
 
 static int
 linux_write_memory (CORE_ADDR memaddr, const unsigned char *myaddr, int len)
@@ -4500,6 +4500,12 @@ linux_write_memory (CORE_ADDR memaddr, const unsigned char *myaddr, int len)
 
   int pid = lwpid_of (get_thread_lwp (current_inferior));
 
+  if (len == 0)
+    {
+      /* Zero length write always succeeds. */
+      return 0;
+    }
+
   if (debug_threads)
     {
       /* Dump up to four bytes.  */

Best wishes,


Jeremy

-- 
Tel:      +44 (1590) 610184
Cell:     +44 (7970) 676050
SkypeID: jeremybennett
Email:   jeremy.bennett@embecosm.com
Web:     www.embecosm.com

Re: [PATCH PR gdb/15236] gdbserver write to linux memory with zero length corrupts stack

[Pedro Alves]

On 03/07/2013 09:12 AM, Jeremy Bennett wrote:
> On Thu, 2013-03-07 at 17:06 +0800, Yao Qi wrote: 
>> On 03/07/2013 04:52 PM, Jeremy Bennett wrote:
>>> 2013-03-07  Jeremy Bennett<jeremy.bennett@embecosm.com>
>>>
>>> 	PR gdb/15236
>>             ^^^ It should be "server".
>>> 	* linux-low.c (linux_write_memory): Return early success if LEN is
>>> 	zero.
> 
> Hi Yao,
> 
> My misunderstanding. Revised ChangeLog entry:
> 
> 2013-03-07  Jeremy Bennett  <jeremy.bennett@embecosm.com>
> 
> 	PR server/15236
> 	* linux-low.c (linux_write_memory): Return early success if LEN is
> 	zero.

Thanks.  Here's what I've applied, with a minor tweak (double-spaces
after periods; s/immediately/always/).

-- >8 --
PR gdb/15236: gdbserver write to linux memory with zero length corrupts stack

PROBLEM:

The function linux_write_memory () in linux-low.c allocates a buffer
on the stack to hold a copy of the data to be written.

  register PTRACE_XFER_TYPE *buffer = (PTRACE_XFER_TYPE *)
    alloca (count * sizeof (PTRACE_XFER_TYPE));

"count" is the number of bytes to be written, rounded up to the
nearest multiple of sizeof (PTRACE_XFER_TYPE) and allowing for not
being an aligned address. The function later uses

  buffer[0] = ptrace (PTRACE_PEEKTEXT, pid,
                      (PTRACE_ARG3_TYPE) (uintptr_t) addr, 0);

The problem is that this function can be called to write zero bytes on
an aligned address, for example when receiving an X packet of length 0
(used to test if 8-bit write is supported). Under these circumstances,
count can be zero.

Since in this case, buffer[0] may never have been allocated, the stack
is corrupted and gdbserver may crash.

SOLUTION:

Writing zero bytes should always succeed. The patch below returns
successfully early if the length is zero, so avoiding the stack
corruption.

Verified on the ARC GDB 7.5.1 port.

2013-03-07  Jeremy Bennett  <jeremy.bennett@embecosm.com>

	PR server/15236
	* linux-low.c (linux_write_memory): Return early success if LEN is
	zero.
---
 gdb/gdbserver/ChangeLog   | 6 ++++++
 gdb/gdbserver/linux-low.c | 8 +++++++-
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
index a8cf78c..67bc149 100644
--- a/gdb/gdbserver/ChangeLog
+++ b/gdb/gdbserver/ChangeLog
@@ -1,3 +1,9 @@
+2013-03-07  Jeremy Bennett  <jeremy.bennett@embecosm.com>
+
+	PR server/15236
+	* linux-low.c (linux_write_memory): Return early success if LEN is
+	zero.
+
 2013-03-05  Corinna Vinschen  <vinschen@redhat.de>
 
 	* configure.srv: Add x86_64-*-cygwin* as target.
diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index c52cd2e..5f03628 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -4481,7 +4481,7 @@ linux_read_memory (CORE_ADDR memaddr, unsigned char *myaddr, int len)
 
 /* Copy LEN bytes of data from debugger memory at MYADDR to inferior's
    memory at MEMADDR.  On failure (cannot write to the inferior)
-   returns the value of errno.  */
+   returns the value of errno.  Always succeeds if LEN is zero.  */
 
 static int
 linux_write_memory (CORE_ADDR memaddr, const unsigned char *myaddr, int len)
@@ -4500,6 +4500,12 @@ linux_write_memory (CORE_ADDR memaddr, const unsigned char *myaddr, int len)
 
   int pid = lwpid_of (get_thread_lwp (current_inferior));
 
+  if (len == 0)
+    {
+      /* Zero length write always succeeds.  */
+      return 0;
+    }
+
   if (debug_threads)
     {
       /* Dump up to four bytes.  */
-- 
1.7.11.7