lkml.org 
[lkml]   [2021]   [Jul]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH v3 4/7] x86/sgx: Add SGX infrastructure to recover from poison
On Wed, Jul 28, 2021, Dave Hansen wrote:
> On 7/28/21 1:46 PM, Tony Luck wrote:
> > +int sgx_memory_failure(unsigned long pfn, int flags)
> > +{
> ...
> > + page->flags |= SGX_EPC_PAGE_POISON;
>
> Is this safe outside of any locks?

It's safe outside of sgx_reclaimer_lock iff this can guarantee nothing else can
reach the page. I'm pretty sure that doesn't hold true here.

> I see the reclaimer doing things like:
>
> epc_page->flags &= ~SGX_EPC_PAGE_RECLAIMER_TRACKED;
>
> I'd worry that this code and other non-atomic epc_page->flags
> manipulation could trample on each other.
>
> This might need to some some atomic bit manipulation *and* convert all
> the other epc_page->flags users.

I don't think atomics would be sufficient as that would open all sorts of possible
races. E.g. this new code in __sgx_sanitize_pages()

page = list_first_entry(dirty_page_list, struct sgx_epc_page, list);

+ if (page->flags & SGX_EPC_PAGE_POISON) {
+ list_del(&page->list);
+ continue;
+ }
+
***HERE***
ret = __eremove(sgx_get_epc_virt_addr(page));

could attempt EREMOVE on a freshly POISONed page. That appears to be "benign"
since ENCLS is wrapped with_ASM_EXTABLE_FAULT, but it feels wrong to add a check
that we know can race.

And similar races for allocation/free could hand out a poisoned page or add one
to the free list.

@@ -585,6 +600,10 @@ struct sgx_epc_page *sgx_alloc_epc_page(void *owner, bool reclaim)

for ( ; ; ) {
page = __sgx_alloc_epc_page();
+
+ if (page->flags & SGX_EPC_PAGE_POISON)
+ continue;
*** HERE ***
+


@@ -630,7 +651,8 @@ void sgx_free_epc_page(struct sgx_epc_page *page)
spin_lock(&node->lock);

page->owner = NULL;
- list_add_tail(&page->list, &node->free_page_list);
+ if (!(page->flags & SGX_EPC_PAGE_POISON))
*** HERE ***
+ list_add_tail(&page->list, &node->free_page_list);


Setting POISON and hoping we eventually notice doesn't sound robust. Maybe some
of these races are unavoidable due to the nature of #MC delivery, but I would hope
the kernel can at least avoid handing out a poisoned page to a different enclave.
\
 
 \ /
  Last update: 2021-07-29 01:00    [W:0.199 / U:0.056 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site