Received-SPF: pass (google.com: domain of linux-kernel+bounces-180093-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <e81e35b2-466b-4804-b717-f3e6dc26bfa2@intel.com>
Date: Wed, 15 May 2024 08:52:33 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 2/2] x86/sgx: Resolve EREMOVE page vs EAUG page data
 race
To: Dmitrii Kuvaiskii <dmitrii.kuvaiskii@intel.com>,
	<dave.hansen@linux.intel.com>, <jarkko@kernel.org>, <kai.huang@intel.com>,
	<haitao.huang@linux.intel.com>, <linux-sgx@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>
CC: <mona.vij@intel.com>, <kailun.qin@intel.com>, <stable@vger.kernel.org>
References: <20240515131240.1304824-1-dmitrii.kuvaiskii@intel.com>
 <20240515131240.1304824-3-dmitrii.kuvaiskii@intel.com>
Content-Language: en-US
From: Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20240515131240.1304824-3-dmitrii.kuvaiskii@intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?T0V3OEdoWFk1eWhXUUtOeXVpVUVYSk84bnJ0RndUdnBLL1RhTG1sa0M3NFJx?=
 =?utf-8?B?TWlMOXZrNzJ2NEF2czhLcXZHUDliVkpGVW8zMG1mYnZ0NThSbWNSVFJQTG1u?=
 =?utf-8?B?QldCOE9tdUNOSkNpd2diUGF1dlNtTTFTeFlFRFNKSllrWmVPQnZxaGw4RXVX?=
 =?utf-8?B?NVNnVGt2dkhqZFZnUE9kT3A4bjhLWUtZU1FkZHhPS2pUL0dmU09SalQ0dFZQ?=
 =?utf-8?B?TXloQnZNbFFvUWJlRjJodzJmeW9Mc1FKL2ltYlhGRE5DUFIvMXhxaTV0ck5Z?=
 =?utf-8?B?OEpFbmZhQVFrbVp3dEE5TThlZ214Z1ArL3gyczVjNnpheFFxR0h4NlJ6MkhF?=
 =?utf-8?B?RHJLTU5EVXRHbDRRa0I4M0NQclE5TUN4bHNXMmVSdzdmRGUvVWJuWHN2eldP?=
 =?utf-8?B?OFNndVJNVzUrTVFkR3lOMGExRkJubktrMzV1VnZ2alpNRkd0aEpyeDRhRGk0?=
 =?utf-8?B?Z1l1MkhZR0szZ0Rjc3dHUDVQb1U1LzFKNURlZEx1SjUwM01hR1FwbjVrL25a?=
 =?utf-8?B?TzVSVkRrWnMzR2xyd0t5SUNiOWZNM3lxbGZTNGZFVmNpY3BWNVhFN0E4b0J4?=
 =?utf-8?B?dFdQTE0zUVdKdjhjampHdDc1TWJMZkRhVHRxK2pSS0ZiQXAyb3ZUU2VCc2d3?=
 =?utf-8?B?WHYrM21DemJUNUpGT3RSVGN2UHlJOW94bmMzT1pPM0FreERuOGpFbFNkRTJM?=
 =?utf-8?B?TjJuQXJMb01Ed2o2aXlBNEh0TUtlM2Y5ZUJOYjB0WU8rZkdpWXBUUjhtd20x?=
 =?utf-8?B?US9rL0FLTDFQdHZUTnZPWGlNZ1RtaFhpV2ZDd1o1RmM3VTNGcXp2WlRrZm9L?=
 =?utf-8?B?YVVsMXJKMnJZck03SklKVk9IYjgrVmI5VFl0SVphR1pIUzc4c0xlM1IzMzVV?=
 =?utf-8?B?aU9uSEhpUlRubzltaUlQa29ta1pEYVNTYzFBVXFJOHozbXRQWkY0L3dON3dD?=
 =?utf-8?B?MTd0T1YzK3VleXVtQjVuUnAySHY2RnpTSkVQRkQzK2VRbFJCUjF4OWFITWc2?=
 =?utf-8?B?aWhDV05PaCticlBzV2lUZmdhaFFNakg5SGxETEhibXI0WkZkY05hczMxQW5r?=
 =?utf-8?B?VVpiTGx0YWFUaE5yVHE2ekZoZFVLUDZyVDJDSEZYN1dZbFltZkN2UkpPZnRM?=
 =?utf-8?B?U3BFdkFLTTROYmxjZVFjeTl5eHNmM3ZIS1duVkcxakpuYi90c0k3K2R3RHIz?=
 =?utf-8?B?am1XVXRoMjJvUlpTNnpYU2c0ZnJYU2tYcy9JaWZrOXhCMmpkT1Rac0ZjTlo4?=
 =?utf-8?B?VUM1b2JiSFZBZkRMZzZjTXpPQTNNNkFVNnVIajZuaXc2bnhLanl2SmRvc1Rr?=
 =?utf-8?B?N3NiTHBTU0k0SkJqVHQ1UG1aS1dDM2NJTWRJelREaUczekYzdHJHWEVLTlIw?=
 =?utf-8?B?SEhaY3orUUlBSVA1YUlBYUlpbmMvTzF1NHdkYlIvV05SVzZFckZHcHp5UlA0?=
 =?utf-8?B?WlozQmhBWTlaNnYrQnlPVStYQWJsSE5xNmhtanJxNEw0ZTBWeC9CMTkvMnpC?=
 =?utf-8?B?d2hXVGMzWkVtNlZzdCtmMFNwdmZoUGlKZDZpMkdYbktlNnJQWEFxd0hwREkz?=
 =?utf-8?B?NXJBeHVsOHBwalZ4ZW1sMzdZUjZYZE9uLzZJSmQ2bTNCTHUvbHU4VTJ2SkVP?=
 =?utf-8?B?Z2hMbHgvenAyOWloalJDUGxyQjhlZ2NKTjJDN1FwNE9PaUxzcnlyZFM2c0lV?=
 =?utf-8?B?Q3oyODh0RWhOSGdZcVJtVk1kc0ZqckVXQ2Qzem5BVURYMVF2M25aQ29adjJu?=
 =?utf-8?B?dkc1TzZLbWN3c1JzV2FqV0EzMjgrVGo2TTJsMTlBYUdaemFmaXNqK0szSFlr?=
 =?utf-8?B?cklYcFV5WHVpZU5mTUVSbkY1WGVuM3FqeVBMUlVYOERPVXJ2RmM4QVJTTjhu?=
 =?utf-8?B?M216N3ZydG1KclU0aC9waFJNdFJaSWpsa2tGUEhhQ0dEa3FnbHA0b1R2U1ps?=
 =?utf-8?B?SGQ4Q0Q4eXNXbDY2K21GSFlWbHJKWUZnYWtucjQ5eWtYbHd6Qno5Z0VDOUtl?=
 =?utf-8?B?a0NFMHJxWmNSeEtHY3JsYXdUYWV6Y2JMc3FpbS9NVjVKYklaMnRpUDJEdDk3?=
 =?utf-8?B?ZDRqME0vSHcxbUtsU1pRMEQzTjFOL1pmS0xnY2dMeGJKNHJIRXhSbzNCOTdE?=
 =?utf-8?B?ejlRem5LZXdnWThQZWtqL2YxdFpDdy9VRnJWN0twRlQxUUtSV3U4bWRqazVj?=
 =?utf-8?B?SXc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 2150a8d3-2baa-4eda-7ebd-08dc74f70a5c
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 May 2024 15:52:36.0851
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: ABuHYFGlE425hzDrtqGPa2Np3sqDY0CDg4FUYDOVNIR6A/4KJFL2Yb5Ocqd8ok6wXW1yVgxE8AVP4bYUxdlRD5H036RL0e+m558HjExvu0g=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR11MB6546
X-OriginatorOrg: intel.com

Hi Dmitrii,

On 5/15/2024 6:12 AM, Dmitrii Kuvaiskii wrote:
> Two enclave threads may try to add and remove the same enclave page
> simultaneously (e.g., if the SGX runtime supports both lazy allocation
> and MADV_DONTNEED semantics). Consider some enclave page added to the
> enclave. User space decides to temporarily remove this page (e.g.,
> emulating the MADV_DONTNEED semantics) on CPU1. At the same time, user
> space performs a memory access on the same page on CPU2, which results
> in a #PF and ultimately in sgx_vma_fault(). Scenario proceeds as
> follows:
> 
> /*
>  * CPU1: User space performs
>  * ioctl(SGX_IOC_ENCLAVE_REMOVE_PAGES)
>  * on enclave page X
>  */
> sgx_encl_remove_pages() {
> 
>   mutex_lock(&encl->lock);
> 
>   entry = sgx_encl_load_page(encl);
>   /*
>    * verify that page is
>    * trimmed and accepted
>    */
> 
>   mutex_unlock(&encl->lock);
> 
>   /*
>    * remove PTE entry; cannot
>    * be performed under lock
>    */
>   sgx_zap_enclave_ptes(encl);
>                                  /*
>                                   * Fault on CPU2 on same page X
>                                   */
>                                  sgx_vma_fault() {
>                                    /*
>                                     * PTE entry was removed, but the
>                                     * page is still in enclave's xarray
>                                     */
>                                    xa_load(&encl->page_array) != NULL ->
>                                    /*
>                                     * SGX driver thinks that this page
>                                     * was swapped out and loads it
>                                     */
>                                    mutex_lock(&encl->lock);
>                                    /*
>                                     * this is effectively a no-op
>                                     */
>                                    entry = sgx_encl_load_page_in_vma();
>                                    /*
>                                     * add PTE entry
>                                     *
>                                     * *BUG*: a PTE is installed for a
>                                     * page in process of being removed
>                                     */
>                                    vmf_insert_pfn(...);
> 
>                                    mutex_unlock(&encl->lock);
>                                    return VM_FAULT_NOPAGE;
>                                  }
>   /*
>    * continue with page removal
>    */
>   mutex_lock(&encl->lock);
> 
>   sgx_encl_free_epc_page(epc_page) {
>     /*
>      * remove page via EREMOVE
>      */
>     /*
>      * free EPC page
>      */
>     sgx_free_epc_page(epc_page);
>   }
> 
>   xa_erase(&encl->page_array);
> 
>   mutex_unlock(&encl->lock);
> }
> 
> Here, CPU1 removed the page. However CPU2 installed the PTE entry on the
> same page. This enclave page becomes perpetually inaccessible (until
> another SGX_IOC_ENCLAVE_REMOVE_PAGES ioctl). This is because the page is
> marked accessible in the PTE entry but is not EAUGed, and any subsequent
> access to this page raises a fault: with the kernel believing there to
> be a valid VMA, the unlikely error code X86_PF_SGX encountered by code
> path do_user_addr_fault() -> access_error() causes the SGX driver's
> sgx_vma_fault() to be skipped and user space receives a SIGSEGV instead.
> The userspace SIGSEGV handler cannot perform EACCEPT because the page
> was not EAUGed. Thus, the user space is stuck with the inaccessible
> page.
> 
> Fix this race by forcing the fault handler on CPU2 to back off if the
> page is currently being removed (on CPU1). This is achieved by
> introducing a new flag SGX_ENCL_PAGE_BEING_REMOVED, which is unset by
> default and set only right-before the first mutex_unlock() in
> sgx_encl_remove_pages(). Upon loading the page, CPU2 checks whether this
> page is being removed, and if yes then CPU2 backs off and waits until
> the page is completely removed. After that, any memory access to this
> page results in a normal "allocate and EAUG a page on #PF" flow.
> 
> Fixes: 9849bb27152c ("x86/sgx: Support complete page removal")
> Cc: stable@vger.kernel.org
> Signed-off-by: Dmitrii Kuvaiskii <dmitrii.kuvaiskii@intel.com>
> ---
>  arch/x86/kernel/cpu/sgx/encl.c  | 3 ++-
>  arch/x86/kernel/cpu/sgx/encl.h  | 3 +++
>  arch/x86/kernel/cpu/sgx/ioctl.c | 1 +
>  3 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
> index 41f14b1a3025..7ccd8b2fce5f 100644
> --- a/arch/x86/kernel/cpu/sgx/encl.c
> +++ b/arch/x86/kernel/cpu/sgx/encl.c
> @@ -257,7 +257,8 @@ static struct sgx_encl_page *__sgx_encl_load_page(struct sgx_encl *encl,
>  
>  	/* Entry successfully located. */
>  	if (entry->epc_page) {
> -		if (entry->desc & SGX_ENCL_PAGE_BEING_RECLAIMED)
> +		if (entry->desc & (SGX_ENCL_PAGE_BEING_RECLAIMED |
> +				   SGX_ENCL_PAGE_BEING_REMOVED))
>  			return ERR_PTR(-EBUSY);
>  
>  		return entry;
> diff --git a/arch/x86/kernel/cpu/sgx/encl.h b/arch/x86/kernel/cpu/sgx/encl.h
> index f94ff14c9486..fff5f2293ae7 100644
> --- a/arch/x86/kernel/cpu/sgx/encl.h
> +++ b/arch/x86/kernel/cpu/sgx/encl.h
> @@ -25,6 +25,9 @@
>  /* 'desc' bit marking that the page is being reclaimed. */
>  #define SGX_ENCL_PAGE_BEING_RECLAIMED	BIT(3)
>  
> +/* 'desc' bit marking that the page is being removed. */
> +#define SGX_ENCL_PAGE_BEING_REMOVED	BIT(2)
> +
>  struct sgx_encl_page {
>  	unsigned long desc;
>  	unsigned long vm_max_prot_bits:8;
> diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
> index b65ab214bdf5..c542d4dd3e64 100644
> --- a/arch/x86/kernel/cpu/sgx/ioctl.c
> +++ b/arch/x86/kernel/cpu/sgx/ioctl.c
> @@ -1142,6 +1142,7 @@ static long sgx_encl_remove_pages(struct sgx_encl *encl,
>  		 * Do not keep encl->lock because of dependency on
>  		 * mmap_lock acquired in sgx_zap_enclave_ptes().
>  		 */
> +		entry->desc |= SGX_ENCL_PAGE_BEING_REMOVED;
>  		mutex_unlock(&encl->lock);
>  
>  		sgx_zap_enclave_ptes(encl, addr);

Thank you very much for tracking down and fixing this issue.

Acked-by: Reinette Chatre <reinette.chatre@intel.com>

Reinette