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

[Li Zhijian]

On 11/30/2018 09:40 PM, Peter Maydell wrote:

On Wed, 21 Nov 2018 at 02:07, Li Zhijian <lizhij...@cn.fujitsu.com> 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 <pbonz...@redhat.com>
CC: Peter Crosthwaite <crosthwaite.pe...@gmail.com>
CC: Richard Henderson <r...@twiddle.net>
Signed-off-by: Li Zhijian <lizhij...@cn.fujitsu.com>

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/20181122133507.30950-1-peter.mayd...@linaro.org/

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


.