diff mbox

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

Message ID CAFEAcA9w+OpFqLDF7fBkm1Qk-4pE346+ayUq8amVvoz1EXV5EA@mail.gmail.com
State Accepted
Headers show

Commit Message

Peter Maydell Aug. 27, 2015, 6:35 p.m. UTC 
On 13 August 2015 at 17:35, Peter Maydell <peter.maydell@linaro.org> 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 <peter.maydell@linaro.org>
> ---

> @@ -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.




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

Comments

Christopher Covington Sept. 14, 2015, 6:36 p.m. UTC | #1 
Hi Peter,

On 08/27/2015 02:35 PM, Peter Maydell wrote:
> On 13 August 2015 at 17:35, Peter Maydell <peter.maydell@linaro.org> 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 <peter.maydell@linaro.org>
>> ---
> 
>> @@ -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.)

> 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.

I find allowing trusted guests to access host files to be a very useful
feature. To me it is very similar to passing through / (root) via VirtIO-9P.
Perhaps a useful way of making sure the user knows what files their guest is
gaining access to would be to have a semihosting path prefix option. That way
access could be allowed nowhere; clearly allow everywhere (/); or clearly be
restricted to, and relative to, a certain sysroot directory
(/home/user/my-sysroot).

Christopher Covington
diff mbox

Patch

--- 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);