[Xen-devel,3/3] xen/arm: Don't crash the domain on invalid HVC immediate

Message ID 20180130161433.6910-4-julien.grall@arm.com
State Superseded
Headers show
Series
  • xen/arm: Inject an exception to the guest rather than crashing it
Related show

Commit Message

Julien Grall Jan. 30, 2018, 4:14 p.m.
domain_crash_synchronous() should only be used when something went wrong
in Xen. It is better to inject to the guest as it will be in better
position to provide helpful information (stack trace...).

Signed-off-by: Julien Grall <julien.grall@arm.com>

---

    We potentially want to return -1 instead. This would make Xen more
    future-proof if we decide to implement the other HVC immediate.
---
 xen/arch/arm/traps.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

Comments

Stefano Stabellini Jan. 30, 2018, 6:24 p.m. | #1
On Tue, 30 Jan 2018, Julien Grall wrote:
> domain_crash_synchronous() should only be used when something went wrong
> in Xen. It is better to inject to the guest as it will be in better
                                                              ^ a better


> position to provide helpful information (stack trace...).
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> ---
> 
>     We potentially want to return -1 instead. This would make Xen more
>     future-proof if we decide to implement the other HVC immediate.
> ---
>  xen/arch/arm/traps.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 843adf4959..18da45dff3 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1473,14 +1473,17 @@ static void do_debug_trap(struct cpu_user_regs *regs, unsigned int code)
>  #endif
>  
>  static void do_trap_hypercall(struct cpu_user_regs *regs, register_t *nr,
> -                              unsigned long iss)
> +                              const union hsr hsr)
>  {
>      arm_hypercall_fn_t call = NULL;
>  
>      BUILD_BUG_ON(NR_hypercalls < ARRAY_SIZE(arm_hypercall_table) );
>  
> -    if ( iss != XEN_HYPERCALL_TAG )
> -        domain_crash_synchronous();
> +    if ( hsr.iss != XEN_HYPERCALL_TAG )
> +    {
> +        gprintk(XENLOG_WARNING, "Invalid HVC imm 0x%x\n", hsr.iss);
> +        return inject_undef_exception(regs, hsr);

It looks a bit weird, given that inject_undef_exception doesn't return
anything. This is just code style so anyway:

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> +    }
>  
>      if ( *nr >= ARRAY_SIZE(arm_hypercall_table) )
>      {
> @@ -2150,7 +2153,7 @@ void do_trap_guest_sync(struct cpu_user_regs *regs)
>          if ( hsr.iss == 0 )
>              return do_trap_hvc_smccc(regs);
>          nr = regs->r12;
> -        do_trap_hypercall(regs, &nr, hsr.iss);
> +        do_trap_hypercall(regs, &nr, hsr);
>          regs->r12 = (uint32_t)nr;
>          break;
>      }
> @@ -2164,7 +2167,7 @@ void do_trap_guest_sync(struct cpu_user_regs *regs)
>  #endif
>          if ( hsr.iss == 0 )
>              return do_trap_hvc_smccc(regs);
> -        do_trap_hypercall(regs, &regs->x16, hsr.iss);
> +        do_trap_hypercall(regs, &regs->x16, hsr);
>          break;
>      case HSR_EC_SMC64:
>          /*
> -- 
> 2.11.0
>
Julien Grall Jan. 30, 2018, 6:26 p.m. | #2
On 30/01/18 18:24, Stefano Stabellini wrote:
> On Tue, 30 Jan 2018, Julien Grall wrote:
>> domain_crash_synchronous() should only be used when something went wrong
>> in Xen. It is better to inject to the guest as it will be in better
>                                                                ^ a better
> 
> 
>> position to provide helpful information (stack trace...).
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>
>> ---
>>
>>      We potentially want to return -1 instead. This would make Xen more
>>      future-proof if we decide to implement the other HVC immediate.
>> ---
>>   xen/arch/arm/traps.c | 13 ++++++++-----
>>   1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>> index 843adf4959..18da45dff3 100644
>> --- a/xen/arch/arm/traps.c
>> +++ b/xen/arch/arm/traps.c
>> @@ -1473,14 +1473,17 @@ static void do_debug_trap(struct cpu_user_regs *regs, unsigned int code)
>>   #endif
>>   
>>   static void do_trap_hypercall(struct cpu_user_regs *regs, register_t *nr,
>> -                              unsigned long iss)
>> +                              const union hsr hsr)
>>   {
>>       arm_hypercall_fn_t call = NULL;
>>   
>>       BUILD_BUG_ON(NR_hypercalls < ARRAY_SIZE(arm_hypercall_table) );
>>   
>> -    if ( iss != XEN_HYPERCALL_TAG )
>> -        domain_crash_synchronous();
>> +    if ( hsr.iss != XEN_HYPERCALL_TAG )
>> +    {
>> +        gprintk(XENLOG_WARNING, "Invalid HVC imm 0x%x\n", hsr.iss);
>> +        return inject_undef_exception(regs, hsr);
> 
> It looks a bit weird, given that inject_undef_exception doesn't return
> anything. This is just code style so anyway:

I followed coding style in other part of the emulation (see do_sysreg 
for instance). I am happy to change that though.

Cheers,

> 
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> 
> 
>> +    }
>>   
>>       if ( *nr >= ARRAY_SIZE(arm_hypercall_table) )
>>       {
>> @@ -2150,7 +2153,7 @@ void do_trap_guest_sync(struct cpu_user_regs *regs)
>>           if ( hsr.iss == 0 )
>>               return do_trap_hvc_smccc(regs);
>>           nr = regs->r12;
>> -        do_trap_hypercall(regs, &nr, hsr.iss);
>> +        do_trap_hypercall(regs, &nr, hsr);
>>           regs->r12 = (uint32_t)nr;
>>           break;
>>       }
>> @@ -2164,7 +2167,7 @@ void do_trap_guest_sync(struct cpu_user_regs *regs)
>>   #endif
>>           if ( hsr.iss == 0 )
>>               return do_trap_hvc_smccc(regs);
>> -        do_trap_hypercall(regs, &regs->x16, hsr.iss);
>> +        do_trap_hypercall(regs, &regs->x16, hsr);
>>           break;
>>       case HSR_EC_SMC64:
>>           /*
>> -- 
>> 2.11.0
>>

Patch

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 843adf4959..18da45dff3 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1473,14 +1473,17 @@  static void do_debug_trap(struct cpu_user_regs *regs, unsigned int code)
 #endif
 
 static void do_trap_hypercall(struct cpu_user_regs *regs, register_t *nr,
-                              unsigned long iss)
+                              const union hsr hsr)
 {
     arm_hypercall_fn_t call = NULL;
 
     BUILD_BUG_ON(NR_hypercalls < ARRAY_SIZE(arm_hypercall_table) );
 
-    if ( iss != XEN_HYPERCALL_TAG )
-        domain_crash_synchronous();
+    if ( hsr.iss != XEN_HYPERCALL_TAG )
+    {
+        gprintk(XENLOG_WARNING, "Invalid HVC imm 0x%x\n", hsr.iss);
+        return inject_undef_exception(regs, hsr);
+    }
 
     if ( *nr >= ARRAY_SIZE(arm_hypercall_table) )
     {
@@ -2150,7 +2153,7 @@  void do_trap_guest_sync(struct cpu_user_regs *regs)
         if ( hsr.iss == 0 )
             return do_trap_hvc_smccc(regs);
         nr = regs->r12;
-        do_trap_hypercall(regs, &nr, hsr.iss);
+        do_trap_hypercall(regs, &nr, hsr);
         regs->r12 = (uint32_t)nr;
         break;
     }
@@ -2164,7 +2167,7 @@  void do_trap_guest_sync(struct cpu_user_regs *regs)
 #endif
         if ( hsr.iss == 0 )
             return do_trap_hvc_smccc(regs);
-        do_trap_hypercall(regs, &regs->x16, hsr.iss);
+        do_trap_hypercall(regs, &regs->x16, hsr);
         break;
     case HSR_EC_SMC64:
         /*