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