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

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

[Andrew Cooper]

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)
-- 
2.1.4

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

[Corneliu ZUZU]

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.

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

[Andrew Cooper]

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

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

[Jan Beulich]

>>> On 01.02.16 at 18:56, <andrew.cooper3@citrix.com> wrote:
> For safety, NULL out the pointers after freeing them, in an attempt to make
> mistakes more obvious in the future.

Except that NULLing isn't really adding that much safety, and we'd
be better off poisoning such pointers. Nevertheless ...

> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

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

[Andrew Cooper]

On 02/02/16 10:43, Jan Beulich wrote:
>>>> On 01.02.16 at 18:56, <andrew.cooper3@citrix.com> wrote:
>> For safety, NULL out the pointers after freeing them, in an attempt to make
>> mistakes more obvious in the future.
> Except that NULLing isn't really adding that much safety, and we'd
> be better off poisoning such pointers. Nevertheless ...

NULLing the pointers would cause things like rtc_deinit() to always blow
up when it followed the NULL pointer.

IMO, we should unconditionally always NULL pointers when freeing a
pointer which isn't in local scope.  It would make issues such as these
completely obvious.

~Andrew

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

[Jan Beulich]

>>> On 02.02.16 at 11:48, <andrew.cooper3@citrix.com> wrote:
> On 02/02/16 10:43, Jan Beulich wrote:
>>>>> On 01.02.16 at 18:56, <andrew.cooper3@citrix.com> wrote:
>>> For safety, NULL out the pointers after freeing them, in an attempt to make
>>> mistakes more obvious in the future.
>> Except that NULLing isn't really adding that much safety, and we'd
>> be better off poisoning such pointers. Nevertheless ...
> 
> NULLing the pointers would cause things like rtc_deinit() to always blow
> up when it followed the NULL pointer.
> 
> IMO, we should unconditionally always NULL pointers when freeing a
> pointer which isn't in local scope.  It would make issues such as these
> completely obvious.

As would poisoning the pointers, yet poisoning has the advantage
of not allowing PV guests to control what the hypervisor might
access when erroneously de-referencing such a pointer.

Jan

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

[Andrew Cooper]

On 02/02/16 10:52, Jan Beulich wrote:
>>>> On 02.02.16 at 11:48, <andrew.cooper3@citrix.com> wrote:
>> On 02/02/16 10:43, Jan Beulich wrote:
>>>>>> On 01.02.16 at 18:56, <andrew.cooper3@citrix.com> wrote:
>>>> For safety, NULL out the pointers after freeing them, in an attempt to make
>>>> mistakes more obvious in the future.
>>> Except that NULLing isn't really adding that much safety, and we'd
>>> be better off poisoning such pointers. Nevertheless ...
>> NULLing the pointers would cause things like rtc_deinit() to always blow
>> up when it followed the NULL pointer.
>>
>> IMO, we should unconditionally always NULL pointers when freeing a
>> pointer which isn't in local scope.  It would make issues such as these
>> completely obvious.
> As would poisoning the pointers, yet poisoning has the advantage
> of not allowing PV guests to control what the hypervisor might
> access when erroneously de-referencing such a pointer.

Hmm.  If we taught xfree() about this poisoned value and it treated it
just as it would NULL, then this would work.

I will put it on my todo list, unless anyone else beats me to it.

~Andrew

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

[Corneliu ZUZU]

On 2/2/2016 12:52 PM, Jan Beulich wrote:
>> NULLing the pointers would cause things like rtc_deinit() to always blow
>> up when it followed the NULL pointer.
>>
>> IMO, we should unconditionally always NULL pointers when freeing a
>> pointer which isn't in local scope.  It would make issues such as these
>> completely obvious.
> As would poisoning the pointers, yet poisoning has the advantage
> of not allowing PV guests to control what the hypervisor might
> access when erroneously de-referencing such a pointer.
>
> Jan

Jan, that sounds interesting. I hope I'm not intruding, but when you 
have the time, could you please expand on this?
Besides distinguishing a nuked pointer from zeroed-out memory, I did not 
know of any other advantage of 0xDEADBEEF pointer poisoning (generally 
or specifically).
How could possibly setting a pointer to NULL allow a PV guest to control 
what the hypervisor might access, if the hypervisor *can't access* a 
NULL pointer?
And can a PV guest write data @ *hypervisor's* 0 page  (virtual and/or 
physical)?

Corneliu.

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

[Jan Beulich]

>>> On 02.02.16 at 12:39, <czuzu@bitdefender.com> wrote:
> On 2/2/2016 12:52 PM, Jan Beulich wrote:
>>> NULLing the pointers would cause things like rtc_deinit() to always blow
>>> up when it followed the NULL pointer.
>>>
>>> IMO, we should unconditionally always NULL pointers when freeing a
>>> pointer which isn't in local scope.  It would make issues such as these
>>> completely obvious.
>> As would poisoning the pointers, yet poisoning has the advantage
>> of not allowing PV guests to control what the hypervisor might
>> access when erroneously de-referencing such a pointer.
> 
> Jan, that sounds interesting. I hope I'm not intruding, but when you 
> have the time, could you please expand on this?
> Besides distinguishing a nuked pointer from zeroed-out memory, I did not 
> know of any other advantage of 0xDEADBEEF pointer poisoning (generally 
> or specifically).
> How could possibly setting a pointer to NULL allow a PV guest to control 
> what the hypervisor might access, if the hypervisor *can't access* a 
> NULL pointer?
> And can a PV guest write data @ *hypervisor's* 0 page  (virtual and/or 
> physical)?

Since the answer to this last question is "yes" (for the virtual page 0
only of course), I suppose the rest is obvious.

Jan

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

[Andrew Cooper]

On 02/02/16 11:39, Corneliu ZUZU wrote:
> On 2/2/2016 12:52 PM, Jan Beulich wrote:
>>> NULLing the pointers would cause things like rtc_deinit() to always
>>> blow
>>> up when it followed the NULL pointer.
>>>
>>> IMO, we should unconditionally always NULL pointers when freeing a
>>> pointer which isn't in local scope.  It would make issues such as these
>>> completely obvious.
>> As would poisoning the pointers, yet poisoning has the advantage
>> of not allowing PV guests to control what the hypervisor might
>> access when erroneously de-referencing such a pointer.
>>
>> Jan
>
> Jan, that sounds interesting. I hope I'm not intruding, but when you
> have the time, could you please expand on this?
> Besides distinguishing a nuked pointer from zeroed-out memory, I did
> not know of any other advantage of 0xDEADBEEF pointer poisoning
> (generally or specifically).

Xen is 64bit only these days.  Bit 47 of a 64bit pointer must be signed
extended, or a #GP fault occurs because of the use of a non-canonical
address.

Therefore, a pointer such as 0xdeadc0de00000000ULL will unconditionally
cause a #GP fault if Xen accidentally used it.

> How could possibly setting a pointer to NULL allow a PV guest to
> control what the hypervisor might access, if the hypervisor *can't
> access* a NULL pointer?
> And can a PV guest write data @ *hypervisor's* 0 page  (virtual and/or
> physical)?

Xen and PV guests share the virtual address space, in exactly the same
way as a native kernel and its userspace.  PV guests can map pages at 0.

Therefore, if Xen were to accidentally follow a NULL pointer, it may not
result in a pagefault.  (Hardware mechanisms such as SMEP and SMAP are
added protection against this, but don't work on older hardware)

~Andrew

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

[Corneliu ZUZU]

On 2/2/2016 2:05 PM, Andrew Cooper wrote:
> Xen and PV guests share the virtual address space, in exactly the same 
> way as a native kernel and its userspace. PV guests can map pages at 
> 0. Therefore, if Xen were to accidentally follow a NULL pointer, it 
> may not result in a pagefault. (Hardware mechanisms such as SMEP and 
> SMAP are added protection against this, but don't work on older hardware)
> ~Andrew 

Thank you, I finally got it.
(I also read 
http://wiki.xenproject.org/wiki/X86_Paravirtualised_Memory_Management , 
cleared things up)

Corneliu.