From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH 14/22] libelf: use C99 bool for booleans Date: Tue, 11 Jun 2013 20:04:10 +0100 Message-ID: <51B774AA.1030408@citrix.com> References: <1370974865-19554-1-git-send-email-ian.jackson@eu.citrix.com> <1370974865-19554-15-git-send-email-ian.jackson@eu.citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1370974865-19554-15-git-send-email-ian.jackson@eu.citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Jackson Cc: "xen-devel@lists.xensource.com" , "mattjd@gmail.com" , "security@xen.org" List-Id: xen-devel@lists.xenproject.org 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 . > > 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 > Acked-by: George Dunlap Reviewed-by: Andrew Cooper > > v3: Use '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 > + > #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;