diff mbox series

[Bug,libelf/31225] New: Crash when using elf_memory() on a compressed section; fixed with s/ELF_C_READ/ELF_C_READ_MMAP/

Message ID bug-31225-10460@http.sourceware.org/bugzilla/
State Superseded
Headers
Series [Bug,libelf/31225] New: Crash when using elf_memory() on a compressed section; fixed with s/ELF_C_READ/ELF_C_READ_MMAP/ |

Commit Message

dodji at seketeli dot org Jan. 9, 2024, 6:52 p.m. UTC 
  https://sourceware.org/bugzilla/show_bug.cgi?id=31225

            Bug ID: 31225
           Summary: Crash when using elf_memory() on a compressed section;
                    fixed with s/ELF_C_READ/ELF_C_READ_MMAP/
           Product: elfutils
           Version: unspecified
            Status: UNCONFIRMED
          Severity: normal
          Priority: P2
         Component: libelf
          Assignee: unassigned at sourceware dot org
          Reporter: bruening at google dot com
                CC: elfutils-devel at sourceware dot org
  Target Milestone: ---

When setting up libelf with elf_memory() we observe repeatable crashes on any
compressed debug section.  The problem seems to be from elf_memory() using the
ELF_C_READ command, which is not really supported: elf32_getshdr.c looks for
ELF_C_READ_MMAP instead and makes a copy of read-only data in that case.

Changing elf_memory() to pass ELF_C_READ_MMAP solves the problem:

--------------------------------------------------------------
 }
--------------------------------------------------------------

There seem to not be any tests of elf_memory() either so maybe there are few
users and that is why no one else has hit this.

More details on the crash which is hit examining ld-linux-x86-64.so.2 which has
compressed sections on this machine:

--------------------------------------------------------------
Program received signal SIGSEGV, Segmentation fault.                            
0x0000555555584764 in elf_compress (scn=scn@entry=0x5555555fa6a8,
type=type@entry=0, flags=flags@entry=0) at elf_compress.c:732
732               shdr->sh_size = scn->zdata_size;                              
(gdb) bt                                                                        
#0  0x0000555555584764 in elf_compress (scn=scn@entry=0x5555555fa6a8,
type=type@entry=0, flags=flags@entry=0) at elf_compress.c:732
#1  0x0000555555575cba in check_section (result=result@entry=0x5555555f8440,
shstrndx=shstrndx@entry=31, scn=scn@entry=0x5555555fa6a8,
inscngrp=inscngrp@entry=false)
    at dwarf_begin_elf.c:238                                                    
#2  0x0000555555576480 in global_read (shstrndx=<optimized out>, elf=<optimized
out>, result=0x5555555f8440) at dwarf_begin_elf.c:452
#3  dwarf_begin_elf (elf=0x5555555f94c0, cmd=DWARF_C_READ, scngrp=0x0) at
dwarf_begin_elf.c:603
(gdb) list                                                                      
727               shdr->sh_flags &= ~SHF_COMPRESSED;                   
728             }                                                               
729           else                                                              
730             {                                                               
731               Elf64_Shdr *shdr = elf64_getshdr (scn);                     
732               shdr->sh_size = scn->zdata_size;                              
733               shdr->sh_addralign = scn->zdata_align;                     
734               shdr->sh_flags &= ~SHF_COMPRESSED;             
735             }                                                               
736                                                                             
(gdb) p shdr                                                                    
$1 = (Elf64_Shdr *) 0x7ffdf79bdc88                                              
(gdb) p *shdr                                                                   
$2 = {                                                                          
  sh_name = 214,                                                                
  sh_type = 1,                                                                  
  sh_flags = 2048,                                                              
  sh_addr = 0,                                                                  
  sh_offset = 640,                                                              
  sh_size = 1151,                                                               
  sh_link = 0,                                                                  
  sh_info = 0,                                                                  
  sh_addralign = 8,                                                             
  sh_entsize = 0                                                                
}                                                                               
(gdb) memquery shdr                                                             
7ffdf7939000-7ffdf79be000 r--p 00000000 fe:01 21499176                  
/usr/lib/debug/.build-id/ac/cffc5784c4a469d09348e3f7ec53a74096fbd3.debug
--------------------------------------------------------------

They do seem to be compressed:
--------------------------------------------------------------
$ readelf -S
/usr/lib/debug/.build-id/ac/cffc5784c4a469d09348e3f7ec53a74096fbd3.debug
  [21] .debug_aranges    PROGBITS         0000000000000000  00000280
       000000000000047f  0000000000000000   C       0     0     8
  [22] .debug_info       PROGBITS         0000000000000000  00000700
       0000000000054087  0000000000000000   C       0     0     8
  [23] .debug_abbrev     PROGBITS         0000000000000000  00054788
       000000000000451e  0000000000000000   C       0     0     8
  [24] .debug_line       PROGBITS         0000000000000000  00058ca8
       000000000000fdbe  0000000000000000   C       0     0     8
  [25] .debug_str        PROGBITS         0000000000000000  00068a68
       00000000000037e6  0000000000000001 MSC       0     0     8
  [26] .debug_line_str   PROGBITS         0000000000000000  0006c250
       00000000000006fc  0000000000000001 MSC       0     0     8
  [27] .debug_loclists   PROGBITS         0000000000000000  0006c950
       000000000000f5f2  0000000000000000   C       0     0     8
  [28] .debug_rnglists   PROGBITS         0000000000000000  0007bf48
       00000000000026b6  0000000000000000   C       0     0     8
Key to Flags:
  W (write), A (alloc), X (execute), M (merge), S (strings), I (info),
  L (link order), O (extra OS processing required), G (group), T (TLS),
  C (compressed), x (unknown), o (OS specific), E (exclude),
  R (retain), D (mbind), l (large), p (processor specific)
--------------------------------------------------------------

Is this fix we found the permanent solution?  I don't know this source code:
not sure why ELF_C_READ exists?  Should it be removed?  If not, should
elf32_getshdr.c and other code consider ELF_C_READ?  Do you want me to submit
this patch plus attempt to add a test, or someone on your end will take this
over?

Comments

dodji at seketeli dot org Jan. 23, 2024, 1:55 p.m. UTC | #1 
https://sourceware.org/bugzilla/show_bug.cgi?id=31225

Mark Wielaard <mark at klomp dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |mark at klomp dot org

--- Comment #1 from Mark Wielaard <mark at klomp dot org> ---
Would you be able to give us a reproducer? And/or provide the 
I don't fully understand how you get at
cffc5784c4a469d09348e3f7ec53a74096fbd3.debug file? Are you using a standard eu
tool or something written by hand using the libdw and libelf libraries.

I think the issue is that something is providing unwritable to elf_memory and
that isn't expected. This might be a bug in elf_compress. And/or a design flaw
in elf_memory since you cannot provide a "mode".

Your fix fix might be correct. I have to think a bit more about it.
dodji at seketeli dot org Jan. 23, 2024, 10:36 p.m. UTC | #2 
https://sourceware.org/bugzilla/show_bug.cgi?id=31225

--- Comment #2 from Derek Bruening <bruening at google dot com> ---
Our usage is inside a large complex code base that mmaps the file on its own. 
It seems that any use of elf_memory() that has mapped the file on their own as
read-only will hit this.

I know it may not be easy to read random code inside our code base but FTR
elf_memory is called here:
https://github.com/DynamoRIO/dynamorio/blob/master/ext/drsyms/drsyms_elf.c#L283

This is our own code which maps the file (this is pre-existing code that worked
with the libelf from elftoolchain):
https://github.com/DynamoRIO/dynamorio/blob/master/ext/drsyms/drsyms_unix_common.c#L134

It looks like we are passing MAP_PRIVATE, so we may be able to change our
mapping to include write privileges (as copy-on-write): is that what elfutils
expects?  I had thought with the ELF_C_READ_MMAP support elfutils deliberately
supported read-only mappings in general?
dodji at seketeli dot org Feb. 1, 2024, 2:39 p.m. UTC | #3 
https://sourceware.org/bugzilla/show_bug.cgi?id=31225

Mark Wielaard <mark at klomp dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2024-02-01
             Status|UNCONFIRMED                 |ASSIGNED
           Assignee|unassigned at sourceware dot org   |mark at klomp dot org

--- Comment #3 from Mark Wielaard <mark at klomp dot org> ---
(In reply to Derek Bruening from comment #2)
> I had thought with the ELF_C_READ_MMAP support elfutils
> deliberately supported read-only mappings in general?

You are basically right. But I had to carefully read the code to make sure this
really was the case when the user used elf_memory. Which, as you also point
out, doesn't have any testcases. So I added one that checks various issues (and
that fails before your suggested change). Please take a look:

https://inbox.sourceware.org/elfutils-devel/20240201143858.930159-1-mark@klomp.org/
dodji at seketeli dot org Feb. 2, 2024, 8:01 p.m. UTC | #4 
https://sourceware.org/bugzilla/show_bug.cgi?id=31225

--- Comment #4 from Derek Bruening <bruening at google dot com> ---
>  So I added one that checks various issues (and that fails before your suggested change). Please take a look:
>
> https://inbox.sourceware.org/elfutils-devel/20240201143858.930159-1-mark@klomp.org/

LGTM!  Thank you for adding the test case.
dodji at seketeli dot org Feb. 2, 2024, 9:45 p.m. UTC | #5 
https://sourceware.org/bugzilla/show_bug.cgi?id=31225

Mark Wielaard <mark at klomp dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|ASSIGNED                    |RESOLVED

--- Comment #5 from Mark Wielaard <mark at klomp dot org> ---
commit cc44ac6740797a23cd0af0cb22bd828d569224b8
Author: Mark Wielaard <mark@klomp.org>
Date:   Thu Feb 1 14:56:18 2024 +0100

    libelf: Treat elf_memory as if using ELF_C_READ_MMAP

    An Elf handle created through elf_memory was treated as if opened with
    ELF_C_READ. Which means libelf believed it had read the memory itself
    and could simply write to it if it wanted (because it wasn't mmaped
    directly on top of a file). This causes issues when that memory was
    actually read-only. Work around this by pretending the memory was
    actually read with ELF_C_READ_MMAP (so directly readable, but not
    writable).

    Add extra tests to elfgetzdata to check using elf_memory with
    read-only memory works as expected.

              * libelf/elf_memory.c (elf_memory): Call
              __libelf_read_mmaped_file with ELF_C_READ_MMAP.
              * tests/elfgetzdata.c (main): Add new "mem" option.
              * tests/run-elfgetzdata.sh: Also run all tests with new
              "mem" option.

    https://sourceware.org/bugzilla/show_bug.cgi?id=31225

    Reported-by: Derek Bruening <bruening@google.com>
    Signed-off-by: Mark Wielaard <mark@klomp.org>
diff mbox series

Patch

diff --git a/libelf/elf_memory.c b/libelf/elf_memory.c
index a47f1d24..13d77cb7 100644
--- a/libelf/elf_memory.c
+++ b/libelf/elf_memory.c
@@ -46,5 +46,5 @@  elf_memory (char *image, size_t size)
       return NULL;
     }

-  return __libelf_read_mmaped_file (-1, image, 0, size, ELF_C_READ, NULL);
+  return __libelf_read_mmaped_file (-1, image, 0, size, ELF_C_READ_MMAP,
NULL);