Re: [PATCH] hw/elf_ops.h: switch to ssize_t for elf loader return type

[Luc Michel]

On 21:28 Wed 06 Oct     , Luc Michel wrote:
> Until now, int was used as the return type for all the ELF
> loader related functions. The returned value is the sum of all loaded
> program headers "MemSize" fields.
> 
> Because of the overflow check in elf_ops.h, trying to load an ELF bigger
> than INT_MAX will fail. Switch to ssize_t to remove this limitation.
> 
> Signed-off-by: Luc Michel <lmichel@kalray.eu>

Ping?
Cc'ing qemu-trivial. I guess it's simple enough.

Thanks.

-- 
Luc

> ---
>  include/hw/elf_ops.h | 25 +++++++++---------
>  include/hw/loader.h  | 60 ++++++++++++++++++++++----------------------
>  hw/core/loader.c     | 60 +++++++++++++++++++++++---------------------
>  3 files changed, 74 insertions(+), 71 deletions(-)
> 
> diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
> index 1c37cec4ae..5c2ea0339e 100644
> --- a/include/hw/elf_ops.h
> +++ b/include/hw/elf_ops.h
> @@ -310,24 +310,25 @@ static struct elf_note *glue(get_elf_note_type, 
> SZ)(struct elf_note *nhdr,
>      }
>  
>      return nhdr;
>  }
>  
> -static int glue(load_elf, SZ)(const char *name, int fd,
> -                              uint64_t (*elf_note_fn)(void *, void *, bool),
> -                              uint64_t (*translate_fn)(void *, uint64_t),
> -                              void *translate_opaque,
> -                              int must_swab, uint64_t *pentry,
> -                              uint64_t *lowaddr, uint64_t *highaddr,
> -                              uint32_t *pflags, int elf_machine,
> -                              int clear_lsb, int data_swab,
> -                              AddressSpace *as, bool load_rom,
> -                              symbol_fn_t sym_cb)
> +static ssize_t glue(load_elf, SZ)(const char *name, int fd,
> +                                  uint64_t (*elf_note_fn)(void *, void *, 
> bool),
> +                                  uint64_t (*translate_fn)(void *, uint64_t),
> +                                  void *translate_opaque,
> +                                  int must_swab, uint64_t *pentry,
> +                                  uint64_t *lowaddr, uint64_t *highaddr,
> +                                  uint32_t *pflags, int elf_machine,
> +                                  int clear_lsb, int data_swab,
> +                                  AddressSpace *as, bool load_rom,
> +                                  symbol_fn_t sym_cb)
>  {
>      struct elfhdr ehdr;
>      struct elf_phdr *phdr = NULL, *ph;
> -    int size, i, total_size;
> +    int size, i;
> +    ssize_t total_size;
>      elf_word mem_size, file_size, data_offset;
>      uint64_t addr, low = (uint64_t)-1, high = 0;
>      GMappedFile *mapped_file = NULL;
>      uint8_t *data = NULL;
>      int ret = ELF_LOAD_FAILED;
> @@ -480,11 +481,11 @@ static int glue(load_elf, SZ)(const char *name, int fd,
>                          }
>                      }
>                  }
>              }
>  
> -            if (mem_size > INT_MAX - total_size) {
> +            if (mem_size > SSIZE_MAX - total_size) {
>                  ret = ELF_LOAD_TOO_BIG;
>                  goto fail;
>              }
>  
>              /* address_offset is hack for kernel images that are
> diff --git a/include/hw/loader.h b/include/hw/loader.h
> index 81104cb02f..4fa485bd61 100644
> --- a/include/hw/loader.h
> +++ b/include/hw/loader.h
> @@ -88,11 +88,11 @@ int load_image_gzipped(const char *filename, hwaddr addr, 
> uint64_t max_sz);
>  #define ELF_LOAD_FAILED       -1
>  #define ELF_LOAD_NOT_ELF      -2
>  #define ELF_LOAD_WRONG_ARCH   -3
>  #define ELF_LOAD_WRONG_ENDIAN -4
>  #define ELF_LOAD_TOO_BIG      -5
> -const char *load_elf_strerror(int error);
> +const char *load_elf_strerror(ssize_t error);
>  
>  /** load_elf_ram_sym:
>   * @filename: Path of ELF file
>   * @elf_note_fn: optional function to parse ELF Note type
>   *               passed via @translate_opaque
> @@ -126,52 +126,52 @@ const char *load_elf_strerror(int error);
>   * ELF header and no checks will be carried out against the machine type.
>   */
>  typedef void (*symbol_fn_t)(const char *st_name, int st_info,
>                              uint64_t st_value, uint64_t st_size);
>  
> -int load_elf_ram_sym(const char *filename,
> +ssize_t load_elf_ram_sym(const char *filename,
> +                         uint64_t (*elf_note_fn)(void *, void *, bool),
> +                         uint64_t (*translate_fn)(void *, uint64_t),
> +                         void *translate_opaque, uint64_t *pentry,
> +                         uint64_t *lowaddr, uint64_t *highaddr,
> +                         uint32_t *pflags, int big_endian, int elf_machine,
> +                         int clear_lsb, int data_swab,
> +                         AddressSpace *as, bool load_rom, symbol_fn_t 
> sym_cb);
> +
> +/** load_elf_ram:
> + * Same as load_elf_ram_sym(), but doesn't allow the caller to specify a
> + * symbol callback function
> + */
> +ssize_t load_elf_ram(const char *filename,
>                       uint64_t (*elf_note_fn)(void *, void *, bool),
>                       uint64_t (*translate_fn)(void *, uint64_t),
>                       void *translate_opaque, uint64_t *pentry,
>                       uint64_t *lowaddr, uint64_t *highaddr, uint32_t *pflags,
> -                     int big_endian, int elf_machine,
> -                     int clear_lsb, int data_swab,
> -                     AddressSpace *as, bool load_rom, symbol_fn_t sym_cb);
> -
> -/** load_elf_ram:
> - * Same as load_elf_ram_sym(), but doesn't allow the caller to specify a
> - * symbol callback function
> - */
> -int load_elf_ram(const char *filename,
> -                 uint64_t (*elf_note_fn)(void *, void *, bool),
> -                 uint64_t (*translate_fn)(void *, uint64_t),
> -                 void *translate_opaque, uint64_t *pentry, uint64_t *lowaddr,
> -                 uint64_t *highaddr, uint32_t *pflags, int big_endian,
> -                 int elf_machine, int clear_lsb, int data_swab,
> -                 AddressSpace *as, bool load_rom);
> +                     int big_endian, int elf_machine, int clear_lsb,
> +                     int data_swab, AddressSpace *as, bool load_rom);
>  
>  /** load_elf_as:
>   * Same as load_elf_ram(), but always loads the elf as ROM
>   */
> -int load_elf_as(const char *filename,
> -                uint64_t (*elf_note_fn)(void *, void *, bool),
> -                uint64_t (*translate_fn)(void *, uint64_t),
> -                void *translate_opaque, uint64_t *pentry, uint64_t *lowaddr,
> -                uint64_t *highaddr, uint32_t *pflags, int big_endian,
> -                int elf_machine, int clear_lsb, int data_swab,
> -                AddressSpace *as);
> +ssize_t load_elf_as(const char *filename,
> +                    uint64_t (*elf_note_fn)(void *, void *, bool),
> +                    uint64_t (*translate_fn)(void *, uint64_t),
> +                    void *translate_opaque, uint64_t *pentry, uint64_t 
> *lowaddr,
> +                    uint64_t *highaddr, uint32_t *pflags, int big_endian,
> +                    int elf_machine, int clear_lsb, int data_swab,
> +                    AddressSpace *as);
>  
>  /** load_elf:
>   * Same as load_elf_as(), but doesn't allow the caller to specify an
>   * AddressSpace.
>   */
> -int load_elf(const char *filename,
> -             uint64_t (*elf_note_fn)(void *, void *, bool),
> -             uint64_t (*translate_fn)(void *, uint64_t),
> -             void *translate_opaque, uint64_t *pentry, uint64_t *lowaddr,
> -             uint64_t *highaddr, uint32_t *pflags, int big_endian,
> -             int elf_machine, int clear_lsb, int data_swab);
> +ssize_t load_elf(const char *filename,
> +                 uint64_t (*elf_note_fn)(void *, void *, bool),
> +                 uint64_t (*translate_fn)(void *, uint64_t),
> +                 void *translate_opaque, uint64_t *pentry, uint64_t *lowaddr,
> +                 uint64_t *highaddr, uint32_t *pflags, int big_endian,
> +                 int elf_machine, int clear_lsb, int data_swab);
>  
>  /** load_elf_hdr:
>   * @filename: Path of ELF file
>   * @hdr: Buffer to populate with header data. Header data will not be
>   * filled if set to NULL.
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index c623318b73..c7f97fdce8 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -324,11 +324,11 @@ static void *load_at(int fd, off_t offset, size_t size)
>  #define elf_sword        int64_t
>  #define bswapSZs     bswap64s
>  #define SZ           64
>  #include "hw/elf_ops.h"
>  
> -const char *load_elf_strerror(int error)
> +const char *load_elf_strerror(ssize_t error)
>  {
>      switch (error) {
>      case 0:
>          return "No error";
>      case ELF_LOAD_FAILED:
> @@ -400,62 +400,64 @@ void load_elf_hdr(const char *filename, void *hdr, bool 
> *is64, Error **errp)
>  fail:
>      close(fd);
>  }
>  
>  /* return < 0 if error, otherwise the number of bytes loaded in memory */
> -int load_elf(const char *filename,
> -             uint64_t (*elf_note_fn)(void *, void *, bool),
> -             uint64_t (*translate_fn)(void *, uint64_t),
> -             void *translate_opaque, uint64_t *pentry, uint64_t *lowaddr,
> -             uint64_t *highaddr, uint32_t *pflags, int big_endian,
> -             int elf_machine, int clear_lsb, int data_swab)
> +ssize_t load_elf(const char *filename,
> +                 uint64_t (*elf_note_fn)(void *, void *, bool),
> +                 uint64_t (*translate_fn)(void *, uint64_t),
> +                 void *translate_opaque, uint64_t *pentry, uint64_t *lowaddr,
> +                 uint64_t *highaddr, uint32_t *pflags, int big_endian,
> +                 int elf_machine, int clear_lsb, int data_swab)
>  {
>      return load_elf_as(filename, elf_note_fn, translate_fn, translate_opaque,
>                         pentry, lowaddr, highaddr, pflags, big_endian,
>                         elf_machine, clear_lsb, data_swab, NULL);
>  }
>  
>  /* return < 0 if error, otherwise the number of bytes loaded in memory */
> -int load_elf_as(const char *filename,
> -                uint64_t (*elf_note_fn)(void *, void *, bool),
> -                uint64_t (*translate_fn)(void *, uint64_t),
> -                void *translate_opaque, uint64_t *pentry, uint64_t *lowaddr,
> -                uint64_t *highaddr, uint32_t *pflags, int big_endian,
> -                int elf_machine, int clear_lsb, int data_swab, AddressSpace 
> *as)
> +ssize_t load_elf_as(const char *filename,
> +                    uint64_t (*elf_note_fn)(void *, void *, bool),
> +                    uint64_t (*translate_fn)(void *, uint64_t),
> +                    void *translate_opaque, uint64_t *pentry, uint64_t 
> *lowaddr,
> +                    uint64_t *highaddr, uint32_t *pflags, int big_endian,
> +                    int elf_machine, int clear_lsb, int data_swab,
> +                    AddressSpace *as)
>  {
>      return load_elf_ram(filename, elf_note_fn, translate_fn, 
> translate_opaque,
>                          pentry, lowaddr, highaddr, pflags, big_endian,
>                          elf_machine, clear_lsb, data_swab, as, true);
>  }
>  
>  /* return < 0 if error, otherwise the number of bytes loaded in memory */
> -int load_elf_ram(const char *filename,
> -                 uint64_t (*elf_note_fn)(void *, void *, bool),
> -                 uint64_t (*translate_fn)(void *, uint64_t),
> -                 void *translate_opaque, uint64_t *pentry, uint64_t *lowaddr,
> -                 uint64_t *highaddr, uint32_t *pflags, int big_endian,
> -                 int elf_machine, int clear_lsb, int data_swab,
> -                 AddressSpace *as, bool load_rom)
> +ssize_t load_elf_ram(const char *filename,
> +                     uint64_t (*elf_note_fn)(void *, void *, bool),
> +                     uint64_t (*translate_fn)(void *, uint64_t),
> +                     void *translate_opaque, uint64_t *pentry,
> +                     uint64_t *lowaddr, uint64_t *highaddr, uint32_t *pflags,
> +                     int big_endian, int elf_machine, int clear_lsb,
> +                     int data_swab, AddressSpace *as, bool load_rom)
>  {
>      return load_elf_ram_sym(filename, elf_note_fn,
>                              translate_fn, translate_opaque,
>                              pentry, lowaddr, highaddr, pflags, big_endian,
>                              elf_machine, clear_lsb, data_swab, as,
>                              load_rom, NULL);
>  }
>  
>  /* return < 0 if error, otherwise the number of bytes loaded in memory */
> -int load_elf_ram_sym(const char *filename,
> -                     uint64_t (*elf_note_fn)(void *, void *, bool),
> -                     uint64_t (*translate_fn)(void *, uint64_t),
> -                     void *translate_opaque, uint64_t *pentry,
> -                     uint64_t *lowaddr, uint64_t *highaddr, uint32_t *pflags,
> -                     int big_endian, int elf_machine,
> -                     int clear_lsb, int data_swab,
> -                     AddressSpace *as, bool load_rom, symbol_fn_t sym_cb)
> +ssize_t load_elf_ram_sym(const char *filename,
> +                         uint64_t (*elf_note_fn)(void *, void *, bool),
> +                         uint64_t (*translate_fn)(void *, uint64_t),
> +                         void *translate_opaque, uint64_t *pentry,
> +                         uint64_t *lowaddr, uint64_t *highaddr,
> +                         uint32_t *pflags, int big_endian, int elf_machine,
> +                         int clear_lsb, int data_swab,
> +                         AddressSpace *as, bool load_rom, symbol_fn_t sym_cb)
>  {
> -    int fd, data_order, target_data_order, must_swab, ret = ELF_LOAD_FAILED;
> +    int fd, data_order, target_data_order, must_swab;
> +    ssize_t ret = ELF_LOAD_FAILED;
>      uint8_t e_ident[EI_NIDENT];
>  
>      fd = open(filename, O_RDONLY | O_BINARY);
>      if (fd < 0) {
>          perror(filename);
> -- 
> 2.17.1
> 

--