Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 5 Sep 2018 20:33:55 +0300
From:   Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
To:     Andy Shevchenko <andy.shevchenko@gmail.com>
Cc:     "maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)" <x86@kernel.org>,
        Platform Driver <platform-driver-x86@vger.kernel.org>,
        Dave Hansen <dave.hansen@intel.com>,
        sean.j.christopherson@intel.com, nhorman@redhat.com,
        npmccallum@redhat.com, linux-sgx@vger.kernel.org,
        serge.ayoun@intel.com, shay.katz-zamir@intel.com,
        suresh.b.siddha@intel.com, Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>,
        "H. Peter Anvin" <hpa@zytor.com>,
        Darren Hart <dvhart@infradead.org>,
        Andy Shevchenko <andy@infradead.org>,
        Herbert Xu <herbert@gondor.apana.org.au>, ebiggers@google.com,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v13 11/13] platform/x86: Intel SGX driver
Message-ID: <20180905173355.GE11368@linux.intel.com>
References: <20180827185507.17087-1-jarkko.sakkinen@linux.intel.com>
 <20180827185507.17087-12-jarkko.sakkinen@linux.intel.com>
 <CAHp75VcqKunkHYNdd287_F7APHwYr7K9QD2PVCkqBqJWG2_E-g@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAHp75VcqKunkHYNdd287_F7APHwYr7K9QD2PVCkqBqJWG2_E-g@mail.gmail.com>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
User-Agent: Mutt/1.9.4 (2018-02-28)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Sep 04, 2018 at 08:59:58PM +0300, Andy Shevchenko wrote:
> On Mon, Aug 27, 2018 at 9:58 PM Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com> wrote:
> >
> > Intel Software Guard eXtensions (SGX) is a set of CPU instructions that
> > can be used by applications to set aside private regions of code and
> > data. The code outside the enclave is disallowed to access the memory
> > inside the enclave by the CPU access control.
> >
> > SGX driver provides a ioctl API for loading and initializing enclaves.
> > Address range for enclaves is reserved with mmap() and they are
> > destroyed with munmap(). Enclave construction, measurement and
> > initialization is done with the provided the ioctl API.
> 
> > +/*
> > + * This file is provided under a dual BSD/GPLv2 license.  When using or
> > + * redistributing this file, you may do so under either license.
> > + *
> > + * GPL LICENSE SUMMARY
> > + *
> > + * Copyright(c) 2016-2017 Intel Corporation.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of version 2 of the GNU General Public License as
> > + * published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful, but
> > + * WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > + * General Public License for more details.
> > + *
> > + * Contact Information:
> > + * Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > + * Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
> > + *
> > + * BSD LICENSE
> > + *
> > + * Copyright(c) 2016-2017 Intel Corporation.
> > + *
> > + * Redistribution and use in source and binary forms, with or without
> > + * modification, are permitted provided that the following conditions
> > + * are met:
> > + *
> > + *   * Redistributions of source code must retain the above copyright
> > + *     notice, this list of conditions and the following disclaimer.
> > + *   * Redistributions in binary form must reproduce the above copyright
> > + *     notice, this list of conditions and the following disclaimer in
> > + *     the documentation and/or other materials provided with the
> > + *     distribution.
> > + *   * Neither the name of Intel Corporation nor the names of its
> > + *     contributors may be used to endorse or promote products derived
> > + *     from this software without specific prior written permission.
> > + *
> > + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> > + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> > + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> > + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> > + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> > + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> > + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> > + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> > + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> > + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> > + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> > + *
> 
> Can't we just put one line with SPDX identifier?
> 
> > + * Authors:
> 
> > + *
> 
> Redundant line.
> 
> > + * Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > + * Suresh Siddha <suresh.b.siddha@intel.com>
> > + */
> 
> > +/**
> 
> > + * struct sgx_enclave_add_page - parameter structure for the
> > + *                               %SGX_IOC_ENCLAVE_ADD_PAGE ioctl
> 
> I don't think multi-line would work nice for short description line.
> 
> > + * @addr:      address within the ELRANGE
> > + * @src:       address for the page data
> > + * @secinfo:   address for the SECINFO data
> > + * @mrmask:    bitmask for the measured 256 byte chunks
> > + */
> 
> > +config INTEL_SGX
> > +       tristate "Intel(R) SGX Driver"
> 
> Spell SGX fully here.
> 
> > +       default n
> 
> Drop this.
> 
> > +++ b/drivers/platform/x86/intel_sgx/Makefile
> > @@ -0,0 +1,13 @@
> > +#
> > +# Intel SGX
> > +#
> 
> No SPDX?  (Same, btw, for Kconfig)
> 
> > +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
> 
> Should be /* */ in headers AFAIR.
> 
> Btw, shouldn't be this file rather under include/linux/platform_data/x86/ ?
> 
> > +#define sgx_pr_ratelimited(level, encl, fmt, ...)                      \
> > +       pr_ ## level ## _ratelimited("[%d:0x%p] " fmt,                  \
> > +                                    pid_nr((encl)->tgid),              \
> > +                                    (void *)(encl)->base, ##__VA_ARGS__)
> > +#define sgx_dbg(encl, fmt, ...) \
> > +       sgx_pr_ratelimited(debug, encl, fmt, ##__VA_ARGS__)
> > +#define sgx_info(encl, fmt, ...) \
> > +       sgx_pr_ratelimited(info, encl, fmt, ##__VA_ARGS__)
> > +#define sgx_warn(encl, fmt, ...) \
> > +       sgx_pr_ratelimited(warn, encl, fmt, ##__VA_ARGS__)
> > +#define sgx_err(encl, fmt, ...) \
> > +       sgx_pr_ratelimited(err, encl, fmt, ##__VA_ARGS__)
> > +#define sgx_crit(encl, fmt, ...) \
> > +       sgx_pr_ratelimited(crit, encl, fmt, ##__VA_ARGS__)
> 
> Rate limited versions do not guarantee all info to be printed (as far
> as I heard).
> If you are going to use them everywhere it would be nice to see that
> RL versions are in use.
> So, please, add suffix _rl at least to your helper macros.
> 
> > +#define SGX_ENCL_PAGE_PCMD_OFFSET(encl_page, encl)             \
> > +({                                                             \
> > +       unsigned long ret;                                      \
> > +       ret = SGX_ENCL_PAGE_BACKING_INDEX(encl_page, encl);     \
> 
> > +       ((ret & 31) * 128);                                     \
> 
> What these magic numbers are about?
> 
> > +})
> 
> > +#define SGX_INVD(ret, encl, fmt, ...)          \
> > +do {                                           \
> > +       if (WARN(ret, "sgx: " fmt, ##__VA_ARGS__))      \
> > +               sgx_invalidate(encl, true);     \
> > +} while (0)
> 
> Perhaps INVD -> INV as I can see as a pattern in kernel.
> 
> > +#include <asm/mman.h>
> 
> linux/* first
> 
> > +#include <linux/delay.h>
> > +#include <linux/file.h>
> > +#include <linux/hashtable.h>
> > +#include <linux/highmem.h>
> > +#include <linux/ratelimit.h>
> > +#include <linux/sched/signal.h>
> > +#include <linux/shmem_fs.h>
> > +#include <linux/slab.h>
> > +#include <linux/suspend.h>
> 
> > +                       if ((entry->desc & SGX_ENCL_PAGE_LOADED) &&
> > +                           (entry->desc & SGX_ENCL_PAGE_TCS) &&
> > +                           !sgx_encl_find(encl->mm, addr, &vma))
> 
> > +               if ((entry->desc & SGX_ENCL_PAGE_LOADED) &&
> > +                   !(entry->desc & SGX_ENCL_PAGE_RECLAIMED)) {
> > +                       if (!__sgx_free_page(entry->epc_page))
> > +                               entry->desc &= ~SGX_ENCL_PAGE_LOADED;
> 
> > +static int sgx_measure(struct sgx_epc_page *secs_page,
> > +                      struct sgx_epc_page *epc_page,
> > +                      u16 mrmask)
> > +{
> 
> > +       int ret = 0;
> 
> I would rather expect this to be closer to where it's used.
> 
> > +       void *secs;
> > +       void *epc;
> > +       int i;
> > +       int j;
> > +
> > +       if (!mrmask)
> > +               return ret;
> 
> return 0; ?
> 
> > +
> > +       secs = sgx_epc_addr(secs_page);
> > +       epc = sgx_epc_addr(epc_page);
> > +
> 
> ... ret = 0;
> 
> Actually, it's not needed, since you are always call __eextend() at
> least once (see above check for !mrmask).
> 
> > +       for (i = 0, j = 1; i < 0x1000 && !ret; i += 0x100, j <<= 1) {
> > +               if (!(j & mrmask))
> > +                       continue;
> > +
> > +               ret = __eextend(secs, (void *)((unsigned long)epc + i));
> > +       }
> 
> Can we rewrite this like
> 
> unsigned long tmp = mrmask;
> 
> for_each_set_bit(j, ...) {
>  ret = ...(... + j * 0x100));
>  if (ret)
>   break;
> }
> 
> ?
> 
> > +       struct sgx_encl *encl;
> 
> > +       encl = container_of(work, struct sgx_encl, add_page_work);
> 
> Could this assignment be directly in definition block?
> 
> > +       } while (!kref_put(&encl->refcount, sgx_encl_release) && !is_empty);
> 
> isn't it the same as  !(... || ...) ?
> 
> > +}
> 
> > +static u32 sgx_calc_ssaframesize(u32 miscselect, u64 xfrm)
> > +{
> > +       u32 size_max = PAGE_SIZE;
> > +       u32 size;
> > +       int i;
> > +
> 
> > +       for (i = 2; i < 64; i++) {
> > +               if (!((1 << i) & xfrm))
> > +                       continue;
> 
> DECLARE_BITMAP(tmp, 64);
> 
> tmp = bitmap_from_u64(tmp, xfrm & ~3);
> for_each_set_bit(i, tmp) {
>  ...
> }
> 
> > +
> > +               size = SGX_SSA_GPRS_SIZE + sgx_xsave_size_tbl[i];
> > +               if (miscselect & SGX_MISC_EXINFO)
> > +                       size += SGX_SSA_MISC_EXINFO_SIZE;
> 
> > +               if (size > size_max)
> > +                       size_max = size;
> 
> size_max = max(size_max, size);
> 
> > +       }
> > +
> 
> > +       return (size_max + PAGE_SIZE - 1) >> PAGE_SHIFT;
> 
> snd_sgbuf_aligned_pages() is the same. Perhaps time to create a helper
> here and in the future it can be moved to some generic one?
> 
> Ooops, it's actually PFN_UP().
> 
> > +}
> > +
> > +static int sgx_validate_secs(const struct sgx_secs *secs,
> > +                            unsigned long ssaframesize)
> > +{
> > +       int i;
> > +
> > +       if (secs->size < (2 * PAGE_SIZE) ||
> 
> > +           (secs->size & (secs->size - 1)) != 0)
> 
> is_power_of_2()
> 
> > +               return -EINVAL;
> > +
> > +       if (secs->base & (secs->size - 1))
> > +               return -EINVAL;
> > +
> > +       if (secs->attributes & SGX_ATTR_RESERVED_MASK ||
> > +           secs->miscselect & sgx_misc_reserved)
> > +               return -EINVAL;
> 
> > +       if ((secs->xfrm & 0x3) != 0x3 || (secs->xfrm & ~sgx_xfrm_mask))
> > +               return -EINVAL;
> 
> Some magic is here
> 
> > +
> > +       /* Check that BNDREGS and BNDCSR are equal. */
> 
> > +       if (((secs->xfrm >> 3) & 1) != ((secs->xfrm >> 4) & 1))
> 
> Perhaps
> 
> (!!(... & BIT(3)) ^ !!(... & BIT(4))) ?
> 
> ...and actually define those magic bits?
> 
> > +               return -EINVAL;
> > +
> > +       if (!secs->ssa_frame_size || ssaframesize > secs->ssa_frame_size)
> > +               return -EINVAL;
> > +
> 
> > +       for (i = 0; i < SGX_SECS_RESERVED1_SIZE; i++)
> > +               if (secs->reserved1[i])
> > +                       return -EINVAL;
> > +
> > +       for (i = 0; i < SGX_SECS_RESERVED2_SIZE; i++)
> > +               if (secs->reserved2[i])
> > +                       return -EINVAL;
> > +
> > +       for (i = 0; i < SGX_SECS_RESERVED3_SIZE; i++)
> > +               if (secs->reserved3[i])
> > +                       return -EINVAL;
> > +
> > +       for (i = 0; i < SGX_SECS_RESERVED4_SIZE; i++)
> > +               if (secs->reserved4[i])
> > +                       return -EINVAL;
> 
> Can you use memchr_inv() in all above cases?
> 
> > +       return 0;
> > +}
> > +
> 
> > +       struct sgx_encl *encl =
> > +               container_of(mn, struct sgx_encl, mmu_notifier);
> 
> One line?
> 
> > +       backing = shmem_file_setup("[dev/sgx]", secs->size + PAGE_SIZE,
> > +                                  VM_NORESERVE);
> > +       if (IS_ERR(backing))
> 
> > +               return (void *)backing;
> 
> ERR_CAST() please.
> 
> > +       pcmd = shmem_file_setup("[dev/sgx]", (secs->size + PAGE_SIZE) >> 5,
> > +                               VM_NORESERVE);
> > +       if (IS_ERR(pcmd)) {
> > +               fput(backing);
> 
> > +               return (void *)pcmd;
> 
> Ditto.
> 
> > +       }
> 
> > +int sgx_encl_create(struct sgx_encl *encl, struct sgx_secs *secs)
> > +{
> > +       struct vm_area_struct *vma;
> > +       struct sgx_pageinfo pginfo;
> > +       struct sgx_secinfo secinfo;
> > +       struct sgx_epc_page *secs_epc;
> > +       long ret;
> > +
> > +       secs_epc = sgx_alloc_page(&encl->secs.impl, 0);
> > +       if (IS_ERR(secs_epc)) {
> 
> > +               ret = PTR_ERR(secs_epc);
> > +               return ret;
> 
> One line.
> 
> > +       }
> 
> > +       pginfo.addr = 0;
> > +       pginfo.contents = (unsigned long)secs;
> > +       pginfo.metadata = (unsigned long)&secinfo;
> > +       pginfo.secs = 0;
> > +       memset(&secinfo, 0, sizeof(secinfo));
> 
> + blank line here.
> 
> > +       ret = __ecreate((void *)&pginfo, sgx_epc_addr(secs_epc));
> 
> > +
> 
> - blank line here.
> 
> > +       if (ret) {
> > +               sgx_dbg(encl, "ECREATE returned %ld\n", ret);
> > +               return ret;
> > +       }
> 
> > +       down_read(&current->mm->mmap_sem);
> > +       ret = sgx_encl_find(current->mm, secs->base, &vma);
> 
> > +       if (ret != -ENOENT) {
> 
> > +               if (!ret)
> > +                       ret = -EINVAL;
> 
> > +               return ret;
> 
> return ret ? ret : -EINVAL;
> 
> > +       }
> 
> > +}
> 
> > +       for (i = 0; i < SGX_SECINFO_RESERVED_SIZE; i++)
> > +               if (secinfo->reserved[i])
> > +                       return -EINVAL;
> 
> memchr_inv() ?
> 
> > +       if (offset & (PAGE_SIZE - 1))
> > +               return false;
> 
> ~PAGE_MASK ?
> 
> > +       if ((tcs->fs_limit & 0xFFF) != 0xFFF)
> > +               return -EINVAL;
> > +
> > +       if ((tcs->gs_limit & 0xFFF) != 0xFFF)
> > +               return -EINVAL;
> 
> Are they ~PAGE_MASK as well? Please define properly.
> 
> > +       if (sgx_validate_secinfo(secinfo))
> > +               return -EINVAL;
> 
> + blank line
> 
> > +       if (page_type == SGX_SECINFO_TCS) {
> > +               ret = sgx_validate_tcs(encl, data);
> > +               if (ret)
> > +                       return ret;
> > +       }
> 
> + blank line
> 
> > +       ret = sgx_encl_grow(encl);
> > +       if (ret)
> > +               return ret;
> 
> + blank line
> 
> > +       mutex_lock(&encl->lock);
> > +       if (encl->flags & (SGX_ENCL_INITIALIZED | SGX_ENCL_DEAD)) {
> > +               mutex_unlock(&encl->lock);
> > +               return -EINVAL;
> > +       }
> 
> + blank line
> 
> > +       encl_page = sgx_encl_alloc_page(encl, addr);
> > +       if (IS_ERR(encl_page)) {
> > +               mutex_unlock(&encl->lock);
> > +               return PTR_ERR(encl_page);
> > +       }
> 
> + blank line
> 
> > +       ret = __sgx_encl_add_page(encl, encl_page, data, secinfo, mrmask);
> > +       if (ret)
> > +               sgx_encl_free_page(encl_page);
> 
> + blank line
> 
> > +       mutex_unlock(&encl->lock);
> 
> > +       u64 mrsigner[4];
> 
> Magic 4.
> 
> > +       for (i = 0; i < SGX_EINIT_SLEEP_COUNT; i++) {
> > +               for (j = 0; j < SGX_EINIT_SPIN_COUNT; j++) {
> > +                       ret = sgx_einit(sigstruct, token, encl->secs.epc_page,
> > +                                       mrsigner);
> 
> > +                       if (ret == SGX_UNMASKED_EVENT)
> > +                               continue;
> 
> > +                       else
> > +                               break;
> 
> Seems redundant
> 
> > +               }
> 
> > +       while (!list_empty(&encl->va_pages)) {
> > +               va_page = list_first_entry(&encl->va_pages, struct sgx_va_page,
> > +                                          list);
> 
> list_for_each_entry_safe() ?
> 
> > +               list_del(&va_page->list);
> > +               sgx_free_page(va_page->epc_page);
> > +               kfree(va_page);
> > +       }
> 
> --
> With Best Regards,
> Andy Shevchenko

Thanks, very detailed! Does not make sense to ack these separately so I
just say that I try to fix them all with care.

/Jarkko