| Message ID | 20190528094953.14898-7-alex.bennee@linaro.org |
|---|---|
| State | Accepted |
| Commit | 0dc077212f6c1897e5bf39d1ab8e6bf23395ac4c |
| Headers | show |
| Series | testing/next (system tests, docker, iotests) | expand |
On Tue, 28 May 2019 at 10:49, Alex Bennée <alex.bennee@linaro.org> wrote: > > Now we have a common semihosting console interface use that for our > string output. However ARM is currently unique in also supporting > semihosting for linux-user so we need to replicate the API in > linux-user. If other architectures gain this support we can move the > file later. > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Hi; Coverity points out an issue in this function (CID 1401700): > +int qemu_semihosting_console_out(CPUArchState *env, target_ulong addr, int len) > +{ > + void *s = lock_user_string(addr); > + len = write(STDERR_FILENO, s, len ? len : strlen(s)); > + unlock_user(s, addr, 0); > + return len; > +} We call lock_user_string(), which can fail and return NULL if the memory pointed to by addr isn't actually readable. But we don't check for the error, so we can pass a NULL pointer to write(). Also it looks a bit dodgy that we are passed in a specific length value but we then go and look at the length of the string, but we trust the specific length value over the length of the string. If len is larger than the real length of the string (including terminating NUL) then the write() will read off the end of the string. thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On Tue, 28 May 2019 at 10:49, Alex Bennée <alex.bennee@linaro.org> wrote: >> >> Now we have a common semihosting console interface use that for our >> string output. However ARM is currently unique in also supporting >> semihosting for linux-user so we need to replicate the API in >> linux-user. If other architectures gain this support we can move the >> file later. >> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > > Hi; Coverity points out an issue in this function (CID 1401700): > > >> +int qemu_semihosting_console_out(CPUArchState *env, target_ulong addr, int len) >> +{ >> + void *s = lock_user_string(addr); >> + len = write(STDERR_FILENO, s, len ? len : strlen(s)); >> + unlock_user(s, addr, 0); >> + return len; >> +} > > We call lock_user_string(), which can fail and return NULL > if the memory pointed to by addr isn't actually readable. > But we don't check for the error, so we can pass a NULL > pointer to write(). Mea culpa - I'd avoided the nastiness of the lock string stuff in the softmmu case but reverted to a naive implementation for linux-user after the fact. I'll send a fix for that. > Also it looks a bit dodgy that we are passed in a > specific length value but we then go and look at the length > of the string, but we trust the specific length value over > the length of the string. If len is larger than the real > length of the string (including terminating NUL) then the > write() will read off the end of the string. It is an admittedly in-elegant hack to deal with the fact we call the same function for outputting a character as well as a string. None of the guests actually give us the length: * @len: length of string or 0 (string is null terminated) We could formalise it by making s/len/is_char/ and making it a bool or just add some more text to the description. > > thanks > -- PMM -- Alex Bennée
On Thu, 30 May 2019 at 13:35, Alex Bennée <alex.bennee@linaro.org> wrote: > > > Peter Maydell <peter.maydell@linaro.org> writes: > > > Also it looks a bit dodgy that we are passed in a > > specific length value but we then go and look at the length > > of the string, but we trust the specific length value over > > the length of the string. If len is larger than the real > > length of the string (including terminating NUL) then the > > write() will read off the end of the string. > > It is an admittedly in-elegant hack to deal with the fact we call the > same function for outputting a character as well as a string. None of > the guests actually give us the length: > > * @len: length of string or 0 (string is null terminated) > > We could formalise it by making s/len/is_char/ and making it a bool or > just add some more text to the description. I think it would be cleaner to have separate functions for "write a char" and "write a string" rather than having one function with a bool flag parameter which every callsite passes as a constant value. thanks -- PMM
diff --git a/linux-user/Makefile.objs b/linux-user/Makefile.objs index 769b8d83362..285c5dfa17a 100644 --- a/linux-user/Makefile.objs +++ b/linux-user/Makefile.objs @@ -6,4 +6,6 @@ obj-y = main.o syscall.o strace.o mmap.o signal.o \ obj-$(TARGET_HAS_BFLT) += flatload.o obj-$(TARGET_I386) += vm86.o obj-$(TARGET_ARM) += arm/nwfpe/ +obj-$(TARGET_ARM) += arm/semihost.o +obj-$(TARGET_AARCH64) += arm/semihost.o obj-$(TARGET_M68K) += m68k-sim.o diff --git a/linux-user/arm/semihost.c b/linux-user/arm/semihost.c new file mode 100644 index 00000000000..9554102a855 --- /dev/null +++ b/linux-user/arm/semihost.c @@ -0,0 +1,24 @@ +/* + * ARM Semihosting Console Support + * + * Copyright (c) 2019 Linaro Ltd + * + * Currently ARM is unique in having support for semihosting support + * in linux-user. So for now we implement the common console API but + * just for arm linux-user. + * + * SPDX-License-Identifier: GPL-2.0-or-later + */ + +#include "qemu/osdep.h" +#include "cpu.h" +#include "hw/semihosting/console.h" +#include "qemu.h" + +int qemu_semihosting_console_out(CPUArchState *env, target_ulong addr, int len) +{ + void *s = lock_user_string(addr); + len = write(STDERR_FILENO, s, len ? len : strlen(s)); + unlock_user(s, addr, 0); + return len; +} diff --git a/target/arm/arm-semi.c b/target/arm/arm-semi.c index d812eef1519..384b01124e1 100644 --- a/target/arm/arm-semi.c +++ b/target/arm/arm-semi.c @@ -27,6 +27,7 @@ #include "cpu.h" #include "hw/semihosting/semihost.h" +#include "hw/semihosting/console.h" #ifdef CONFIG_USER_ONLY #include "qemu.h" @@ -313,32 +314,10 @@ target_ulong do_arm_semihosting(CPUARMState *env) return set_swi_errno(ts, close(arg0)); } case TARGET_SYS_WRITEC: - { - char c; - - if (get_user_u8(c, args)) - /* FIXME - should this error code be -TARGET_EFAULT ? */ - return (uint32_t)-1; - /* Write to debug console. stderr is near enough. */ - if (use_gdb_syscalls()) { - return arm_gdb_syscall(cpu, arm_semi_cb, "write,2,%x,1", args); - } else { - return write(STDERR_FILENO, &c, 1); - } - } + qemu_semihosting_console_out(env, args, 1); + return 0xdeadbeef; case TARGET_SYS_WRITE0: - if (!(s = lock_user_string(args))) - /* FIXME - should this error code be -TARGET_EFAULT ? */ - return (uint32_t)-1; - len = strlen(s); - if (use_gdb_syscalls()) { - return arm_gdb_syscall(cpu, arm_semi_cb, "write,2,%x,%x", - args, len); - } else { - ret = write(STDERR_FILENO, s, len); - } - unlock_user(s, args, 0); - return ret; + return qemu_semihosting_console_out(env, args, 0); case TARGET_SYS_WRITE: GET_ARG(0); GET_ARG(1);