Re: [Qemu-devel] [PATCH v2 1/5] unify len and addr type for memory/address APIs

[Peter Maydell]

On Wed, 21 Nov 2018 at 02:07, Li Zhijian <address@hidden> wrote:
>
> Some address/memory APIs have different type between 'hwaddr addr' and
> 'int len'. It is very unsafety, espcially 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>
> Signed-off-by: Li Zhijian <address@hidden>

Hi; this patch is almost all good -- there's just a couple of
functions here which either don't need a change or should
be using something other than hwaddr, which I've commented on
below.

> ---
>  exec.c                    | 49 
> ++++++++++++++++++++++++-----------------------
>  include/exec/cpu-all.h    |  2 +-
>  include/exec/cpu-common.h | 10 +++++-----
>  include/exec/memory.h     | 20 +++++++++----------
>  4 files changed, 41 insertions(+), 40 deletions(-)
>
> diff --git a/exec.c b/exec.c
> index bb6170d..05823ae 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2719,7 +2719,8 @@ static const MemoryRegionOps notdirty_mem_ops = {
>  };
>
>  /* Generate a debug exception if a watchpoint has been hit.  */
> -static void check_watchpoint(int offset, int len, MemTxAttrs attrs, int 
> flags)
> +static void check_watchpoint(hwaddr offset,  unsigned len,
> +                             MemTxAttrs attrs, int flags)

This one doesn't need to change -- the offset is the offset within
the page, so it is always going to fit easily within an "int".

>  {
>      CPUState *cpu = current_cpu;
>      CPUClass *cc = CPU_GET_CLASS(cpu);


> @@ -3099,9 +3100,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, hwaddr len, int is_write)
>  {
> -    int l, flags;
> +    hwaddr l;
> +    int flags;
>      target_ulong page;
>      void * p;

This one is operating on guest virtual addresses, so len should
be a target_ulong, like the addr parameter, as should the "l" variable.

> @@ -3389,7 +3391,7 @@ enum write_rom_type {
>  };
>
>  static inline void cpu_physical_memory_write_rom_internal(AddressSpace *as,
> -    hwaddr addr, const uint8_t *buf, int len, enum write_rom_type type)
> +    hwaddr addr, const uint8_t *buf, hwaddr len, enum write_rom_type type)
>  {
>      hwaddr l;
>      uint8_t *ptr;

Just a note that I have a patchset on-list which renames this
function and cpu_physical_memory_write_rom(), so depending on
what order your patch and mine get into master one of us will
have to rebase (or Paolo might fix up the conflict for us). It's
not a difficult one to fix, so no big deal.

https://patchew.org/QEMU/address@hidden/

>  /* 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, hwaddr len, int is_write)
>  {
> -    int l;
> -    hwaddr phys_addr;
> +    hwaddr l, phys_addr;
>      target_ulong page;

Here again l and len should be target_ulong, as these are
virtual addresses.

>      cpu_synchronize_state(cpu);
> diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
> index 117d2fb..4b56672 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, hwaddr len, int is_write);

target_ulong.

thanks
-- PMM