Re: [Qemu-devel] [PATCH for-4.0 v3 1/4] unify len and addr type for memory/address APIs

[Philippe Mathieu-Daudé]

Hi Li,

On 3/12/18 15:48, Li Zhijian wrote:
> Some address/memory APIs have different type between
> 'hwaddr/target_ulong addr' and 'int len'. It is very unsafety, espcially

I'm not native English speaker, but I think this should be spell:

... "very unsafe, especially" ...

> some APIs will be passed a non-int len by caller which might cause
> overflow quietly.
> Below is an potential overflow case:
>     dma_memory_read(uint32_t len)
>       -> dma_memory_rw(uint32_t len)
>         -> dma_memory_rw_relaxed(uint32_t len)
>           -> address_space_rw(int len) # len overflow
> 
> CC: Paolo Bonzini <address@hidden>
> CC: Peter Crosthwaite <address@hidden>
> CC: Richard Henderson <address@hidden>
> CC: Peter Maydell <address@hidden>
> Signed-off-by: Li Zhijian <address@hidden>
> 
> ---
> V3: use the same type between len and addr(Peter Maydell)
>     rebase code basing on https://patchew.org/QEMU/address@hidden/
> ---
>  exec.c                    | 47 
> +++++++++++++++++++++++------------------------
>  include/exec/cpu-all.h    |  2 +-
>  include/exec/cpu-common.h |  8 ++++----
>  include/exec/memory.h     | 22 +++++++++++-----------
>  4 files changed, 39 insertions(+), 40 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 6e875f0..f475974 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2848,10 +2848,10 @@ static const MemoryRegionOps watch_mem_ops = {
>  };
>  
>  static MemTxResult flatview_read(FlatView *fv, hwaddr addr,
> -                                      MemTxAttrs attrs, uint8_t *buf, int 
> len);
> +                                      MemTxAttrs attrs, uint8_t *buf, hwaddr 
> len);
>  static MemTxResult flatview_write(FlatView *fv, hwaddr addr, MemTxAttrs 
> attrs,
> -                                  const uint8_t *buf, int len);
> -static bool flatview_access_valid(FlatView *fv, hwaddr addr, int len,
> +                                  const uint8_t *buf, hwaddr len);
> +static bool flatview_access_valid(FlatView *fv, hwaddr addr, hwaddr len,
>                                    bool is_write, MemTxAttrs attrs);
>  
>  static MemTxResult subpage_read(void *opaque, hwaddr addr, uint64_t *data,
> @@ -3099,10 +3099,10 @@ MemoryRegion *get_system_io(void)
>  /* physical memory access (slow version, mainly for debug) */
>  #if defined(CONFIG_USER_ONLY)
>  int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
> -                        uint8_t *buf, int len, int is_write)
> +                        uint8_t *buf, target_ulong len, int is_write)
>  {
> -    int l, flags;
> -    target_ulong page;
> +    int flags;
> +    target_ulong l, page;
>      void * p;
>  
>      while (len > 0) {
> @@ -3215,7 +3215,7 @@ static bool prepare_mmio_access(MemoryRegion *mr)
>  static MemTxResult flatview_write_continue(FlatView *fv, hwaddr addr,
>                                             MemTxAttrs attrs,
>                                             const uint8_t *buf,
> -                                           int len, hwaddr addr1,
> +                                           hwaddr len, hwaddr addr1,
>                                             hwaddr l, MemoryRegion *mr)
>  {
>      uint8_t *ptr;
> @@ -3260,7 +3260,7 @@ static MemTxResult flatview_write_continue(FlatView 
> *fv, hwaddr addr,
>  
>  /* Called from RCU critical section.  */
>  static MemTxResult flatview_write(FlatView *fv, hwaddr addr, MemTxAttrs 
> attrs,
> -                                  const uint8_t *buf, int len)
> +                                  const uint8_t *buf, hwaddr len)
>  {
>      hwaddr l;
>      hwaddr addr1;
> @@ -3278,7 +3278,7 @@ static MemTxResult flatview_write(FlatView *fv, hwaddr 
> addr, MemTxAttrs attrs,
>  /* Called within RCU critical section.  */
>  MemTxResult flatview_read_continue(FlatView *fv, hwaddr addr,
>                                     MemTxAttrs attrs, uint8_t *buf,
> -                                   int len, hwaddr addr1, hwaddr l,
> +                                   hwaddr len, hwaddr addr1, hwaddr l,
>                                     MemoryRegion *mr)
>  {
>      uint8_t *ptr;
> @@ -3321,7 +3321,7 @@ MemTxResult flatview_read_continue(FlatView *fv, hwaddr 
> addr,
>  
>  /* Called from RCU critical section.  */
>  static MemTxResult flatview_read(FlatView *fv, hwaddr addr,
> -                                 MemTxAttrs attrs, uint8_t *buf, int len)
> +                                 MemTxAttrs attrs, uint8_t *buf, hwaddr len)
>  {
>      hwaddr l;
>      hwaddr addr1;
> @@ -3334,7 +3334,7 @@ static MemTxResult flatview_read(FlatView *fv, hwaddr 
> addr,
>  }
>  
>  MemTxResult address_space_read_full(AddressSpace *as, hwaddr addr,
> -                                    MemTxAttrs attrs, uint8_t *buf, int len)
> +                                    MemTxAttrs attrs, uint8_t *buf, hwaddr 
> len)
>  {
>      MemTxResult result = MEMTX_OK;
>      FlatView *fv;
> @@ -3351,7 +3351,7 @@ MemTxResult address_space_read_full(AddressSpace *as, 
> hwaddr addr,
>  
>  MemTxResult address_space_write(AddressSpace *as, hwaddr addr,
>                                  MemTxAttrs attrs,
> -                                const uint8_t *buf, int len)
> +                                const uint8_t *buf, hwaddr len)
>  {
>      MemTxResult result = MEMTX_OK;
>      FlatView *fv;
> @@ -3367,7 +3367,7 @@ MemTxResult address_space_write(AddressSpace *as, 
> hwaddr addr,
>  }
>  
>  MemTxResult address_space_rw(AddressSpace *as, hwaddr addr, MemTxAttrs attrs,
> -                             uint8_t *buf, int len, bool is_write)
> +                             uint8_t *buf, hwaddr len, bool is_write)
>  {
>      if (is_write) {
>          return address_space_write(as, addr, attrs, buf, len);
> @@ -3377,7 +3377,7 @@ MemTxResult address_space_rw(AddressSpace *as, hwaddr 
> addr, MemTxAttrs attrs,
>  }
>  
>  void cpu_physical_memory_rw(hwaddr addr, uint8_t *buf,
> -                            int len, int is_write)
> +                            hwaddr len, int is_write)
>  {
>      address_space_rw(&address_space_memory, addr, MEMTXATTRS_UNSPECIFIED,
>                       buf, len, is_write);
> @@ -3392,7 +3392,7 @@ static inline MemTxResult 
> address_space_write_rom_internal(AddressSpace *as,
>                                                             hwaddr addr,
>                                                             MemTxAttrs attrs,
>                                                             const uint8_t 
> *buf,
> -                                                           int len,
> +                                                           hwaddr len,
>                                                             enum 
> write_rom_type type)
>  {
>      hwaddr l;
> @@ -3432,13 +3432,13 @@ static inline MemTxResult 
> address_space_write_rom_internal(AddressSpace *as,
>  /* used for ROM loading : can write in RAM and ROM */
>  MemTxResult address_space_write_rom(AddressSpace *as, hwaddr addr,
>                                      MemTxAttrs attrs,
> -                                    const uint8_t *buf, int len)
> +                                    const uint8_t *buf, hwaddr len)
>  {
>      return address_space_write_rom_internal(as, addr, attrs,
>                                              buf, len, WRITE_DATA);
>  }
>  
> -void cpu_flush_icache_range(hwaddr start, int len)
> +void cpu_flush_icache_range(hwaddr start, hwaddr len)
>  {
>      /*
>       * This function should do the same thing as an icache flush that was
> @@ -3541,7 +3541,7 @@ static void cpu_notify_map_clients(void)
>      qemu_mutex_unlock(&map_client_list_lock);
>  }
>  
> -static bool flatview_access_valid(FlatView *fv, hwaddr addr, int len,
> +static bool flatview_access_valid(FlatView *fv, hwaddr addr, hwaddr len,
>                                    bool is_write, MemTxAttrs attrs)
>  {
>      MemoryRegion *mr;
> @@ -3564,7 +3564,7 @@ static bool flatview_access_valid(FlatView *fv, hwaddr 
> addr, int len,
>  }
>  
>  bool address_space_access_valid(AddressSpace *as, hwaddr addr,
> -                                int len, bool is_write,
> +                                hwaddr len, bool is_write,
>                                  MemTxAttrs attrs)
>  {
>      FlatView *fv;
> @@ -3817,7 +3817,7 @@ static inline MemoryRegion 
> *address_space_translate_cached(
>   */
>  void
>  address_space_read_cached_slow(MemoryRegionCache *cache, hwaddr addr,
> -                                   void *buf, int len)
> +                                   void *buf, hwaddr len)
>  {
>      hwaddr addr1, l;
>      MemoryRegion *mr;
> @@ -3835,7 +3835,7 @@ address_space_read_cached_slow(MemoryRegionCache 
> *cache, hwaddr addr,
>   */
>  void
>  address_space_write_cached_slow(MemoryRegionCache *cache, hwaddr addr,
> -                                    const void *buf, int len)
> +                                    const void *buf, hwaddr len)
>  {
>      hwaddr addr1, l;
>      MemoryRegion *mr;
> @@ -3858,11 +3858,10 @@ address_space_write_cached_slow(MemoryRegionCache 
> *cache, hwaddr addr,
>  
>  /* virtual memory access for debug (includes writing to ROM) */
>  int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
> -                        uint8_t *buf, int len, int is_write)
> +                        uint8_t *buf, target_ulong len, int is_write)
>  {
> -    int l;
>      hwaddr phys_addr;
> -    target_ulong page;
> +    target_ulong l, page;
>  
>      cpu_synchronize_state(cpu);
>      while (len > 0) {
> diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
> index 117d2fb..b16c9ec 100644
> --- a/include/exec/cpu-all.h
> +++ b/include/exec/cpu-all.h
> @@ -367,7 +367,7 @@ void dump_opcount_info(FILE *f, fprintf_function 
> cpu_fprintf);
>  #endif /* !CONFIG_USER_ONLY */
>  
>  int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
> -                        uint8_t *buf, int len, int is_write);
> +                        uint8_t *buf, target_ulong len, int is_write);
>  
>  int cpu_exec(CPUState *cpu);
>  
> diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
> index 2ad2d6d..63ec1f9 100644
> --- a/include/exec/cpu-common.h
> +++ b/include/exec/cpu-common.h
> @@ -83,14 +83,14 @@ size_t qemu_ram_pagesize(RAMBlock *block);
>  size_t qemu_ram_pagesize_largest(void);
>  
>  void cpu_physical_memory_rw(hwaddr addr, uint8_t *buf,
> -                            int len, int is_write);
> +                            hwaddr len, int is_write);
>  static inline void cpu_physical_memory_read(hwaddr addr,
> -                                            void *buf, int len)
> +                                            void *buf, hwaddr len)
>  {
>      cpu_physical_memory_rw(addr, buf, len, 0);
>  }
>  static inline void cpu_physical_memory_write(hwaddr addr,
> -                                             const void *buf, int len)
> +                                             const void *buf, hwaddr len)
>  {
>      cpu_physical_memory_rw(addr, (void *)buf, len, 1);
>  }
> @@ -111,7 +111,7 @@ bool cpu_physical_memory_is_io(hwaddr phys_addr);
>   */
>  void qemu_flush_coalesced_mmio_buffer(void);
>  
> -void cpu_flush_icache_range(hwaddr start, int len);
> +void cpu_flush_icache_range(hwaddr start, hwaddr len);
>  
>  extern struct MemoryRegion io_mem_rom;
>  extern struct MemoryRegion io_mem_notdirty;
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index ffd23ed..6235f77 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -1773,7 +1773,7 @@ void address_space_destroy(AddressSpace *as);
>   */
>  MemTxResult address_space_rw(AddressSpace *as, hwaddr addr,
>                               MemTxAttrs attrs, uint8_t *buf,
> -                             int len, bool is_write);
> +                             hwaddr len, bool is_write);
>  
>  /**
>   * address_space_write: write to address space.
> @@ -1790,7 +1790,7 @@ MemTxResult address_space_rw(AddressSpace *as, hwaddr 
> addr,
>   */
>  MemTxResult address_space_write(AddressSpace *as, hwaddr addr,
>                                  MemTxAttrs attrs,
> -                                const uint8_t *buf, int len);
> +                                const uint8_t *buf, hwaddr len);
>  
>  /**
>   * address_space_write_rom: write to address space, including ROM.
> @@ -1816,7 +1816,7 @@ MemTxResult address_space_write(AddressSpace *as, 
> hwaddr addr,
>   */
>  MemTxResult address_space_write_rom(AddressSpace *as, hwaddr addr,
>                                      MemTxAttrs attrs,
> -                                    const uint8_t *buf, int len);
> +                                    const uint8_t *buf, hwaddr len);
>  
>  /* address_space_ld*: load from an address space
>   * address_space_st*: store to an address space
> @@ -2017,7 +2017,7 @@ static inline MemoryRegion 
> *address_space_translate(AddressSpace *as,
>   * @is_write: indicates the transfer direction
>   * @attrs: memory attributes
>   */
> -bool address_space_access_valid(AddressSpace *as, hwaddr addr, int len,
> +bool address_space_access_valid(AddressSpace *as, hwaddr addr, hwaddr len,
>                                  bool is_write, MemTxAttrs attrs);
>  
>  /* address_space_map: map a physical memory region into a host virtual 
> address
> @@ -2054,19 +2054,19 @@ void address_space_unmap(AddressSpace *as, void 
> *buffer, hwaddr len,
>  
>  /* Internal functions, part of the implementation of address_space_read.  */
>  MemTxResult address_space_read_full(AddressSpace *as, hwaddr addr,
> -                                    MemTxAttrs attrs, uint8_t *buf, int len);
> +                                    MemTxAttrs attrs, uint8_t *buf, hwaddr 
> len);
>  MemTxResult flatview_read_continue(FlatView *fv, hwaddr addr,
>                                     MemTxAttrs attrs, uint8_t *buf,
> -                                   int len, hwaddr addr1, hwaddr l,
> +                                   hwaddr len, hwaddr addr1, hwaddr l,
>                                     MemoryRegion *mr);
>  void *qemu_map_ram_ptr(RAMBlock *ram_block, ram_addr_t addr);
>  
>  /* Internal functions, part of the implementation of 
> address_space_read_cached
>   * and address_space_write_cached.  */
>  void address_space_read_cached_slow(MemoryRegionCache *cache,
> -                                    hwaddr addr, void *buf, int len);
> +                                    hwaddr addr, void *buf, hwaddr len);
>  void address_space_write_cached_slow(MemoryRegionCache *cache,
> -                                     hwaddr addr, const void *buf, int len);
> +                                     hwaddr addr, const void *buf, hwaddr 
> len);
>  
>  static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
>  {
> @@ -2094,7 +2094,7 @@ static inline bool memory_access_is_direct(MemoryRegion 
> *mr, bool is_write)
>  static inline __attribute__((__always_inline__))
>  MemTxResult address_space_read(AddressSpace *as, hwaddr addr,
>                                 MemTxAttrs attrs, uint8_t *buf,
> -                               int len)
> +                               hwaddr len)
>  {
>      MemTxResult result = MEMTX_OK;
>      hwaddr l, addr1;
> @@ -2133,7 +2133,7 @@ MemTxResult address_space_read(AddressSpace *as, hwaddr 
> addr,
>   */
>  static inline void
>  address_space_read_cached(MemoryRegionCache *cache, hwaddr addr,
> -                          void *buf, int len)
> +                          void *buf, hwaddr len)
>  {
>      assert(addr < cache->len && len <= cache->len - addr);
>      if (likely(cache->ptr)) {
> @@ -2153,7 +2153,7 @@ address_space_read_cached(MemoryRegionCache *cache, 
> hwaddr addr,
>   */
>  static inline void
>  address_space_write_cached(MemoryRegionCache *cache, hwaddr addr,
> -                           void *buf, int len)
> +                           void *buf, hwaddr len)
>  {
>      assert(addr < cache->len && len <= cache->len - addr);
>      if (likely(cache->ptr)) {
>