xen/arm64: entry: Actually skip do_trap_*() when an SError is triggered

[PATCH] xen/arm64: entry: Actually skip do_trap_*() when an SError is triggered

Posted by Julien Grall 1 year, 6 months ago

From: Julien Grall <jgrall@amazon.com>

For SErrors, we support two configurations:
  * Every SErrors will result to a panic in Xen
  * We will forward SErrors triggered by a VM back to itself

For the latter case, we want to skip the call to do_trap_*() because the PC
was already adjusted.

However, the alternative used to decide between the two configurations
is inverted. This would result to the VM corrupting itself if:
  * x19 is non-zero in the panic case
  * advance PC too much in the second case

Solve the issue by switch from alternative_if to alternative_if_not.

Fixes: a458d3bd0d25 ("xen/arm: entry: Ensure the guest state is synced when receiving a vSError")
Signed-off-by: Julien Grall <jgrall@amazon.com>

----

This is a candidate to be backported to all supported tree.

I don't have a setup where I can easily inject SError. But this was tested
by setting x19 to 1 just before the first alternative and use "serrors=panic".

Before this patch, Linux would get stuck.
---
 xen/arch/arm/arm64/entry.S | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
index 6251135ebdd2..fab10f8a0d26 100644
--- a/xen/arch/arm/arm64/entry.S
+++ b/xen/arch/arm/arm64/entry.S
@@ -259,7 +259,7 @@
          * apart. The easiest way is to duplicate the few instructions
          * that need to be skipped.
          */
-        alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT
+        alternative_if_not SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT
         cbnz      x19, 1f
         mov       x0, sp
         bl        do_trap_\trap
-- 
2.40.1

Re: [PATCH] xen/arm64: entry: Actually skip do_trap_*() when an SError is triggered

Posted by Michal Orzel 1 year, 6 months ago

Hi Julien,

On 06/08/2024 14:48, Julien Grall wrote:
> 
> 
> From: Julien Grall <jgrall@amazon.com>
> 
> For SErrors, we support two configurations:
>   * Every SErrors will result to a panic in Xen
>   * We will forward SErrors triggered by a VM back to itself
> 
> For the latter case, we want to skip the call to do_trap_*() because the PC
> was already adjusted.
> 
> However, the alternative used to decide between the two configurations
> is inverted. This would result to the VM corrupting itself if:
>   * x19 is non-zero in the panic case
>   * advance PC too much in the second case
> 
> Solve the issue by switch from alternative_if to alternative_if_not.
> 
> Fixes: a458d3bd0d25 ("xen/arm: entry: Ensure the guest state is synced when receiving a vSError")
> Signed-off-by: Julien Grall <jgrall@amazon.com>
Acked-by: Michal Orzel <michal.orzel@amd.com>

> 
> ----
3 instead of 4 dashes

~Michal

Re: [PATCH] xen/arm64: entry: Actually skip do_trap_*() when an SError is triggered

Posted by Julien Grall 1 year, 6 months ago


On 06/08/2024 14:26, Michal Orzel wrote:
> Hi Julien,
> 
> On 06/08/2024 14:48, Julien Grall wrote:
>>
>>
>> From: Julien Grall <jgrall@amazon.com>
>>
>> For SErrors, we support two configurations:
>>    * Every SErrors will result to a panic in Xen
>>    * We will forward SErrors triggered by a VM back to itself
>>
>> For the latter case, we want to skip the call to do_trap_*() because the PC
>> was already adjusted.
>>
>> However, the alternative used to decide between the two configurations
>> is inverted. This would result to the VM corrupting itself if:
>>    * x19 is non-zero in the panic case
>>    * advance PC too much in the second case
>>
>> Solve the issue by switch from alternative_if to alternative_if_not.
>>
>> Fixes: a458d3bd0d25 ("xen/arm: entry: Ensure the guest state is synced when receiving a vSError")
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
> Acked-by: Michal Orzel <michal.orzel@amd.com>
> 
>>
>> ----
> 3 instead of 4 dashes

I will fixed it.

The patchqueue tool I am using will strip anything after "---". So I am 
using ---- to version changelog... I tend to forget that I need to 
manually call sed -i 's/^----/---/' *.patch before sending every patch.

Cheers,

-- 
Julien Grall

Re: [PATCH] xen/arm64: entry: Actually skip do_trap_*() when an SError is triggered

Posted by Julien Grall 1 year, 5 months ago

Hi,

On 06/08/2024 14:30, Julien Grall wrote:
> 
> 
> On 06/08/2024 14:26, Michal Orzel wrote:
>> Hi Julien,
>>
>> On 06/08/2024 14:48, Julien Grall wrote:
>>>
>>>
>>> From: Julien Grall <jgrall@amazon.com>
>>>
>>> For SErrors, we support two configurations:
>>>    * Every SErrors will result to a panic in Xen
>>>    * We will forward SErrors triggered by a VM back to itself
>>>
>>> For the latter case, we want to skip the call to do_trap_*() because 
>>> the PC
>>> was already adjusted.
>>>
>>> However, the alternative used to decide between the two configurations
>>> is inverted. This would result to the VM corrupting itself if:
>>>    * x19 is non-zero in the panic case
>>>    * advance PC too much in the second case
>>>
>>> Solve the issue by switch from alternative_if to alternative_if_not.
>>>
>>> Fixes: a458d3bd0d25 ("xen/arm: entry: Ensure the guest state is 
>>> synced when receiving a vSError")
>>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>> Acked-by: Michal Orzel <michal.orzel@amd.com>
>>
>>>
>>> ----
>> 3 instead of 4 dashes
> 
> I will fixed it.
> 
> The patchqueue tool I am using will strip anything after "---". So I am 
> using ---- to version changelog... I tend to forget that I need to 
> manually call sed -i 's/^----/---/' *.patch before sending every patch.

This is now committed.

Cheers,

-- 
Julien Grall