linux-kernel - Re: [PATCH v11 08/13] x86, sgx: added 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:   Tue, 19 Jun 2018 16:25:34 +0300
From:   Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>
To:     Dave Hansen <dave.hansen@...el.com>
Cc:     x86@...nel.org, platform-driver-x86@...r.kernel.org,
        sean.j.christopherson@...el.com, nhorman@...hat.com,
        npmccallum@...hat.com, Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        "H. Peter Anvin" <hpa@...or.com>,
        "open list:X86 ARCHITECTURE (32-BIT AND 64-BIT)" 
        <linux-kernel@...r.kernel.org>,
        "open list:INTEL SGX" <intel-sgx-kernel-dev@...ts.01.org>
Subject: Re: [PATCH v11 08/13] x86, sgx: added ENCLS wrappers

On Fri, Jun 08, 2018 at 10:43:50AM -0700, Dave Hansen wrote:
> On 06/08/2018 10:09 AM, Jarkko Sakkinen wrote:
> > This commit adds wrappers for Intel(R) SGX ENCLS opcode functionality.
> 
> What's ENCLS?  I know what an opcode is, but I don't know what "opcode
> functionality" is.  Could you give us more than a single, cryptic
> sentence, please?

Sure.

> > +enum sgx_commands {
> > +	ECREATE	= 0x0,
> > +	EADD	= 0x1,
> > +	EINIT	= 0x2,
> > +	EREMOVE	= 0x3,
> > +	EDGBRD	= 0x4,
> > +	EDGBWR	= 0x5,
> > +	EEXTEND	= 0x6,
> > +	ELDU	= 0x8,
> > +	EBLOCK	= 0x9,
> > +	EPA	= 0xA,
> > +	EWB	= 0xB,
> > +	ETRACK	= 0xC,
> > +	EAUG	= 0xD,
> > +	EMODPR	= 0xE,
> > +	EMODT	= 0xF,
> > +};
> 
> Again, please differentiate hardware-defined values from
> software-defines ones.  Also, would it hurt to expand the acronyms a
> bit, like:
> 
> +	ELDU	= 0x8, /* LoaD Underpants */

Not a bad idea at all.

> > +#define SGX_FN(name, params...)		\
> > +{					\
> > +	void *epc;			\
> > +	int ret;			\
> > +	epc = sgx_get_page(epc_page);	\
> > +	ret = __##name(params);		\
> > +	sgx_put_page(epc);		\
> > +	return ret;			\
> > +}
> 
> Have I seen sgx_*_page() yet in this series?  This seems out of order.

Oops, thanks for spotting this out.

> > +#define BUILD_SGX_FN(fn, name)				\
> > +static inline int fn(struct sgx_epc_page *epc_page)	\
> > +	SGX_FN(name, epc)
> > +BUILD_SGX_FN(sgx_eremove, eremove)
> > +BUILD_SGX_FN(sgx_eblock, eblock)
> > +BUILD_SGX_FN(sgx_etrack, etrack)
> > +BUILD_SGX_FN(sgx_epa, epa)
> > +
> > +static inline int sgx_emodpr(struct sgx_secinfo *secinfo,
> > +			     struct sgx_epc_page *epc_page)
> > +	SGX_FN(emodpr, secinfo, epc)
> > +static inline int sgx_emodt(struct sgx_secinfo *secinfo,
> > +			    struct sgx_epc_page *epc_page)
> > +	SGX_FN(emodt, secinfo, epc)
> 
> Wow, that's hideous.
> 
> Can't you just do:
> 
> BUILD_SGX_FN(__sgx_emopt, foo)
> 
> static inline int sgx_emodt(struct sgx_secinfo *secinfo,
> 			    struct sgx_epc_page *epc_page)
> {
> 	return __sgx_emopt(secinfo, page);
> }
> 
> Also, this entire patch seems rather comment-free.  Was that intentional?

Something that I've ignored (big series) but I'll add comments to
the next version.

/Jarkko