[prev in list] [next in list] [prev in thread] [next in thread] 

List:       xen-devel
Subject:    Re: [Xen-devel] [PATCH 5/7] xen/arm: traps: Avoid BUG_ON() in do_trap_brk()
From:       Andrew Cooper <andrew.cooper3 () citrix ! com>
Date:       2019-07-31 14:48:49
Message-ID: a613bab7-8e88-8a4f-6298-42d8baef6517 () citrix ! com
[Download RAW message or body]

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@arm.com>
>>>> ---
>>>>   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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[prev in list] [next in list] [prev in thread] [next in thread]