Re: [PATCH] xen/x86: Fix memory leak in vcpu_create() error path

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 29/09/2020 07:18, Jan Beulich wrote:
> On 28.09.2020 17:47, Andrew Cooper wrote:
>> Various paths in vcpu_create() end up calling paging_update_paging_modes(),
>> which eventually allocate a monitor pagetable if one doesn't exist.
>>
>> However, an error in vcpu_create() results in the vcpu being cleaned up
>> locally, and not put onto the domain's vcpu list.  Therefore, the monitor
>> table is not freed by {hap,shadow}_teardown()'s loop.  This is caught by
>> assertions later that we've successfully freed the entire hap/shadow memory
>> pool.
>>
>> The per-vcpu loops in domain teardown logic is conceptually wrong, but exist
>> due to insufficient existing structure in the existing logic.
>>
>> Break paging_vcpu_teardown() out of paging_teardown(), with mirrored 
>> breakouts
>> in the hap/shadow code, and use it from arch_vcpu_create()'s error path.  
>> This
>> fixes the memory leak.
>>
>> The new {hap,shadow}_vcpu_teardown() must be idempotent, and are written to 
>> be
>> as tolerable as possible, with the minimum number of safety checks possible.
>> In particular, drop the mfn_valid() check - if junk is in these fields, then
>> Xen is going to explode anyway.
>>
>> Reported-by: Michał Leszczyński <michal.leszczynski@xxxxxxx>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Thanks.  (Wow it really is a long time since needing to drop everything
for security work...)

>> --- a/xen/arch/x86/mm/hap/hap.c
>> +++ b/xen/arch/x86/mm/hap/hap.c
>> @@ -563,30 +563,37 @@ void hap_final_teardown(struct domain *d)
>>      paging_unlock(d);
>>  }
>>  
>> +void hap_vcpu_teardown(struct vcpu *v)
>> +{
>> +    struct domain *d = v->domain;
>> +    mfn_t mfn;
>> +
>> +    paging_lock(d);
>> +
>> +    if ( !paging_mode_hap(d) || !v->arch.paging.mode )
>> +        goto out;
> Any particular reason you don't use paging_get_hostmode() (as the
> original code did) here? Any particular reason for the seemingly
> redundant (and hence somewhat in conflict with the description's
> "with the minimum number of safety checks possible")
> paging_mode_hap()?

Yes to both.  As you spotted, I converted the shadow side first, and
made the two consistent.

The paging_mode_{shadow,hap})() is necessary for idempotency.  These
functions really might get called before paging is set up, for an early
failure in domain_create().

The paging mode has nothing really to do with hostmode/guestmode/etc. 
It is the only way of expressing the logic where it is clear that the
lower pointer dereferences are trivially safe.  (Also, the guestmode
predicate isn't going to survive the nested virt work.  It's
conceptually broken.)

~Andrew