Re: [Xen-devel] [PATCH 10/22] libelf: check nul-terminated strings properly

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 07/06/13 19:27, Ian Jackson wrote:
> It is not safe to simply take pointers into the ELF and use them as C
> pointers.  They might not be properly nul-terminated (and the pointers
> might be wild).
>
> So we are going to introduce a new function elf_strval for safely
> getting strings.  This will check that the addresses are in range and
> that there is a proper nul-terminated string.  Of course it might
> discover that there isn't.  In that case, it will be made to fail.
> This means that elf_note_name might fail, too.
>
> For the benefit of call sites which are just going to pass the value
> to a printf-like function, we provide elf_strfmt which returns
> "(invalid)" on failure rather than NULL.
>
> In this patch we introduce dummy definitions of these functions.  We
> introduce calls to elf_strval and elf_strfmt everywhere, and update
> all the call sites with appropriate error checking.
>
> There is not yet any semantic change, since before this patch all the
> places where we introduce elf_strval dereferenced the value anyway, so
> it mustn't have been NULL.
>
> In future patches, when elf_strval is made able return NULL, when it
> does so it will mark the elf "broken" so that an appropriate
> diagnostic can be printed.
>
> This is part of the fix to a security issue, XSA-55.
>
> Signed-off-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> Acked-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
>
> v2: Fix coding style, in one "if" statement.
> ---
>  tools/xcutils/readnotes.c          |   10 +++++++---
>  xen/common/libelf/libelf-dominfo.c |   13 ++++++++++---
>  xen/common/libelf/libelf-tools.c   |   10 +++++++---
>  xen/include/xen/libelf.h           |    7 +++++--
>  4 files changed, 29 insertions(+), 11 deletions(-)
>
> diff --git a/tools/xcutils/readnotes.c b/tools/xcutils/readnotes.c
> index 7ff2530..ca86ba5 100644
> --- a/tools/xcutils/readnotes.c
> +++ b/tools/xcutils/readnotes.c
> @@ -63,7 +63,7 @@ struct setup_header {
>  static void print_string_note(const char *prefix, struct elf_binary *elf,
>                             ELF_HANDLE_DECL(elf_note) note)
>  {
> -     printf("%s: %s\n", prefix, (char*)elf_note_desc(elf, note));
> +     printf("%s: %s\n", prefix, elf_strfmt(elf, elf_note_desc(elf, note)));
>  }
>  
>  static void print_numeric_note(const char *prefix, struct elf_binary *elf,
> @@ -103,10 +103,13 @@ static int print_notes(struct elf_binary *elf, 
> ELF_HANDLE_DECL(elf_note) start,
>  {
>       ELF_HANDLE_DECL(elf_note) note;
>       int notes_found = 0;
> +     const char *this_note_name;
>  
>       for ( note = start; ELF_HANDLE_PTRVAL(note) < ELF_HANDLE_PTRVAL(end); 
> note = elf_note_next(elf, note) )
>       {
> -             if (0 != strcmp(elf_note_name(elf, note), "Xen"))
> +             this_note_name = elf_note_name(elf, note);
> +             if (NULL == this_note_name ||
> +                 0 != strcmp(this_note_name, "Xen"))
>                       continue;

Consistency?

At various places in this patch, you differ between a single if
statement and two if statements to do this new extra checking for NULL.

The most concise would be

if ( !ptr || !strcmp(ptr, "Xen") )

but it could certainly be argued that that is perhaps too concise.

~Andrew

>  
>               notes_found++;
> @@ -294,7 +297,8 @@ int main(int argc, char **argv)
>  
>       shdr = elf_shdr_by_name(&elf, "__xen_guest");
>       if (ELF_HANDLE_VALID(shdr))
> -             printf("__xen_guest: %s\n", (char*)elf_section_start(&elf, 
> shdr));
> +             printf("__xen_guest: %s\n",
> +                       elf_strfmt(&elf, elf_section_start(&elf, shdr)));
>  
>       return 0;
>  }
> diff --git a/xen/common/libelf/libelf-dominfo.c 
> b/xen/common/libelf/libelf-dominfo.c
> index 566f6f9..ba0dc83 100644
> --- a/xen/common/libelf/libelf-dominfo.c
> +++ b/xen/common/libelf/libelf-dominfo.c
> @@ -137,7 +137,10 @@ int elf_xen_parse_note(struct elf_binary *elf,
>  
>      if ( note_desc[type].str )
>      {
> -        str = elf_note_desc(elf, note);
> +        str = elf_strval(elf, elf_note_desc(elf, note));
> +        if (str == NULL)
> +            /* elf_strval will mark elf broken if it fails so no need to log 
> */
> +            return 0;
>          elf_msg(elf, "%s: %s = \"%s\"\n", __FUNCTION__,
>                  note_desc[type].name, str);
>          parms->elf_notes[type].type = XEN_ENT_STR;
> @@ -220,6 +223,7 @@ static int elf_xen_parse_notes(struct elf_binary *elf,
>  {
>      int xen_elfnotes = 0;
>      ELF_HANDLE_DECL(elf_note) note;
> +    const char *note_name;
>  
>      parms->elf_note_start = start;
>      parms->elf_note_end   = end;
> @@ -227,7 +231,10 @@ static int elf_xen_parse_notes(struct elf_binary *elf,
>            ELF_HANDLE_PTRVAL(note) < parms->elf_note_end;
>            note = elf_note_next(elf, note) )
>      {
> -        if ( strcmp(elf_note_name(elf, note), "Xen") )
> +        note_name = elf_note_name(elf, note);
> +        if ( note_name == NULL )
> +            continue;
> +        if ( strcmp(note_name, "Xen") )
>              continue;
>          if ( elf_xen_parse_note(elf, parms, note) )
>              return -1;
> @@ -541,7 +548,7 @@ int elf_xen_parse(struct elf_binary *elf,
>                  parms->elf_note_start = ELF_INVALID_PTRVAL;
>                  parms->elf_note_end   = ELF_INVALID_PTRVAL;
>                  elf_msg(elf, "%s: __xen_guest: \"%s\"\n", __FUNCTION__,
> -                        parms->guest_info);
> +                        elf_strfmt(elf, parms->guest_info));
>                  elf_xen_parse_guest_info(elf, parms);
>                  break;
>              }
> diff --git a/xen/common/libelf/libelf-tools.c 
> b/xen/common/libelf/libelf-tools.c
> index bf68bcd..fa7dedd 100644
> --- a/xen/common/libelf/libelf-tools.c
> +++ b/xen/common/libelf/libelf-tools.c
> @@ -119,7 +119,7 @@ const char *elf_section_name(struct elf_binary *elf,
>      if ( ELF_PTRVAL_INVALID(elf->sec_strtab) )
>          return "unknown";
>  
> -    return elf->sec_strtab + elf_uval(elf, shdr, sh_name);
> +    return elf_strval(elf, elf->sec_strtab + elf_uval(elf, shdr, sh_name));
>  }
>  
>  ELF_PTRVAL_CONST_VOID elf_section_start(struct elf_binary *elf, 
> ELF_HANDLE_DECL(elf_shdr) shdr)
> @@ -151,6 +151,7 @@ ELF_HANDLE_DECL(elf_sym) elf_sym_by_name(struct 
> elf_binary *elf, const char *sym
>      ELF_PTRVAL_CONST_VOID end = elf_section_end(elf, elf->sym_tab);
>      ELF_HANDLE_DECL(elf_sym) sym;
>      uint64_t info, name;
> +    const char *sym_name;
>  
>      for ( ; ptr < end; ptr += elf_size(elf, sym) )
>      {
> @@ -159,7 +160,10 @@ ELF_HANDLE_DECL(elf_sym) elf_sym_by_name(struct 
> elf_binary *elf, const char *sym
>          name = elf_uval(elf, sym, st_name);
>          if ( ELF32_ST_BIND(info) != STB_GLOBAL )
>              continue;
> -        if ( strcmp(elf->sym_strtab + name, symbol) )
> +        sym_name = elf_strval(elf, elf->sym_strtab + name);
> +        if ( sym_name == NULL ) /* out of range, oops */
> +            return ELF_INVALID_HANDLE(elf_sym);
> +        if ( strcmp(sym_name, symbol) )
>              continue;
>          return sym;
>      }
> @@ -177,7 +181,7 @@ ELF_HANDLE_DECL(elf_sym) elf_sym_by_index(struct 
> elf_binary *elf, int index)
>  
>  const char *elf_note_name(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) 
> note)
>  {
> -    return ELF_HANDLE_PTRVAL(note) + elf_size(elf, note);
> +    return elf_strval(elf, ELF_HANDLE_PTRVAL(note) + elf_size(elf, note));
>  }
>  
>  ELF_PTRVAL_CONST_VOID elf_note_desc(struct elf_binary *elf, 
> ELF_HANDLE_DECL(elf_note) note)
> diff --git a/xen/include/xen/libelf.h b/xen/include/xen/libelf.h
> index dc9b5ae..0c2c11e 100644
> --- a/xen/include/xen/libelf.h
> +++ b/xen/include/xen/libelf.h
> @@ -252,6 +252,9 @@ uint64_t elf_access_unsigned(struct elf_binary *elf, 
> ELF_PTRVAL_CONST_VOID ptr,
>  uint64_t elf_round_up(struct elf_binary *elf, uint64_t addr);
>  
>  
> +#define elf_strval(elf,x) ((const char*)(x)) /* may return NULL in the 
> future */
> +#define elf_strfmt(elf,x) ((const char*)(x)) /* will return (invalid) 
> instead */
> +
>  #define elf_memcpy_safe(elf, dst, src, sz) memcpy((dst),(src),(sz))
>  #define elf_memset_safe(elf, dst, c, sz)   memset((dst),(c),(sz))
>    /*
> @@ -279,7 +282,7 @@ ELF_HANDLE_DECL(elf_shdr) elf_shdr_by_name(struct 
> elf_binary *elf, const char *n
>  ELF_HANDLE_DECL(elf_shdr) elf_shdr_by_index(struct elf_binary *elf, int 
> index);
>  ELF_HANDLE_DECL(elf_phdr) elf_phdr_by_index(struct elf_binary *elf, int 
> index);
>  
> -const char *elf_section_name(struct elf_binary *elf, 
> ELF_HANDLE_DECL(elf_shdr) shdr);
> +const char *elf_section_name(struct elf_binary *elf, 
> ELF_HANDLE_DECL(elf_shdr) shdr); /* might return NULL if inputs are invalid */
>  ELF_PTRVAL_CONST_VOID elf_section_start(struct elf_binary *elf, 
> ELF_HANDLE_DECL(elf_shdr) shdr);
>  ELF_PTRVAL_CONST_VOID elf_section_end(struct elf_binary *elf, 
> ELF_HANDLE_DECL(elf_shdr) shdr);
>  
> @@ -289,7 +292,7 @@ ELF_PTRVAL_CONST_VOID elf_segment_end(struct elf_binary 
> *elf, ELF_HANDLE_DECL(el
>  ELF_HANDLE_DECL(elf_sym) elf_sym_by_name(struct elf_binary *elf, const char 
> *symbol);
>  ELF_HANDLE_DECL(elf_sym) elf_sym_by_index(struct elf_binary *elf, int index);
>  
> -const char *elf_note_name(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) 
> note);
> +const char *elf_note_name(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) 
> note); /* may return NULL */
>  ELF_PTRVAL_CONST_VOID elf_note_desc(struct elf_binary *elf, 
> ELF_HANDLE_DECL(elf_note) note);
>  uint64_t elf_note_numeric(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) 
> note);
>  uint64_t elf_note_numeric_array(struct elf_binary *, 
> ELF_HANDLE_DECL(elf_note),


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel