Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Content-Type: text/plain; charset=iso-8859-15; format=flowed; delsp=yes
To:     "linux-sgx@vger.kernel.org" <linux-sgx@vger.kernel.org>,
        "dave.hansen@linux.intel.com" <dave.hansen@linux.intel.com>,
        "x86@kernel.org" <x86@kernel.org>,
        "jarkko@kernel.org" <jarkko@kernel.org>,
        "bp@alien8.de" <bp@alien8.de>,
        "mingo@redhat.com" <mingo@redhat.com>,
        "tglx@linutronix.de" <tglx@linutronix.de>,
        "hpa@zytor.com" <hpa@zytor.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "Huang, Kai" <kai.huang@intel.com>
Cc:     "kristen@linux.intel.com" <kristen@linux.intel.com>,
        "Chatre, Reinette" <reinette.chatre@intel.com>,
        "stable@vger.kernel.org" <stable@vger.kernel.org>,
        "Christopherson,, Sean" <seanjc@google.com>
Subject: Re: [PATCH] x86/sgx: fix a NULL pointer
References: <38deca3161bd4c5f1698fd7b6c43aa3c7b94d3da.camel@intel.com>
 <20230718004529.16404-1-haitao.huang@linux.intel.com>
 <a0dbb519550bae23c2bd02cd9c5940f4e9b2a38a.camel@intel.com>
Date:   Mon, 17 Jul 2023 21:42:23 -0500
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
From:   "Haitao Huang" <haitao.huang@linux.intel.com>
Organization: Intel
Message-ID: <op.179a4xs0wjvjmi@hhuan26-mobl.amr.corp.intel.com>
In-Reply-To: <a0dbb519550bae23c2bd02cd9c5940f4e9b2a38a.camel@intel.com>
User-Agent: Opera Mail/1.0 (Win32)
Precedence: bulk

Hi
On Mon, 17 Jul 2023 20:39:31 -0500, Huang, Kai <kai.huang@intel.com> wrote:

> On Mon, 2023-07-17 at 17:45 -0700, Haitao Huang wrote:
>> Under heavy load, the SGX EPC reclaimer (ksgxd) may reclaim the SECS EPC
>> page for an enclave and set encl->secs.epc_page to NULL. But the SECS
>> EPC page is used for EAUG in the SGX #PF handler without checking for
>> NULL and reloading.
>>
>> Fix this by checking if SECS is loaded before EAUG and loading it if it  
>> was
>> reclaimed.
>
> Nit:
>
> Looks the sentence break of the second paragraph isn't consistent with  
> the first
> paragraph, i.e., looks the first line is too long and the "was" should  
> be broken
> to the second line.
>
Yeah, I think I forgot to reformat this line after revising.

> And I think even for this simple patch, you are sending too frequently.   
> The
> patch subject should contain the version number too so people can  
> distinguish it
> from the previous one.  Even better, please use "--base=auto" when  
> generating
> the patch so people can know the base commit to apply to.

I'll send the next one as V2 and start a separate email thread.

>>
>> Fixes: 5a90d2c3f5ef ("x86/sgx: Support adding of pages to an  
>> initialized enclave")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Haitao Huang <haitao.huang@linux.intel.com>
>> ---
>>  arch/x86/kernel/cpu/sgx/encl.c | 25 ++++++++++++++++++++-----
>>  1 file changed, 20 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/sgx/encl.c  
>> b/arch/x86/kernel/cpu/sgx/encl.c
>> index 2a0e90fe2abc..2ab544da1664 100644
>> --- a/arch/x86/kernel/cpu/sgx/encl.c
>> +++ b/arch/x86/kernel/cpu/sgx/encl.c
>> @@ -235,6 +235,16 @@ static struct sgx_epc_page *sgx_encl_eldu(struct  
>> sgx_encl_page *encl_page,
>>  	return epc_page;
>>  }
>>
>> +static struct sgx_epc_page *sgx_encl_load_secs(struct sgx_encl *encl)
>> +{
>> +	struct sgx_epc_page *epc_page = encl->secs.epc_page;
>> +
>> +	if (!epc_page)
>> +		epc_page = sgx_encl_eldu(&encl->secs, NULL);
>> +
>> +	return epc_page;
>> +}
>> +
>>  static struct sgx_encl_page *__sgx_encl_load_page(struct sgx_encl  
>> *encl,
>>  						  struct sgx_encl_page *entry)
>>  {
>> @@ -248,11 +258,9 @@ static struct sgx_encl_page  
>> *__sgx_encl_load_page(struct sgx_encl *encl,
>>  		return entry;
>>  	}
>>
>> -	if (!(encl->secs.epc_page)) {
>> -		epc_page = sgx_encl_eldu(&encl->secs, NULL);
>> -		if (IS_ERR(epc_page))
>> -			return ERR_CAST(epc_page);
>> -	}
>> +	epc_page = sgx_encl_load_secs(encl);
>> +	if (IS_ERR(epc_page))
>> +		return ERR_CAST(epc_page);
>>
>>  	epc_page = sgx_encl_eldu(entry, encl->secs.epc_page);
>>  	if (IS_ERR(epc_page))
>> @@ -339,6 +347,13 @@ static vm_fault_t sgx_encl_eaug_page(struct  
>> vm_area_struct *vma,
>>
>>  	mutex_lock(&encl->lock);
>>
>> +	epc_page = sgx_encl_load_secs(encl);
>> +	if (IS_ERR(epc_page)) {
>> +		if (PTR_ERR(epc_page) == -EBUSY)
>> +			vmret =  VM_FAULT_NOPAGE;
> 				^
> Nit: two spaces here (yeah you copied from the existing code, and sorry  
> forgot
> to point out in the previous version).
>
Sure. will fix in V2.

Thanks
Haitao