| Message ID | 20220628045403.508716-3-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL,01/60] semihosting: Move exec/softmmu-semi.h to semihosting/softmmu-uaccess.h | expand |
On Tue, 28 Jun 2022 at 05:54, Richard Henderson <richard.henderson@linaro.org> wrote: > > We were reporting unconditional success for these functions; > pass on any failure from cpu_memory_rw_debug. > > Reviewed-by: Peter Maydell <peter.maydell@linaro.org> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> So, this commit makes us check the cpu_memory_rw_debug() return value in the softmmu_lock_user() function; but Coverity points out (CID 1490294) that we still aren't checking the return value from when we call it in softmmu_unlock_user()... What, if anything, should we do about that? In particular, I think that for semihosting calls that write to guest memory, if the guest passes us a pointer to read-only memory we're not going to find out about that until unlock time. thanks -- PMM
On 7/29/22 07:31, Peter Maydell wrote: > On Tue, 28 Jun 2022 at 05:54, Richard Henderson > <richard.henderson@linaro.org> wrote: >> >> We were reporting unconditional success for these functions; >> pass on any failure from cpu_memory_rw_debug. >> >> Reviewed-by: Peter Maydell <peter.maydell@linaro.org> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > > So, this commit makes us check the cpu_memory_rw_debug() > return value in the softmmu_lock_user() function; but Coverity > points out (CID 1490294) that we still aren't checking the > return value from when we call it in softmmu_unlock_user()... > > What, if anything, should we do about that? In particular, > I think that for semihosting calls that write to guest memory, > if the guest passes us a pointer to read-only memory we're > not going to find out about that until unlock time. Not even then, because address_space_write_rom will in fact write to rom, nevermind virtual page permissions. Moreover, there are no failure conditions from address_space_write_rom. It skips non-{ram,rom} and always returns OK. It's probable that we should be using cpu_memory_rw_debug at all, but should be using a higher-level interface, something that (1) checks the virtual page permissions and (2) probably rejects semihosting to mmio. But in the short term, I think we can just ignore the warning. r~
On 28/6/22 06:53, Richard Henderson wrote: > We were reporting unconditional success for these functions; > pass on any failure from cpu_memory_rw_debug. > > Reviewed-by: Peter Maydell <peter.maydell@linaro.org> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > include/semihosting/softmmu-uaccess.h | 91 ++++++++++++--------------- > 1 file changed, 39 insertions(+), 52 deletions(-) > > diff --git a/include/semihosting/softmmu-uaccess.h b/include/semihosting/softmmu-uaccess.h > index e69e3c8548..5246a91570 100644 > --- a/include/semihosting/softmmu-uaccess.h > +++ b/include/semihosting/softmmu-uaccess.h > @@ -12,82 +12,69 @@ > > #include "cpu.h" > > -static inline uint64_t softmmu_tget64(CPUArchState *env, target_ulong addr) > -{ > - uint64_t val; > +#define get_user_u64(val, addr) \ These macros match their user emulation equivalent, at the price of loosing the 'env' argument, still used. This confuses meta transformation tools such Coccinelle spatch. Not a big deal, just FTR. > + ({ uint64_t val_ = 0; \ > + int ret_ = cpu_memory_rw_debug(env_cpu(env), (addr), \ ^^^ > + &val_, sizeof(val_), 0); \ > + (val) = tswap64(val_); ret_; }) > > - cpu_memory_rw_debug(env_cpu(env), addr, (uint8_t *)&val, 8, 0); > - return tswap64(val); > -}
diff --git a/include/semihosting/softmmu-uaccess.h b/include/semihosting/softmmu-uaccess.h index e69e3c8548..5246a91570 100644 --- a/include/semihosting/softmmu-uaccess.h +++ b/include/semihosting/softmmu-uaccess.h @@ -12,82 +12,69 @@ #include "cpu.h" -static inline uint64_t softmmu_tget64(CPUArchState *env, target_ulong addr) -{ - uint64_t val; +#define get_user_u64(val, addr) \ + ({ uint64_t val_ = 0; \ + int ret_ = cpu_memory_rw_debug(env_cpu(env), (addr), \ + &val_, sizeof(val_), 0); \ + (val) = tswap64(val_); ret_; }) - cpu_memory_rw_debug(env_cpu(env), addr, (uint8_t *)&val, 8, 0); - return tswap64(val); -} +#define get_user_u32(val, addr) \ + ({ uint32_t val_ = 0; \ + int ret_ = cpu_memory_rw_debug(env_cpu(env), (addr), \ + &val_, sizeof(val_), 0); \ + (val) = tswap32(val_); ret_; }) -static inline uint32_t softmmu_tget32(CPUArchState *env, target_ulong addr) -{ - uint32_t val; +#define get_user_u8(val, addr) \ + ({ uint8_t val_ = 0; \ + int ret_ = cpu_memory_rw_debug(env_cpu(env), (addr), \ + &val_, sizeof(val_), 0); \ + (val) = val_; ret_; }) - cpu_memory_rw_debug(env_cpu(env), addr, (uint8_t *)&val, 4, 0); - return tswap32(val); -} - -static inline uint32_t softmmu_tget8(CPUArchState *env, target_ulong addr) -{ - uint8_t val; - - cpu_memory_rw_debug(env_cpu(env), addr, &val, 1, 0); - return val; -} - -#define get_user_u64(arg, p) ({ arg = softmmu_tget64(env, p); 0; }) -#define get_user_u32(arg, p) ({ arg = softmmu_tget32(env, p) ; 0; }) -#define get_user_u8(arg, p) ({ arg = softmmu_tget8(env, p) ; 0; }) #define get_user_ual(arg, p) get_user_u32(arg, p) -static inline void softmmu_tput64(CPUArchState *env, - target_ulong addr, uint64_t val) -{ - val = tswap64(val); - cpu_memory_rw_debug(env_cpu(env), addr, (uint8_t *)&val, 8, 1); -} +#define put_user_u64(val, addr) \ + ({ uint64_t val_ = tswap64(val); \ + cpu_memory_rw_debug(env_cpu(env), (addr), &val_, sizeof(val_), 1); }) + +#define put_user_u32(val, addr) \ + ({ uint32_t val_ = tswap32(val); \ + cpu_memory_rw_debug(env_cpu(env), (addr), &val_, sizeof(val_), 1); }) -static inline void softmmu_tput32(CPUArchState *env, - target_ulong addr, uint32_t val) -{ - val = tswap32(val); - cpu_memory_rw_debug(env_cpu(env), addr, (uint8_t *)&val, 4, 1); -} -#define put_user_u64(arg, p) ({ softmmu_tput64(env, p, arg) ; 0; }) -#define put_user_u32(arg, p) ({ softmmu_tput32(env, p, arg) ; 0; }) #define put_user_ual(arg, p) put_user_u32(arg, p) -static void *softmmu_lock_user(CPUArchState *env, - target_ulong addr, target_ulong len, int copy) +static void *softmmu_lock_user(CPUArchState *env, target_ulong addr, + target_ulong len, bool copy) { - uint8_t *p; - /* TODO: Make this something that isn't fixed size. */ - p = malloc(len); + void *p = malloc(len); if (p && copy) { - cpu_memory_rw_debug(env_cpu(env), addr, p, len, 0); + if (cpu_memory_rw_debug(env_cpu(env), addr, p, len, 0)) { + free(p); + p = NULL; + } } return p; } #define lock_user(type, p, len, copy) softmmu_lock_user(env, p, len, copy) + static char *softmmu_lock_user_string(CPUArchState *env, target_ulong addr) { - char *p; - char *s; - uint8_t c; /* TODO: Make this something that isn't fixed size. */ - s = p = malloc(1024); + char *s = malloc(1024); + size_t len = 0; + if (!s) { return NULL; } do { - cpu_memory_rw_debug(env_cpu(env), addr, &c, 1, 0); - addr++; - *(p++) = c; - } while (c); + if (cpu_memory_rw_debug(env_cpu(env), addr++, s + len, 1, 0)) { + free(s); + return NULL; + } + } while (s[len++]); return s; } #define lock_user_string(p) softmmu_lock_user_string(env, p) + static void softmmu_unlock_user(CPUArchState *env, void *p, target_ulong addr, target_ulong len) {