On Tue, Aug 16, 2022 at 5:03 AM Peter Maydell <peter.mayd...@linaro.org> wrote: > > Currently our semihosting implementations generally prohibit use of > semihosting calls in system emulation from the guest userspace. This > is a very long standing behaviour justified originally "to provide > some semblance of security" (since code with access to the > semihosting ABI can do things like read and write arbitrary files on > the host system). However, it is sometimes useful to be able to run > trusted guest code which performs semihosting calls from guest > userspace, notably for test code. Add a command line suboption to > the existing semihosting-config option group so that you can > explicitly opt in to semihosting from guest userspace with > -semihosting-config userspace=on > > (There is no equivalent option for the user-mode emulator, because > there by definition all code runs in userspace and has access to > semihosting already.) > > This commit adds the infrastructure for the command line option and > adds a bool 'is_user' parameter to the function > semihosting_userspace_enabled() that target code can use to check > whether it should be permitting the semihosting call for userspace. > It mechanically makes all the callsites pass 'false', so they > continue checking "is semihosting enabled in general". Subsequent > commits will make each target that implements semihosting honour the > userspace=on option by passing the correct value and removing > whatever "don't do this for userspace" checking they were doing by > hand. > > Signed-off-by: Peter Maydell <peter.mayd...@linaro.org>
Reviewed-by: Alistair Francis <alistair.fran...@wdc.com> Alistair > --- > include/semihosting/semihost.h | 10 ++++++++-- > semihosting/config.c | 10 ++++++++-- > softmmu/vl.c | 2 +- > stubs/semihost.c | 2 +- > target/arm/translate-a64.c | 2 +- > target/arm/translate.c | 6 +++--- > target/m68k/op_helper.c | 2 +- > target/nios2/translate.c | 2 +- > target/xtensa/translate.c | 6 +++--- > qemu-options.hx | 11 +++++++++-- > 10 files changed, 36 insertions(+), 17 deletions(-) > > diff --git a/include/semihosting/semihost.h b/include/semihosting/semihost.h > index 93a3c21b44d..efd2efa25ae 100644 > --- a/include/semihosting/semihost.h > +++ b/include/semihosting/semihost.h > @@ -27,7 +27,7 @@ typedef enum SemihostingTarget { > } SemihostingTarget; > > #ifdef CONFIG_USER_ONLY > -static inline bool semihosting_enabled(void) > +static inline bool semihosting_enabled(bool is_user) > { > return true; > } > @@ -52,7 +52,13 @@ static inline const char *semihosting_get_cmdline(void) > return NULL; > } > #else /* !CONFIG_USER_ONLY */ > -bool semihosting_enabled(void); > +/** > + * semihosting_enabled: > + * @is_user: true if guest code is in usermode (i.e. not privileged) > + * > + * Return true if guest code is allowed to make semihosting calls. > + */ > +bool semihosting_enabled(bool is_user); > SemihostingTarget semihosting_get_target(void); > const char *semihosting_get_arg(int i); > int semihosting_get_argc(void); > diff --git a/semihosting/config.c b/semihosting/config.c > index e171d4d6bc3..89a17596879 100644 > --- a/semihosting/config.c > +++ b/semihosting/config.c > @@ -34,6 +34,9 @@ QemuOptsList qemu_semihosting_config_opts = { > { > .name = "enable", > .type = QEMU_OPT_BOOL, > + }, { > + .name = "userspace", > + .type = QEMU_OPT_BOOL, > }, { > .name = "target", > .type = QEMU_OPT_STRING, > @@ -50,6 +53,7 @@ QemuOptsList qemu_semihosting_config_opts = { > > typedef struct SemihostingConfig { > bool enabled; > + bool userspace_enabled; > SemihostingTarget target; > char **argv; > int argc; > @@ -59,9 +63,9 @@ typedef struct SemihostingConfig { > static SemihostingConfig semihosting; > static const char *semihost_chardev; > > -bool semihosting_enabled(void) > +bool semihosting_enabled(bool is_user) > { > - return semihosting.enabled; > + return semihosting.enabled && (!is_user || > semihosting.userspace_enabled); > } > > SemihostingTarget semihosting_get_target(void) > @@ -137,6 +141,8 @@ int qemu_semihosting_config_options(const char *optarg) > if (opts != NULL) { > semihosting.enabled = qemu_opt_get_bool(opts, "enable", > true); > + semihosting.userspace_enabled = qemu_opt_get_bool(opts, "userspace", > + false); > const char *target = qemu_opt_get(opts, "target"); > /* setup of chardev is deferred until they are initialised */ > semihost_chardev = qemu_opt_get(opts, "chardev"); > diff --git a/softmmu/vl.c b/softmmu/vl.c > index 706bd7cff79..3593f1d7821 100644 > --- a/softmmu/vl.c > +++ b/softmmu/vl.c > @@ -1822,7 +1822,7 @@ static void qemu_apply_machine_options(QDict *qdict) > { > object_set_properties_from_keyval(OBJECT(current_machine), qdict, false, > &error_fatal); > > - if (semihosting_enabled() && !semihosting_get_argc()) { > + if (semihosting_enabled(false) && !semihosting_get_argc()) { > /* fall back to the -kernel/-append */ > semihosting_arg_fallback(current_machine->kernel_filename, > current_machine->kernel_cmdline); > } > diff --git a/stubs/semihost.c b/stubs/semihost.c > index f486651afbb..d65c9fd5dcf 100644 > --- a/stubs/semihost.c > +++ b/stubs/semihost.c > @@ -23,7 +23,7 @@ QemuOptsList qemu_semihosting_config_opts = { > }; > > /* Queries to config status default to off */ > -bool semihosting_enabled(void) > +bool semihosting_enabled(bool is_user) > { > return false; > } > diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c > index 163df8c6157..3decc8da573 100644 > --- a/target/arm/translate-a64.c > +++ b/target/arm/translate-a64.c > @@ -2219,7 +2219,7 @@ static void disas_exc(DisasContext *s, uint32_t insn) > * it is required for halting debug disabled: it will UNDEF. > * Secondly, "HLT 0xf000" is the A64 semihosting syscall instruction. > */ > - if (semihosting_enabled() && imm16 == 0xf000) { > + if (semihosting_enabled(false) && imm16 == 0xf000) { > #ifndef CONFIG_USER_ONLY > /* In system mode, don't allow userspace access to semihosting, > * to provide some semblance of security (and for consistency > diff --git a/target/arm/translate.c b/target/arm/translate.c > index ad617b99481..b85be8a818d 100644 > --- a/target/arm/translate.c > +++ b/target/arm/translate.c > @@ -1169,7 +1169,7 @@ static inline void gen_hlt(DisasContext *s, int imm) > * semihosting, to provide some semblance of security > * (and for consistency with our 32-bit semihosting). > */ > - if (semihosting_enabled() && > + if (semihosting_enabled(false) && > #ifndef CONFIG_USER_ONLY > s->current_el != 0 && > #endif > @@ -6556,7 +6556,7 @@ static bool trans_BKPT(DisasContext *s, arg_BKPT *a) > /* BKPT is OK with ECI set and leaves it untouched */ > s->eci_handled = true; > if (arm_dc_feature(s, ARM_FEATURE_M) && > - semihosting_enabled() && > + semihosting_enabled(false) && > #ifndef CONFIG_USER_ONLY > !IS_USER(s) && > #endif > @@ -8764,7 +8764,7 @@ static bool trans_SVC(DisasContext *s, arg_SVC *a) > { > const uint32_t semihost_imm = s->thumb ? 0xab : 0x123456; > > - if (!arm_dc_feature(s, ARM_FEATURE_M) && semihosting_enabled() && > + if (!arm_dc_feature(s, ARM_FEATURE_M) && semihosting_enabled(false) && > #ifndef CONFIG_USER_ONLY > !IS_USER(s) && > #endif > diff --git a/target/m68k/op_helper.c b/target/m68k/op_helper.c > index d9937ca8dc5..4b3dfec1306 100644 > --- a/target/m68k/op_helper.c > +++ b/target/m68k/op_helper.c > @@ -203,7 +203,7 @@ static void cf_interrupt_all(CPUM68KState *env, int is_hw) > cf_rte(env); > return; > case EXCP_HALT_INSN: > - if (semihosting_enabled() > + if (semihosting_enabled(false) > && (env->sr & SR_S) != 0 > && (env->pc & 3) == 0 > && cpu_lduw_code(env, env->pc - 4) == 0x4e71 > diff --git a/target/nios2/translate.c b/target/nios2/translate.c > index 3a037a68cc4..2b556683422 100644 > --- a/target/nios2/translate.c > +++ b/target/nios2/translate.c > @@ -818,7 +818,7 @@ static void gen_break(DisasContext *dc, uint32_t code, > uint32_t flags) > #ifndef CONFIG_USER_ONLY > /* The semihosting instruction is "break 1". */ > R_TYPE(instr, code); > - if (semihosting_enabled() && instr.imm5 == 1) { > + if (semihosting_enabled(false) && instr.imm5 == 1) { > t_gen_helper_raise_exception(dc, EXCP_SEMIHOST); > return; > } > diff --git a/target/xtensa/translate.c b/target/xtensa/translate.c > index 70e11eeb459..dc475a4274b 100644 > --- a/target/xtensa/translate.c > +++ b/target/xtensa/translate.c > @@ -2364,9 +2364,9 @@ static uint32_t test_exceptions_simcall(DisasContext > *dc, > bool ill = true; > #else > /* Between RE.2 and RE.3 simcall opcode's become nop for the hardware. */ > - bool ill = dc->config->hw_version <= 250002 && !semihosting_enabled(); > + bool ill = dc->config->hw_version <= 250002 && > !semihosting_enabled(false); > #endif > - if (ill || !semihosting_enabled()) { > + if (ill || !semihosting_enabled(false)) { > qemu_log_mask(LOG_GUEST_ERROR, "SIMCALL but semihosting is > disabled\n"); > } > return ill ? XTENSA_OP_ILL : 0; > @@ -2376,7 +2376,7 @@ static void translate_simcall(DisasContext *dc, const > OpcodeArg arg[], > const uint32_t par[]) > { > #ifndef CONFIG_USER_ONLY > - if (semihosting_enabled()) { > + if (semihosting_enabled(false)) { > gen_helper_simcall(cpu_env); > } > #endif > diff --git a/qemu-options.hx b/qemu-options.hx > index 3f23a42fa87..4e7111abe3d 100644 > --- a/qemu-options.hx > +++ b/qemu-options.hx > @@ -4614,12 +4614,12 @@ SRST > information about the facilities this enables. > ERST > DEF("semihosting-config", HAS_ARG, QEMU_OPTION_semihosting_config, > - "-semihosting-config > [enable=on|off][,target=native|gdb|auto][,chardev=id][,arg=str[,...]]\n" \ > + "-semihosting-config > [enable=on|off][,target=native|gdb|auto][,chardev=id][,userspace=on|off][,arg=str[,...]]\n" > \ > " semihosting configuration\n", > QEMU_ARCH_ARM | QEMU_ARCH_M68K | QEMU_ARCH_XTENSA | > QEMU_ARCH_MIPS | QEMU_ARCH_NIOS2 | QEMU_ARCH_RISCV) > SRST > -``-semihosting-config > [enable=on|off][,target=native|gdb|auto][,chardev=id][,arg=str[,...]]`` > +``-semihosting-config > [enable=on|off][,target=native|gdb|auto][,chardev=id][,userspace=on|off][,arg=str[,...]]`` > Enable and configure semihosting (ARM, M68K, Xtensa, MIPS, Nios II, > RISC-V > only). > > @@ -4646,6 +4646,13 @@ SRST > Send the output to a chardev backend output for native or auto > output when not in gdb > > + ``userspace=on|off`` > + Allows code running in guest userspace to access the semihosting > + interface. The default is that only privileged guest code can > + make semihosting calls. Note that setting ``userspace=on`` should > + only be used if all guest code is trusted (for example, in > + bare-metal test case code). > + > ``arg=str1,arg=str2,...`` > Allows the user to pass input arguments, and can be used > multiple times to build up a list. The old-style > -- > 2.25.1 > >