Re: [Xen-devel] [PATCH 5/7] xen/arm: traps: Avoid BUG_ON() in do_trap_brk()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 30/07/2019 18:00, Stefano Stabellini wrote:
> On Tue, 30 Jul 2019, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 7/29/19 11:02 PM, Stefano Stabellini wrote:
>>> On Tue, 23 Jul 2019, Julien Grall wrote:
>>>> At the moment, do_trap_brk() is using a BUG_ON() to check the hardware
>>>> has been correctly configured during boot.
>>>>
>>>> Any error when configuring the hardware could result to a guest 'brk'
>>>> trapping in the hypervisor and crash it.
>>>>
>>>> This is pretty harsh to kill Xen when actually killing the guest would
>>>> be enough as misconfiguring this trap would not lead to exposing
>>>> sensitive data. Replace the BUG_ON() with crashing the guest.
>>>>
>>>> Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
>>>> ---
>>>>   xen/arch/arm/traps.c | 11 ++++++++---
>>>>   1 file changed, 8 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>>>> index 132686ee0f..ef37ca6bde 100644
>>>> --- a/xen/arch/arm/traps.c
>>>> +++ b/xen/arch/arm/traps.c
>>>> @@ -1304,10 +1304,15 @@ int do_bug_frame(const struct cpu_user_regs *regs,
>>>> vaddr_t pc)
>>>>   #ifdef CONFIG_ARM_64
>>>>   static void do_trap_brk(struct cpu_user_regs *regs, const union hsr hsr)
>>>>   {
>>>> -    /* HCR_EL2.TGE and MDCR_EL2.TDE are not set so we never receive
>>>> -     * software breakpoint exception for EL1 and EL0 here.
>>>> +    /*
>>>> +     * HCR_EL2.TGE and MDCR_EL2.TDR are currently not set. So we should
>>>> +     * never receive software breakpoing exception for EL1 and EL0 here.
>>>>        */
>>>> -    BUG_ON(!hyp_mode(regs));
>>>> +    if ( !hyp_mode(regs) )
>>>> +    {
>>>> +        domain_crash(current->domain);
>>>> +        return;
>>>> +    }
>>> This is a good change to have. I am wondering if it would make sense to
>>> also printk some debug messages, maybe even show_execution_state. If so,
>>> we need to be careful that it's only done in debug builds, or it is rate
>>> limited. What do you think?
>> Messages are already printed by domain_crash(...). But I don't see the 
>> concern
>> about non-debug build or even not rate limiting... If the domain crash, then
>> it will not cause anymore print and therefore you can't spam the console 
>> here.
> Ah yes, you are quite right

I still have a series pending to force people to put a useful printk()
in, because there is nothing more infuriating than to find a
domain_crash() with no clarifying context.

It should be a gprintk(XENLOG_ERR, and will eventually be folded into
domain_crash()'s prototype.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel