linux-kernel - Re: [PATCH 01/25] x86/sgx: Add shortlog descriptions to ENCLS wrappers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 11 Dec 2021 07:28:08 +0200
From:   Jarkko Sakkinen <jarkko@...nel.org>
To:     Reinette Chatre <reinette.chatre@...el.com>
Cc:     dave.hansen@...ux.intel.com, tglx@...utronix.de, bp@...en8.de,
        luto@...nel.org, mingo@...hat.com, linux-sgx@...r.kernel.org,
        x86@...nel.org, seanjc@...gle.com, kai.huang@...el.com,
        cathy.zhang@...el.com, cedric.xing@...el.com,
        haitao.huang@...el.com, mark.shanahan@...el.com, hpa@...or.com,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 01/25] x86/sgx: Add shortlog descriptions to ENCLS
 wrappers

On Mon, 2021-12-06 at 13:13 -0800, Reinette Chatre wrote:
> > * "Create an SECS page in the Enclave Page Cache (EPC)"
> > * "Add a Version Array (VA) page to the Enclave Page Cache (EPC)"
> > 
> > They should have similar descriptions, e.g.
> > 
> > * "Initialize an EPC page into SGX Enclave Control Structure (SECS) page."
> > * "Initialize an EPC page into Version Array (VA) page."
> 
> Will do. Did you intentionally omit the articles or would you be ok if I 
> change it to:
> 
> "Initialize an EPC page into an SGX Enclave Control Structure (SECS) page."
> "Initialize an EPC page into a Version Array (VA) page."
> 
> I also notice that you prefer the comments to end with a period and I 
> will do so for all in the next version.

Looks fine to me.

> > > +/* Extend uninitialized enclave measurement */
> > >   static inline int __eextend(void *secs, void *addr)
> > >   {
> > >   	return __encls_2(EEXTEND, secs, addr);
> > >   }
> > 
> > That description does not make __eextend any less cryptic.
> > 
> > Something like this would be already more informative:
> > 
> > /* Hash a 256 byte region of an enclave page to SECS:MRENCLAVE. */
> 
> Thank you, I will use this description.
> 
> > 
> > This same remark applies to the rest of these comments. They should
> > provide a clue what the wrapper does rather than an English open coded
> > function name.
> 
> Please see below for another attempt that includes your proposed changes 
> so far. What do you think?
> 
> __ecreate():
> /* Initialize an EPC page into an SGX Enclave Control Structure (SECS) 
> page. */
> 
> __eextend():
> /* Hash a 256 byte region of an enclave page to SECS:MRENCLAVE. */
> 
> __eadd():
> /* Copy a source page from non-enclave memory into the EPC. */

Perhaps:

/* 
 * Associate an EPC page to an enclave either as a REG or TCS page
 * populated with the provided data.
 */

This is more aligned with your description for __eremove().

> 
> __einit():
> /* Finalize enclave build, initialize enclave for user code execution */
> 
> __eremove():
> /* Disassociate EPC page from its enclave and mark it as unused. */
> 
> __edbgwr():
> /* Copy data to an EPC page belonging to a debug enclave. */
> 
> __edbgrd():
> /* Copy data from an EPC page belonging to a debug enclave. */
> 
> __etrack():
> /* Track that software has completed the required TLB address clears. */
> 
> __eldu():
> /* Load, verify, and unblock an Enclave Page Cache (EPC) page. */
> 
> __eblock():
> /* Make EPC page inaccessible to enclave, ready to be written to memory. */
> 
> __epa():
> /* Initialize an EPC page into a Version Array (VA) page. */
> 
> __ewb():
> /* Invalidate an EPC page and write it out to main memory. */
> 
> 
> Reinette

/Jarkko