[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] x86/hvm: Fix double free from vlapic_init() early error path


  • To: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • Date: Wed, 31 Mar 2021 14:56:27 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=3HjMmrzvK0BzFoQTQ+HyoK8vV4DZsaL0cdU3Kvbt8xI=; b=QVPevwBtlsrvVLOC0GIPBVgyzZn8zyxoCFEGe1dIaeGJ3ady+o8U46n1w+xNu2/fyV1cDu7JbmsOFa41citbPditT/T6tL2ffDy/Hd/AIiByW1S4aT6DF1KBK2aNSYw2NNzehwNRjrcjIE6quemNjpYYOGG70FcJY8/ed/C6nPKn3dW8WM7/EVMeRQ3sQf2uscbzlrguGZEmWODoxf+ww2oBppYDwzknBfJ0s7O2DEe7sg0eB3MBrtzKVnJAy7ZMTpm0XbtGs6neS/w2p+om7BPt8niAYxk3wXORX7/Lw0JcoJ1M9aastvPVbyRcKZfmFnNbFfTaDIwYqCZHS7Cm0g==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=P0w5iSkG0+lY7QmX02UDQ0SR/DlZEbDUkxiclEPrZrGiqXod2uAZ1+bPJRkhEE2Xs3ddpDVebkmc9esUFq0JLdAmshK4lmXDR/ct0L1vdnE5wtI5glxSPfg+VtNLusVR+E+hEkRMo2yPLf9Tsx+r+Vx1vcQMp+75C1pcJElYedhS70otKKuPWeUOqri9DECxrfLu/ElqSx17sfl3k6skzeNGRiGSPc230GsUVkquE44srIg8uNtZb98ZI0e2FE941qvTNU0i2SD6RcsWqVmeZ209Y+FlEUbcASxlq+PgIqjWg6tgydGR/WfXUSIGym48aDoASJkvhBcPsziPZWzPaA==
  • Authentication-results: esa2.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Jan Beulich <JBeulich@xxxxxxxx>, Wei Liu <wl@xxxxxxx>
  • Delivery-date: Wed, 31 Mar 2021 13:56:42 +0000
  • Ironport-hdrordr: A9a23:cJ+SzqhhC8xLvQNYl/VEdMqhrHBQX5tx3DAbvn1ZSRFFG/Gwv/ uF2NwGyB75jysQUnk8mdaGfJKNW2/Y6IQd2/hzAZ6LZyOjnGezNolt4c/ZwzPmEzDj7eI178 ddWoBEIpnLAVB+5Pya3CCRGdwt2cTC1aiui/vXwXsFd3AUV4hL6QBlBgGHVmh/QwdbDZQ0fa DsmPZvjTymZHgRc4CHFmAINtKz6eHjubDHRVo9BxAh4BSTlj/A0tTHOjWRwxt2aUI1/Z4M6m 7A+jaJgpmLk/b+8RPE0n+W0pI+oqqc9vJmJOihzvcYMS/tjAHAXvUhZ5SnsCouqO+irHYG+e O82SsIBMh453PPcmzdm3KEsGSNv1heiQ6G9XaijXTuusD/Tj4hYvAx+L5xSAfT6EYrobhHoc R29l+ZrJZeAFfhmynw9rHzJmlXv3e0unYrnKoviWVeW+IlGcZshLEYlXkld6soLWbRx4AjDe V0SOTb4u8+SyLHU1np+k1UhPC8VHU6GRmLBmIYvNaO7jRQlHdli2MF2c02hB47hdEAYqgBw9 6BHrVjlblIQMNTR7l6Hv09Tcy+DXGIaQ7QMViVPU/sGMg8SjDwgq+yxI9wyPCheZQOwpd3so /GSklkuWk7fF+rLsGSwptR8FToTH+mVTrgjuFSjqIJ/4HUdf7OC2muWVoum8yvr7E0GcvAQc u+P5pQHrvtNm3rFYFV3xDvWpVbJHUEOfdl/eoTaharmIbmO4fqvuvUfLL4P7z2CwspXWv5Hz 8CUVHIVYN9x3HufkW9rAnaWnvrdEC614l3CrLm8+8az5VIMoVNtwMSmEmo/83jE0wajoUGOG 9FZJ/3mKKyome7uUzS6X9yBxZbBkFJpLP6U31LogcOO1jucakKvsiefWw65grCGjZPC+ftVC JPrVV+/qy6a7aKwzo5Nt6hOmWGy2cIqGmSVJcakK2b7cLjcpc1Z6xWAJBZJEHuLVhYiAxqoG BMZEs4XUfZDCrpkrjgpocTHvvje951hxqLLcZYpWnEj1iVodgiSxIgLmWTeP/SpTxrZjJPwn Vt7qcUgdO76EeSAFp6pN59DXphRyC8BqlcAAGMeYNO84qbCD1YfCOtnjyVixY6Z2zw0V4d71 aRbRG8SLXzGVxatWlf0qH2tGx1bXmGO319cWp7t4oVLxWdyylO+N7OXKCp32eXZlZq+JBBDB jMfSYSLgRyx9q+yR6Sn3KYGW87w4g1V9atf4gLYvXd3GigJ5aPkrxDF/hI/Ix9PNSrqeMTV/ mDEjXlYQ/QGqcs2waPoGwiNzQxoH44kenw0BmN1hny4FcvRf7TKk9hXbcVPpWV6HXlXe+B1N F8gcguteW9dmX3Zdju89CcUxdTbhfSq3WxVecmtNRdur8zrqJ6G93DSiTTvUs3qikWPYPxjg cTUa576LfONstmeNETYTtQ+h4smM6UJEUmvwTqCoYFDB0Qpm6eO8nM76vDqLIpDEHEvgf2NF WF+yBW/vvOXUK4pPYnIrN1JX4TZFk36Xxk8u/HapbZDx+ycfpfuFW9KX2wfdZmOee4MKRVqg w/5d6Gn+WaLXWlnA/RuCZ2OaJI/SKsR9ioDAeFBO5P9Ji7ND2389+Xyd/2iC2yTz2xL1kcj8 lCc0cba8xYkDksjIEtyEGJO+XKi1Ngl0Eb+C1tk17mx5Ov72jaF1xXKAGxuOQiYRBDdnyTyd nf+eeW1H7h8CFI1JnKGkBXZMxPEbErP//KBjYrL9MRsr6u97cuhSoGYA5GNR9PtAzA
  • Ironport-sdr: h9pEeLFlNb3mEf+YQ8gWz5jo2vIN9b9Zd1mhpxpf19m6DJ8DG2dHteDHZ1jn1KpExbOKLhzo6q PXBQwncgX6pGAB1y1vmwbAwecXNXoQovWIkhF6K1Y651GUcdBNil2NmLrJH6TL15qa5Lb4lIvq rkH13SnZ43mtAbfnLGxeDYK0GgVKqweHkRxe9VuWx/6Y9maOspEmdbBsj/PMAL0e2ilMykQ6b2 f75l4U1zwUnPQerVITvd1KIHgLvQy15JsMRpRdZu4hBrsMYN6tUIUOeGkoUHMX+Rs1BnAjqTrC jw4=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 31/03/2021 14:49, Roger Pau Monné wrote:
> On Wed, Mar 31, 2021 at 02:31:25PM +0100, Andrew Cooper wrote:
>> vlapic_init()'s caller calls vlapic_destroy() on error.  Therefore, the error
>> path from __map_domain_page_global() failing would doubly free
>> vlapic->regs_page.
>>
>> Rework vlapic_destroy() to be properly idempotent, introducing the necessary
>> UNMAP_DOMAIN_PAGE_GLOBAL() and FREE_DOMHEAP_PAGE() wrappers.
>>
>> Rearrange vlapic_init() to group all trivial initialisation, and leave all
>> cleanup to the caller, in line with our longer term plans.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> ---
>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>> CC: Wei Liu <wl@xxxxxxx>
>> ---
>>  xen/arch/x86/hvm/vlapic.c     | 11 ++++-------
>>  xen/include/xen/domain_page.h |  5 +++++
>>  xen/include/xen/mm.h          |  6 ++++++
>>  3 files changed, 15 insertions(+), 7 deletions(-)
>>
>> diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
>> index 5e21fb4937..da030f41b5 100644
>> --- a/xen/arch/x86/hvm/vlapic.c
>> +++ b/xen/arch/x86/hvm/vlapic.c
>> @@ -1608,7 +1608,9 @@ int vlapic_init(struct vcpu *v)
>>          return 0;
>>      }
>>  
>> +    /* Trivial init. */
>>      vlapic->pt.source = PTSRC_lapic;
>> +    spin_lock_init(&vlapic->esr_lock);
>>  
>>      vlapic->regs_page = alloc_domheap_page(v->domain, MEMF_no_owner);
>>      if ( !vlapic->regs_page )
>> @@ -1616,17 +1618,12 @@ int vlapic_init(struct vcpu *v)
>>  
>>      vlapic->regs = __map_domain_page_global(vlapic->regs_page);
>>      if ( vlapic->regs == NULL )
>> -    {
>> -        free_domheap_page(vlapic->regs_page);
>>          return -ENOMEM;
>> -    }
>>  
>>      clear_page(vlapic->regs);
>>  
>>      vlapic_reset(vlapic);
>>  
>> -    spin_lock_init(&vlapic->esr_lock);
>> -
>>      tasklet_init(&vlapic->init_sipi.tasklet, vlapic_init_sipi_action, v);
>>  
>>      if ( v->vcpu_id == 0 )
>> @@ -1645,8 +1642,8 @@ void vlapic_destroy(struct vcpu *v)
>>      tasklet_kill(&vlapic->init_sipi.tasklet);
>>      TRACE_0D(TRC_HVM_EMUL_LAPIC_STOP_TIMER);
>>      destroy_periodic_time(&vlapic->pt);
>> -    unmap_domain_page_global(vlapic->regs);
>> -    free_domheap_page(vlapic->regs_page);
>> +    UNMAP_DOMAIN_PAGE_GLOBAL(vlapic->regs);
> I think you need to check whether vlapic->regs_page is NULL here...
>
>> +    FREE_DOMHEAP_PAGE(vlapic->regs_page);
>>  }
>>  
>>  /*
>> diff --git a/xen/include/xen/domain_page.h b/xen/include/xen/domain_page.h
>> index a182d33b67..0cb7f2aad3 100644
>> --- a/xen/include/xen/domain_page.h
>> +++ b/xen/include/xen/domain_page.h
>> @@ -77,4 +77,9 @@ static inline void unmap_domain_page_global(const void 
>> *va) {};
>>      (p) = NULL;                     \
>>  } while ( false )
>>  
>> +#define UNMAP_DOMAIN_PAGE_GLOBAL(p) do {   \
>> +    unmap_domain_page_global(p);           \
>> +    (p) = NULL;                            \
>> +} while ( false )
>> +
>>  #endif /* __XEN_DOMAIN_PAGE_H__ */
>> diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
>> index 667f9dac83..c274e2eac4 100644
>> --- a/xen/include/xen/mm.h
>> +++ b/xen/include/xen/mm.h
>> @@ -85,6 +85,12 @@ bool scrub_free_pages(void);
>>  } while ( false )
>>  #define FREE_XENHEAP_PAGE(p) FREE_XENHEAP_PAGES(p, 0)
>>  
>> +#define FREE_DOMHEAP_PAGES(p, o) do { \
>> +    free_domheap_pages(p, o);         \
> ...as both unmap_domain_page_global and free_domheap_pages don't
> support being passed a NULL pointer.
>
> Passing such NULL pointer will result in unmap_domain_page_global
> hitting an assert AFAICT, and free_domheap_pages will try to use
> page_get_owner(NULL).

Urgh - very good points.

Do we perhaps want to take the opportunity to make these functions
tolerate NULL, to simplify all cleanup code across the hypervisor?

~Andrew



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.