2021-12-01 19:23:54

by Reinette Chatre

[permalink] [raw]
Subject: [PATCH 02/25] x86/sgx: Add wrappers for SGX2 functions

The SGX ENCLS instruction uses EAX to specify an SGX function and
may require additional registers, depending on the SGX function.
ENCLS invokes the specified privileged SGX function for managing
and debugging enclaves. Several macros are used to wrap the ENCLS
functionality.

Add ENCLS wrappers for the SGX2 EMODPR, EMODT, and EAUG functions
that can make changes to pages of an initialized SGX enclave. The
EMODPR function is used to restrict enclave page permissions
as maintained within the enclave (Enclave Page Cache Map (EPCM)
permissions). The EMODT function is used to change the type of an
enclave page. The EAUG function is used to dynamically add enclave
pages to an initialized enclave.

EMODPR and EMODT accepts two parameters and can fault as well as return
an SGX error code. EAUG also accepts two parameters but does not return
an SGX error code. Use existing macros for all new functions.

Expand enum sgx_return_code with the possible EMODPR and EMODT
return codes.

Signed-off-by: Reinette Chatre <[email protected]>
---
arch/x86/include/asm/sgx.h | 5 +++++
arch/x86/kernel/cpu/sgx/encls.h | 18 ++++++++++++++++++
2 files changed, 23 insertions(+)

diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h
index 05f3e21f01a7..ebae2a153c66 100644
--- a/arch/x86/include/asm/sgx.h
+++ b/arch/x86/include/asm/sgx.h
@@ -47,17 +47,22 @@ enum sgx_encls_function {

/**
* enum sgx_return_code - The return code type for ENCLS, ENCLU and ENCLV
+ * %SGX_EPC_PAGE_CONFLICT: Page is being written by other ENCLS function.
* %SGX_NOT_TRACKED: Previous ETRACK's shootdown sequence has not
* been completed yet.
* %SGX_CHILD_PRESENT SECS has child pages present in the EPC.
* %SGX_INVALID_EINITTOKEN: EINITTOKEN is invalid and enclave signer's
* public key does not match IA32_SGXLEPUBKEYHASH.
+ * %SGX_PAGE_NOT_MODIFIABLE: The EPC page cannot be modified because it
+ * is in the PENDING or MODIFIED state.
* %SGX_UNMASKED_EVENT: An unmasked event, e.g. INTR, was received
*/
enum sgx_return_code {
+ SGX_EPC_PAGE_CONFLICT = 7,
SGX_NOT_TRACKED = 11,
SGX_CHILD_PRESENT = 13,
SGX_INVALID_EINITTOKEN = 16,
+ SGX_PAGE_NOT_MODIFIABLE = 20,
SGX_UNMASKED_EVENT = 128,
};

diff --git a/arch/x86/kernel/cpu/sgx/encls.h b/arch/x86/kernel/cpu/sgx/encls.h
index 241b766265d3..243c30301ddb 100644
--- a/arch/x86/kernel/cpu/sgx/encls.h
+++ b/arch/x86/kernel/cpu/sgx/encls.h
@@ -238,4 +238,22 @@ static inline int __ewb(struct sgx_pageinfo *pginfo, void *addr,
return __encls_ret_3(EWB, pginfo, addr, va);
}

+/* Restrict the permissions of an Enclave Page Cache (EPC) page */
+static inline int __emodpr(struct sgx_secinfo *secinfo, void *addr)
+{
+ return __encls_ret_2(EMODPR, secinfo, addr);
+}
+
+/* Change the type of an Enclave Page Cache (EPC) page */
+static inline int __emodt(struct sgx_secinfo *secinfo, void *addr)
+{
+ return __encls_ret_2(EMODT, secinfo, addr);
+}
+
+/* Add a page to an initialized enclave */
+static inline int __eaug(struct sgx_pageinfo *pginfo, void *addr)
+{
+ return __encls_2(EAUG, pginfo, addr);
+}
+
#endif /* _X86_ENCLS_H */
--
2.25.1



2021-12-04 22:04:13

by Jarkko Sakkinen

[permalink] [raw]
Subject: Re: [PATCH 02/25] x86/sgx: Add wrappers for SGX2 functions

On Wed, Dec 01, 2021 at 11:23:00AM -0800, Reinette Chatre wrote:
> The SGX ENCLS instruction uses EAX to specify an SGX function and
> may require additional registers, depending on the SGX function.
> ENCLS invokes the specified privileged SGX function for managing
> and debugging enclaves. Several macros are used to wrap the ENCLS
> functionality.
>
> Add ENCLS wrappers for the SGX2 EMODPR, EMODT, and EAUG functions
> that can make changes to pages of an initialized SGX enclave. The
> EMODPR function is used to restrict enclave page permissions
> as maintained within the enclave (Enclave Page Cache Map (EPCM)
> permissions). The EMODT function is used to change the type of an
> enclave page. The EAUG function is used to dynamically add enclave
> pages to an initialized enclave.
>
> EMODPR and EMODT accepts two parameters and can fault as well as return
> an SGX error code. EAUG also accepts two parameters but does not return
> an SGX error code. Use existing macros for all new functions.
>
> Expand enum sgx_return_code with the possible EMODPR and EMODT
> return codes.

These implementation details only obfuscate this commit message, and
it is way too high-level to be useful e.g. for kernel maintenance.

I'd replace it with something like:

"
Add wrappers for ENCLS leaf functions EAUG, EMODT and EMODPR,
which roughly take two steps:

1. EAUG creates a new EPCM entry.
EMODT and EMODPR modify an existing EPCM entry.
2. Set either .PR = 1 (EMODPR), .MODIFY = 1 (EMODT) or .PENDING = 1 (AUG).

The bit is reset by the enclave by invoking ENCLU leaf function EACCEPT
or EACCEPTCOPY, which will result the EPCM change becoming effective.
"

The current commit message is also not addressing these:

1. What happens if enclaves accesses a memory address with either .PR,
.MODIFY or .PENDING set in EPCM, other than by the means of EACCEPT
or EACCEPTCOPY?
2. The calling conditions (e.g. concerning TLB's and ETRACK/IPI/etc
dance related to it).


If this information was properly contained here, discussing about the
following commits would be much easier.

/Jarkko

2021-12-06 21:15:47

by Reinette Chatre

[permalink] [raw]
Subject: Re: [PATCH 02/25] x86/sgx: Add wrappers for SGX2 functions

Hi Jarkko,

On 12/4/2021 2:04 PM, Jarkko Sakkinen wrote:
> On Wed, Dec 01, 2021 at 11:23:00AM -0800, Reinette Chatre wrote:
>> The SGX ENCLS instruction uses EAX to specify an SGX function and
>> may require additional registers, depending on the SGX function.
>> ENCLS invokes the specified privileged SGX function for managing
>> and debugging enclaves. Several macros are used to wrap the ENCLS
>> functionality.
>>
>> Add ENCLS wrappers for the SGX2 EMODPR, EMODT, and EAUG functions
>> that can make changes to pages of an initialized SGX enclave. The
>> EMODPR function is used to restrict enclave page permissions
>> as maintained within the enclave (Enclave Page Cache Map (EPCM)
>> permissions). The EMODT function is used to change the type of an
>> enclave page. The EAUG function is used to dynamically add enclave
>> pages to an initialized enclave.
>>
>> EMODPR and EMODT accepts two parameters and can fault as well as return
>> an SGX error code. EAUG also accepts two parameters but does not return
>> an SGX error code. Use existing macros for all new functions.
>>
>> Expand enum sgx_return_code with the possible EMODPR and EMODT
>> return codes.
>
> These implementation details only obfuscate this commit message, and
> it is way too high-level to be useful e.g. for kernel maintenance.

2c273671d0df ("x86/sgx: Add wrappers for ENCLS functions") seemed to be
good enough for kernel maintenance, but ok.

>
> I'd replace it with something like:
>
> "
> Add wrappers for ENCLS leaf functions EAUG, EMODT and EMODPR,
> which roughly take two steps:
>
> 1. EAUG creates a new EPCM entry.
> EMODT and EMODPR modify an existing EPCM entry.
> 2. Set either .PR = 1 (EMODPR), .MODIFY = 1 (EMODT) or .PENDING = 1 (AUG).
>
> The bit is reset by the enclave by invoking ENCLU leaf function EACCEPT
> or EACCEPTCOPY, which will result the EPCM change becoming effective.
> "

I can use this if the SGX2 functions continues to be introduced in a
single patch but ...

>
> The current commit message is also not addressing these:
>
> 1. What happens if enclaves accesses a memory address with either .PR,
> .MODIFY or .PENDING set in EPCM, other than by the means of EACCEPT
> or EACCEPTCOPY?
> 2. The calling conditions (e.g. concerning TLB's and ETRACK/IPI/etc
> dance related to it).

... adding this information for all three SGX functions would be too
much for one patch so I think that I should rather split this into three
patches, each introducing a single SGX2 function with all the details
you require. But ...


... the intent of this patch was just to introduce the wrappers of the
SGX2 functions. These details surrounding the flows when using these
functions are addressed in the patches that use them. It sounds to me
that you want to duplicate that information here where the wrappers are
added. Looking ahead you do require the same information in the
changelogs of the patches that use these wrappers so I would like to
confirm if you would like to see three separate patches with the details
duplicating the information provided later or if you would like to see a
single patch with the three wrappers and the changelog that you recommend?

> If this information was properly contained here, discussing about the
> following commits would be much easier.

The commits using these functions should have clear content on the flows
surrounding them. I see there is work to do and I will review them to
ensure that.

Reinette