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

[Corneliu ZUZU <czuzu@bitdefender.com>]

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.
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)?

Corneliu.