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@xxxxxxxxx>
>> ---
>> 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@xxxxxxxxx/
Reinette
