hppa: avoid NULL dereference of sym_map in elf_machine_rela()

Message ID BLU436-SMTP91D615856A23994E55223E979A0@phx.gbl
State Not applicable
Headers

Commit Message

John David Anglin Oct. 31, 2014, 11:40 p.m. UTC
  On 31-Oct-14, at 6:38 PM, Aaro Koskinen wrote:

>> Rather, the right solution is probably to find (or add) some switch  
>> to GCC
>> that changes what it emits from being a call to abort to be something
>> different.  It's possible that abort is OK and we just need to  
>> define an
>> appropriately minimal abort in rtld.  But it needs to be looked into.
>
> The other quick workaround I could find is -fno-delete-null-pointer- 
> checks.
>
> "[...] other optimization passes in GCC use this flag to control  
> global
> dataflow analyses that eliminate useless checks for null pointers  
> [...]"

This suggests that GCC deletes a null check that is useful.

The attached patch implements a trap insn and __builtin_trap().  It  
generates
a conditional trap (SIGFPE).  Carlos has used another instruction that  
generates
an illegal instruction trap.

Dave
--
John David Anglin	dave.anglin@bell.net
  

Comments

Roland McGrath Nov. 1, 2014, 2:35 a.m. UTC | #1
Oh, yeah.  abort is the generic fallback for __builtin_trap.  Certainly
just supporting __builtin_trap directly in a compiler backend (with no
outcall) is best.  But this issue might come up in other places or affect
other machines, and the set of machines and compilers we support for
building libc today most likely includes others that don't have a proper
__builtin_trap.  

So we might as well fix abort in rtld.  We already have __assert_fail in
elf/dl-minimal.c for similar issues with enabling assert in rtld.  We could
just add an abort there.  It would be ideal to avoid any actual abort calls
in our own source code sneaking into rtld, because it's far better if
they're _dl_fatal_printf so there's a message.  But I can't think of a
reasonable way to make compiler-generated abort calls link while preventing
explicit abort calls in the source from linking, so we can live without
that ideal.
  
Rich Felker Nov. 1, 2014, 4:02 a.m. UTC | #2
On Fri, Oct 31, 2014 at 07:35:59PM -0700, Roland McGrath wrote:
> Oh, yeah.  abort is the generic fallback for __builtin_trap.  Certainly
> just supporting __builtin_trap directly in a compiler backend (with no
> outcall) is best.  But this issue might come up in other places or affect
> other machines, and the set of machines and compilers we support for
> building libc today most likely includes others that don't have a proper
> __builtin_trap.  

While glibc has to work around it for the time being (either by not
using __builtin_trap, or by providing abort here), I think it would be
nice to press for fixing this on the compiler side. There's no good
reason for the compiler to generate a call to abort when every ISA
I've ever seen has HCF-type instructions that can be used directly
with no dependency on a function call. Aside from the ldso issues,
__builtin_trap or similar compiler-generated traps might be used in
places where the call is impossible, like handling a fatal condition
where the GOT is corrupt, where the TCB is corrupt (in which case
abort can't properly raise signals for the calling thread), or where
the function pointer used to make syscalls (via vdso on x86, for
instance) is corrupt. In the worst case, this corruption was caused by
an attacker who has happily put the address of shellcode at one of the
above places.

Rich
  
Roland McGrath Nov. 3, 2014, 6:10 p.m. UTC | #3
> While glibc has to work around it for the time being (either by not
> using __builtin_trap, or by providing abort here), I think it would be
> nice to press for fixing this on the compiler side. There's no good
> reason for the compiler to generate a call to abort when every ISA
> I've ever seen has HCF-type instructions that can be used directly
> with no dependency on a function call. Aside from the ldso issues,
> __builtin_trap or similar compiler-generated traps might be used in
> places where the call is impossible, like handling a fatal condition
> where the GOT is corrupt, where the TCB is corrupt (in which case
> abort can't properly raise signals for the calling thread), or where
> the function pointer used to make syscalls (via vdso on x86, for
> instance) is corrupt. In the worst case, this corruption was caused by
> an attacker who has happily put the address of shellcode at one of the
> above places.

It's very straightforward to define it for any machine that lacks it.  I
haven't actually done a survey of existing GCC backends.  You might want to
do that and report bugs to individual backends.
  

Patch

Index: config/pa/pa.md
===================================================================
--- config/pa/pa.md	(revision 216987)
+++ config/pa/pa.md	(working copy)
@@ -123,7 +123,7 @@ 
 ;; type "binary" insns have two input operands (1,2) and one output (0)
 
 (define_attr "type"
-  "move,unary,binary,shift,nullshift,compare,load,store,uncond_branch,branch,cbranch,fbranch,call,sibcall,dyncall,fpload,fpstore,fpalu,fpcc,fpmulsgl,fpmuldbl,fpdivsgl,fpdivdbl,fpsqrtsgl,fpsqrtdbl,multi,milli,sh_func_adrs,parallel_branch,fpstore_load,store_fpload"
+  "move,unary,binary,shift,nullshift,compare,load,store,uncond_branch,branch,cbranch,fbranch,call,sibcall,dyncall,fpload,fpstore,fpalu,fpcc,fpmulsgl,fpmuldbl,fpdivsgl,fpdivdbl,fpsqrtsgl,fpsqrtdbl,multi,milli,sh_func_adrs,parallel_branch,fpstore_load,store_fpload,trap"
   (const_string "binary"))
 
 (define_attr "pa_combine_type"
@@ -175,7 +175,7 @@ 
 ;; Disallow instructions which use the FPU since they will tie up the FPU
 ;; even if the instruction is nullified.
 (define_attr "in_nullified_branch_delay" "false,true"
-  (if_then_else (and (eq_attr "type" "!uncond_branch,branch,cbranch,fbranch,call,sibcall,dyncall,multi,milli,sh_func_adrs,fpcc,fpalu,fpmulsgl,fpmuldbl,fpdivsgl,fpdivdbl,fpsqrtsgl,fpsqrtdbl,parallel_branch")
+  (if_then_else (and (eq_attr "type" "!uncond_branch,branch,cbranch,fbranch,call,sibcall,dyncall,multi,milli,sh_func_adrs,fpcc,fpalu,fpmulsgl,fpmuldbl,fpdivsgl,fpdivdbl,fpsqrtsgl,fpsqrtdbl,parallel_branch,trap")
 		     (eq_attr "length" "4")
 		     (not (match_test "RTX_FRAME_RELATED_P (insn)")))
 		(const_string "true")
@@ -183,7 +183,7 @@ 
 
 ;; For calls and millicode calls.
 (define_attr "in_call_delay" "false,true"
-  (if_then_else (and (eq_attr "type" "!uncond_branch,branch,cbranch,fbranch,call,sibcall,dyncall,multi,milli,sh_func_adrs,parallel_branch")
+  (if_then_else (and (eq_attr "type" "!uncond_branch,branch,cbranch,fbranch,call,sibcall,dyncall,multi,milli,sh_func_adrs,parallel_branch,trap")
 		     (eq_attr "length" "4")
 		     (not (match_test "RTX_FRAME_RELATED_P (insn)")))
 		(const_string "true")
@@ -5324,6 +5324,15 @@ 
   [(set_attr "type" "binary,binary")
    (set_attr "length" "4,4")])
 
+;; Trap instructions.
+
+(define_insn "trap"
+  [(trap_if (const_int 1) (const_int 0))]
+  ""
+  "{addit|addi,tc},<> 1,%%r0,%%r0"
+  [(set_attr "type" "trap")
+   (set_attr "length" "4")])
+
 ;; Clobbering a "register_operand" instead of a match_scratch
 ;; in operand3 of millicode calls avoids spilling %r1 and
 ;; produces better code.