| Message ID | 20220107181618.6597-3-kristen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/sgx: Limit EPC overcommit | expand |
On Fri, Jan 07, 2022 at 10:16:17AM -0800, Kristen Carlson Accardi wrote: > SGX may allow EPC pages to be overcommitted. If the system is > out of enclave memory, EPC pages are swapped to normal RAM via > a per enclave shared memory area. This shared memory is not > charged to the enclave or the task mapping it, making it hard > to account for using normal methods. > > In order to avoid unlimited usage of normal RAM, enclaves must be > charged for each new page used for backing storage, and uncharged > when they are no longer using a backing page. > > Modify the existing flow for requesting backing pages to reduce the > available backing page counter and confirm that the limit has not > been exceeded. Backing page usage for loading EPC pages back out of > the shared memory do not incur a charge. > > When a backing page is released from usage, increment the available > backing page counter. > > When swapping EPC pages to RAM, in addition to storing the page > contents, SGX must store some additional metadata to protect > against a malicious kernel when the page is swapped back in. This > additional metadata is called Paging Crypto MetaData. PCMD is > allocated from the same shared memory area as the backing page > contents and consumes RAM the same way. > > PCMD is 128 bytes in size, and there is one PCMD structure per > page written to shared RAM. The page index for the PCMD page is > calculated from the page index of the backing page, so it is possible > that the PCMD structures are not packed into the minimum number of > pages possible. If 32 PCMDs can fit onto a single page, then PCMD > usage is 1/32 of total EPC pages. In the worst case, PCMD can > consume the same amount of RAM as EPC backing pages (1:1). For > simplicity, this implementation does not account for PCMD page usage. > > Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com> > --- > arch/x86/kernel/cpu/sgx/encl.c | 76 ++++++++++++++++++++++++++++++++-- > arch/x86/kernel/cpu/sgx/encl.h | 6 ++- > arch/x86/kernel/cpu/sgx/main.c | 6 +-- > 3 files changed, 80 insertions(+), 8 deletions(-) > > diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c > index 001808e3901c..8be6f0592bdc 100644 > --- a/arch/x86/kernel/cpu/sgx/encl.c > +++ b/arch/x86/kernel/cpu/sgx/encl.c > @@ -32,7 +32,7 @@ static int __sgx_encl_eldu(struct sgx_encl_page *encl_page, > else > page_index = PFN_DOWN(encl->size); > > - ret = sgx_encl_get_backing(encl, page_index, &b); > + ret = sgx_encl_lookup_backing(encl, page_index, &b); > if (ret) > return ret; > > @@ -407,6 +407,12 @@ void sgx_encl_release(struct kref *ref) > sgx_encl_free_epc_page(entry->epc_page); > encl->secs_child_cnt--; > entry->epc_page = NULL; > + } else { > + /* > + * If there is no epc_page, it means it has been > + * swapped out. Uncharge the backing storage. > + */ > + sgx_uncharge_mem(); > } > > kfree(entry); > @@ -574,8 +580,8 @@ static struct page *sgx_encl_get_backing_page(struct sgx_encl *encl, > * 0 on success, > * -errno otherwise. > */ > -int sgx_encl_get_backing(struct sgx_encl *encl, unsigned long page_index, > - struct sgx_backing *backing) > +static int sgx_encl_get_backing(struct sgx_encl *encl, unsigned long page_index, > + struct sgx_backing *backing) > { > pgoff_t pcmd_index = PFN_DOWN(encl->size) + 1 + (page_index >> 5); > struct page *contents; > @@ -601,6 +607,62 @@ int sgx_encl_get_backing(struct sgx_encl *encl, unsigned long page_index, > return 0; > } > > +/** > + * sgx_encl_alloc_backing() - allocate a new backing storage page > + * @encl: an enclave pointer > + * @page_index: enclave page index > + * @backing: data for accessing backing storage for the page > + * > + * Confirm that the global overcommit limit has not been reached before > + * requesting a new backing storage page for storing the encrypted contents > + * and Paging Crypto MetaData (PCMD) of an enclave page. This is called when > + * there is no existing backing page, just before writing to the backing > + * storage with EWB. > + * > + * Return: > + * 0 on success, > + * -errno otherwise. > + */ > +int sgx_encl_alloc_backing(struct sgx_encl *encl, unsigned long page_index, > + struct sgx_backing *backing) > +{ > + int ret; > + > + if (sgx_charge_mem()) > + return -ENOMEM; > + > + ret = sgx_encl_get_backing(encl, page_index, backing); > + if (ret) > + sgx_uncharge_mem(); > + > + return ret; > +} > + > +/** > + * sgx_encl_lookup_backing() - retrieve an existing backing storage page > + * @encl: an enclave pointer > + * @page_index: enclave page index > + * @backing: data for accessing backing storage for the page > + * > + * Retrieve a backing page for loading data back into an EPC page with ELDU. > + * This call does not cause a charge to the overcommit limit because a page > + * has already been allocated, but has been swapped out or is in RAM > + * > + * It is the caller's responsibility to ensure that it is appropriate to > + * use sgx_encl_lookup_backing() rather than sgx_encl_alloc_backing(). If > + * lookup is not used correctly, this will cause an allocation that is > + * not accounted for. > + * > + * Return: > + * 0 on success, > + * -errno otherwise. > + */ > +int sgx_encl_lookup_backing(struct sgx_encl *encl, unsigned long page_index, > + struct sgx_backing *backing) > +{ > + return sgx_encl_get_backing(encl, page_index, backing); > +} IMHO, sgx_encl_backing() should be open-coded here. > + > /** > * sgx_encl_put_backing() - Unpin the backing storage > * @backing: data for accessing backing storage for the page > @@ -608,9 +670,17 @@ int sgx_encl_get_backing(struct sgx_encl *encl, unsigned long page_index, > */ > void sgx_encl_put_backing(struct sgx_backing *backing, bool do_write) > { > + /* > + * If the page is being written to by the reclaimer, it is > + * still in use and the backing page usage should not be > + * uncharged. However, if the page is not being written to, > + * it is no longer in use and may be uncharged. > + */ > if (do_write) { > set_page_dirty(backing->pcmd); > set_page_dirty(backing->contents); > + } else { > + sgx_uncharge_mem(); > } > > put_page(backing->pcmd); > diff --git a/arch/x86/kernel/cpu/sgx/encl.h b/arch/x86/kernel/cpu/sgx/encl.h > index fec43ca65065..8ffb8a83263f 100644 > --- a/arch/x86/kernel/cpu/sgx/encl.h > +++ b/arch/x86/kernel/cpu/sgx/encl.h > @@ -105,8 +105,10 @@ int sgx_encl_may_map(struct sgx_encl *encl, unsigned long start, > > void sgx_encl_release(struct kref *ref); > int sgx_encl_mm_add(struct sgx_encl *encl, struct mm_struct *mm); > -int sgx_encl_get_backing(struct sgx_encl *encl, unsigned long page_index, > - struct sgx_backing *backing); > +int sgx_encl_alloc_backing(struct sgx_encl *encl, unsigned long page_index, > + struct sgx_backing *backing); This might sound overly long but it really would make this a lot clearer than alloc, which can mean pretty much anything: sgx_encl_lookup_and_charge_backing() What do you think? That leaves no guesswork what the function does. > +int sgx_encl_lookup_backing(struct sgx_encl *encl, unsigned long page_index, > + struct sgx_backing *backing); > void sgx_encl_put_backing(struct sgx_backing *backing, bool do_write); > int sgx_encl_test_and_clear_young(struct mm_struct *mm, > struct sgx_encl_page *page); > diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c > index 8a7bfe0d9023..20ccd9c5b184 100644 > --- a/arch/x86/kernel/cpu/sgx/main.c > +++ b/arch/x86/kernel/cpu/sgx/main.c > @@ -347,8 +347,8 @@ static void sgx_reclaimer_write(struct sgx_epc_page *epc_page, > encl->secs_child_cnt--; > > if (!encl->secs_child_cnt && test_bit(SGX_ENCL_INITIALIZED, &encl->flags)) { > - ret = sgx_encl_get_backing(encl, PFN_DOWN(encl->size), > - &secs_backing); > + ret = sgx_encl_alloc_backing(encl, PFN_DOWN(encl->size), > + &secs_backing); > if (ret) > goto out; > > @@ -418,7 +418,7 @@ static void sgx_reclaim_pages(void) > goto skip; > > page_index = PFN_DOWN(encl_page->desc - encl_page->encl->base); > - ret = sgx_encl_get_backing(encl_page->encl, page_index, &backing[i]); > + ret = sgx_encl_alloc_backing(encl_page->encl, page_index, &backing[i]); > if (ret) > goto skip; > > -- > 2.20.1 > /Jarkko
On Sat, 2022-01-08 at 18:05 +0200, Jarkko Sakkinen wrote: > On Fri, Jan 07, 2022 at 10:16:17AM -0800, Kristen Carlson Accardi > wrote: > > SGX may allow EPC pages to be overcommitted. If the system is > > out of enclave memory, EPC pages are swapped to normal RAM via > > a per enclave shared memory area. This shared memory is not > > charged to the enclave or the task mapping it, making it hard > > to account for using normal methods. > > > > In order to avoid unlimited usage of normal RAM, enclaves must be > > charged for each new page used for backing storage, and uncharged > > when they are no longer using a backing page. > > > > Modify the existing flow for requesting backing pages to reduce the > > available backing page counter and confirm that the limit has not > > been exceeded. Backing page usage for loading EPC pages back out of > > the shared memory do not incur a charge. > > > > When a backing page is released from usage, increment the available > > backing page counter. > > > > When swapping EPC pages to RAM, in addition to storing the page > > contents, SGX must store some additional metadata to protect > > against a malicious kernel when the page is swapped back in. This > > additional metadata is called Paging Crypto MetaData. PCMD is > > allocated from the same shared memory area as the backing page > > contents and consumes RAM the same way. > > > > PCMD is 128 bytes in size, and there is one PCMD structure per > > page written to shared RAM. The page index for the PCMD page is > > calculated from the page index of the backing page, so it is > > possible > > that the PCMD structures are not packed into the minimum number of > > pages possible. If 32 PCMDs can fit onto a single page, then PCMD > > usage is 1/32 of total EPC pages. In the worst case, PCMD can > > consume the same amount of RAM as EPC backing pages (1:1). For > > simplicity, this implementation does not account for PCMD page > > usage. > > > > Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com> > > --- > > arch/x86/kernel/cpu/sgx/encl.c | 76 > > ++++++++++++++++++++++++++++++++-- > > arch/x86/kernel/cpu/sgx/encl.h | 6 ++- > > arch/x86/kernel/cpu/sgx/main.c | 6 +-- > > 3 files changed, 80 insertions(+), 8 deletions(-) > > > > diff --git a/arch/x86/kernel/cpu/sgx/encl.c > > b/arch/x86/kernel/cpu/sgx/encl.c > > index 001808e3901c..8be6f0592bdc 100644 > > --- a/arch/x86/kernel/cpu/sgx/encl.c > > +++ b/arch/x86/kernel/cpu/sgx/encl.c > > @@ -32,7 +32,7 @@ static int __sgx_encl_eldu(struct sgx_encl_page > > *encl_page, > > else > > page_index = PFN_DOWN(encl->size); > > > > - ret = sgx_encl_get_backing(encl, page_index, &b); > > + ret = sgx_encl_lookup_backing(encl, page_index, &b); > > if (ret) > > return ret; > > > > @@ -407,6 +407,12 @@ void sgx_encl_release(struct kref *ref) > > sgx_encl_free_epc_page(entry->epc_page); > > encl->secs_child_cnt--; > > entry->epc_page = NULL; > > + } else { > > + /* > > + * If there is no epc_page, it means it has > > been > > + * swapped out. Uncharge the backing storage. > > + */ > > + sgx_uncharge_mem(); > > } > > > > kfree(entry); > > @@ -574,8 +580,8 @@ static struct page > > *sgx_encl_get_backing_page(struct sgx_encl *encl, > > * 0 on success, > > * -errno otherwise. > > */ > > -int sgx_encl_get_backing(struct sgx_encl *encl, unsigned long > > page_index, > > - struct sgx_backing *backing) > > +static int sgx_encl_get_backing(struct sgx_encl *encl, unsigned > > long page_index, > > + struct sgx_backing *backing) > > { > > pgoff_t pcmd_index = PFN_DOWN(encl->size) + 1 + (page_index >> > > 5); > > struct page *contents; > > @@ -601,6 +607,62 @@ int sgx_encl_get_backing(struct sgx_encl > > *encl, unsigned long page_index, > > return 0; > > } > > > > +/** > > + * sgx_encl_alloc_backing() - allocate a new backing storage page > > + * @encl: an enclave pointer > > + * @page_index: enclave page index > > + * @backing: data for accessing backing storage for the page > > + * > > + * Confirm that the global overcommit limit has not been reached > > before > > + * requesting a new backing storage page for storing the encrypted > > contents > > + * and Paging Crypto MetaData (PCMD) of an enclave page. This is > > called when > > + * there is no existing backing page, just before writing to the > > backing > > + * storage with EWB. > > + * > > + * Return: > > + * 0 on success, > > + * -errno otherwise. > > + */ > > +int sgx_encl_alloc_backing(struct sgx_encl *encl, unsigned long > > page_index, > > + struct sgx_backing *backing) > > +{ > > + int ret; > > + > > + if (sgx_charge_mem()) > > + return -ENOMEM; > > + > > + ret = sgx_encl_get_backing(encl, page_index, backing); > > + if (ret) > > + sgx_uncharge_mem(); > > + > > + return ret; > > +} > > + > > +/** > > + * sgx_encl_lookup_backing() - retrieve an existing backing > > storage page > > + * @encl: an enclave pointer > > + * @page_index: enclave page index > > + * @backing: data for accessing backing storage for the page > > + * > > + * Retrieve a backing page for loading data back into an EPC page > > with ELDU. > > + * This call does not cause a charge to the overcommit limit > > because a page > > + * has already been allocated, but has been swapped out or is in > > RAM > > + * > > + * It is the caller's responsibility to ensure that it is > > appropriate to > > + * use sgx_encl_lookup_backing() rather than > > sgx_encl_alloc_backing(). If > > + * lookup is not used correctly, this will cause an allocation > > that is > > + * not accounted for. > > + * > > + * Return: > > + * 0 on success, > > + * -errno otherwise. > > + */ > > +int sgx_encl_lookup_backing(struct sgx_encl *encl, unsigned long > > page_index, > > + struct sgx_backing *backing) > > +{ > > + return sgx_encl_get_backing(encl, page_index, backing); > > +} > > IMHO, sgx_encl_backing() should be open-coded here. I can understand your hesitation, but I agree with Dave here that wrapping the function makes the code more clear. I would prefer to keep this the way it is.
On 1/14/22 9:51 AM, Kristen Carlson Accardi wrote: >>> +int sgx_encl_lookup_backing(struct sgx_encl *encl, unsigned long >>> page_index, >>> + struct sgx_backing *backing) >>> +{ >>> + return sgx_encl_get_backing(encl, page_index, backing); >>> +} >> IMHO, sgx_encl_backing() should be open-coded here. > I can understand your hesitation, but I agree with Dave here that > wrapping the function makes the code more clear. I would prefer to keep > this the way it is. I'd also like to see sgx_encl_lookup_backing() and sgx_encl_alloc_backing() diverge more in the future. For instance, sgx_encl_alloc_backing() could ensure that the page does not exist in the file before doing the sgx_encl_get_backing() call. This would ensure that it truly *does* allocate a page and does not just return a previously-allocated page. sgx_encl_lookup_backing() could ensure the opposite: that the page *DOES* exist in the file before doing the sgx_encl_get_backing() call. This would ensure that it does not allocate a page in a case where we expected an old, existing page to be present.
On Fri, Jan 14, 2022 at 09:51:14AM -0800, Kristen Carlson Accardi wrote: > On Sat, 2022-01-08 at 18:05 +0200, Jarkko Sakkinen wrote: > > On Fri, Jan 07, 2022 at 10:16:17AM -0800, Kristen Carlson Accardi > > wrote: > > > SGX may allow EPC pages to be overcommitted. If the system is > > > out of enclave memory, EPC pages are swapped to normal RAM via > > > a per enclave shared memory area. This shared memory is not > > > charged to the enclave or the task mapping it, making it hard > > > to account for using normal methods. > > > > > > In order to avoid unlimited usage of normal RAM, enclaves must be > > > charged for each new page used for backing storage, and uncharged > > > when they are no longer using a backing page. > > > > > > Modify the existing flow for requesting backing pages to reduce the > > > available backing page counter and confirm that the limit has not > > > been exceeded. Backing page usage for loading EPC pages back out of > > > the shared memory do not incur a charge. > > > > > > When a backing page is released from usage, increment the available > > > backing page counter. > > > > > > When swapping EPC pages to RAM, in addition to storing the page > > > contents, SGX must store some additional metadata to protect > > > against a malicious kernel when the page is swapped back in. This > > > additional metadata is called Paging Crypto MetaData. PCMD is > > > allocated from the same shared memory area as the backing page > > > contents and consumes RAM the same way. > > > > > > PCMD is 128 bytes in size, and there is one PCMD structure per > > > page written to shared RAM. The page index for the PCMD page is > > > calculated from the page index of the backing page, so it is > > > possible > > > that the PCMD structures are not packed into the minimum number of > > > pages possible. If 32 PCMDs can fit onto a single page, then PCMD > > > usage is 1/32 of total EPC pages. In the worst case, PCMD can > > > consume the same amount of RAM as EPC backing pages (1:1). For > > > simplicity, this implementation does not account for PCMD page > > > usage. > > > > > > Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com> > > > --- > > > arch/x86/kernel/cpu/sgx/encl.c | 76 > > > ++++++++++++++++++++++++++++++++-- > > > arch/x86/kernel/cpu/sgx/encl.h | 6 ++- > > > arch/x86/kernel/cpu/sgx/main.c | 6 +-- > > > 3 files changed, 80 insertions(+), 8 deletions(-) > > > > > > diff --git a/arch/x86/kernel/cpu/sgx/encl.c > > > b/arch/x86/kernel/cpu/sgx/encl.c > > > index 001808e3901c..8be6f0592bdc 100644 > > > --- a/arch/x86/kernel/cpu/sgx/encl.c > > > +++ b/arch/x86/kernel/cpu/sgx/encl.c > > > @@ -32,7 +32,7 @@ static int __sgx_encl_eldu(struct sgx_encl_page > > > *encl_page, > > > else > > > page_index = PFN_DOWN(encl->size); > > > > > > - ret = sgx_encl_get_backing(encl, page_index, &b); > > > + ret = sgx_encl_lookup_backing(encl, page_index, &b); > > > if (ret) > > > return ret; > > > > > > @@ -407,6 +407,12 @@ void sgx_encl_release(struct kref *ref) > > > sgx_encl_free_epc_page(entry->epc_page); > > > encl->secs_child_cnt--; > > > entry->epc_page = NULL; > > > + } else { > > > + /* > > > + * If there is no epc_page, it means it has > > > been > > > + * swapped out. Uncharge the backing storage. > > > + */ > > > + sgx_uncharge_mem(); > > > } > > > > > > kfree(entry); > > > @@ -574,8 +580,8 @@ static struct page > > > *sgx_encl_get_backing_page(struct sgx_encl *encl, > > > * 0 on success, > > > * -errno otherwise. > > > */ > > > -int sgx_encl_get_backing(struct sgx_encl *encl, unsigned long > > > page_index, > > > - struct sgx_backing *backing) > > > +static int sgx_encl_get_backing(struct sgx_encl *encl, unsigned > > > long page_index, > > > + struct sgx_backing *backing) > > > { > > > pgoff_t pcmd_index = PFN_DOWN(encl->size) + 1 + (page_index >> > > > 5); > > > struct page *contents; > > > @@ -601,6 +607,62 @@ int sgx_encl_get_backing(struct sgx_encl > > > *encl, unsigned long page_index, > > > return 0; > > > } > > > > > > +/** > > > + * sgx_encl_alloc_backing() - allocate a new backing storage page > > > + * @encl: an enclave pointer > > > + * @page_index: enclave page index > > > + * @backing: data for accessing backing storage for the page > > > + * > > > + * Confirm that the global overcommit limit has not been reached > > > before > > > + * requesting a new backing storage page for storing the encrypted > > > contents > > > + * and Paging Crypto MetaData (PCMD) of an enclave page. This is > > > called when > > > + * there is no existing backing page, just before writing to the > > > backing > > > + * storage with EWB. > > > + * > > > + * Return: > > > + * 0 on success, > > > + * -errno otherwise. > > > + */ > > > +int sgx_encl_alloc_backing(struct sgx_encl *encl, unsigned long > > > page_index, > > > + struct sgx_backing *backing) > > > +{ > > > + int ret; > > > + > > > + if (sgx_charge_mem()) > > > + return -ENOMEM; > > > + > > > + ret = sgx_encl_get_backing(encl, page_index, backing); > > > + if (ret) > > > + sgx_uncharge_mem(); > > > + > > > + return ret; > > > +} > > > + > > > +/** > > > + * sgx_encl_lookup_backing() - retrieve an existing backing > > > storage page > > > + * @encl: an enclave pointer > > > + * @page_index: enclave page index > > > + * @backing: data for accessing backing storage for the page > > > + * > > > + * Retrieve a backing page for loading data back into an EPC page > > > with ELDU. > > > + * This call does not cause a charge to the overcommit limit > > > because a page > > > + * has already been allocated, but has been swapped out or is in > > > RAM > > > + * > > > + * It is the caller's responsibility to ensure that it is > > > appropriate to > > > + * use sgx_encl_lookup_backing() rather than > > > sgx_encl_alloc_backing(). If > > > + * lookup is not used correctly, this will cause an allocation > > > that is > > > + * not accounted for. > > > + * > > > + * Return: > > > + * 0 on success, > > > + * -errno otherwise. > > > + */ > > > +int sgx_encl_lookup_backing(struct sgx_encl *encl, unsigned long > > > page_index, > > > + struct sgx_backing *backing) > > > +{ > > > + return sgx_encl_get_backing(encl, page_index, backing); > > > +} > > > > IMHO, sgx_encl_backing() should be open-coded here. > > I can understand your hesitation, but I agree with Dave here that > wrapping the function makes the code more clear. I would prefer to keep > this the way it is. What if sgx_encl_get_backing() was changed as "static inline", if the only motivation is encapsulation? /Jarkko
On 1/14/22 3:52 PM, Jarkko Sakkinen wrote: >> I can understand your hesitation, but I agree with Dave here that >> wrapping the function makes the code more clear. I would prefer to keep >> this the way it is. > What if sgx_encl_get_backing() was changed as "static inline", if the > only motivation is encapsulation? What would the purpose be of adding an 'inline' to the function definition?
On Fri, Jan 14, 2022 at 09:55:51AM -0800, Dave Hansen wrote: > On 1/14/22 9:51 AM, Kristen Carlson Accardi wrote: > >>> +int sgx_encl_lookup_backing(struct sgx_encl *encl, unsigned long > >>> page_index, > >>> + struct sgx_backing *backing) > >>> +{ > >>> + return sgx_encl_get_backing(encl, page_index, backing); > >>> +} > >> IMHO, sgx_encl_backing() should be open-coded here. > > I can understand your hesitation, but I agree with Dave here that > > wrapping the function makes the code more clear. I would prefer to keep > > this the way it is. > > I'd also like to see sgx_encl_lookup_backing() and > sgx_encl_alloc_backing() diverge more in the future. > > For instance, sgx_encl_alloc_backing() could ensure that the page does > not exist in the file before doing the sgx_encl_get_backing() call. > This would ensure that it truly *does* allocate a page and does not just > return a previously-allocated page. > > sgx_encl_lookup_backing() could ensure the opposite: that the page > *DOES* exist in the file before doing the sgx_encl_get_backing() call. > This would ensure that it does not allocate a page in a case where we > expected an old, existing page to be present. Would it be a too big tretch to add these and make the whole scheme fully legit? Does not sound like an extremely huge stretch and there is now a full cycle amount of time make it happen before 5.18 merge window. /Jarkko
On 1/14/22 3:57 PM, Jarkko Sakkinen wrote: > Would it be a too big tretch to add these and make the whole scheme > fully legit? Does not sound like an extremely huge stretch and there is now > a full cycle amount of time make it happen before 5.18 merge window. No, it's not a big stretch. If anyone wants it for 5.18 (or whatever) just send the patch.
On Fri, Jan 14, 2022 at 04:16:41PM -0800, Dave Hansen wrote: > On 1/14/22 3:57 PM, Jarkko Sakkinen wrote: > > Would it be a too big tretch to add these and make the whole scheme > > fully legit? Does not sound like an extremely huge stretch and there is now > > a full cycle amount of time make it happen before 5.18 merge window. > > No, it's not a big stretch. If anyone wants it for 5.18 (or whatever) > just send the patch. Fixing this to work with the current mainline is first and foremost needed: https://lore.kernel.org/linux-sgx/YeHqW8AaQ3HZZoQx@iki.fi/T/#t With a fixeed version of that, wouldn't it be then just matter of calling shmem_getpage() with SGX_NOALLOC to check the state of the file in both calls? /Jarkko
On Fri, Jan 14, 2022 at 03:55:43PM -0800, Dave Hansen wrote: > On 1/14/22 3:52 PM, Jarkko Sakkinen wrote: > >> I can understand your hesitation, but I agree with Dave here that > >> wrapping the function makes the code more clear. I would prefer to keep > >> this the way it is. > > What if sgx_encl_get_backing() was changed as "static inline", if the > > only motivation is encapsulation? > > What would the purpose be of adding an 'inline' to the function definition? Agreed, not much sense to do this. I just had troubles to get the way things were encapsulated before your response. /Jarkko
diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c index 001808e3901c..8be6f0592bdc 100644 --- a/arch/x86/kernel/cpu/sgx/encl.c +++ b/arch/x86/kernel/cpu/sgx/encl.c @@ -32,7 +32,7 @@ static int __sgx_encl_eldu(struct sgx_encl_page *encl_page, else page_index = PFN_DOWN(encl->size); - ret = sgx_encl_get_backing(encl, page_index, &b); + ret = sgx_encl_lookup_backing(encl, page_index, &b); if (ret) return ret; @@ -407,6 +407,12 @@ void sgx_encl_release(struct kref *ref) sgx_encl_free_epc_page(entry->epc_page); encl->secs_child_cnt--; entry->epc_page = NULL; + } else { + /* + * If there is no epc_page, it means it has been + * swapped out. Uncharge the backing storage. + */ + sgx_uncharge_mem(); } kfree(entry); @@ -574,8 +580,8 @@ static struct page *sgx_encl_get_backing_page(struct sgx_encl *encl, * 0 on success, * -errno otherwise. */ -int sgx_encl_get_backing(struct sgx_encl *encl, unsigned long page_index, - struct sgx_backing *backing) +static int sgx_encl_get_backing(struct sgx_encl *encl, unsigned long page_index, + struct sgx_backing *backing) { pgoff_t pcmd_index = PFN_DOWN(encl->size) + 1 + (page_index >> 5); struct page *contents; @@ -601,6 +607,62 @@ int sgx_encl_get_backing(struct sgx_encl *encl, unsigned long page_index, return 0; } +/** + * sgx_encl_alloc_backing() - allocate a new backing storage page + * @encl: an enclave pointer + * @page_index: enclave page index + * @backing: data for accessing backing storage for the page + * + * Confirm that the global overcommit limit has not been reached before + * requesting a new backing storage page for storing the encrypted contents + * and Paging Crypto MetaData (PCMD) of an enclave page. This is called when + * there is no existing backing page, just before writing to the backing + * storage with EWB. + * + * Return: + * 0 on success, + * -errno otherwise. + */ +int sgx_encl_alloc_backing(struct sgx_encl *encl, unsigned long page_index, + struct sgx_backing *backing) +{ + int ret; + + if (sgx_charge_mem()) + return -ENOMEM; + + ret = sgx_encl_get_backing(encl, page_index, backing); + if (ret) + sgx_uncharge_mem(); + + return ret; +} + +/** + * sgx_encl_lookup_backing() - retrieve an existing backing storage page + * @encl: an enclave pointer + * @page_index: enclave page index + * @backing: data for accessing backing storage for the page + * + * Retrieve a backing page for loading data back into an EPC page with ELDU. + * This call does not cause a charge to the overcommit limit because a page + * has already been allocated, but has been swapped out or is in RAM + * + * It is the caller's responsibility to ensure that it is appropriate to + * use sgx_encl_lookup_backing() rather than sgx_encl_alloc_backing(). If + * lookup is not used correctly, this will cause an allocation that is + * not accounted for. + * + * Return: + * 0 on success, + * -errno otherwise. + */ +int sgx_encl_lookup_backing(struct sgx_encl *encl, unsigned long page_index, + struct sgx_backing *backing) +{ + return sgx_encl_get_backing(encl, page_index, backing); +} + /** * sgx_encl_put_backing() - Unpin the backing storage * @backing: data for accessing backing storage for the page @@ -608,9 +670,17 @@ int sgx_encl_get_backing(struct sgx_encl *encl, unsigned long page_index, */ void sgx_encl_put_backing(struct sgx_backing *backing, bool do_write) { + /* + * If the page is being written to by the reclaimer, it is + * still in use and the backing page usage should not be + * uncharged. However, if the page is not being written to, + * it is no longer in use and may be uncharged. + */ if (do_write) { set_page_dirty(backing->pcmd); set_page_dirty(backing->contents); + } else { + sgx_uncharge_mem(); } put_page(backing->pcmd); diff --git a/arch/x86/kernel/cpu/sgx/encl.h b/arch/x86/kernel/cpu/sgx/encl.h index fec43ca65065..8ffb8a83263f 100644 --- a/arch/x86/kernel/cpu/sgx/encl.h +++ b/arch/x86/kernel/cpu/sgx/encl.h @@ -105,8 +105,10 @@ int sgx_encl_may_map(struct sgx_encl *encl, unsigned long start, void sgx_encl_release(struct kref *ref); int sgx_encl_mm_add(struct sgx_encl *encl, struct mm_struct *mm); -int sgx_encl_get_backing(struct sgx_encl *encl, unsigned long page_index, - struct sgx_backing *backing); +int sgx_encl_alloc_backing(struct sgx_encl *encl, unsigned long page_index, + struct sgx_backing *backing); +int sgx_encl_lookup_backing(struct sgx_encl *encl, unsigned long page_index, + struct sgx_backing *backing); void sgx_encl_put_backing(struct sgx_backing *backing, bool do_write); int sgx_encl_test_and_clear_young(struct mm_struct *mm, struct sgx_encl_page *page); diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c index 8a7bfe0d9023..20ccd9c5b184 100644 --- a/arch/x86/kernel/cpu/sgx/main.c +++ b/arch/x86/kernel/cpu/sgx/main.c @@ -347,8 +347,8 @@ static void sgx_reclaimer_write(struct sgx_epc_page *epc_page, encl->secs_child_cnt--; if (!encl->secs_child_cnt && test_bit(SGX_ENCL_INITIALIZED, &encl->flags)) { - ret = sgx_encl_get_backing(encl, PFN_DOWN(encl->size), - &secs_backing); + ret = sgx_encl_alloc_backing(encl, PFN_DOWN(encl->size), + &secs_backing); if (ret) goto out; @@ -418,7 +418,7 @@ static void sgx_reclaim_pages(void) goto skip; page_index = PFN_DOWN(encl_page->desc - encl_page->encl->base); - ret = sgx_encl_get_backing(encl_page->encl, page_index, &backing[i]); + ret = sgx_encl_alloc_backing(encl_page->encl, page_index, &backing[i]); if (ret) goto skip;
SGX may allow EPC pages to be overcommitted. If the system is out of enclave memory, EPC pages are swapped to normal RAM via a per enclave shared memory area. This shared memory is not charged to the enclave or the task mapping it, making it hard to account for using normal methods. In order to avoid unlimited usage of normal RAM, enclaves must be charged for each new page used for backing storage, and uncharged when they are no longer using a backing page. Modify the existing flow for requesting backing pages to reduce the available backing page counter and confirm that the limit has not been exceeded. Backing page usage for loading EPC pages back out of the shared memory do not incur a charge. When a backing page is released from usage, increment the available backing page counter. When swapping EPC pages to RAM, in addition to storing the page contents, SGX must store some additional metadata to protect against a malicious kernel when the page is swapped back in. This additional metadata is called Paging Crypto MetaData. PCMD is allocated from the same shared memory area as the backing page contents and consumes RAM the same way. PCMD is 128 bytes in size, and there is one PCMD structure per page written to shared RAM. The page index for the PCMD page is calculated from the page index of the backing page, so it is possible that the PCMD structures are not packed into the minimum number of pages possible. If 32 PCMDs can fit onto a single page, then PCMD usage is 1/32 of total EPC pages. In the worst case, PCMD can consume the same amount of RAM as EPC backing pages (1:1). For simplicity, this implementation does not account for PCMD page usage. Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com> --- arch/x86/kernel/cpu/sgx/encl.c | 76 ++++++++++++++++++++++++++++++++-- arch/x86/kernel/cpu/sgx/encl.h | 6 ++- arch/x86/kernel/cpu/sgx/main.c | 6 +-- 3 files changed, 80 insertions(+), 8 deletions(-)