Re: [PATCH v36 12/24] x86/sgx: Add SGX_IOC_ENCLAVE_CREATE

[Jarkko Sakkinen]

On Wed, Aug 26, 2020 at 04:52:39PM +0200, Borislav Petkov wrote:
> On Thu, Jul 16, 2020 at 04:52:51PM +0300, Jarkko Sakkinen wrote:
> > diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
> > index 59472cd6a11d..35f713e3a267 100644
> > --- a/Documentation/userspace-api/ioctl/ioctl-number.rst
> > +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
> > @@ -323,6 +323,7 @@ Code  Seq#    Include File                                           Comments
> >                                                                       <mailto:tlewis@xxxxxxxxxxxxxx>
> >  0xA3  90-9F  linux/dtlk.h
> >  0xA4  00-1F  uapi/linux/tee.h                                        Generic TEE subsystem
> > +0xA4  00-1F  uapi/asm/sgx.h                                          Intel SGX subsystem (a legit conflict as TEE and SGX do not co-exist)
> 
> Again, maybe add <mailto:linux-sgx@xxxxxxxxxxxxxxx> ?
> 
> This is from a previous review - please be more careful when addressing
> review comments - either do them or object to them but silently ignoring
> them is not cool.
> 
> >  0xAA  00-3F  linux/uapi/linux/userfaultfd.h
> >  0xAB  00-1F  linux/nbd.h
> >  0xAC  00-1F  linux/raw.h
> > diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
> > new file mode 100644
> > index 000000000000..3787d278e84b
> > --- /dev/null
> > +++ b/arch/x86/include/uapi/asm/sgx.h
> > @@ -0,0 +1,25 @@
> > +/* SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) WITH Linux-syscall-note */
> 
> checkpatch is not happy about something:
> 
> WARNING: 'SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) WITH Linux-syscall-note */' is not supported in LICENSES/...
> #79: FILE: arch/x86/include/uapi/asm/sgx.h:1:
> +/* SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) WITH Linux-syscall-note */

I don't know what has gone in my head when I wrote that but it looks
plain wrong even without running checkpatch.pl

The line should express the dijunction of "GPL-2.0+ WITH
Linux-syscall-note" and "BSD-3-Clause".

Grepping the kernel tree, I can find 34 instances of these in uapi
files:

  /* SPDX-License-Identifier: ((GPL-2.0+ WITH Linux-syscall-note) OR BSD-3-Clause) */

I have not checked if this passes checkpatch.pl yet, but I would
be surprised if that did not pass (obviously I'll check that).

> 
> ...
> 
> > +/**
> > + * sgx_ioc_enclave_create - handler for %SGX_IOC_ENCLAVE_CREATE
> > + * @filep:	open file to /dev/sgx
> 
> Also from a previous review:
> 
> "That's
> 
> @encl: enclave pointer
> 
> or so."

Yes, for sure. Thanks.

> > + * @arg:	userspace pointer to a struct sgx_enclave_create instance
> > + *
> > + * Allocate kernel data structures for a new enclave and execute ECREATE after
> > + * verifying the correctness of the provided SECS.
> > + *
> > + * Note, enforcement of restricted and disallowed attributes is deferred until
> > + * sgx_ioc_enclave_init(), only the architectural correctness of the SECS is
> > + * checked by sgx_ioc_enclave_create().
> 
> From that same review:
> 
> "Well, I don't see that checking. Where is it?"
> 
> Ok, I'm going to stop here. Please go over v33's review and either
> address *all* feedback or incorporate it into your patches if you agree
> with it but do not silently ignore it. One of the things I very strongly
> detest is ignored review comments.

I'm sorry about that. This was not intentional. I'll revisit them by
going through all your responses from here:

  https://patchwork.kernel.org/patch/11581715/

v34 had the splitting of the big driver patch into multiple patches.

During that process I've obviously failed to address these.

> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette

/Jarkko