Re: [PATCH 14/22] libelf: use C99 bool for booleans

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

On 11/06/13 19:20, Ian Jackson wrote:
> We want to remove uses of "int" because signed integers have
> undesirable undefined behaviours on overflow.  Malicious compilers can
> turn apparently-correct code into code with security vulnerabilities
> etc.
>
> In this patch we change all the booleans in libelf to C99 bool,
> from <stdbool.h>.
>
> For the one visible libelf boolean in libxc's public interface we
> retain the use of int to avoid changing the ABI; libxc converts it to
> a bool for consumption by libelf.
>
> It is OK to change all values only ever used as booleans to _Bool
> (bool) because conversion from any scalar type to a _Bool works the
> same as the boolean test in if() or ?: and is always defined (C99
> 6.3.1.2).  But we do need to check that all these variables really are
> only ever used that way.  (It is theoretically possible that the old
> code truncated some 64-bit values to 32-bit ints which might become
> zero depending on the value, which would mean a behavioural change in
> this patch, but it seems implausible that treating 0x????????00000000
> as false could have been intended.)
>
> This is part of the fix to a security issue, XSA-55.
>
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
> Acked-by: George Dunlap <george.dunlap@eu.citrix.com>

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

>
> v3: Use <stdbool.h>'s bool (or _Bool) instead of defining elf_bool.
>     Split this into a separate patch.
> ---
>  tools/libxc/xc_dom_elfloader.c     |    8 ++++----
>  xen/common/libelf/libelf-dominfo.c |    2 +-
>  xen/common/libelf/libelf-loader.c  |    4 ++--
>  xen/common/libelf/libelf-private.h |    2 +-
>  xen/common/libelf/libelf-tools.c   |   10 +++++-----
>  xen/include/xen/libelf.h           |   18 ++++++++++--------
>  6 files changed, 23 insertions(+), 21 deletions(-)
>
> diff --git a/tools/libxc/xc_dom_elfloader.c b/tools/libxc/xc_dom_elfloader.c
> index a0d39b3..8f9c2fb 100644
> --- a/tools/libxc/xc_dom_elfloader.c
> +++ b/tools/libxc/xc_dom_elfloader.c
> @@ -34,7 +34,7 @@
>  /* ------------------------------------------------------------------------ */
>  
>  static void log_callback(struct elf_binary *elf, void *caller_data,
> -                         int iserr, const char *fmt, va_list al) {
> +                         bool iserr, const char *fmt, va_list al) {
>      xc_interface *xch = caller_data;
>  
>      xc_reportv(xch,
> @@ -46,7 +46,7 @@ static void log_callback(struct elf_binary *elf, void *caller_data,
>  
>  void xc_elf_set_logfile(xc_interface *xch, struct elf_binary *elf,
>                          int verbose) {
> -    elf_set_log(elf, log_callback, xch, verbose);
> +    elf_set_log(elf, log_callback, xch, verbose /* convert to bool */);
>  }
>  
>  /* ------------------------------------------------------------------------ */
> @@ -82,7 +82,7 @@ static char *xc_dom_guest_type(struct xc_dom_image *dom,
>  /* ------------------------------------------------------------------------ */
>  /* parse elf binary                                                         */
>  
> -static int check_elf_kernel(struct xc_dom_image *dom, int verbose)
> +static int check_elf_kernel(struct xc_dom_image *dom, bool verbose)
>  {
>      if ( dom->kernel_blob == NULL )
>      {
> @@ -110,7 +110,7 @@ static int xc_dom_probe_elf_kernel(struct xc_dom_image *dom)
>  }
>  
>  static int xc_dom_load_elf_symtab(struct xc_dom_image *dom,
> -                                  struct elf_binary *elf, int load)
> +                                  struct elf_binary *elf, bool load)
>  {
>      struct elf_binary syms;
>      ELF_HANDLE_DECL_NONCONST(elf_shdr) shdr; ELF_HANDLE_DECL(elf_shdr) shdr2;
> diff --git a/xen/common/libelf/libelf-dominfo.c b/xen/common/libelf/libelf-dominfo.c
> index b9a4e25..c4ced67 100644
> --- a/xen/common/libelf/libelf-dominfo.c
> +++ b/xen/common/libelf/libelf-dominfo.c
> @@ -101,7 +101,7 @@ int elf_xen_parse_note(struct elf_binary *elf,
>  /* *INDENT-OFF* */
>      static const struct {
>          char *name;
> -        int str;
> +        bool str;
>      } note_desc[] = {
>          [XEN_ELFNOTE_ENTRY] = { "ENTRY", 0},
>          [XEN_ELFNOTE_HYPERCALL_PAGE] = { "HYPERCALL_PAGE", 0},
> diff --git a/xen/common/libelf/libelf-loader.c b/xen/common/libelf/libelf-loader.c
> index 6c43c34..798f88b 100644
> --- a/xen/common/libelf/libelf-loader.c
> +++ b/xen/common/libelf/libelf-loader.c
> @@ -92,7 +92,7 @@ int elf_init(struct elf_binary *elf, const char *image_input, size_t size)
>  }
>  
>  #ifndef __XEN__
> -void elf_call_log_callback(struct elf_binary *elf, int iserr,
> +void elf_call_log_callback(struct elf_binary *elf, bool iserr,
>                             const char *fmt,...) {
>      va_list al;
>  
> @@ -107,7 +107,7 @@ void elf_call_log_callback(struct elf_binary *elf, int iserr,
>  }
>      
>  void elf_set_log(struct elf_binary *elf, elf_log_callback *log_callback,
> -                 void *log_caller_data, int verbose)
> +                 void *log_caller_data, bool verbose)
>  {
>      elf->log_callback = log_callback;
>      elf->log_caller_data = log_caller_data;
> diff --git a/xen/common/libelf/libelf-private.h b/xen/common/libelf/libelf-private.h
> index 0bd9e66..ea7e197 100644
> --- a/xen/common/libelf/libelf-private.h
> +++ b/xen/common/libelf/libelf-private.h
> @@ -77,7 +77,7 @@
>  #define elf_err(elf, fmt, args ... )                    \
>      elf_call_log_callback(elf, 1, fmt , ## args );
>  
> -void elf_call_log_callback(struct elf_binary*, int iserr, const char *fmt,...);
> +void elf_call_log_callback(struct elf_binary*, bool iserr, const char *fmt,...);
>  
>  #define safe_strcpy(d,s)                        \
>  do { strncpy((d),(s),sizeof((d))-1);            \
> diff --git a/xen/common/libelf/libelf-tools.c b/xen/common/libelf/libelf-tools.c
> index b613593..0b7b2b6 100644
> --- a/xen/common/libelf/libelf-tools.c
> +++ b/xen/common/libelf/libelf-tools.c
> @@ -31,7 +31,7 @@ const char *elf_check_broken(const struct elf_binary *elf)
>      return elf->broken;
>  }
>  
> -static int elf_ptrval_in_range(elf_ptrval ptrval, uint64_t size,
> +static bool elf_ptrval_in_range(elf_ptrval ptrval, uint64_t size,
>                                 const void *region, uint64_t regionsize)
>      /*
>       * Returns true if the putative memory area [ptrval,ptrval+size>
> @@ -53,7 +53,7 @@ static int elf_ptrval_in_range(elf_ptrval ptrval, uint64_t size,
>      return 1;
>  }
>  
> -int elf_access_ok(struct elf_binary * elf,
> +bool elf_access_ok(struct elf_binary * elf,
>                    uint64_t ptrval, size_t size)
>  {
>      if ( elf_ptrval_in_range(ptrval, size, elf->image_base, elf->size) )
> @@ -92,7 +92,7 @@ uint64_t elf_access_unsigned(struct elf_binary * elf, elf_ptrval base,
>                               uint64_t moreoffset, size_t size)
>  {
>      elf_ptrval ptrval = base + moreoffset;
> -    int need_swap = elf_swap(elf);
> +    bool need_swap = elf_swap(elf);
>      const uint8_t *u8;
>      const uint16_t *u16;
>      const uint32_t *u32;
> @@ -332,7 +332,7 @@ ELF_HANDLE_DECL(elf_note) elf_note_next(struct elf_binary *elf, ELF_HANDLE_DECL(
>  
>  /* ------------------------------------------------------------------------ */
>  
> -int elf_is_elfbinary(const void *image_start, size_t image_size)
> +bool elf_is_elfbinary(const void *image_start, size_t image_size)
>  {
>      const Elf32_Ehdr *ehdr = image_start;
>  
> @@ -342,7 +342,7 @@ int elf_is_elfbinary(const void *image_start, size_t image_size)
>      return IS_ELF(*ehdr);
>  }
>  
> -int elf_phdr_is_loadable(struct elf_binary *elf, ELF_HANDLE_DECL(elf_phdr) phdr)
> +bool elf_phdr_is_loadable(struct elf_binary *elf, ELF_HANDLE_DECL(elf_phdr) phdr)
>  {
>      uint64_t p_type = elf_uval(elf, phdr, p_type);
>      uint64_t p_flags = elf_uval(elf, phdr, p_flags);
> diff --git a/xen/include/xen/libelf.h b/xen/include/xen/libelf.h
> index df93f2c..32b3ce2 100644
> --- a/xen/include/xen/libelf.h
> +++ b/xen/include/xen/libelf.h
> @@ -29,6 +29,8 @@
>  #error define architectural endianness
>  #endif
>  
> +#include <stdbool.h>
> +
>  #undef ELFSIZE
>  #include "elfstructs.h"
>  #ifdef __XEN__
> @@ -42,7 +44,7 @@
>  
>  struct elf_binary;
>  typedef void elf_log_callback(struct elf_binary*, void *caller_data,
> -                              int iserr, const char *fmt, va_list al);
> +                              bool iserr, const char *fmt, va_list al);
>  
>  #endif
>  
> @@ -237,7 +239,7 @@ struct elf_binary {
>      elf_log_callback *log_callback;
>      void *log_caller_data;
>  #endif
> -    int verbose;
> +    bool verbose;
>      const char *broken;
>  };
>  
> @@ -301,8 +303,8 @@ void elf_memset_safe(struct elf_binary*, elf_ptrval dst, int c, size_t);
>     * outside permitted areas.
>     */
>  
> -int elf_access_ok(struct elf_binary * elf,
> -                  uint64_t ptrval, size_t size);
> +bool elf_access_ok(struct elf_binary * elf,
> +                   uint64_t ptrval, size_t size);
>  
>  #define elf_store_val(elf, type, ptr, val)                              \
>      ({                                                                  \
> @@ -351,9 +353,9 @@ uint64_t elf_note_numeric_array(struct elf_binary *, ELF_HANDLE_DECL(elf_note),
>  ELF_HANDLE_DECL(elf_note) elf_note_next(struct elf_binary *elf, ELF_HANDLE_DECL(elf_note) note);
>  
>  /* (Only) checks that the image has the right magic number. */
> -int elf_is_elfbinary(const void *image_start, size_t image_size);
> +bool elf_is_elfbinary(const void *image_start, size_t image_size);
>  
> -int elf_phdr_is_loadable(struct elf_binary *elf, ELF_HANDLE_DECL(elf_phdr) phdr);
> +bool elf_phdr_is_loadable(struct elf_binary *elf, ELF_HANDLE_DECL(elf_phdr) phdr);
>  
>  /* ------------------------------------------------------------------------ */
>  /* xc_libelf_loader.c                                                       */
> @@ -367,7 +369,7 @@ int elf_init(struct elf_binary *elf, const char *image, size_t size);
>  void elf_set_verbose(struct elf_binary *elf);
>  #else
>  void elf_set_log(struct elf_binary *elf, elf_log_callback*,
> -                 void *log_caller_pointer, int verbose);
> +                 void *log_caller_pointer, bool verbose);
>  #endif
>  
>  void elf_parse_binary(struct elf_binary *elf);
> @@ -419,7 +421,7 @@ struct elf_dom_parms {
>      char xen_ver[16];
>      char loader[16];
>      int pae;
> -    int bsd_symtab;
> +    bool bsd_symtab;
>      uint64_t virt_base;
>      uint64_t virt_entry;
>      uint64_t virt_hypercall;