Re: [PATCH 2/2] xen/arm: p2m: Populate pages for GICv2 mapping in arch_domain_create()

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 17/10/2022 21:36, Julien Grall wrote:
> Hi Andrew,
>
> On 17/10/2022 20:12, Andrew Cooper wrote:
>> From: Henry Wang <Henry.Wang@xxxxxxx>
>>
>> The XSA-409 fixes discovered that the GICv2 path tries to create P2M
>> mappings
>> in the domain_create() path.  This fails, as the P2M pool is empty
>> before a
>> XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION hypercall.
>>
>> As a stopgap, automatically give domains 16 pages of P2M memory. 
>> This is
>> large enough to allow the GICv2 case to work, but small enough to not
>> introduce a continuation worry.
>>
>> A consequence is that, for later error paths domain_create(), we end
>> up in
>> p2m_final_teardown() with a nonzero P2M pool.  Such a domain has no
>> vCPUs, and
>> has never been scheduled, so free the memory directly.
>>
>> Fixes: cbea5a1149ca ("xen/arm: Allocate and free P2M pages from the
>> P2M pool")
>> Suggested-by: Julien Grall <jgrall@xxxxxxxxxx>
>
> This is not really in the spirit of my original suggestion anymore

Ok, I have dropped it.

> ... In fact, you drop all the explanations regarding how the code is
> fragile (e.g. we are relying on early mapping to not take any extra
> reference). Maybe you don't care, but I do as Henry and I spent ages
> to figure out all the corner cases.

I presume you're referring to the todo?  If so, that's an statement, not
an explanation of what is suddenly different about it.

What has XSA-409 changed in this regard?  Because it looks like the
answer is nothing and the GICv2 path was similarly fragile beforehand. 
In which case, why it is appropriate content for a security patch?

>
>> Signed-off-by: Henry Wang <Henry.Wang@xxxxxxx>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> ---
>> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>> CC: Julien Grall <julien@xxxxxxx>
>> CC: Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
>> CC: Bertrand Marquis <bertrand.marquis@xxxxxxx>
>> CC: Henry Wang <Henry.Wang@xxxxxxx>
>> ---
>>   xen/arch/arm/p2m.c | 43 +++++++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 41 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index 6826f6315080..76a0e31c6c8c 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -1736,8 +1736,36 @@ void p2m_final_teardown(struct domain *d)
>>       if ( !p2m->domain )
>>           return;
>>   -    ASSERT(page_list_empty(&p2m->pages));
>> -    ASSERT(page_list_empty(&d->arch.paging.p2m_freelist));
>> +    /*
>> +     * On the domain_create() error path only, we can end up here
>> with a
>> +     * non-zero P2M pool.
>> +     *
>> +     * At present, this is a maximum of 16 pages, spread between
>> p2m->pages
>> +     * and the free list.  The domain has never been scheduled (it
>> has no
>> +     * vcpus), so there is TLB maintenance to perform; just free
>> everything.
>> +     */
>> +    if ( !page_list_empty(&p2m->pages) ||
>> +         !page_list_empty(&d->arch.paging.p2m_freelist) )
>> +    {
>> +        struct page_info *pg;
>> +
>> +        /*
>> +         * There's no sensible "in the domain_create() error path"
>> predicate,
>> +         * so simply sanity check that we don't have unexpected work
>> to do.
>> +         */
>> +        ASSERT(d->arch.paging.p2m_total_pages <= 16);
>> +
>> +        spin_lock(&d->arch.paging.lock);
>> +
>> +        while ( (pg = page_list_remove_head(&p2m->pages)) )
>> +            free_domheap_page(pg);
>> +        while ( (pg =
>> page_list_remove_head(&d->arch.paging.p2m_freelist)) )
>> +            free_domheap_page(pg);
>> +
>> +        d->arch.paging.p2m_total_pages = 0;
>> +
>> +        spin_unlock(&d->arch.paging.lock);
>> +    }
>
> ... you are hardcoding both p2m_teardown() and p2m_set_allocation().
> IMO this is not an improvement at all. It is just making the code more
> complex than necessary and lack all the explanation on the assumptions.
>
> So while I am fine with your patch #1 (already reviewed it), there is
> a better patch from Henry on the ML. So we should take his (rebased)
> instead of yours.

If by better, you mean something that still has errors, then sure.

There's a really good reason why you cannot safely repurpose
p2m_teardown().  It's written expecting a fully constructed domain -
which is fine because that's how it is used.  It doesn't cope safely
with an partially constructed domain.


Yes, this code is a bit nasty, but it's less buggy than all attempts
presented thus far, specifically because it avoids inappropriate
repurposing of infrastructure.

At this point, we're a week after the publishing of XSA-409 and CI is
still red across the board.  There were multiple failings which have
lead to this situation, that the security team need to deal with, but
right now, we need a correct fix and we need it yesterday.

~Andrew