linux-kernel - Re: [PATCH] x86/sgx: fix a NULL pointer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <op.179a4xs0wjvjmi@hhuan26-mobl.amr.corp.intel.com>
Date:   Mon, 17 Jul 2023 21:42:23 -0500
From:   "Haitao Huang" <haitao.huang@...ux.intel.com>
To:     "linux-sgx@...r.kernel.org" <linux-sgx@...r.kernel.org>,
        "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
        "x86@...nel.org" <x86@...nel.org>,
        "jarkko@...nel.org" <jarkko@...nel.org>,
        "bp@...en8.de" <bp@...en8.de>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "hpa@...or.com" <hpa@...or.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Huang, Kai" <kai.huang@...el.com>
Cc:     "kristen@...ux.intel.com" <kristen@...ux.intel.com>,
        "Chatre, Reinette" <reinette.chatre@...el.com>,
        "stable@...r.kernel.org" <stable@...r.kernel.org>,
        "Christopherson,, Sean" <seanjc@...gle.com>
Subject: Re: [PATCH] x86/sgx: fix a NULL pointer

Hi
On Mon, 17 Jul 2023 20:39:31 -0500, Huang, Kai <kai.huang@...el.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@...r.kernel.org
>> Signed-off-by: Haitao Huang <haitao.huang@...ux.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