On Fri, May 06, 2022 at 03:40:26PM -0700, Reinette Chatre wrote:
> 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/
Yes, this does make sense to me. Important bit was to refactor
set_page_dirty() calls out but for sure put_page()'s can still
be wrapped.
As I responded to Dave just few moments ago, one option would be
*possibly* to do the truncation part in ksgxd but I hope we hope
we don't have to go to that because it would mean also extra
book-keeping...
Took a bit of effort but I finally got this reproduced with my
laptop with this CPU:
Intel(R) Core(TM) i7-1065G7 CPU @ 1.30GHz
That's why I've had some radio silence in the meanwhile. In other
words, I should be now able to also to test the fix locally.
PS. I'm sorry if I've double-replied to this. Was not sure if
my first response left yesterday, and could not find it from
lore (and this does contain anyway stuff that was not incldued
into that).
BR, Jarkko
