Re: [XEN v5 1/3] xen/arm: Introduce CONFIG_PARTIAL_EMULATION and "partial-emulation" cmd option

[Michal Orzel <michal.orzel@xxxxxxx>]

On 21/02/2024 13:20, Julien Grall wrote:
> 
> 
> On 21/02/2024 11:33, Ayan Kumar Halder wrote:
>> Hi Jan,
>>
>> On 21/02/2024 07:09, Jan Beulich wrote:
>>> On 20.02.2024 16:22, Ayan Kumar Halder wrote:
>>>> On 20/02/2024 12:33, Jan Beulich wrote:
>>>>> On 20.02.2024 13:17, Ayan Kumar Halder wrote:
>>>>>> --- a/SUPPORT.md
>>>>>> +++ b/SUPPORT.md
>>>>>> @@ -101,6 +101,18 @@ Extension to the GICv3 interrupt controller to
>>>>>> support MSI.
>>>>>>        Status: Experimental
>>>>>> +### ARM/Partial Emulation
>>>>>> +
>>>>>> +Enable partial emulation of registers, otherwise considered
>>>>>> unimplemented,
>>>>>> +that would normally trigger a fault injection.
>>>>>> +
>>>>>> +    Status: Supported, with caveats
>>>>>> +
>>>>>> +Bugs allowing the userspace to attack the guest OS will not be
>>>>>> considered
>>>>>> +security vulnerabilities.
>>>>>> +
>>>>>> +Bugs that could compromise Xen will be considered security
>>>>>> vulnerabilities.
>>>>> ... the odd statement regarding in-guest vulnerabilities that might be
>>>>> introduced. I see only two ways of treating this as supported: Either
>>>>> you simply refuse emulation when the access is from user space,
>>>> I am wondering how do we enforce that.
>>>>
>>>> Let me try to understand this with the current implementation of partial
>>>> emulation for system registers.
>>>>
>>>> 1. DBGDTRTX_EL0 :- I understand that EL0 access to this register will
>>>> cause a trap to EL2. The reason being MDCR_EL2.TDA == 1.
>>>>
>>>> In that case, if we refuse emulation then an undef exception is injected
>>>> to the guest (this is the behavior as of today even without this patch).
>>>>
>>>> So, are you saying that the undef exception is to be injected to the
>>>> user space process. This may be possible for Arm64 guests
>>>> (inject_undef64_exception() needs to be changed).
>>> No, injection is always to the guest, not to a specific entity within the
>>> guest. That ought to be the same on bare hardware: An exception when
>>> raised has an architecturally defined entry point for handling. That'll
>>> typically be kernel code. The handler then figures out whether the source
>>> of the exception was in user or kernel mode. For user mode code, the
>>> kernel may or may not try to handle the exception and then continue the
>>> user mode process. If it can't or doesn't want to handle it, it'll raise
>>> (in UNIX terms) a signal to the process. That signal, in turn, may or may
>>> not be fatal to the process. But such an exception from user mode should
>>> never be fatal to the guest as a whole.
>> Thanks for explaining it so well.
>>>
>>>> However for Arm32 guests, this may not be possible as the mode changes
>>>> to PSR_MODE_UND.
>>> I'm afraid my Arm foo isn't good enough to understand this. On the
>>> surface
>>> it looks to violate above outlined principle.
>>>
>>>> Let me know if I understood you correctly.
>>>>
>>>>> or you
>>>>> support that mode of emulation as much as that of kernel space
>>>>> accesses.
>>>> Do you mean we support partial emulation only for traps from EL1 mode ?
>>> Possibly.
>>
>> Now, I understand you. We will allow partial_emulation only from kernel
>> mode.
>>
>> So we need to do sth :-
>>
>> if ( 'partial_emulation == true' && '!regs_mode_is_user(regs)' )
>>
>> {
>>
>>       /* partial_emulation logic */
>>
>> }
>>
>> I am ok with this.
> 
> The helpers take a min_el. So you can have simpler code and set the
> value to 1 (rather than 0 today).
> 
>>
>> And the updates message will be
>>
>> ### ARM/Partial Emulation
>>
>> Enable partial emulation of registers, otherwise considered unimplemented,
>> that would normally trigger a fault injection.
>>
>>      Status: Supported, with caveats
>>
>> Partial emulation for traps from userspace is not allowed.
> 
> I am not sure how this helps...
>>
>> Bugs that could compromise Xen will be considered security vulnerabilities.
> 
> ... you are still implying that any userland breaching the kernel will
> not be supported because of a bug.
> 
> But, I don't see how preventing the userland to access a register will
> help. In theory you could have a register that can only be accessed by
> EL1 but has an impact to EL0.
> 
> By definition, it means that we don't faithfully follow the Arm Arm and
> anything can happen. Which is why I wanted to exclude security support
> from userland and kernel.
> 
> I think the current registers are low risk. But I am under the
> impression that you may wan to add more partial emulation. So we need to
> make some sort of statement that doesn't put any burden to the security
> team for future registers.
> 
> An option would be to explicitly list the registers in SUPPORT.md. As I
> mentioned above, I think the registers you currently emulate are
> low-risk to introduce a security bug. So I would be ok to fully security
> support them. This would need to be re-assessed for new registers.
> 
> I am open to other suggestions.
+1. Providing partial emulation support only for kernel mode where the Arm ARM 
allows for user mode
access is confusing and would require excessive number of comments to back up 
the implementation.
While it might be ok for DCC, imposing such limit on all the future use cases 
sounds too severe.

I think the following would be ok:

Status: Supported, with caveats

Only the following system registers are security supported:
 - ...

~Michal