Re: [PATCH v7 00/22] XSA55 libelf fixes for unstable

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 11/06/13 19:20, Ian Jackson wrote:
> This is version 7 of my series to try to fix libelf and the domain
> loader.

Fantastic.  This entire series is now

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

I shall get the latest version into testing, but given the delta I do
not expect any issues.

~Andrew

>
> This is available via git:
>   http://xenbits.xen.org/gitweb/?p=people/iwj/xen-unstable.git;a=summary
>   git://xenbits.xen.org/people/iwj/xen-unstable.git
> in the commits
>   xsa55-unstable-base-rebasing..xsa55-unstable-rebasing
>
> Here is a summary of the state of series:
>
> A  01/21 libelf: abolish libelf-relocate.c
>    02/21 libxc: introduce xc_dom_seg_to_ptr_pages
>    03/21 libxc: Fix range checking in xc_dom_pfn_to_ptr etc.
> A  04/21 libelf: add `struct elf_binary*' parameter to elf_load_image
> A  05/21 libelf: abolish elf_sval and elf_access_signed
> A  06/21 libelf: move include of <asm/guest_access.h> to top of file
> A  07/21 libelf/xc_dom_load_elf_symtab: Do not use "syms" uninitialised
> A* 08/21 libxl: introduce macros for memory access and pointer handling
> A  09/21 tools/xcutils/readnotes: adjust print_l1_mfn_valid_note
> A- 10/21 libelf: check nul-terminated strings properly
>  - 11/21 libxl: check all pointer accesses
> A- 12/21 libxl: Check pointer references in elf_is_elfbinary
> A  13/21 libelf: Make all callers call elf_check_broken
> a  14/21 libelf: use C99 bool for booleans
>    15/21 libelf: use only unsigned integers
>    16/21 libelf: check loops for running away
> A  17/21 libelf: abolish obsolete macros
>    18/21 libxc: Add range checking to xc_dom_binloader
>  * 19/21 libxc: check failure of xc_dom_*_to_ptr, xc_map_foreign_range
>  * 20/21 libxc: check return values from malloc
>    21/21 libxc: range checks in xc_dom_p2m_host and _guest
>    22/22 libxc: check blob size before proceeding in xc_dom_check_gzip
>
> Key to symbols:
>  *   Updated in this version of the series.
>  +   New patch in this version.
>  /   Updated but only to remove changes into a separate patch.
>  -   Updated with style, comment or commit message changes only.
>  a   Acked/reviwed by one reviewer.
>  A   Acked/reviwed by more than one reviewer.
> Also in every patch:
> Updated commit msgs to correct email address for me.
>
>
> libelf, and some of its callers, did not do nearly enough checking on
> their input.  Invalid inputs could cause signed integer arithmetic
> overflows and wild pointer dereferences.
>
> In this series we try to systematically eliminate this problem in a
> way which has a reasonable chance of (i) still accepting all
> previously-accepted ELF images (ii) not having remaining security
> bugs, in a form which can be reviewied to verify (i) and (ii).
>
> Additionally, we fix some related security bugs in the domain loading
> code.
>
> The approach is:
>
> (i) Remove all uses of signed integers (of any kind).  That
>     elmininates all integer overflows as sources of undefined
>     behaviour.  Of course it still means that we can get incorrect
>     values throughout the code.
>
> (ii) Replace all uses of pointers, both pointers into the supplied
>     ELF image, and pointers into the output (where we are loading)
>     by uintptr_t.  That eliminates all pointer arithmetic overflows as
>     sources of undefined behaviour.  Of course it still means that we
>     can get incorrect and unreasonable "pointer" values.
>
> (iii) But these pointer values will be in uintptr_t, which cannot be
>     simply dereferenced by mistake.  We will replace all dereferences
>     by macros which do range checking; out of range reads will read 0
>     and out of range writes will be ignored.  Happily most (but not
>     all) of the reads in the code already go through macros which
>     abstract endianness[1] and/or 32/64bitness.
>
>     [1] Although not all the accesses use endian-aware techniques so
>     in fact the code can't cope with foreign-endian ELFs.  This is a
>     problem for another day.
>
> (iv) Look for all loops and check that they are guaranteed to
>     terminate.
>
> To enable verification of correctness of these changes I provide them
> as a series roughly as follows:
>
> 1-6:
>    Pre-patches which make a few semantically neutral or semantically
>    correct changes.  For human review.
>
> 7: Introduces a set of macros to abstract away pointer arithmetic and
>    input and output image memory accesses in libelf.  Use these macros
>    everwhere they are applicable.  However, define the macros in a way
>    that corresponds to the existing code.  That this patch has no
>    functional change can be verified by comparing the before-and-after
>    assembler output from the compiler.
>
> 9. Introduce some macros for dealing with nul-terminated strings,
>    defined so as not to have any functional change at this stage.
>
> 10. Change the macro definitions, and introduce the new pseudopointer
>    types, pointer range checking, etc.  For close human review.  Each
>    macro change is justified in the commit message.  This patch
>    eliminates most of the potential wild pointer accesses.
>
> 8,11-12,14-15:
>    Smaller patches for human review, fixing some leftover bugs,
>    including ensuring that all loops terminate.
>
> 13: Eliminate signed integers.  Replace every "int", "int*_t",
>    "long" and most "char"s by corresponding unsigned types.  This
>     eliminates all integer arithmetic overflows.
>
> After this patch, libelf should be safe against hostile input:
>
>  * All arithmetic operations on values from the input file use
>    unsigned arithmetic which is guaranteed to be defined (although
>    it may of course result in wrong answers);
>
>  * All pointer accesses based on pointers to locations which depend on
>    the input file go via our range-checking accessors; accesses which
>    are not to the input or output regions are ignored (reads returning
>    0).
>
>  * The loops have been checked to ensure that they terminate and are at
>    worst O(image_size).
>
>  * Whenever an array variable was declared, the code has been manually
>    reviewed looking for possible out-of-bounds accesses.
>
> This is XSA-55.