|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 10/22] libelf: check nul-terminated strings properly
On 11/06/13 19:20, 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>
Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>
> v7: Change readnotes.c check to use two if statements rather than ||.
>
> v2: Fix coding style, in one "if" statement.
> ---
> tools/xcutils/readnotes.c | 11 ++++++++---
> xen/common/libelf/libelf-dominfo.c | 13 ++++++++++---
> xen/common/libelf/libelf-tools.c | 10 +++++++---
> xen/include/xen/libelf.h | 7 +++++--
> 4 files changed, 30 insertions(+), 11 deletions(-)
>
> diff --git a/tools/xcutils/readnotes.c b/tools/xcutils/readnotes.c
> index 7ff2530..cfae994 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,14 @@ 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)
> + continue;
> + if (0 != strcmp(this_note_name, "Xen"))
> continue;
>
> notes_found++;
> @@ -294,7 +298,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 7bd3bdb..28c7b11 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
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |