Re: [RFC PATCH 2/4] x86/sgx: Set dirty bit after modifying page contents

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Jarkko,

On 5/6/2022 3:27 PM, Jarkko Sakkinen wrote:
> On Thu, 2022-04-28 at 13:11 -0700, Reinette Chatre wrote:
>> Recent commit 08999b2489b4 ("x86/sgx: Free backing memory
>> after faulting the enclave page") expanded __sgx_encl_eldu()
>> to clear an enclave page's PCMD (Paging Crypto MetaData)
>> from the PCMD page in the backing store after the enclave
>> page is restored to the enclave.
>>
>> Since the PCMD page in the backing store is modified the page
>> should be set as dirty when releasing the reference to
>> ensure the modified data is retained.
>>
>> Fixes: 08999b2489b4 ("x86/sgx: Free backing memory after faulting the enclave page")
>> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
>> ---
>>  arch/x86/kernel/cpu/sgx/encl.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
>> index e5d2661800ac..e03f124ce772 100644
>> --- a/arch/x86/kernel/cpu/sgx/encl.c
>> +++ b/arch/x86/kernel/cpu/sgx/encl.c
>> @@ -98,7 +98,7 @@ static int __sgx_encl_eldu(struct sgx_encl_page *encl_page,
>>         kunmap_atomic(pcmd_page);
>>         kunmap_atomic((void *)(unsigned long)pginfo.contents);
>>  
>> -       sgx_encl_put_backing(&b, false);
>> +       sgx_encl_put_backing(&b, true);
>>  
>>         sgx_encl_truncate_backing_page(encl, page_index);
>>  
> 
> So, the implementation is:
> 
> void sgx_encl_put_backing(struct sgx_backing *backing, bool do_write)
> {
> 	if (do_write) {
> 		set_page_dirty(backing->pcmd);
> 		set_page_dirty(backing->contents);
> 	}
> 
> 	put_page(backing->pcmd);
> 	put_page(backing->contents);
> }
> 
> And we only want to set dirty for PCMD part, right?
> 
> Thus, perhaps we should fix it instead by:
> 
> 	set_page_dirty(backing->pcmd);
> 	put_page(backing->pcmd):
> 	put_page(backing->contents);
> 	
> ?	
> 
> I would not mind getting rid of that function anyway. It's kind
> of bad wrapping IMHO.

Could we rather just change sgx_encl_put_backing() to be:

void sgx_encl_put_backing(struct sgx_backing *backing)
{
	put_page(backing->pcmd);
	put_page(backing->contents);
}

Two reasons:

1) Instead of getting rid of sgx_encl_put_backing() we can keep
   it so that its name reflects its work and its usage remains
   symmetrical to sgx_encl_get_backing() that will stay and
   the code thus continues to be clear on the page references.

2) By moving the set_page_dirty() out of that function the
   set_page_dirty() can be called _before_ writing data to
   the page, which is the right thing to do per:
   https://lore.kernel.org/linux-sgx/c057af3d-b7fb-34cd-0d75-989fca0e67fe@intel.com/

Reinette