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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, 18 Jul 2023 09:30:11 -0500, Dave Hansen <dave.hansen@xxxxxxxxx> wrote:

On 7/17/23 13:29, Haitao Huang wrote:
...
@@ -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;
+		goto err_out_unlock;
+	}

Whenever I see one of these "make sure it isn't NULL", I always jump to
asking what *keeps* it from becoming NULL again.  In both cases here, I
think that's encl->lock.

Yes, encl->lock protects all enclave states, the xarray holding encl_pages, SECS, VAs, etc.

A comment would be really nice here, maybe on sgx_encl_load_secs(). Maybe:

/*
 * Ensure the SECS page is not swapped out.  Must be called with
 * encl->lock to protect _____ and ensure the SECS page is not
 * swapped out again.
 */

Thanks for the suggestion. Lock should be held for the duration of SECS usage.
So something like this?
/*
 * Ensure the SECS page is not swapped out.  Must be called with
 * encl->lock to protect the enclave states including SECS and
 * ensure the SECS page is not swapped out again while being used.
 */


diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
index 166692f2d501..4662a364ce62 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -257,6 +257,10 @@ static void sgx_reclaimer_write(struct sgx_epc_page *epc_page,

 	mutex_lock(&encl->lock);

+	/* Should not be possible */
+	if (WARN_ON(!(encl->secs.epc_page)))
+		goto out;

That comment isn't super helpful.  We generally don't WARN_ON() things
that should happen.  *Why* is it not possible?


When this part of code is reached, the reclaimer is holding at least one reclaimable EPC page to reclaim for the enclave and the code below only reclaims SECS when no reclaimable EPCs (number of SECS children being zero) of the enclave left. So it should not be possible. I'll remove this change because this is really not needed for fixing the bug as Kai pointed out.

I added this for sanity check when implementing multiple EPC tracking lists for cgroups. At one point there were list corruption issues if moving EPCs between lists not managed well. With those straightened out, and clear definitions of EPC states for moving them from one list to another, I no longer see much value to keep this even in later cgroup patches.

Thanks
Haitao



[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux