From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 8499 invoked by alias); 23 Oct 2014 15:39:50 -0000 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 Received: (qmail 8488 invoked by uid 89); 23 Oct 2014 15:39:50 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.0 required=5.0 tests=AWL,BAYES_00,KAM_STOCKGEN autolearn=no version=3.3.2 X-HELO: rock.gnat.com Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Thu, 23 Oct 2014 15:39:48 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id DCF3B1162C9; Thu, 23 Oct 2014 11:39:46 -0400 (EDT) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id B2HdrePsABec; Thu, 23 Oct 2014 11:39:46 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id AEE2D1162C1; Thu, 23 Oct 2014 11:39:46 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id 3E7CF4113A; Thu, 23 Oct 2014 08:39:47 -0700 (PDT) Date: Thu, 23 Oct 2014 15:39:00 -0000 From: Joel Brobecker To: Yao Qi Cc: gdb-patches@sourceware.org Subject: [RFA] ARM: stricter __stack_chk_guard check during prologue (was: "Re: over-permissive stack_chk_guard on ARM") Message-ID: <20141023153947.GA11707@adacore.com> References: <20141022142231.GF4786@adacore.com> <87y4s7h553.fsf@codesourcery.com> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="/9DWx/yDrRhgMJTb" Content-Disposition: inline In-Reply-To: <87y4s7h553.fsf@codesourcery.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-SW-Source: 2014-10/txt/msg00605.txt.bz2 --/9DWx/yDrRhgMJTb Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-length: 2223 > > /* Step 2: ldr Rd, [Rn, #immed], encoding T1. */ > > /* Step 3: str Rd, [Rn, #immed], encoding T1. */ > > > > Looking at the code and the function description, it seems to me > > that the normal situation would be what the comment alluded to, > > and that if it was the entire story, we wouldn't have needed > > the code doing steps 2 & 3. But, looking at the email archives > > Sorry, I don't understand why do you think steps 2 & 3 are not needed? > Do you mean we don't have to go to step 2 & 3 if we can't find symbol > __stack_chk_guard in step 1? I see what you mean. I misread the code that it would stop if the address was pointing to the __stack_chk_guard symbol. But in reality, it stops if it is pointing to a symbol that is NOT __stack_chk_guard. > > as well as the bug report initially referenced, I can't find > > really any explanation for what prompted you to add that code. > > I would need that in order to adjust the heuristics without > > breaking your situation. > > Currently, we do so in order to handle the case symbol __stack_chk_guard > is removed, as the comments said: > > /* If name of symbol doesn't start with '__stack_chk_guard', this > instruction sequence is not for stack protector. If symbol is > removed, we conservatively think this sequence is for stack > protector. */ > > However, I don't recall under what circumstance symbol > '__stack_chk_guard' can be removed. __stack_chk_guard is in .dynsym > section, so it can't be removed. (I presume symbols in .dynsym can't be > removed, correct me if I am wrong). If I am correct, we can restrict > the condition in step 1 that return early if the symbol name doesn't > start with '__stack_chk_guard'. Then, step 2 & 3 is not needed, or we > can keep them as a sanity check? For heuristics, I would keep the heuristics as strict as possible to avoid possible incorrect matches, so I would keep it. What do you think of the attached patch? gdb/ChangeLog: arm-tdep.c (arm_skip_stack_protector): Return early if address loaded by first "ldr" instruction does not have a corresponding minimal symbol. Tested on arm-eabi using AdaCore's testsuite. Thanks, -- Joel --/9DWx/yDrRhgMJTb Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="0001-ARM-stricter-__stack_chk_guard-check-during-prologue.patch" Content-length: 3196 >From 6dbcc17c72575438c4c6a41c1d10938ad1a08090 Mon Sep 17 00:00:00 2001 From: Joel Brobecker Date: Thu, 23 Oct 2014 08:25:20 -0700 Subject: [PATCH] ARM: stricter __stack_chk_guard check during prologue analysis We are trying to insert a breakpoint on line 4 for the following Ada code. 3 procedure STR is 4 XX : String (1 .. Blocks.Sz) := (others => 'X'); -- STOP 5 K : Integer; 6 begin 7 K := 13; The code generated on ARM (-march=armv7-m) starts like this: (gdb) disass str'address Dump of assembler code for function _ada_str: --# Line str.adb:3 0x08000014 <+0>: push {r4, r7, lr} 0x08000016 <+2>: sub sp, #28 0x08000018 <+4>: add r7, sp, #0 0x0800001a <+6>: mov r3, sp 0x0800001c <+8>: mov r4, r3 --# Line str.adb:4 0x0800001e <+10>: ldr r3, [pc, #84] ; (0x8000074 <_ada_str+96>) 0x08000020 <+12>: ldr r3, [r3, #0] 0x08000022 <+14>: str r3, [r7, #20] 0x08000024 <+16>: ldr r3, [r7, #20] [...] When computing the address related to str.adb:4, GDB correctly resolves it to 0x0800001e first, but then considers the next 3 instructions as being part of the prologue because it thinks they are part of stack-protector code. As a result, instead of inserting the breakpoint at line 4, it skips those instruction and consequently the rest of the instructions until the next line start, which his line 7. The stack-protector code is expected to start like this... ldr Rn, .Label .... .Lable: .word __stack_chk_guard ... but the implementation actually accepts a sequence where the ldr location points to an address for which there is no symbol. It only aborts if the address points to a symbol which is not __stack_chk_guard. Since the __stack_chk_guard symbol is always expected to exist when used (it lives in .dynsym), this patch fixes the issue by requiring that the ldr gets the address of the __stack_chk_guard symbol. If the address could not be resolved, then it rejects the sequence as being stack-protector code. gdb/ChangeLog: arm-tdep.c (arm_skip_stack_protector): Return early if address loaded by first "ldr" instruction does not have a corresponding minimal symbol. Tested on arm-eabi using AdaCore's testsuite. --- gdb/arm-tdep.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c index e2559ec..c7f7d97 100644 --- a/gdb/arm-tdep.c +++ b/gdb/arm-tdep.c @@ -1309,8 +1309,8 @@ arm_skip_stack_protector(CORE_ADDR pc, struct gdbarch *gdbarch) /* If name of symbol doesn't start with '__stack_chk_guard', this instruction sequence is not for stack protector. If symbol is removed, we conservatively think this sequence is for stack protector. */ - if (stack_chk_guard.minsym - && strncmp (MSYMBOL_LINKAGE_NAME (stack_chk_guard.minsym), + if (stack_chk_guard.minsym == NULL + || strncmp (MSYMBOL_LINKAGE_NAME (stack_chk_guard.minsym), "__stack_chk_guard", strlen ("__stack_chk_guard")) != 0) return pc; -- 1.9.1 --/9DWx/yDrRhgMJTb--