[v2,1/5] unify len and addr type for memory/address APIs

Commit Message

Li Zhijian Nov. 21, 2018, 2:06 a.m. UTC

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 <[e-mail]>
CC: Peter Crosthwaite <[e-mail]>
CC: Richard Henderson <[e-mail]>
Signed-off-by: Li Zhijian <[e-mail]>
---
 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(-)

Comments

Peter Maydell Nov. 30, 2018, 1:40 p.m. UTC | #1

On Wed, 21 Nov 2018 at 02:07, Li Zhijian <[e-mail]> 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 <[e-mail]>
> CC: Peter Crosthwaite <[e-mail]>
> CC: Richard Henderson <[e-mail]>
> Signed-off-by: Li Zhijian <[e-mail]>

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/[e-mail]/

>  /* 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

Li Zhijian Dec. 3, 2018, 6:42 a.m. UTC | #2

On 11/30/2018 09:40 PM, Peter Maydell wrote:
> On Wed, 21 Nov 2018 at 02:07, Li Zhijian <[e-mail]> 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 <[e-mail]>
>> CC: Peter Crosthwaite <[e-mail]>
>> CC: Richard Henderson <[e-mail]>
>> Signed-off-by: Li Zhijian <[e-mail]>
> 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(-)
>>
>>
>> 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".

Got it

>
>> @@ -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.

Okay, i will update it

>
>> @@ -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/[e-mail]/

thanks a lot for the information.

>
>>   /* 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.

Got it



>>       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.

Got it

Thanks
Zhijian



> thanks
> -- PMM
>
>
> .
>

Patch

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)
 {
     CPUState *cpu = current_cpu;
     CPUClass *cc = CPU_GET_CLASS(cpu);
@@ -2848,10 +2849,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,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;
 
@@ -3215,7 +3217,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 +3262,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 +3280,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 +3323,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 +3336,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 +3353,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 +3369,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 +3379,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);
@@ -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;
@@ -3427,12 +3429,12 @@  static inline void cpu_physical_memory_write_rom_internal(AddressSpace *as,
 
 /* used for ROM loading : can write in RAM and ROM */
 void cpu_physical_memory_write_rom(AddressSpace *as, hwaddr addr,
-                                   const uint8_t *buf, int len)
+                                   const uint8_t *buf, hwaddr len)
 {
     cpu_physical_memory_write_rom_internal(as, addr, 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
@@ -3534,7 +3536,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;
@@ -3557,7 +3559,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;
@@ -3810,7 +3812,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;
@@ -3828,7 +3830,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;
@@ -3851,10 +3853,9 @@  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, hwaddr len, int is_write)
 {
-    int l;
-    hwaddr phys_addr;
+    hwaddr l, phys_addr;
     target_ulong page;
 
     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);
 
 int cpu_exec(CPUState *cpu);
 
diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
index 18b40d6..44b3554 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);
 }
@@ -112,8 +112,8 @@  bool cpu_physical_memory_is_io(hwaddr phys_addr);
 void qemu_flush_coalesced_mmio_buffer(void);
 
 void cpu_physical_memory_write_rom(AddressSpace *as, hwaddr addr,
-                                   const uint8_t *buf, int len);
-void cpu_flush_icache_range(hwaddr start, int len);
+                                   const uint8_t *buf, hwaddr 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 8e61450..841fa6f 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_ld*: load from an address space
  * address_space_st*: store to an address space
@@ -1991,7 +1991,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
@@ -2028,19 +2028,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)
 {
@@ -2068,7 +2068,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;
@@ -2107,7 +2107,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)) {
@@ -2127,7 +2127,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)) {