From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 24673 invoked by alias); 6 Mar 2013 19:07:28 -0000 Received: (qmail 24660 invoked by uid 22791); 6 Mar 2013 19:07:27 -0000 X-SWARE-Spam-Status: No, hits=-7.9 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_SPAMHAUS_DROP,KHOP_THREADED,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,RP_MATCHES_RCVD,SPF_HELO_PASS,TW_DW X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 06 Mar 2013 19:06:43 +0000 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r26J6dlD017618 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Wed, 6 Mar 2013 14:06:39 -0500 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id r26J6b5h029441; Wed, 6 Mar 2013 14:06:38 -0500 Message-ID: <513793BD.2090804@redhat.com> Date: Wed, 06 Mar 2013 19:07:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130219 Thunderbird/17.0.3 MIME-Version: 1.0 To: jeremy.bennett@embecosm.com CC: gdb-patches@sourceware.org Subject: Re: [PATCH PR gdb/15236] gdbserver write to linux memory with zero length corrupts stack References: <1362593035.2235.57.camel@laria> In-Reply-To: <1362593035.2235.57.camel@laria> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2013-03/txt/msg00225.txt.bz2 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 > > PR gdb/15236 > * linux-low.c (linux_write_memory): Return early success if len is > zero. > > +2013-03-06 Jeremy Bennett > + > + 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 > > * 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