linux-kernel - Re: [PATCH v36 12/24] x86/sgx: Add SGX_IOC_ENCLAVE

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]

Message-ID: <20200827132436.GA4674@linux.intel.com>
Date:   Thu, 27 Aug 2020 16:24:36 +0300
From:   Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>
To:     Borislav Petkov <bp@...en8.de>
Cc:     x86@...nel.org, linux-sgx@...r.kernel.org,
        linux-kernel@...r.kernel.org, Jethro Beekman <jethro@...tanix.com>,
        Haitao Huang <haitao.huang@...ux.intel.com>,
        Chunyang Hui <sanqian.hcy@...fin.com>,
        Jordan Hand <jorhand@...ux.microsoft.com>,
        Nathaniel McCallum <npmccallum@...hat.com>,
        Seth Moore <sethmo@...gle.com>,
        Sean Christopherson <sean.j.christopherson@...el.com>,
        Suresh Siddha <suresh.b.siddha@...el.com>,
        akpm@...ux-foundation.org, andriy.shevchenko@...ux.intel.com,
        asapek@...gle.com, cedric.xing@...el.com, chenalexchen@...gle.com,
        conradparker@...gle.com, cyhanish@...gle.com,
        dave.hansen@...el.com, haitao.huang@...el.com,
        josh@...htriplett.org, kai.huang@...el.com, kai.svahn@...el.com,
        kmoy@...gle.com, ludloff@...gle.com, luto@...nel.org,
        nhorman@...hat.com, puiterwijk@...hat.com, rientjes@...gle.com,
        tglx@...utronix.de, yaozhangx@...gle.com
Subject: Re: [PATCH v36 12/24] x86/sgx: Add SGX_IOC_ENCLAVE_CREATE

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@...dspring.com>
> >  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@...r.kernel.org> ?
> 
> 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