| Message ID | 20220304093524.397485-14-jarkko@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC,v2.1,01/30] x86/sgx: Add short descriptions to ENCLS wrappers | expand |
On Fri, Mar 04, 2022 at 11:35:08AM +0200, Jarkko Sakkinen wrote: > +#define SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS \ > + _IOWR(SGX_MAGIC, 0x05, struct sgx_enclave_restrict_perm) What if this was replaced with just SGX_IOC_ENCLAVE_RESET_PAGES, which would simply do EMODPR with PROT_NONE? The main ingredient of EMODPR is to flush out the TLB's, and move a page to pending state, which cannot be done from inside the enclave. It's there because of microarchitecture constraints, and less so to work as a reasonable permission control mechanism (actually it does terrible job on that side and only confuses). Once you have this magic TLB reset button in place you can just do one EACCEPT and EMODPE inside the enclave and you're done. This is also kind of atomic in the sense that EACCEPT free's a page with no rights so no misuse can happend before EMODPE has tuned EPCM. BR, Jarkko
On Wed, Mar 09, 2022 at 10:52:22AM +0200, Jarkko Sakkinen wrote: > On Fri, Mar 04, 2022 at 11:35:08AM +0200, Jarkko Sakkinen wrote: > > +#define SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS \ > > + _IOWR(SGX_MAGIC, 0x05, struct sgx_enclave_restrict_perm) > > What if this was replaced with just SGX_IOC_ENCLAVE_RESET_PAGES, which > would simply do EMODPR with PROT_NONE? The main ingredient of EMODPR is to > flush out the TLB's, and move a page to pending state, which cannot be done > from inside the enclave. > > It's there because of microarchitecture constraints, and less so to work as > a reasonable permission control mechanism (actually it does terrible job on > that side and only confuses). > > Once you have this magic TLB reset button in place you can just do one > EACCEPT and EMODPE inside the enclave and you're done. > > This is also kind of atomic in the sense that EACCEPT free's a page with no > rights so no misuse can happend before EMODPE has tuned EPCM. I wonder if this type of pattern could be made work out for Graphene: 1. SGX_IOC_ENCLAVE_RESET_PAGES 2. EACCEPT + EMODPE This kind of delivers EMODP that everyone has been looking for. BR, Jarkko
Hi Jarkko, On 3/9/2022 1:35 AM, Jarkko Sakkinen wrote: > On Wed, Mar 09, 2022 at 10:52:22AM +0200, Jarkko Sakkinen wrote: >> On Fri, Mar 04, 2022 at 11:35:08AM +0200, Jarkko Sakkinen wrote: >>> +#define SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS \ >>> + _IOWR(SGX_MAGIC, 0x05, struct sgx_enclave_restrict_perm) >> >> What if this was replaced with just SGX_IOC_ENCLAVE_RESET_PAGES, which >> would simply do EMODPR with PROT_NONE? The main ingredient of EMODPR is to >> flush out the TLB's, and move a page to pending state, which cannot be done >> from inside the enclave. I see the main ingredient as running EMODPR to restrict the EPCM permissions. If the user wants to use SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS just to flush TLB it is already possible since attempting to use EMODPR to relax permissions does not change any permissions (although it still sets EPCM.PR) but yet will still flush the TLB. Even so, you have a very good point that removing SGX_IOC_ENCLAVE_RELAX_PERMISSIONS removes the ability for users to flush the TLB after an EMODPE. If there are thus PTEs present at the time the user runs EMODPE the pages would not be accessible with the new permissions. Repurposing SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS with PROT_NONE to accomplish this is not efficient because: - For the OS to flush the TLB the enclave pages need not be in the EPC but in order to run EMODPR the enclave page needs to be in the EPC. In an oversubscribed environment running EMODPR unnecessarily can thus introduce a significant delay. Please see the performance comparison I did in https://lore.kernel.org/linux-sgx/77e81306-6b03-4b09-2df2-48e09e2e79d5@intel.com/ The test shows that running EMODPR unnecessarily can be orders of magnitude slower. - Running EMODPR on an enclave page sets the EPCM.PR bin in the enclave page that needs to be cleared with an EACCEPT from within the enclave. If the user just wants to reset the TLB after running EMODPE then it should not be necessary to run EACCEPT again to reset EPCM.PR. Resetting the TLB is exactly what SGX_IOC_ENCLAVE_RELAX_PERMISSIONS did in an efficient way - it is quick (no need to load pages into EPC) and it does not require EACCEPT to clear EPCM.PR. It looks like we need SGX_IOC_ENCLAVE_RELAX_PERMISSIONS back. We could rename it to SGX_IOC_ENCLAVE_RESET_PAGES if you prefer. >> It's there because of microarchitecture constraints, and less so to work as >> a reasonable permission control mechanism (actually it does terrible job on >> that side and only confuses). >> >> Once you have this magic TLB reset button in place you can just do one >> EACCEPT and EMODPE inside the enclave and you're done. >> >> This is also kind of atomic in the sense that EACCEPT free's a page with no >> rights so no misuse can happend before EMODPE has tuned EPCM. > > I wonder if this type of pattern could be made work out for Graphene: > > 1. SGX_IOC_ENCLAVE_RESET_PAGES > 2. EACCEPT + EMODPE > > This kind of delivers EMODP that everyone has been looking for. EACCEPT will result in page table entries created for the enclave page. EMODPE will be able to relax the permissions but TLB flush would be required to access the page with the new permissions. SGX_IOC_ENCLAVE_RELAX_PERMISSIONS (renamed to SGX_IOC_ENCLAVE_RESET_PAGES?) that does just a TLB flush is required to be after EMODPE. Reinette
Hi Jarkko, Apologies, after thinking about this more I believe that I misunderstood your proposal in my original answer. On 3/9/2022 8:59 AM, Reinette Chatre wrote: > Hi Jarkko, > > On 3/9/2022 1:35 AM, Jarkko Sakkinen wrote: >> On Wed, Mar 09, 2022 at 10:52:22AM +0200, Jarkko Sakkinen wrote: >>> On Fri, Mar 04, 2022 at 11:35:08AM +0200, Jarkko Sakkinen wrote: >>>> +#define SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS \ >>>> + _IOWR(SGX_MAGIC, 0x05, struct sgx_enclave_restrict_perm) >>> >>> What if this was replaced with just SGX_IOC_ENCLAVE_RESET_PAGES, which >>> would simply do EMODPR with PROT_NONE? The main ingredient of EMODPR is to >>> flush out the TLB's, and move a page to pending state, which cannot be done >>> from inside the enclave. Why not keep the flexibility of supporting all permission restrictions? It is already possible to call SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS with PROT_NONE, no? > I see the main ingredient as running EMODPR to restrict the EPCM permissions. If > the user wants to use SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS just to flush TLB it is > already possible since attempting to use EMODPR to relax permissions does not > change any permissions (although it still sets EPCM.PR) but yet will still > flush the TLB. > > Even so, you have a very good point that removing SGX_IOC_ENCLAVE_RELAX_PERMISSIONS > removes the ability for users to flush the TLB after an EMODPE. If there are > thus PTEs present at the time the user runs EMODPE the pages would not be > accessible with the new permissions. > > Repurposing SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS with PROT_NONE to accomplish > this is not efficient because: > - For the OS to flush the TLB the enclave pages need not be in the EPC but > in order to run EMODPR the enclave page needs to be in the EPC. In an > oversubscribed environment running EMODPR unnecessarily can thus introduce > a significant delay. Please see the performance comparison I did in > https://lore.kernel.org/linux-sgx/77e81306-6b03-4b09-2df2-48e09e2e79d5@intel.com/ > The test shows that running EMODPR unnecessarily can be orders of magnitude slower. > - Running EMODPR on an enclave page sets the EPCM.PR bin in the enclave page > that needs to be cleared with an EACCEPT from within the enclave. > If the user just wants to reset the TLB after running EMODPE then it should > not be necessary to run EACCEPT again to reset EPCM.PR. > > Resetting the TLB is exactly what SGX_IOC_ENCLAVE_RELAX_PERMISSIONS did in an > efficient way - it is quick (no need to load pages into EPC) and it does not > require EACCEPT to clear EPCM.PR. > > It looks like we need SGX_IOC_ENCLAVE_RELAX_PERMISSIONS back. We could > rename it to SGX_IOC_ENCLAVE_RESET_PAGES if you prefer. > >>> It's there because of microarchitecture constraints, and less so to work as >>> a reasonable permission control mechanism (actually it does terrible job on >>> that side and only confuses). >>> >>> Once you have this magic TLB reset button in place you can just do one >>> EACCEPT and EMODPE inside the enclave and you're done. >>> >>> This is also kind of atomic in the sense that EACCEPT free's a page with no >>> rights so no misuse can happend before EMODPE has tuned EPCM. >> >> I wonder if this type of pattern could be made work out for Graphene: >> >> 1. SGX_IOC_ENCLAVE_RESET_PAGES >> 2. EACCEPT + EMODPE >> >> This kind of delivers EMODP that everyone has been looking for. > As I understand it this is currently possible with SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS. Reinette
On Wed, Mar 09, 2022 at 08:59:42AM -0800, Reinette Chatre wrote: > Hi Jarkko, > > On 3/9/2022 1:35 AM, Jarkko Sakkinen wrote: > > On Wed, Mar 09, 2022 at 10:52:22AM +0200, Jarkko Sakkinen wrote: > >> On Fri, Mar 04, 2022 at 11:35:08AM +0200, Jarkko Sakkinen wrote: > >>> +#define SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS \ > >>> + _IOWR(SGX_MAGIC, 0x05, struct sgx_enclave_restrict_perm) > >> > >> What if this was replaced with just SGX_IOC_ENCLAVE_RESET_PAGES, which > >> would simply do EMODPR with PROT_NONE? The main ingredient of EMODPR is to > >> flush out the TLB's, and move a page to pending state, which cannot be done > >> from inside the enclave. > > I see the main ingredient as running EMODPR to restrict the EPCM permissions. If > the user wants to use SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS just to flush TLB it is > already possible since attempting to use EMODPR to relax permissions does not > change any permissions (although it still sets EPCM.PR) but yet will still > flush the TLB. It's not just to flush the TLB. It also resets permissions to zero from which it is easy to set the exact permissions with EMODPE. > Even so, you have a very good point that removing SGX_IOC_ENCLAVE_RELAX_PERMISSIONS > removes the ability for users to flush the TLB after an EMODPE. If there are > thus PTEs present at the time the user runs EMODPE the pages would not be > accessible with the new permissions. > > Repurposing SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS with PROT_NONE to accomplish > this is not efficient because: > - For the OS to flush the TLB the enclave pages need not be in the EPC but > in order to run EMODPR the enclave page needs to be in the EPC. In an > oversubscribed environment running EMODPR unnecessarily can thus introduce > a significant delay. Please see the performance comparison I did in > https://lore.kernel.org/linux-sgx/77e81306-6b03-4b09-2df2-48e09e2e79d5@intel.com/ > The test shows that running EMODPR unnecessarily can be orders of magnitude slower. > - Running EMODPR on an enclave page sets the EPCM.PR bin in the enclave page > that needs to be cleared with an EACCEPT from within the enclave. > If the user just wants to reset the TLB after running EMODPE then it should > not be necessary to run EACCEPT again to reset EPCM.PR. > > Resetting the TLB is exactly what SGX_IOC_ENCLAVE_RELAX_PERMISSIONS did in an > efficient way - it is quick (no need to load pages into EPC) and it does not > require EACCEPT to clear EPCM.PR. > > It looks like we need SGX_IOC_ENCLAVE_RELAX_PERMISSIONS back. We could > rename it to SGX_IOC_ENCLAVE_RESET_PAGES if you prefer. Please do not add it. We do not have any use for it. It's not only used to flush TLB's so it would not do any good. I just use it with fixed PROT_NONE permissions. > >> It's there because of microarchitecture constraints, and less so to work as > >> a reasonable permission control mechanism (actually it does terrible job on > >> that side and only confuses). > >> > >> Once you have this magic TLB reset button in place you can just do one > >> EACCEPT and EMODPE inside the enclave and you're done. > >> > >> This is also kind of atomic in the sense that EACCEPT free's a page with no > >> rights so no misuse can happend before EMODPE has tuned EPCM. > > > > I wonder if this type of pattern could be made work out for Graphene: > > > > 1. SGX_IOC_ENCLAVE_RESET_PAGES > > 2. EACCEPT + EMODPE > > > > This kind of delivers EMODP that everyone has been looking for. > > EACCEPT will result in page table entries created for the enclave page. EMODPE > will be able to relax the permissions but TLB flush would be required to > access the page with the new permissions. SGX_IOC_ENCLAVE_RELAX_PERMISSIONS > (renamed to SGX_IOC_ENCLAVE_RESET_PAGES?) that does just a TLB flush is > required to be after EMODPE. For EMODPE TLB flush is not required. I even verified this from Mark Shanahan. And since access rights are zero, the page cannot be deferenced by threads before EMODPE. BR, Jarkko
On Thu, Mar 10, 2022 at 01:35:53AM +0200, Jarkko Sakkinen wrote: > On Wed, Mar 09, 2022 at 08:59:42AM -0800, Reinette Chatre wrote: > > Hi Jarkko, > > > > On 3/9/2022 1:35 AM, Jarkko Sakkinen wrote: > > > On Wed, Mar 09, 2022 at 10:52:22AM +0200, Jarkko Sakkinen wrote: > > >> On Fri, Mar 04, 2022 at 11:35:08AM +0200, Jarkko Sakkinen wrote: > > >>> +#define SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS \ > > >>> + _IOWR(SGX_MAGIC, 0x05, struct sgx_enclave_restrict_perm) > > >> > > >> What if this was replaced with just SGX_IOC_ENCLAVE_RESET_PAGES, which > > >> would simply do EMODPR with PROT_NONE? The main ingredient of EMODPR is to > > >> flush out the TLB's, and move a page to pending state, which cannot be done > > >> from inside the enclave. > > > > I see the main ingredient as running EMODPR to restrict the EPCM permissions. If > > the user wants to use SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS just to flush TLB it is > > already possible since attempting to use EMODPR to relax permissions does not > > change any permissions (although it still sets EPCM.PR) but yet will still > > flush the TLB. > > It's not just to flush the TLB. It also resets permissions to zero from > which it is easy to set the exact permissions with EMODPE. > > > Even so, you have a very good point that removing SGX_IOC_ENCLAVE_RELAX_PERMISSIONS > > removes the ability for users to flush the TLB after an EMODPE. If there are > > thus PTEs present at the time the user runs EMODPE the pages would not be > > accessible with the new permissions. > > > > Repurposing SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS with PROT_NONE to accomplish > > this is not efficient because: > > - For the OS to flush the TLB the enclave pages need not be in the EPC but > > in order to run EMODPR the enclave page needs to be in the EPC. In an > > oversubscribed environment running EMODPR unnecessarily can thus introduce > > a significant delay. Please see the performance comparison I did in > > https://lore.kernel.org/linux-sgx/77e81306-6b03-4b09-2df2-48e09e2e79d5@intel.com/ > > The test shows that running EMODPR unnecessarily can be orders of magnitude slower. > > - Running EMODPR on an enclave page sets the EPCM.PR bin in the enclave page > > that needs to be cleared with an EACCEPT from within the enclave. > > If the user just wants to reset the TLB after running EMODPE then it should > > not be necessary to run EACCEPT again to reset EPCM.PR. > > > > Resetting the TLB is exactly what SGX_IOC_ENCLAVE_RELAX_PERMISSIONS did in an > > efficient way - it is quick (no need to load pages into EPC) and it does not > > require EACCEPT to clear EPCM.PR. > > > > It looks like we need SGX_IOC_ENCLAVE_RELAX_PERMISSIONS back. We could > > rename it to SGX_IOC_ENCLAVE_RESET_PAGES if you prefer. > > Please do not add it. We do not have any use for it. It's not only used > to flush TLB's so it would not do any good. I just use it with fixed > PROT_NONE permissions. > > > >> It's there because of microarchitecture constraints, and less so to work as > > >> a reasonable permission control mechanism (actually it does terrible job on > > >> that side and only confuses). > > >> > > >> Once you have this magic TLB reset button in place you can just do one > > >> EACCEPT and EMODPE inside the enclave and you're done. > > >> > > >> This is also kind of atomic in the sense that EACCEPT free's a page with no > > >> rights so no misuse can happend before EMODPE has tuned EPCM. > > > > > > I wonder if this type of pattern could be made work out for Graphene: > > > > > > 1. SGX_IOC_ENCLAVE_RESET_PAGES > > > 2. EACCEPT + EMODPE > > > > > > This kind of delivers EMODP that everyone has been looking for. > > > > EACCEPT will result in page table entries created for the enclave page. EMODPE > > will be able to relax the permissions but TLB flush would be required to > > access the page with the new permissions. SGX_IOC_ENCLAVE_RELAX_PERMISSIONS > > (renamed to SGX_IOC_ENCLAVE_RESET_PAGES?) that does just a TLB flush is > > required to be after EMODPE. > > For EMODPE TLB flush is not required. I even verified this from Mark > Shanahan. And since access rights are zero, the page cannot be > deferenced by threads before EMODPE. I'm fine of course keeping SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS as it is too. It's at least future-proof that way. BR, Jarkko
Hi Jarkko, On 3/9/2022 3:35 PM, Jarkko Sakkinen wrote: > On Wed, Mar 09, 2022 at 08:59:42AM -0800, Reinette Chatre wrote: >> Hi Jarkko, >> >> On 3/9/2022 1:35 AM, Jarkko Sakkinen wrote: >>> On Wed, Mar 09, 2022 at 10:52:22AM +0200, Jarkko Sakkinen wrote: >>>> On Fri, Mar 04, 2022 at 11:35:08AM +0200, Jarkko Sakkinen wrote: >>>>> +#define SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS \ >>>>> + _IOWR(SGX_MAGIC, 0x05, struct sgx_enclave_restrict_perm) >>>> >>>> What if this was replaced with just SGX_IOC_ENCLAVE_RESET_PAGES, which >>>> would simply do EMODPR with PROT_NONE? The main ingredient of EMODPR is to >>>> flush out the TLB's, and move a page to pending state, which cannot be done >>>> from inside the enclave. >> >> I see the main ingredient as running EMODPR to restrict the EPCM permissions. If >> the user wants to use SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS just to flush TLB it is >> already possible since attempting to use EMODPR to relax permissions does not >> change any permissions (although it still sets EPCM.PR) but yet will still >> flush the TLB. > > It's not just to flush the TLB. It also resets permissions to zero from > which it is easy to set the exact permissions with EMODPE. > >> Even so, you have a very good point that removing SGX_IOC_ENCLAVE_RELAX_PERMISSIONS >> removes the ability for users to flush the TLB after an EMODPE. If there are >> thus PTEs present at the time the user runs EMODPE the pages would not be >> accessible with the new permissions. >> >> Repurposing SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS with PROT_NONE to accomplish >> this is not efficient because: >> - For the OS to flush the TLB the enclave pages need not be in the EPC but >> in order to run EMODPR the enclave page needs to be in the EPC. In an >> oversubscribed environment running EMODPR unnecessarily can thus introduce >> a significant delay. Please see the performance comparison I did in >> https://lore.kernel.org/linux-sgx/77e81306-6b03-4b09-2df2-48e09e2e79d5@intel.com/ >> The test shows that running EMODPR unnecessarily can be orders of magnitude slower. >> - Running EMODPR on an enclave page sets the EPCM.PR bin in the enclave page >> that needs to be cleared with an EACCEPT from within the enclave. >> If the user just wants to reset the TLB after running EMODPE then it should >> not be necessary to run EACCEPT again to reset EPCM.PR. >> >> Resetting the TLB is exactly what SGX_IOC_ENCLAVE_RELAX_PERMISSIONS did in an >> efficient way - it is quick (no need to load pages into EPC) and it does not >> require EACCEPT to clear EPCM.PR. >> >> It looks like we need SGX_IOC_ENCLAVE_RELAX_PERMISSIONS back. We could >> rename it to SGX_IOC_ENCLAVE_RESET_PAGES if you prefer. > > Please do not add it. We do not have any use for it. It's not only used > to flush TLB's so it would not do any good. I just use it with fixed > PROT_NONE permissions. > >>>> It's there because of microarchitecture constraints, and less so to work as >>>> a reasonable permission control mechanism (actually it does terrible job on >>>> that side and only confuses). >>>> >>>> Once you have this magic TLB reset button in place you can just do one >>>> EACCEPT and EMODPE inside the enclave and you're done. >>>> >>>> This is also kind of atomic in the sense that EACCEPT free's a page with no >>>> rights so no misuse can happend before EMODPE has tuned EPCM. >>> >>> I wonder if this type of pattern could be made work out for Graphene: >>> >>> 1. SGX_IOC_ENCLAVE_RESET_PAGES >>> 2. EACCEPT + EMODPE >>> >>> This kind of delivers EMODP that everyone has been looking for. >> >> EACCEPT will result in page table entries created for the enclave page. EMODPE >> will be able to relax the permissions but TLB flush would be required to >> access the page with the new permissions. SGX_IOC_ENCLAVE_RELAX_PERMISSIONS >> (renamed to SGX_IOC_ENCLAVE_RESET_PAGES?) that does just a TLB flush is >> required to be after EMODPE. > > For EMODPE TLB flush is not required. I even verified this from Mark > Shanahan. And since access rights are zero, the page cannot be > deferenced by threads before EMODPE. > Understood. I realized my mistake only after sending the email and attempted to correct it in the following. Sorry for the noise. Reinette
Hi Jarkko, On 3/9/2022 3:42 PM, Jarkko Sakkinen wrote: > On Thu, Mar 10, 2022 at 01:35:53AM +0200, Jarkko Sakkinen wrote: >> On Wed, Mar 09, 2022 at 08:59:42AM -0800, Reinette Chatre wrote: >>> Hi Jarkko, >>> >>> On 3/9/2022 1:35 AM, Jarkko Sakkinen wrote: >>>> On Wed, Mar 09, 2022 at 10:52:22AM +0200, Jarkko Sakkinen wrote: >>>>> On Fri, Mar 04, 2022 at 11:35:08AM +0200, Jarkko Sakkinen wrote: >>>>>> +#define SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS \ >>>>>> + _IOWR(SGX_MAGIC, 0x05, struct sgx_enclave_restrict_perm) >>>>> >>>>> What if this was replaced with just SGX_IOC_ENCLAVE_RESET_PAGES, which >>>>> would simply do EMODPR with PROT_NONE? The main ingredient of EMODPR is to >>>>> flush out the TLB's, and move a page to pending state, which cannot be done >>>>> from inside the enclave. >>> >>> I see the main ingredient as running EMODPR to restrict the EPCM permissions. If >>> the user wants to use SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS just to flush TLB it is >>> already possible since attempting to use EMODPR to relax permissions does not >>> change any permissions (although it still sets EPCM.PR) but yet will still >>> flush the TLB. >> >> It's not just to flush the TLB. It also resets permissions to zero from >> which it is easy to set the exact permissions with EMODPE. >> >>> Even so, you have a very good point that removing SGX_IOC_ENCLAVE_RELAX_PERMISSIONS >>> removes the ability for users to flush the TLB after an EMODPE. If there are >>> thus PTEs present at the time the user runs EMODPE the pages would not be >>> accessible with the new permissions. >>> >>> Repurposing SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS with PROT_NONE to accomplish >>> this is not efficient because: >>> - For the OS to flush the TLB the enclave pages need not be in the EPC but >>> in order to run EMODPR the enclave page needs to be in the EPC. In an >>> oversubscribed environment running EMODPR unnecessarily can thus introduce >>> a significant delay. Please see the performance comparison I did in >>> https://lore.kernel.org/linux-sgx/77e81306-6b03-4b09-2df2-48e09e2e79d5@intel.com/ >>> The test shows that running EMODPR unnecessarily can be orders of magnitude slower. >>> - Running EMODPR on an enclave page sets the EPCM.PR bin in the enclave page >>> that needs to be cleared with an EACCEPT from within the enclave. >>> If the user just wants to reset the TLB after running EMODPE then it should >>> not be necessary to run EACCEPT again to reset EPCM.PR. >>> >>> Resetting the TLB is exactly what SGX_IOC_ENCLAVE_RELAX_PERMISSIONS did in an >>> efficient way - it is quick (no need to load pages into EPC) and it does not >>> require EACCEPT to clear EPCM.PR. >>> >>> It looks like we need SGX_IOC_ENCLAVE_RELAX_PERMISSIONS back. We could >>> rename it to SGX_IOC_ENCLAVE_RESET_PAGES if you prefer. >> >> Please do not add it. We do not have any use for it. It's not only used >> to flush TLB's so it would not do any good. I just use it with fixed >> PROT_NONE permissions. >> >>>>> It's there because of microarchitecture constraints, and less so to work as >>>>> a reasonable permission control mechanism (actually it does terrible job on >>>>> that side and only confuses). >>>>> >>>>> Once you have this magic TLB reset button in place you can just do one >>>>> EACCEPT and EMODPE inside the enclave and you're done. >>>>> >>>>> This is also kind of atomic in the sense that EACCEPT free's a page with no >>>>> rights so no misuse can happend before EMODPE has tuned EPCM. >>>> >>>> I wonder if this type of pattern could be made work out for Graphene: >>>> >>>> 1. SGX_IOC_ENCLAVE_RESET_PAGES >>>> 2. EACCEPT + EMODPE >>>> >>>> This kind of delivers EMODP that everyone has been looking for. >>> >>> EACCEPT will result in page table entries created for the enclave page. EMODPE >>> will be able to relax the permissions but TLB flush would be required to >>> access the page with the new permissions. SGX_IOC_ENCLAVE_RELAX_PERMISSIONS >>> (renamed to SGX_IOC_ENCLAVE_RESET_PAGES?) that does just a TLB flush is >>> required to be after EMODPE. >> >> For EMODPE TLB flush is not required. I even verified this from Mark >> Shanahan. And since access rights are zero, the page cannot be >> deferenced by threads before EMODPE. > > I'm fine of course keeping SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS as it > is too. It's at least future-proof that way. Will do. Reinette
On Wed, Mar 09, 2022 at 04:10:27PM -0800, Reinette Chatre wrote: > Hi Jarkko, > > On 3/9/2022 3:35 PM, Jarkko Sakkinen wrote: > > On Wed, Mar 09, 2022 at 08:59:42AM -0800, Reinette Chatre wrote: > >> Hi Jarkko, > >> > >> On 3/9/2022 1:35 AM, Jarkko Sakkinen wrote: > >>> On Wed, Mar 09, 2022 at 10:52:22AM +0200, Jarkko Sakkinen wrote: > >>>> On Fri, Mar 04, 2022 at 11:35:08AM +0200, Jarkko Sakkinen wrote: > >>>>> +#define SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS \ > >>>>> + _IOWR(SGX_MAGIC, 0x05, struct sgx_enclave_restrict_perm) > >>>> > >>>> What if this was replaced with just SGX_IOC_ENCLAVE_RESET_PAGES, which > >>>> would simply do EMODPR with PROT_NONE? The main ingredient of EMODPR is to > >>>> flush out the TLB's, and move a page to pending state, which cannot be done > >>>> from inside the enclave. > >> > >> I see the main ingredient as running EMODPR to restrict the EPCM permissions. If > >> the user wants to use SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS just to flush TLB it is > >> already possible since attempting to use EMODPR to relax permissions does not > >> change any permissions (although it still sets EPCM.PR) but yet will still > >> flush the TLB. > > > > It's not just to flush the TLB. It also resets permissions to zero from > > which it is easy to set the exact permissions with EMODPE. > > > >> Even so, you have a very good point that removing SGX_IOC_ENCLAVE_RELAX_PERMISSIONS > >> removes the ability for users to flush the TLB after an EMODPE. If there are > >> thus PTEs present at the time the user runs EMODPE the pages would not be > >> accessible with the new permissions. > >> > >> Repurposing SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS with PROT_NONE to accomplish > >> this is not efficient because: > >> - For the OS to flush the TLB the enclave pages need not be in the EPC but > >> in order to run EMODPR the enclave page needs to be in the EPC. In an > >> oversubscribed environment running EMODPR unnecessarily can thus introduce > >> a significant delay. Please see the performance comparison I did in > >> https://lore.kernel.org/linux-sgx/77e81306-6b03-4b09-2df2-48e09e2e79d5@intel.com/ > >> The test shows that running EMODPR unnecessarily can be orders of magnitude slower. > >> - Running EMODPR on an enclave page sets the EPCM.PR bin in the enclave page > >> that needs to be cleared with an EACCEPT from within the enclave. > >> If the user just wants to reset the TLB after running EMODPE then it should > >> not be necessary to run EACCEPT again to reset EPCM.PR. > >> > >> Resetting the TLB is exactly what SGX_IOC_ENCLAVE_RELAX_PERMISSIONS did in an > >> efficient way - it is quick (no need to load pages into EPC) and it does not > >> require EACCEPT to clear EPCM.PR. > >> > >> It looks like we need SGX_IOC_ENCLAVE_RELAX_PERMISSIONS back. We could > >> rename it to SGX_IOC_ENCLAVE_RESET_PAGES if you prefer. > > > > Please do not add it. We do not have any use for it. It's not only used > > to flush TLB's so it would not do any good. I just use it with fixed > > PROT_NONE permissions. > > > >>>> It's there because of microarchitecture constraints, and less so to work as > >>>> a reasonable permission control mechanism (actually it does terrible job on > >>>> that side and only confuses). > >>>> > >>>> Once you have this magic TLB reset button in place you can just do one > >>>> EACCEPT and EMODPE inside the enclave and you're done. > >>>> > >>>> This is also kind of atomic in the sense that EACCEPT free's a page with no > >>>> rights so no misuse can happend before EMODPE has tuned EPCM. > >>> > >>> I wonder if this type of pattern could be made work out for Graphene: > >>> > >>> 1. SGX_IOC_ENCLAVE_RESET_PAGES > >>> 2. EACCEPT + EMODPE > >>> > >>> This kind of delivers EMODP that everyone has been looking for. > >> > >> EACCEPT will result in page table entries created for the enclave page. EMODPE > >> will be able to relax the permissions but TLB flush would be required to > >> access the page with the new permissions. SGX_IOC_ENCLAVE_RELAX_PERMISSIONS > >> (renamed to SGX_IOC_ENCLAVE_RESET_PAGES?) that does just a TLB flush is > >> required to be after EMODPE. > > > > For EMODPE TLB flush is not required. I even verified this from Mark > > Shanahan. And since access rights are zero, the page cannot be > > deferenced by threads before EMODPE. > > > > Understood. I realized my mistake only after sending the email and attempted > to correct it in the following. Sorry for the noise. Please do not! It's really important this is looked from every angle before it hits the mainline :-) BR, Jarkko
diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h index f4b81587e90b..107ffb0a0b48 100644 --- a/arch/x86/include/uapi/asm/sgx.h +++ b/arch/x86/include/uapi/asm/sgx.h @@ -29,6 +29,8 @@ enum sgx_page_flags { _IOW(SGX_MAGIC, 0x03, struct sgx_enclave_provision) #define SGX_IOC_VEPC_REMOVE_ALL \ _IO(SGX_MAGIC, 0x04) +#define SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS \ + _IOWR(SGX_MAGIC, 0x05, struct sgx_enclave_restrict_perm) /** * struct sgx_enclave_create - parameter structure for the @@ -76,6 +78,25 @@ struct sgx_enclave_provision { __u64 fd; }; +/** + * struct sgx_enclave_restrict_perm - parameters for ioctl + * %SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS + * @offset: starting page offset (page aligned relative to enclave base + * address defined in SECS) + * @length: length of memory (multiple of the page size) + * @secinfo: address for the SECINFO data containing the new permission bits + * for pages in range described by @offset and @length + * @result: (output) SGX result code of ENCLS[EMODPR] function + * @count: (output) bytes successfully changed (multiple of page size) + */ +struct sgx_enclave_restrict_perm { + __u64 offset; + __u64 length; + __u64 secinfo; + __u64 result; + __u64 count; +}; + struct sgx_enclave_run; /** diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c index 6cc732b88c37..baf798a793a2 100644 --- a/arch/x86/kernel/cpu/sgx/encl.c +++ b/arch/x86/kernel/cpu/sgx/encl.c @@ -90,8 +90,8 @@ static struct sgx_epc_page *sgx_encl_eldu(struct sgx_encl_page *encl_page, return epc_page; } -static struct sgx_encl_page *sgx_encl_load_page(struct sgx_encl *encl, - unsigned long addr) +struct sgx_encl_page *sgx_encl_load_page(struct sgx_encl *encl, + unsigned long addr) { struct sgx_epc_page *epc_page; struct sgx_encl_page *entry; diff --git a/arch/x86/kernel/cpu/sgx/encl.h b/arch/x86/kernel/cpu/sgx/encl.h index 94ccea8fbbf2..965cfc7b2b93 100644 --- a/arch/x86/kernel/cpu/sgx/encl.h +++ b/arch/x86/kernel/cpu/sgx/encl.h @@ -119,4 +119,7 @@ void sgx_free_va_slot(struct sgx_va_page *va_page, unsigned int offset); bool sgx_va_page_full(struct sgx_va_page *va_page); void sgx_encl_free_epc_page(struct sgx_epc_page *page); +struct sgx_encl_page *sgx_encl_load_page(struct sgx_encl *encl, + unsigned long addr); + #endif /* _X86_ENCL_H */ diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c index 0c211af8e948..a848f6c38781 100644 --- a/arch/x86/kernel/cpu/sgx/ioctl.c +++ b/arch/x86/kernel/cpu/sgx/ioctl.c @@ -692,6 +692,280 @@ static long sgx_ioc_enclave_provision(struct sgx_encl *encl, void __user *arg) return sgx_set_attribute(&encl->attributes_mask, params.fd); } +static unsigned long vm_prot_from_secinfo(u64 secinfo_perm) +{ + unsigned long vm_prot; + + vm_prot = _calc_vm_trans(secinfo_perm, SGX_SECINFO_R, PROT_READ) | + _calc_vm_trans(secinfo_perm, SGX_SECINFO_W, PROT_WRITE) | + _calc_vm_trans(secinfo_perm, SGX_SECINFO_X, PROT_EXEC); + vm_prot = calc_vm_prot_bits(vm_prot, 0); + + return vm_prot; +} + +/* + * Ensure enclave is ready for SGX2 functions. Readiness is checked + * by ensuring the hardware supports SGX2 and the enclave is initialized + * and thus able to handle requests to modify pages within it. + */ +static int sgx_ioc_sgx2_ready(struct sgx_encl *encl) +{ + if (!(cpu_feature_enabled(X86_FEATURE_SGX2))) + return -ENODEV; + + if (!test_bit(SGX_ENCL_INITIALIZED, &encl->flags)) + return -EINVAL; + + return 0; +} + +/* + * Return valid permission fields from a secinfo structure provided by + * user space. The secinfo structure is required to only have bits in + * the permission fields set. + */ +static int sgx_perm_from_user_secinfo(void __user *_secinfo, u64 *secinfo_perm) +{ + struct sgx_secinfo secinfo; + u64 perm; + + if (copy_from_user(&secinfo, (void __user *)_secinfo, + sizeof(secinfo))) + return -EFAULT; + + if (secinfo.flags & ~SGX_SECINFO_PERMISSION_MASK) + return -EINVAL; + + if (memchr_inv(secinfo.reserved, 0, sizeof(secinfo.reserved))) + return -EINVAL; + + perm = secinfo.flags & SGX_SECINFO_PERMISSION_MASK; + + if ((perm & SGX_SECINFO_W) && !(perm & SGX_SECINFO_R)) + return -EINVAL; + + *secinfo_perm = perm; + + return 0; +} + +/* + * Some SGX functions require that no cached linear-to-physical address + * mappings are present before they can succeed. Collaborate with + * hardware via ENCLS[ETRACK] to ensure that all cached + * linear-to-physical address mappings belonging to all threads of + * the enclave are cleared. See sgx_encl_cpumask() for details. + */ +static int sgx_enclave_etrack(struct sgx_encl *encl) +{ + void *epc_virt; + int ret; + + epc_virt = sgx_get_epc_virt_addr(encl->secs.epc_page); + ret = __etrack(epc_virt); + if (ret) { + /* + * ETRACK only fails when there is an OS issue. For + * example, two consecutive ETRACK was sent without + * completed IPI between. + */ + pr_err_once("ETRACK returned %d (0x%x)", ret, ret); + /* + * Send IPIs to kick CPUs out of the enclave and + * try ETRACK again. + */ + on_each_cpu_mask(sgx_encl_cpumask(encl), sgx_ipi_cb, NULL, 1); + ret = __etrack(epc_virt); + if (ret) { + pr_err_once("ETRACK repeat returned %d (0x%x)", + ret, ret); + return -EFAULT; + } + } + on_each_cpu_mask(sgx_encl_cpumask(encl), sgx_ipi_cb, NULL, 1); + + return 0; +} + +/** + * sgx_enclave_restrict_perm() - Restrict EPCM permissions and align OS view + * @encl: Enclave to which the pages belong. + * @modp: Checked parameters from user on which pages need modifying. + * @secinfo_perm: New (validated) permission bits. + * + * Return: + * - 0: Success. + * - -errno: Otherwise. + */ +static long sgx_enclave_restrict_perm(struct sgx_encl *encl, + struct sgx_enclave_restrict_perm *modp, + u64 secinfo_perm) +{ + struct sgx_encl_page *entry; + struct sgx_secinfo secinfo; + unsigned long vm_prot; + unsigned long addr; + unsigned long c; + void *epc_virt; + int ret; + + memset(&secinfo, 0, sizeof(secinfo)); + secinfo.flags = secinfo_perm; + + vm_prot = vm_prot_from_secinfo(secinfo_perm); + + for (c = 0 ; c < modp->length; c += PAGE_SIZE) { + addr = encl->base + modp->offset + c; + + mutex_lock(&encl->lock); + + entry = sgx_encl_load_page(encl, addr); + if (IS_ERR(entry)) { + ret = PTR_ERR(entry) == -EBUSY ? -EAGAIN : -EFAULT; + goto out_unlock; + } + + /* + * Changing EPCM permissions is only supported on regular + * SGX pages. Attempting this change on other pages will + * result in #PF. + */ + if (entry->type != SGX_PAGE_TYPE_REG) { + ret = -EINVAL; + goto out_unlock; + } + + /* + * Do not verify if current runtime protection bits are what + * is being requested. The enclave may have relaxed EPCM + * permissions calls without letting the kernel know and + * thus permission restriction may still be needed even if + * from the kernel's perspective the permissions are unchanged. + */ + + /* New permissions should never exceed vetted permissions. */ + if ((entry->vm_max_prot_bits & vm_prot) != vm_prot) { + ret = -EPERM; + goto out_unlock; + } + + /* Make sure page stays around while releasing mutex. */ + if (sgx_unmark_page_reclaimable(entry->epc_page)) { + ret = -EAGAIN; + goto out_unlock; + } + + mutex_unlock(&encl->lock); + /* + * Do not keep encl->lock because of dependency on + * mmap_lock acquired in sgx_zap_enclave_ptes(). + */ + sgx_zap_enclave_ptes(encl, addr); + + mutex_lock(&encl->lock); + + /* Change EPCM permissions. */ + epc_virt = sgx_get_epc_virt_addr(entry->epc_page); + ret = __emodpr(&secinfo, epc_virt); + if (encls_faulted(ret)) { + /* + * All possible faults should be avoidable: + * parameters have been checked, will only change + * permissions of a regular page, and no concurrent + * SGX1/SGX2 ENCLS instructions since these + * are protected with mutex. + */ + pr_err_once("EMODPR encountered exception %d\n", + ENCLS_TRAPNR(ret)); + ret = -EFAULT; + goto out_reclaim; + } + if (encls_failed(ret)) { + modp->result = ret; + ret = -EFAULT; + goto out_reclaim; + } + + ret = sgx_enclave_etrack(encl); + if (ret) { + ret = -EFAULT; + goto out_reclaim; + } + + sgx_mark_page_reclaimable(entry->epc_page); + mutex_unlock(&encl->lock); + } + + ret = 0; + goto out; + +out_reclaim: + sgx_mark_page_reclaimable(entry->epc_page); +out_unlock: + mutex_unlock(&encl->lock); +out: + modp->count = c; + + return ret; +} + +/** + * sgx_ioc_enclave_restrict_perm() - handler for + * %SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS + * @encl: an enclave pointer + * @arg: userspace pointer to a &struct sgx_enclave_restrict_perm + * instance + * + * SGX2 distinguishes between relaxing and restricting the enclave page + * permissions maintained by the hardware (EPCM permissions) of pages + * belonging to an initialized enclave (after SGX_IOC_ENCLAVE_INIT). + * + * EPCM permissions cannot be restricted from within the enclave, the enclave + * requires the kernel to run the privileged level 0 instructions ENCLS[EMODPR] + * and ENCLS[ETRACK]. An attempt to relax EPCM permissions with this call + * will be ignored by the hardware. + * + * Enclave page permissions are not allowed to exceed the maximum vetted + * permissions maintained in &struct sgx_encl_page->vm_max_prot_bits. + * + * Return: + * - 0: Success + * - -errno: Otherwise + */ +static long sgx_ioc_enclave_restrict_perm(struct sgx_encl *encl, + void __user *arg) +{ + struct sgx_enclave_restrict_perm params; + u64 secinfo_perm; + long ret; + + ret = sgx_ioc_sgx2_ready(encl); + if (ret) + return ret; + + if (copy_from_user(¶ms, arg, sizeof(params))) + return -EFAULT; + + if (sgx_validate_offset_length(encl, params.offset, params.length)) + return -EINVAL; + + ret = sgx_perm_from_user_secinfo((void __user *)params.secinfo, + &secinfo_perm); + if (ret) + return ret; + + if (params.result || params.count) + return -EINVAL; + + ret = sgx_enclave_restrict_perm(encl, ¶ms, secinfo_perm); + + if (copy_to_user(arg, ¶ms, sizeof(params))) + return -EFAULT; + + return ret; +} + long sgx_ioctl(struct file *filep, unsigned int cmd, unsigned long arg) { struct sgx_encl *encl = filep->private_data; @@ -713,6 +987,9 @@ long sgx_ioctl(struct file *filep, unsigned int cmd, unsigned long arg) case SGX_IOC_ENCLAVE_PROVISION: ret = sgx_ioc_enclave_provision(encl, (void __user *)arg); break; + case SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS: + ret = sgx_ioc_enclave_restrict_perm(encl, (void __user *)arg); + break; default: ret = -ENOIOCTLCMD; break;