Bug 21199 - elf_update might "fill" over existing section data
Summary: elf_update might "fill" over existing section data
Status: RESOLVED FIXED
Alias: None
Product: elfutils
Classification: Unclassified
Component: libelf (show other bugs)
Version: unspecified
: P2 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-02-24 11:45 UTC by Mark Wielaard
Modified: 2017-04-03 22:06 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Wielaard 2017-02-24 11:45:04 UTC
When adding or changing data for sections, when there is also existing, non-dirty, sections then elf_update might write "fill" over some existing data.

This happens if the sections that aren't changed have some extra space between them. For example because they should be aligned. If two such sections are both not dirty then the last_position/offset written is not updated correctly. Only the data d_size of the existing data is added, missing any extra space needed for e.g. alignment. Then when dirty section data is later written too much "fill" is written because elf_update thinks there is a larger gap than there really is. Overwriting existing data.

Proposed patch that always updates the last_position/offset to the start of the section even if there is no changed/dirty section data:

diff --git a/libelf/elf32_updatefile.c b/libelf/elf32_updatefile.c
index 8dd85d1..7ac9951 100644
--- a/libelf/elf32_updatefile.c
+++ b/libelf/elf32_updatefile.c
@@ -343,9 +343,10 @@ __elfw2(LIBELFBITS,updatemmap) (Elf *elf, int change_bo, size_t shnum)
                  {
                    fill_mmap (dl->data.d.d_off, last_position, scn_start,
                               shdr_start, shdr_end);
-                   last_position = scn_start + dl->data.d.d_off;
                  }
 
+               last_position = scn_start + dl->data.d.d_off;
+
                if ((scn->flags | dl->flags | elf->flags) & ELF_F_DIRTY)
                  {
                    /* Let it go backward if the sections use a bogus
@@ -353,8 +354,6 @@ __elfw2(LIBELFBITS,updatemmap) (Elf *elf, int change_bo, size_t shnum)
                       user's section data with the latest one, rather than
                       crashing.  */
 
-                   last_position = scn_start + dl->data.d.d_off;
-
                    if (unlikely (change_bo))
                      {
 #if EV_NUM != 2
@@ -728,6 +727,8 @@ __elfw2(LIBELFBITS,updatefile) (Elf *elf, int change_bo, size_t shnum)
                      }
                  }
 
+               last_offset = scn_start + dl->data.d.d_off;
+
                if ((scn->flags | dl->flags | elf->flags) & ELF_F_DIRTY)
                  {
                    char tmpbuf[MAX_TMPBUF];
@@ -738,8 +739,6 @@ __elfw2(LIBELFBITS,updatefile) (Elf *elf, int change_bo, size_t shnum)
                       user's section data with the latest one, rather than
                       crashing.  */
 
-                   last_offset = scn_start + dl->data.d.d_off;
-
                    if (unlikely (change_bo))
                      {
 #if EV_NUM != 2
Comment 1 Mark Wielaard 2017-03-27 15:12:45 UTC
Patch posted:
https://sourceware.org/ml/elfutils-devel/2017-q1/msg00126.html
Comment 2 Mark Wielaard 2017-04-03 22:06:41 UTC
commit a3bf8f0852d0f66911dcf879c5a1fcff3cb4cb46
Author: Mark Wielaard <mark@klomp.org>
Date:   Mon Mar 27 17:01:57 2017 +0200

    libelf: Always update last_offset in updatefile and updatemmap.
    
    When ELF section data was used, but not updated or marked as dirty and
    there also existed non-dirty sections and some padding was needed between
    the sections (possibly because of alignment) then elf_update might write
    "fill" over some of the existing data. This happened because in that case
    the last_position was not updated correctly.
    
    Includes a new testcase fillfile that fails before this patch by showing
    fill instead of the expected data in some section data. It succeeds with
    this patch.
    
    https://sourceware.org/bugzilla/show_bug.cgi?id=21199
    
    Signed-off-by: Mark Wielaard <mark@klomp.org>