Re: [PATCH] x86/hvm: Fix use-after-free introduced by c/s 428607a

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 02/02/16 08:00, Corneliu ZUZU wrote:
> On 2/1/2016 7:56 PM, Andrew Cooper wrote:
>> c/s 428607a "x86: shrink 'struct domain', was already PAGE_SIZE"
>> introduced a
>> use-after-free error during domain destruction, because of the order
>> in which
>> timers are torn down.
>>
>>    (XEN) Xen call trace:
>>    (XEN)    [<ffff82d08013344e>] spinlock.c#check_lock+0x1e/0x40
>>    (XEN)    [<ffff82d08013349b>] _spin_lock+0x11/0x52
>>    (XEN)    [<ffff82d0801e8076>] vpt.c#pt_lock+0x24/0x40
>>    (XEN)    [<ffff82d0801e88f4>] destroy_periodic_time+0x18/0x81
>>    (XEN)    [<ffff82d0801e1089>] rtc_deinit+0x53/0x78
>>    (XEN)    [<ffff82d0801d1e5a>] hvm_domain_destroy+0x52/0x69
>>    (XEN)    [<ffff82d08016a758>] arch_domain_destroy+0x1a/0x98
>>    (XEN)    [<ffff82d080107cd5>]
>> domain.c#complete_domain_destroy+0x6f/0x182
>>    (XEN)    [<ffff82d080126a19>]
>> rcupdate.c#rcu_process_callbacks+0x144/0x1a6
>>    (XEN)    [<ffff82d080132c52>] softirq.c#__do_softirq+0x82/0x8d
>>    (XEN)    [<ffff82d080132caa>] do_softirq+0x13/0x15
>>    (XEN)    [<ffff82d080248ae1>] entry.o#process_softirqs+0x21/0x30
>>    (XEN)
>>    (XEN)
>>    (XEN) ****************************************
>>    (XEN) Panic on CPU 3:
>>    (XEN) GENERAL PROTECTION FAULT
>>    (XEN) [error_code=0000]
>>    (XEN) ****************************************
>>
>> Defer the freeing of d->arch.hvm_domain.pl_time until all timers have
>> been
>> destroyed.
>>
>> For safety, NULL out the pointers after freeing them, in an attempt
>> to make
>> mistakes more obvious in the future.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Corneliu ZUZU <czuzu@bitdefender.com>
>> ---
>>   xen/arch/x86/hvm/hvm.c | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>> index f24400d..38c65b3 100644
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -1674,8 +1674,10 @@ void hvm_domain_relinquish_resources(struct
>> domain *d)
>>   void hvm_domain_destroy(struct domain *d)
>>   {
>>       xfree(d->arch.hvm_domain.io_handler);
>> +    d->arch.hvm_domain.io_handler = NULL;
>> +
>>       xfree(d->arch.hvm_domain.params);
>> -    xfree(d->arch.hvm_domain.pl_time);
>> +    d->arch.hvm_domain.params = NULL;
>>         hvm_destroy_cacheattr_region_list(d);
>>   @@ -1686,6 +1688,9 @@ void hvm_domain_destroy(struct domain *d)
>>       rtc_deinit(d);
>>       stdvga_deinit(d);
>>       vioapic_deinit(d);
>> +
>> +    xfree(d->arch.hvm_domain.pl_time);
>> +    d->arch.hvm_domain.pl_time = NULL;
>>   }
>>     static int hvm_save_tsc_adjust(struct domain *d,
>> hvm_domain_context_t *h)
>
> Ups, really sorry, ashamed to say I've done the mistake of not
> actually testing this on a machine (working on ARM here). Won't happen
> again, if I'm to send another patch I'll be sure to actually setup an
> X86 Dom0 & at least do some basic tests.

At least a boot test would certainly be nice.  In this case however, the
build passed all but a single one of the XenServer sanity tests, and
even the failure here was just chance that the region got reused in a
way which would be noticed.

This is, after all, precisely the reason why development branches are
not generally known to be stable.

> Regarding the "set to NULL after free" practice, wouldn't it be wise
> to define an xfreeandnull(void**) macro and use the "unsafe" xfree
> only when it makes sense to (proly most of the time it won't)?

This idea has been suggested in the past.  I can't remember what became
of it.

~Andrew