Re: [PATCH] Fix section corruption bug

Re: [PATCH] Fix section corruption bug

[Thilo Schulz]

[-- Attachment #1: Type: text/plain, Size: 2058 bytes --]

On Tuesday 10 June 2014 11:48:15 Mark Wielaard wrote:
> On Mon, 2014-06-09 at 21:05 +0200, Thilo Schulz wrote:
> > When adding data to existing sections in ELF files, libelf may corrupt
> > those sections, i.e. overwrite the existing data if certain conditions
> > are met.
> > 
> > If an Elf_Scn structure has seen a call to elf_rawdata(scn) before but no
> > call to elf_getdata(scn), scn->read_data flag is set, but not
> > scn->data_list_rear.
> 
> Do you happen to have a small testcase that shows the buggy behavior?

Sure. This is an excerpt from the final program. A short word on what it is 
supposed to do in my practical application:
I am doing a project for the AVR platform, which is mixed C/assembly. For 
command parser functions that are called out of assembly into C code, I need 
to replace the final return statements with rjmps back to 
program code in an object file generated from my assembly.
So I want to  add new symbols and relocations to a cmd.o file.

Find as attachment a simple program, which checks for the presence of the two 
symbols cmd_response and cmd_noresponse and adds them if they don't exist.
Since the AVR architecture is 8 bit, I am only using Elf32_ structures 
throughout, so you may need to compile test .o objects with -m32 for the test 
program to work on them. 

> I was wondering whether we want to check scn->rawdata.s directly, or if
> we could rely on ELF_F_FILEDATA being set for scn->flags?

Seems reasonable though I don't know the code as well as you do I guess.

As a further note: A similar bug, albeit for slightly different reasons, occurs 
when adding relocations. Adding a relocation with elf_newdata() then 
elf_update() 
results in the old data being "forgotten" if there was no elf_getdata() call 
before to load that data into memory. The cause is a bit different because in 
this case, there was not a call to elf_rawdata() before and this still 
happened. I imagine, this might also be a problem for string tables.

-- 
Best regards,
Thilo Schulz

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: elf-test.c --]
[-- Type: text/x-csrc, Size: 5930 bytes --]

/*
===========================================================================
Copyright (C) 2014 Essential Nature (Thilo Schulz)

Fix the command parse functions so as to replace ret
with an rjmp to the appropriate location
===========================================================================
*/

#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <sysexits.h>
#include <string.h>
#include <errno.h>

#include <libelf.h>
#include <gelf.h>

int isbe = 0;
int found_resp, found_noresp;
Elf32_Sym fresp, fnoresp;

void do_help(char *progname)
{
  printf("Usage: %s [options] <filename>\n"
         "Modify the elf files for the command parser so as to use rjmp instead\n"
         "of ret instruction to return to main interrupt service routine.\n\n"
         "Options:\n"
         "  -h                          Print this help\n"
         "\n",
         progname);
}

Elf_Scn *get_section(Elf *obj, Elf32_Word sh_type, const char *secname)
{
  Elf_Scn *cursec;
  Elf32_Shdr *curhdr;
  size_t shstrndex;
  char *name;
  
  if(elf_getshdrstrndx(obj, &shstrndex))
  {
    fprintf(stderr, "Couldn't get list of section names: %s\n", elf_errmsg(-1));
    exit(EX_DATAERR);
  }

  cursec = NULL;
  
  while((cursec = elf_nextscn(obj, cursec)))
  {
    if(!(curhdr = elf32_getshdr(cursec)))
    {
      fprintf(stderr, "Couldn't get section header: %s\n", elf_errmsg(-1));
      exit(EX_DATAERR);
    }
    
    if(name = elf_strptr(obj, shstrndex, curhdr->sh_name))
    {
      if(curhdr->sh_type == sh_type && (!secname || !strcmp(name, secname)))
        return cursec;
    }
  }
  
  return NULL;
}

size_t add_stringtotable(Elf *obj, size_t tableind, char *str)
{
  Elf_Scn *strsec;
  Elf32_Shdr *strhdr;
  Elf_Data *data;
  size_t lastofs;

  if(!(strsec = elf_getscn(obj, tableind)))
  {
    fprintf(stderr, "Couldn't get section to string table %zd: %s\n", elf_errmsg(-1));
    exit(EX_DATAERR);
  }

  if(!(strhdr = elf32_getshdr(strsec)))
  {
    fprintf(stderr, "Couldn't get header of string table\n");
    exit(EX_DATAERR);
  }
  
  // Workaround for bug in libelf
//  data = elf_getdata(strsec, NULL);
  
  if(!(data = elf_newdata(strsec)))
  {
    fprintf(stderr, "Couldn't add data to string table: %s\n", elf_errmsg(-1));
    exit(EX_DATAERR);
  }
  
  data->d_align = 1;
  data->d_buf = str;
  data->d_size = strlen(str) + 1;
  data->d_type = ELF_T_BYTE;
  
  lastofs = strhdr->sh_size;
  strhdr->sh_size += data->d_size;
  
  return lastofs;
}

void add_undefinedsym(Elf *obj, Elf_Scn *scn, Elf32_Sym *dest, size_t tableind, char *str)
{
  Elf_Data *data;
  GElf_Sym undef;

  if(!(data = elf_newdata(scn)))
  {
    fprintf(stderr, "Couldn't add symbol %s to symbol table: %s\n", str, elf_errmsg(-1));
    exit(EX_DATAERR);
  }

  undef.st_name = add_stringtotable(obj, tableind, str);
  undef.st_value = 0;
  undef.st_size = 0;
  undef.st_info = GELF_ST_INFO(STB_GLOBAL, STT_NOTYPE);
  undef.st_other = STV_DEFAULT;
  undef.st_shndx = 0;

  data->d_align = 4;
  data->d_buf = dest;
  data->d_size = sizeof(*dest);
  data->d_type = ELF_T_SYM;

  if(!gelf_update_sym(data, 0, &undef))
  {
    fprintf(stderr, "Couldn't add symbol %s to symbol table: %s\n", str, elf_errmsg(-1));
    exit(EX_DATAERR);
  }
}

int modify_elf(const char *fname)
{
  int fd;
  Elf *obj;
  Elf32_Ehdr *ehdr;
  Elf_Scn *symsec, *progsec;
  Elf32_Shdr *symsechdr;
  Elf_Data *data, *progdata;
  GElf_Sym sym;
  char *name;
  int numsym, index;
  
  if(elf_version(EV_CURRENT) == EV_NONE)
  {
    fprintf(stderr, "Could not initialize ELF library: %s\n", elf_errmsg(-1));
    return EX_SOFTWARE;
  }
  
  if((fd = open(fname, O_RDWR, 0)) < 0)
  {
    fprintf(stderr, "Couldn't open elf file: %s\n", strerror(errno));
    return EX_NOINPUT;
  }
  
  if(!(obj = elf_begin(fd, ELF_C_RDWR, NULL)))
  {
    fprintf(stderr, "Couldn't parse elf file: %s\n", elf_errmsg(-1));
    return EX_DATAERR;
  }

  if(!(ehdr = elf32_getehdr(obj)))
  {
    fprintf(stderr, "Failed to retrieve elf header: %s\n", elf_errmsg(-1));
    return EX_DATAERR;
  }

  if((ehdr->e_ident[EI_DATA] & ELFDATA2MSB))
    isbe = 1;

  symsec = get_section(obj, SHT_SYMTAB, ".symtab");
  
  if(!symsec)
  {
    fprintf(stderr, "No symbol section found\n");
    return EX_DATAERR;
  }
  
  data = NULL;
  if(!(data = elf_getdata(symsec, data)) || !data->d_size)
  {
    fprintf(stderr, "No data associated with symbol section\n");
    return EX_DATAERR;
  }

  if(!(symsechdr = elf32_getshdr(symsec)))
  {
    fprintf(stderr, "Couldn't get header of symbol section\n");
    return EX_DATAERR;
  }
  
  numsym = 0;
  found_resp = 0;
  found_noresp = 0;
  
  while(gelf_getsym(data, numsym, &sym))
  {
    name = elf_strptr(obj, symsechdr->sh_link, sym.st_name);

    if(!strcmp(name, "cmd_response"))
      found_resp = numsym;
    else if(!strcmp(name, "cmd_noresponse"))    
      found_noresp = numsym;

    numsym++;
  }
  
  /*
   * Make sure symbol table contains undefined symbols cmd_response and cmd_noresponse
   */

  if(!found_noresp)
  {
    add_undefinedsym(obj, symsec, &fnoresp, symsechdr->sh_link, "cmd_noresponse");
    found_noresp = numsym++;
  }

  if(!found_resp)
  {
    add_undefinedsym(obj, symsec, &fresp, symsechdr->sh_link, "cmd_response");
    found_resp = numsym++;
  }

  elf_update(obj, ELF_C_WRITE);
  
  elf_end(obj);
  
  if(close(fd) < 0)
  {
    fprintf(stderr, "Couldn't close elf file: %s\n", strerror(errno));
    return EX_NOINPUT;
  }
}

int main(int argc, char **argv)
{
  int opt;
  
   if(argc < 2)
  {
    if(argc < 1)
      do_help("elf-fixcmd");
    else
      do_help(argv[0]);

    return EX_OK;
  }
  
  while((opt = getopt(argc, argv, "h")) != -1)
  {
    switch(opt)
    {
      case 'h':
        do_help(argv[0]);
        return EX_OK;
      break;
      default:
        return EX_USAGE;
    }
  }
  
  return modify_elf(argv[optind]);
}

Re: [PATCH] Fix section corruption bug

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 1713 bytes --]

On Thu, 2014-06-12 at 14:30 +0200, Mark Wielaard wrote:
> On Tue, 2014-06-10 at 15:31 +0200, Thilo Schulz wrote:
> > > I was wondering whether we want to check scn->rawdata.s directly, or if
> > > we could rely on ELF_F_FILEDATA being set for scn->flags?
> > 
> > Seems reasonable though I don't know the code as well as you do I guess.
> 
> I wish I understood the code very well :) But now that I wrote the
> testcase and you pointed out the second bug, I am not sure of the fix
> anymore. It does seem to fix the first issue, but then you immediately
> hit the second.
> 
> > As a further note: A similar bug, albeit for slightly different reasons, occurs 
> > when adding relocations. Adding a relocation with elf_newdata() then 
> > elf_update() 
> > results in the old data being "forgotten" if there was no elf_getdata() call 
> > before to load that data into memory. The cause is a bit different because in 
> > this case, there was not a call to elf_rawdata() before and this still 
> > happened. I imagine, this might also be a problem for string tables.
> 
> Indeed. The attached testcase shows both issues. Calling elf_getdata()
> and then elf_newdata() works as expected. But elf_newdata drops all
> existing data when elf_rawdata is called before and elf_newdata keeps
> the size, but not the actual content bytes of existing data of a section
> if elf_getdata isn't called before.
> 
> Still scratching my head a little how to resolve both issues properly.

Sorry this took 9 months... But I believe these issues have finally been
resolved in current git elfutils. At least my testcase now works as
expected. Hope it also now works for your code.

Cheers,

Mark

Re: [PATCH] Fix section corruption bug

[Thilo Schulz]

[-- Attachment #1: Type: text/plain, Size: 1125 bytes --]

On Thursday 12 June 2014 14:30:52 Mark Wielaard wrote:
> Thanks. It think that shows the second bug you describe.

First bug. My testcase first uses elf_strptr() to get the strings for the 
symbols, and that func internally calls elf_rawdata() thus triggering the first 
bug described.

> Still scratching my head a little how to resolve both issues properly.

Hmm.. I didn't do any debugging on the second bug reported, but it seems to me 
that once a section was marked dirty, outputting to a new elf causes it to 
only consider data explicitly added to the lib internal memory representation 
of the elf.
It _did_ write out the data I added via elf_newdata(), and entered the 
appropriate offsets into the header file, but completely omitted copying of the 
original data.

These issues can all be worked around with by explicitly calling 
elf_getdata().

Still, I'm a bit baffled that a bug like this could exist for so long in .. well 
I guess these are kind of core developer utils, right?
Or maybe I'm doing some really sick stuff noone else dreamt up yet ;)

-- 
Best regards,
Thilo Schulz

Re: [PATCH] Fix section corruption bug

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 2153 bytes --]

On Tue, 2014-06-10 at 15:31 +0200, Thilo Schulz wrote:
> On Tuesday 10 June 2014 11:48:15 Mark Wielaard wrote:
> > On Mon, 2014-06-09 at 21:05 +0200, Thilo Schulz wrote:
> > > When adding data to existing sections in ELF files, libelf may corrupt
> > > those sections, i.e. overwrite the existing data if certain conditions
> > > are met.
> > > 
> > > If an Elf_Scn structure has seen a call to elf_rawdata(scn) before but no
> > > call to elf_getdata(scn), scn->read_data flag is set, but not
> > > scn->data_list_rear.
> > 
> > Do you happen to have a small testcase that shows the buggy behavior?
> 
> Sure. This is an excerpt from the final program.

Thanks. It think that shows the second bug you describe.
It helped me write a specific test case for both issues (attached).

> > I was wondering whether we want to check scn->rawdata.s directly, or if
> > we could rely on ELF_F_FILEDATA being set for scn->flags?
> 
> Seems reasonable though I don't know the code as well as you do I guess.

I wish I understood the code very well :) But now that I wrote the
testcase and you pointed out the second bug, I am not sure of the fix
anymore. It does seem to fix the first issue, but then you immediately
hit the second.

> As a further note: A similar bug, albeit for slightly different reasons, occurs 
> when adding relocations. Adding a relocation with elf_newdata() then 
> elf_update() 
> results in the old data being "forgotten" if there was no elf_getdata() call 
> before to load that data into memory. The cause is a bit different because in 
> this case, there was not a call to elf_rawdata() before and this still 
> happened. I imagine, this might also be a problem for string tables.

Indeed. The attached testcase shows both issues. Calling elf_getdata()
and then elf_newdata() works as expected. But elf_newdata drops all
existing data when elf_rawdata is called before and elf_newdata keeps
the size, but not the actual content bytes of existing data of a section
if elf_getdata isn't called before.

Still scratching my head a little how to resolve both issues properly.

Thanks,

Mark

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: newdata.c --]
[-- Type: text/x-csrc, Size: 6137 bytes --]

/* Test program for elf_newdata.
   Copyright (C) 2014 Red Hat, Inc.
   This file is part of elfutils.

   This file is free software; you can redistribute it and/or modify
   it under the terms of the GNU General Public License as published by
   the Free Software Foundation; either version 3 of the License, or
   (at your option) any later version.

   elfutils is distributed in the hope that it will be useful, but
   WITHOUT ANY WARRANTY; without even the implied warranty of
   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
   GNU General Public License for more details.

   You should have received a copy of the GNU General Public License
   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */

#include <errno.h>
#include <error.h>
#include <stdio.h>
#include <stdbool.h>
#include <stdlib.h>
#include <string.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <unistd.h>
#include <libelf.h>

/* Adds a new "Hello World" string to the section.  */
static void
add_data (Elf_Scn *scn)
{
  printf ("Add Hello World\n");

  Elf_Data *data = elf_newdata (scn);
  if (data == NULL)
    error (1, 0, "elf_newdata: %s", elf_errmsg (-1));

  data->d_align = 1;
  data->d_buf = "Hello World";
  data->d_type = ELF_T_BYTE;
  data->d_size = sizeof ("Hello World");
  data->d_version = EV_CURRENT;
}

/* Creates a new minimal ELF file with a .data section containing
   "Hello World".  Returns the name of the new file.  */
static const char *
create_elf ()
{
  Elf *elf;
  int fd;

  static char name[] = "test.XXXXXX";
  fd = mkstemp (name);
  if (fd < 0)
    error (1, errno, "mkstemp");

  elf = elf_begin (fd, ELF_C_WRITE, NULL);
  if (elf == NULL)
    error (1, 0, "elfbegin: %s", elf_errmsg (-1));

  Elf32_Ehdr *ehdr = elf32_newehdr (elf);
  if (ehdr == NULL)
    error (1, 0, "elf32_newehdr: %s", elf_errmsg (-1));

  /* Our data.  */
  Elf_Scn *scn = elf_newscn (elf);
  if (scn == NULL)
    error (1, 0, "elf_newscn: %s", elf_errmsg (-1));

  add_data (scn);

  Elf32_Shdr *shdr = elf32_getshdr(scn);
  if (shdr == NULL)
    error (1, 0, "elf_getshdr: %s", elf_errmsg (-1));

  shdr->sh_name = 1; /* .data */
  shdr->sh_type = SHT_PROGBITS;
  shdr->sh_flags = SHF_STRINGS;
  shdr->sh_entsize = 0;

  /* The section string table.  */
  scn = elf_newscn (elf);
  if (scn == NULL)
    error (1, 0, "elf_newscn: %s", elf_errmsg (-1));

  Elf_Data *data = elf_newdata (scn);
  if (data == NULL)
    error (1, 0, "elf_newdata: %s", elf_errmsg (-1));

  // Quick and dirty string table for section zero, our new data section,
  // and this shstrtab section itself.
  char string_table[] = { '\0',
			  '.', 'd', 'a', 't', 'a', '\0',
			  '.', 's', 'h', 's', 't', 'r', 't', 'a', 'b', '\0' };

  data->d_align = 1;
  data->d_off = 0;
  data->d_buf = string_table;
  data->d_type = ELF_T_BYTE;
  data->d_size = sizeof (string_table);
  data->d_version = EV_CURRENT;

  shdr = elf32_getshdr(scn);
  if (shdr == NULL)
    error (1, 0, "elf_getshdr: %s", elf_errmsg (-1));

  shdr->sh_name = 7; /* .shstrtab */
  shdr->sh_type = SHT_STRTAB;
  shdr->sh_flags = SHF_STRINGS;
  shdr->sh_entsize = 0;

  size_t ndx = elf_ndxscn (scn);
  ehdr->e_shstrndx = ndx;
  ehdr->e_version = EV_CURRENT;

  /* Write it all out. */
  if (elf_update (elf, ELF_C_WRITE) < 0)
    error (1, 0, "elf_update: %s", elf_errmsg (-1));

  if (elf_end (elf) < 0)
    error (1, 0, "elf_end: %s", elf_errmsg (-1));

  if (close (fd) < 0)
    error (1, errno, "close");

  return name;
}

/* Opens an existing ELF file, checks that it has a .data section, if
   read is true checks that it contains one or more "Hello World"
   strings, and adds an extra "Hello World" to that section.  Gets the
   section data either through elf_getdata () or elf_rawdata () based
   on the last argument.  */
static void
modify_elf (const char *name, bool read, bool raw)
{
  int fd = open (name, O_RDWR);
  if (fd < 0)
    error (1, errno, "open '%s'", name);

  Elf *elf = elf_begin (fd, ELF_C_RDWR, NULL);
  if (elf == NULL)
    error (1, 0, "elf_begin: %s", elf_errmsg (-1));

  /* Get the section header string table index.  */
  size_t ndx;
  if (elf_getshdrstrndx (elf, &ndx) < 0)
    error (1, 0, "elf_getshdrstrndx: %s", elf_errmsg (-1));

  /* Get the .data section. */
  Elf_Scn *scn = NULL;
  Elf32_Shdr *shdr = NULL;
  while ((scn = elf_nextscn (elf, scn)) != NULL)
    {
      shdr = elf32_getshdr (scn);
      if (shdr != NULL && shdr->sh_type == SHT_PROGBITS)
	{
	  const char *sname = elf_strptr (elf, ndx, shdr->sh_name);
	  if (sname != NULL && strcmp (sname, ".data") == 0)
	    break;
	}
    }

  if (scn == NULL)
    error (1, 0, "No PROGBITS .data section.");

  if (read)
    {
      Elf_Data *data;
      if (raw)
	{
	  data  = elf_rawdata (scn, NULL);
	  if (data == NULL)
	    error (1, 0, "elf_rawdata: %s", elf_errmsg (-1));
	}
      else
	{
	  data  = elf_getdata (scn, NULL);
	  if (data == NULL)
	    error (1, 0, "elf_getdata: %s", elf_errmsg (-1));
	}

      /* Is it all Hello World?  */
      printf ("data size: %zd\n", data->d_size);
      int nr = data->d_size / sizeof ("Hello World");
      printf ("strings: %d\n", nr);
      char *str = data->d_buf;
      for (int i = 0; i < nr; i++)
	{
	  printf ("%s\n", (char *) str);
	  str += sizeof ("Hello World");
	}
    }

  /* And add one more string... */
  add_data (scn);

  /* Write it all out. */
  if (elf_update (elf, ELF_C_WRITE) < 0)
    error (1, 0, "elf_update: %s", elf_errmsg (-1));

  if (elf_end (elf) < 0)
    error (1, 0, "elf_end: %s", elf_errmsg (-1));

  if (close (fd) < 0)
    error (1, errno, "close");
}

int
main (int argc, char **argv)
{
  // Initialize libelf.
  if (elf_version (EV_CURRENT) == EV_NONE)
    error (1, 0, "elf_version: %s", elf_errmsg (-1));

  const char *name = create_elf ();
  printf ("name: %s\n", name);
  modify_elf (name, false, false); /* Don't read, just add.   */
  modify_elf (name, true, false);  /* Read with elf_getdata.  */
  modify_elf (name, true, true);   /* Read with elf_rawdata.  */
  modify_elf (name, true, false);  /* Read with elf_getdata again.  */

  return 0;
}

Re: [PATCH] Fix section corruption bug

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 868 bytes --]

Hi,

On Mon, 2014-06-09 at 21:05 +0200, Thilo Schulz wrote:
> When adding data to existing sections in ELF files, libelf may corrupt
> those sections, i.e. overwrite the existing data if certain conditions are
> met.
> 
> If an Elf_Scn structure has seen a call to elf_rawdata(scn) before but no
> call to elf_getdata(scn), scn->read_data flag is set, but not
> scn->data_list_rear.

Do you happen to have a small testcase that shows the buggy behavior? 

> Thus, elf_newdata(scn) incorrectly detects a "new user added section" when
> really it is a section with live, valid data that will be overwritten by
> elf_update(), corrupting the section.
>
> This patch fixes this incorrect behaviour.

I was wondering whether we want to check scn->rawdata.s directly, or if
we could rely on ELF_F_FILEDATA being set for scn->flags?

Thanks,

Mark

[PATCH] Fix section corruption bug

[Thilo Schulz]

[-- Attachment #1: Type: text/plain, Size: 1174 bytes --]

Hi,

When adding data to existing sections in ELF files, libelf may corrupt
those sections, i.e. overwrite the existing data if certain conditions are
met.

If an Elf_Scn structure has seen a call to elf_rawdata(scn) before but no
call to elf_getdata(scn), scn->read_data flag is set, but not
scn->data_list_rear.
Thus, elf_newdata(scn) incorrectly detects a "new user added section" when
really it is a section with live, valid data that will be overwritten by
elf_update(), corrupting the section.

This patch fixes this incorrect behaviour.

Signed-off-by: Thilo Schulz <thilo@tjps.eu>
---
 libelf/elf_newdata.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libelf/elf_newdata.c b/libelf/elf_newdata.c
index 90d1813..f90eb0a 100644
--- a/libelf/elf_newdata.c
+++ b/libelf/elf_newdata.c
@@ -64,7 +64,7 @@ elf_newdata (Elf_Scn *scn)
 
   rwlock_wrlock (scn->elf->lock);
 
-  if (scn->data_read && scn->data_list_rear == NULL)
+  if (scn->data_read && scn->data_list_rear == NULL && !scn->rawdata.s)
     {
       /* This means the section was created by the user and this is the
 	 first data.  */
-- 
1.7.10.4