Re: [Qemu-devel] [PATCH 9/9] target-arm: Wire up HLT 0xf000 as the A64 semihosting instruction

[Peter Maydell]

On 13 August 2015 at 17:35, Peter Maydell <address@hidden> wrote:
> For the A64 instruction set, the semihosting call instruction
> is 'HLT 0xf000'. Wire this up to call do_arm_semihosting()
> if semihosting is enabled.
>
> Signed-off-by: Peter Maydell <address@hidden>
> ---

> @@ -1553,8 +1554,17 @@ static void disas_exc(DisasContext *s, uint32_t insn)
>              unallocated_encoding(s);
>              break;
>          }
> -        /* HLT */
> -        unsupported_encoding(s, insn);
> +        /* HLT. This has two purposes.
> +         * Architecturally, it is an external halting debug instruction.
> +         * Since QEMU doesn't implement external debug, we treat this as
> +         * it is required for halting debug disabled: it will UNDEF.
> +         * Secondly, "HLT 0xf000" is the A64 semihosting syscall instruction.
> +         */
> +        if (semihosting_enabled() && imm16 == 0xf000) {
> +            gen_exception_internal_insn(s, 0, EXCP_SEMIHOST);
> +        } else {
> +            unsupported_encoding(s, insn);
> +        }

Christopher pointed out to me at KVM Forum that this isn't
consistent with how we do 32-bit ARM semihosting, which has a
check to prevent its use from userspace in system emulation.
(The idea is that semihosting is basically a "guest can pwn
your host" API, so giving access to it to guest userspace is
kind of brave.)

I propose to squash the following change into this patch as I
put it into target-arm.next.


--- a/target-arm/translate-a64.c
+++ b/target-arm/translate-a64.c
@@ -1561,6 +1561,16 @@ static void disas_exc(DisasContext *s, uint32_t insn)
          * Secondly, "HLT 0xf000" is the A64 semihosting syscall instruction.
          */
         if (semihosting_enabled() && 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
+             * with our 32-bit semihosting).
+             */
+            if (s->current_el == 0) {
+                unsupported_encoding(s, insn);
+                break;
+            }
+#endif
             gen_exception_internal_insn(s, 0, EXCP_SEMIHOST);
         } else {
             unsupported_encoding(s, insn);


This brings it into line with the 32-bit code.

There is a usecase for allowing unfettered access to semihosting
in system emulation mode (basically, running bare metal test
binaries). I think we should deal with that by having a separate
command line option for "userspace semihosting access is OK",
which changes the behaviour for both 32-bit and 64-bit semihosting
APIs. Alternatively, we could instead allow userspace to use
"safe" parts of the semihosting API, like "print to stdout",
but not the less safe parts like "open and write to arbitrary
host files". Or we could decide that this safety check isn't
actually very useful (no other model/debug environment has it
that I know of) and drop it entirely; but that makes me a little
nervous.

(It would actually be nice to be able to say "I'd like the
guest kernel to be able to do early printk via semihosting
without trusting it to open files etc", for that matter.)

thanks
-- PMM