Linux-Sgx Archive on lore.kernel.org
 help / color / Atom feed
From: Andy Lutomirski <luto@kernel.org>
To: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Cc: X86 ML <x86@kernel.org>,
	Platform Driver <platform-driver-x86@vger.kernel.org>,
	linux-sgx@vger.kernel.org, Dave Hansen <dave.hansen@intel.com>,
	"Christopherson, Sean J" <sean.j.christopherson@intel.com>,
	nhorman@redhat.com, npmccallum@redhat.com, "Ayoun,
	Serge" <serge.ayoun@intel.com>,
	shay.katz-zamir@intel.com,
	Haitao Huang <haitao.huang@linux.intel.com>,
	Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	"Svahn, Kai" <kai.svahn@intel.com>,
	mark.shanahan@intel.com, Ingo Molnar <mingo@redhat.com>,
	Borislav Petkov <bp@alien8.de>, "H. Peter Anvin" <hpa@zytor.com>,
	Darren Hart <dvhart@infradead.org>,
	Andy Shevchenko <andy@infradead.org>,
	"open list:X86 ARCHITECTURE (32-BIT AND 64-BIT)" 
	<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v17 18/23] platform/x86: Intel SGX driver
Date: Mon, 17 Dec 2018 21:55:08 -0800
Message-ID: <CALCETrX1XG1kCrVfx1athBaCWp4H0xoKnjoDjFOqR4nf_VqBrA@mail.gmail.com> (raw)
In-Reply-To: <20181116010412.23967-19-jarkko.sakkinen@linux.intel.com>

On Thu, Nov 15, 2018 at 5:08 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.

This is a very partial review.

> +int sgx_encl_find(struct mm_struct *mm, unsigned long addr,
> +                 struct vm_area_struct **vma)
> +{
> +       struct vm_area_struct *result;
> +       struct sgx_encl *encl;
> +
> +       result = find_vma(mm, addr);
> +       if (!result || result->vm_ops != &sgx_vm_ops || addr < result->vm_start)
> +               return -EINVAL;
> +
> +       encl = result->vm_private_data;
> +       *vma = result;
> +
> +       return encl ? 0 : -ENOENT;
> +}

I realize that this function may go away entirely but, if you keep it:
what are the locking rules?  What, if anything, prevents another
thread from destroying the enclave after sgx_encl_find() returns?

> +static int sgx_validate_secs(const struct sgx_secs *secs,
> +                            unsigned long ssaframesize)
> +{

...

> +       if (secs->attributes & SGX_ATTR_MODE64BIT) {
> +               if (secs->size > sgx_encl_size_max_64)
> +                       return -EINVAL;
> +       } else {
> +               /* On 64-bit architecture allow 32-bit encls only in
> +                * the compatibility mode.
> +                */
> +               if (!test_thread_flag(TIF_ADDR32))
> +                       return -EINVAL;
> +               if (secs->size > sgx_encl_size_max_32)
> +                       return -EINVAL;
> +       }

Why do we need the 32-bit-on-64-bit check?  In general, anything that
checks per-task or per-mm flags like TIF_ADDR32 is IMO likely to be
problematic.  You're allowing 64-bit enclaves in 32-bit tasks, so I'm
guessing you could just delete the check.

> +
> +       if (!(secs->xfrm & XFEATURE_MASK_FP) ||
> +           !(secs->xfrm & XFEATURE_MASK_SSE) ||
> +           (((secs->xfrm >> XFEATURE_BNDREGS) & 1) !=
> +            ((secs->xfrm >> XFEATURE_BNDCSR) & 1)) ||
> +           (secs->xfrm & ~sgx_xfrm_mask))
> +               return -EINVAL;

Do we need to check that the enclave doesn't use xfeatures that the
kernel doesn't know about?  Or are they all safe by design in enclave
mode?

> +static int sgx_encl_pm_notifier(struct notifier_block *nb,
> +                               unsigned long action, void *data)
> +{
> +       struct sgx_encl *encl = container_of(nb, struct sgx_encl, pm_notifier);
> +
> +       if (action != PM_SUSPEND_PREPARE && action != PM_HIBERNATION_PREPARE)
> +               return NOTIFY_DONE;

Hmm.  There's an argument to made that omitting this would better
exercise the code that handles fully asynchronous loss of an enclave.
Also, I think you're unnecessarily killing enclaves when suspend is
attempted but fails.

> +
> +static int sgx_get_key_hash(const void *modulus, void *hash)
> +{
> +       struct crypto_shash *tfm;
> +       int ret;
> +
> +       tfm = crypto_alloc_shash("sha256", 0, CRYPTO_ALG_ASYNC);
> +       if (IS_ERR(tfm))
> +               return PTR_ERR(tfm);
> +
> +       ret = __sgx_get_key_hash(tfm, modulus, hash);
> +
> +       crypto_free_shash(tfm);
> +       return ret;
> +}
> +

I'm so sorry you had to deal with this API.  Once Zinc lands, you
could clean this up :)


> +static int sgx_encl_get(unsigned long addr, struct sgx_encl **encl)
> +{
> +       struct mm_struct *mm = current->mm;
> +       struct vm_area_struct *vma;
> +       int ret;
> +
> +       if (addr & (PAGE_SIZE - 1))
> +               return -EINVAL;
> +
> +       down_read(&mm->mmap_sem);
> +
> +       ret = sgx_encl_find(mm, addr, &vma);
> +       if (!ret) {
> +               *encl = vma->vm_private_data;
> +
> +               if ((*encl)->flags & SGX_ENCL_SUSPEND)
> +                       ret = SGX_POWER_LOST_ENCLAVE;
> +               else
> +                       kref_get(&(*encl)->refcount);
> +       }

Hmm.  This version has explicit refcounting.

> +static int sgx_mmap(struct file *file, struct vm_area_struct *vma)
> +{
> +       vma->vm_ops = &sgx_vm_ops;
> +       vma->vm_flags |= VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP | VM_IO |
> +                        VM_DONTCOPY;
> +
> +       return 0;
> +}
> +
> +static unsigned long sgx_get_unmapped_area(struct file *file,
> +                                          unsigned long addr,
> +                                          unsigned long len,
> +                                          unsigned long pgoff,
> +                                          unsigned long flags)
> +{
> +       if (len < 2 * PAGE_SIZE || (len & (len - 1)))
> +               return -EINVAL;
> +
> +       if (len > sgx_encl_size_max_64)
> +               return -EINVAL;
> +
> +       if (len > sgx_encl_size_max_32 && test_thread_flag(TIF_ADDR32))
> +               return -EINVAL;

Generally speaking, this type of check wants to be
in_compat_syscall().  But I'm not sure I understand why you need it at
all.

> +static void sgx_ipi_cb(void *info)
> +{
> +}
> +
> +void sgx_flush_cpus(struct sgx_encl *encl)
> +{
> +       on_each_cpu_mask(mm_cpumask(encl->mm), sgx_ipi_cb, NULL, 1);
> +}

Please add a comment explaining what this promises to do.

  parent reply index

Thread overview: 161+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20181116010412.23967-1-jarkko.sakkinen@linux.intel.com>
2018-11-16  1:01 ` [PATCH v17 01/23] x86/sgx: Update MAINTAINERS Jarkko Sakkinen
2018-11-16 14:22   ` Borislav Petkov
2018-11-16 15:07     ` Jarkko Sakkinen
2018-11-16 20:24       ` Borislav Petkov
2018-11-18  8:20         ` Jarkko Sakkinen
2018-11-16  1:01 ` [PATCH v17 02/23] x86/cpufeatures: Add Intel-defined SGX feature bit Jarkko Sakkinen
2018-11-16 14:28   ` Borislav Petkov
2018-11-16 15:13     ` Jarkko Sakkinen
2018-11-16 15:18       ` Jarkko Sakkinen
2018-11-16 20:53         ` Borislav Petkov
2018-11-16  1:01 ` [PATCH v17 03/23] x86/cpufeatures: Add SGX sub-features (as Linux-defined bits) Jarkko Sakkinen
2018-11-16 14:37   ` Borislav Petkov
2018-11-16 15:38     ` Sean Christopherson
2018-11-16 23:31   ` Dave Hansen
2018-11-18  8:36     ` Jarkko Sakkinen
2018-11-16  1:01 ` [PATCH v17 04/23] x86/msr: Add IA32_FEATURE_CONTROL.SGX_ENABLE definition Jarkko Sakkinen
2018-11-16  1:01 ` [PATCH v17 05/23] x86/cpufeatures: Add Intel-defined SGX_LC feature bit Jarkko Sakkinen
2018-11-16  1:01 ` [PATCH v17 06/23] x86/cpu/intel: Detect SGX support and update caps appropriately Jarkko Sakkinen
2018-11-16 23:32   ` Dave Hansen
2018-11-18  8:37     ` Jarkko Sakkinen
2018-11-21 18:17   ` Borislav Petkov
2018-11-24 13:54     ` Jarkko Sakkinen
2018-11-16  1:01 ` [PATCH v17 07/23] x86/mm: x86/sgx: Add new 'PF_SGX' page fault error code bit Jarkko Sakkinen
2018-11-16 23:33   ` Dave Hansen
2018-11-18  8:38     ` Jarkko Sakkinen
2018-11-16  1:01 ` [PATCH v17 08/23] x86/mm: x86/sgx: Signal SIGSEGV for userspace #PFs w/ PF_SGX Jarkko Sakkinen
2018-11-16  1:01 ` [PATCH v17 09/23] x86/sgx: Define SGX1 and SGX2 ENCLS leafs Jarkko Sakkinen
2018-11-16  1:01 ` [PATCH v17 10/23] x86/sgx: Add ENCLS architectural error codes Jarkko Sakkinen
2018-11-16  1:01 ` [PATCH v17 11/23] x86/sgx: Add SGX1 and SGX2 architectural data structures Jarkko Sakkinen
2018-11-16  1:01 ` [PATCH v17 12/23] x86/sgx: Add definitions for SGX's CPUID leaf and variable sub-leafs Jarkko Sakkinen
2018-11-16  1:01 ` [PATCH v17 13/23] x86/msr: Add SGX Launch Control MSR definitions Jarkko Sakkinen
2018-11-16 17:29   ` Sean Christopherson
2018-11-18  8:19     ` Jarkko Sakkinen
2018-11-16  1:01 ` [PATCH v17 14/23] x86/sgx: Add wrappers for ENCLS leaf functions Jarkko Sakkinen
2018-11-16  1:01 ` [PATCH v17 15/23] x86/sgx: Enumerate and track EPC sections Jarkko Sakkinen
2018-11-16  1:01 ` [PATCH v17 16/23] x86/sgx: Add functions to allocate and free EPC pages Jarkko Sakkinen
2018-11-16  1:01 ` [PATCH v17 17/23] x86/sgx: Add sgx_einit() for initializing enclaves Jarkko Sakkinen
2018-11-16  1:01 ` [PATCH v17 18/23] platform/x86: Intel SGX driver Jarkko Sakkinen
2018-11-16  1:37   ` Randy Dunlap
2018-11-16 11:23     ` Jarkko Sakkinen
2018-11-19 15:06   ` Jarkko Sakkinen
2018-11-19 16:22     ` Jethro Beekman
2018-11-19 17:19       ` Jarkko Sakkinen
2018-11-19 17:39         ` Jethro Beekman
2018-11-20 10:58           ` Jarkko Sakkinen
2018-11-21 15:24             ` Jarkko Sakkinen
2018-11-19 18:18         ` Andy Lutomirski
2018-11-20 11:00           ` Jarkko Sakkinen
2018-11-19 15:29   ` Andy Lutomirski
2018-11-19 16:19     ` Jarkko Sakkinen
2018-11-19 16:59       ` Andy Lutomirski
2018-11-20 12:04         ` Jarkko Sakkinen
2018-11-22 11:12           ` Dr. Greg
2018-11-22 15:21             ` Andy Lutomirski
2018-11-24 17:21               ` Jarkko Sakkinen
2018-11-24 20:13                 ` Dr. Greg
2018-11-26 21:15                   ` Jarkko Sakkinen
2018-11-25 14:53                 ` Jarkko Sakkinen
2018-11-25 16:22                   ` Andy Lutomirski
2018-11-25 18:55                     ` Dr. Greg
2018-11-25 23:51                       ` Jarkko Sakkinen
     [not found]                       ` <D45BC005-5064-4C75-B486-4E43C454E2F6@amacapital.net>
2018-11-26  0:37                         ` Andy Lutomirski
2018-11-26 11:00                           ` Dr. Greg
2018-11-26 18:22                             ` Andy Lutomirski
2018-11-26 22:16                             ` Jarkko Sakkinen
2018-11-26 21:51                     ` Jarkko Sakkinen
2018-11-26 23:04                       ` Jarkko Sakkinen
2018-11-27  8:55                         ` Dr. Greg
2018-11-27 16:41                           ` Jarkko Sakkinen
2018-11-27 17:55                             ` Andy Lutomirski
2018-11-28 10:49                               ` Dr. Greg
2018-11-28 19:22                                 ` Jarkko Sakkinen
2018-12-10 10:49                                   ` Dr. Greg
2018-12-12 18:00                                     ` Jarkko Sakkinen
2018-12-14 23:59                                       ` Dr. Greg
2018-12-15  0:06                                         ` Sean Christopherson
2018-12-15 23:22                                           ` Dr. Greg
2018-12-17 14:27                                             ` Sean Christopherson
2018-12-17 13:28                                           ` Jarkko Sakkinen
2018-12-17 13:39                                             ` Jarkko Sakkinen
2018-12-17 14:08                                               ` Jarkko Sakkinen
2018-12-17 14:13                                                 ` Jarkko Sakkinen
2018-12-17 16:34                                                   ` Dr. Greg
2018-12-17 17:31                                                 ` Sean Christopherson
2018-12-17 17:49                                                   ` Jarkko Sakkinen
2018-12-17 18:09                                                     ` Sean Christopherson
2018-12-17 18:23                                                       ` Jarkko Sakkinen
2018-12-17 18:46                                                         ` Sean Christopherson
2018-12-17 19:36                                                           ` Jarkko Sakkinen
2018-11-27 16:46                           ` Jarkko Sakkinen
2018-11-28 21:52                           ` Andy Lutomirski
2018-11-27  7:46                       ` Jethro Beekman
2018-11-27 16:36                         ` Jarkko Sakkinen
2018-11-22 20:56             ` Andy Lutomirski
2018-11-23 10:39               ` Dr. Greg
2018-11-24 16:45                 ` Jarkko Sakkinen
2018-11-28  5:08                   ` Jarkko Sakkinen
2018-11-28  5:38                     ` Jethro Beekman
2018-12-09 17:01         ` Pavel Machek
2018-11-20 11:15     ` Dr. Greg
2018-11-24 16:15       ` Jarkko Sakkinen
2018-11-24 19:24         ` Dr. Greg
2018-11-26 19:39           ` Jarkko Sakkinen
2018-12-09 17:01     ` Pavel Machek
2018-12-10 14:46       ` Dr. Greg
2018-12-17 17:45   ` Dave Hansen
2018-12-17 18:01     ` Jarkko Sakkinen
2018-12-17 18:07       ` Dave Hansen
2018-12-17 18:31         ` Jarkko Sakkinen
2018-12-17 18:36       ` Sean Christopherson
2018-12-17 18:43         ` Jarkko Sakkinen
2018-12-17 18:47           ` Dave Hansen
2018-12-17 19:12             ` Andy Lutomirski
2018-12-17 19:17               ` Dave Hansen
2018-12-17 19:25                 ` Andy Lutomirski
2018-12-17 19:54                   ` Jarkko Sakkinen
2018-12-17 19:49                 ` Jarkko Sakkinen
2018-12-17 19:53                   ` Dave Hansen
2018-12-17 19:55                     ` Andy Lutomirski
2018-12-17 20:03                       ` Dave Hansen
2018-12-17 20:10                         ` Andy Lutomirski
2018-12-17 20:15                           ` Dave Hansen
2018-12-17 22:36                             ` Sean Christopherson
2018-12-18  1:40                           ` Jarkko Sakkinen
2018-12-17 22:20               ` Sean Christopherson
2018-12-18  1:39                 ` Jarkko Sakkinen
2018-12-18  3:27                   ` Jarkko Sakkinen
2018-12-18  5:02                     ` Andy Lutomirski
2018-12-18 13:27                       ` Jarkko Sakkinen
2018-12-18  4:55                   ` Andy Lutomirski
2018-12-18 13:18                     ` Jarkko Sakkinen
2018-12-18  4:59                 ` Andy Lutomirski
2018-12-18 13:11                   ` Jarkko Sakkinen
2018-12-18 15:44                   ` Sean Christopherson
2018-12-18 18:53                     ` Sean Christopherson
2018-12-19  5:00                       ` Jarkko Sakkinen
2018-12-19  5:13                         ` Jarkko Sakkinen
2018-12-21 18:28                         ` Sean Christopherson
2018-12-22  0:01                           ` Jarkko Sakkinen
2018-12-19  4:47                     ` Jarkko Sakkinen
2018-12-19  5:24                       ` Jarkko Sakkinen
2018-12-18  1:17               ` Jarkko Sakkinen
2018-12-18  1:31                 ` Jarkko Sakkinen
2018-12-17 18:48           ` Sean Christopherson
2018-12-17 19:09             ` Dave Hansen
2018-12-17 19:37               ` Jarkko Sakkinen
2018-12-17 19:40                 ` Dave Hansen
2018-12-17 19:33             ` Jarkko Sakkinen
2018-12-17 20:21               ` Jarkko Sakkinen
2018-12-18 13:13                 ` Jarkko Sakkinen
2018-12-18 15:46                   ` Sean Christopherson
2018-12-18  5:55   ` Andy Lutomirski [this message]
2018-12-19  5:22     ` Jarkko Sakkinen
2018-11-16  1:01 ` [PATCH v17 19/23] platform/x86: sgx: Add swapping functionality to the " Jarkko Sakkinen
2018-11-16  1:01 ` [PATCH v17 20/23] x86/sgx: Add a simple swapper for the EPC memory manager Jarkko Sakkinen
2018-11-16  1:01 ` [PATCH v17 21/23] platform/x86: ptrace() support for the SGX driver Jarkko Sakkinen
2018-11-16  1:01 ` [PATCH v17 22/23] x86/sgx: SGX documentation Jarkko Sakkinen
2018-12-03  3:28   ` Randy Dunlap
2018-12-03  9:32     ` Jarkko Sakkinen
2018-11-16  1:01 ` [PATCH v17 23/23] selftests/x86: Add a selftest for SGX Jarkko Sakkinen
2018-11-16 11:17 ` [PATCH v17 00/23] Intel SGX1 support Jarkko Sakkinen

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CALCETrX1XG1kCrVfx1athBaCWp4H0xoKnjoDjFOqR4nf_VqBrA@mail.gmail.com \
    --to=luto@kernel.org \
    --cc=andriy.shevchenko@linux.intel.com \
    --cc=andy@infradead.org \
    --cc=bp@alien8.de \
    --cc=dave.hansen@intel.com \
    --cc=dvhart@infradead.org \
    --cc=haitao.huang@linux.intel.com \
    --cc=hpa@zytor.com \
    --cc=jarkko.sakkinen@linux.intel.com \
    --cc=kai.svahn@intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-sgx@vger.kernel.org \
    --cc=mark.shanahan@intel.com \
    --cc=mingo@redhat.com \
    --cc=nhorman@redhat.com \
    --cc=npmccallum@redhat.com \
    --cc=platform-driver-x86@vger.kernel.org \
    --cc=sean.j.christopherson@intel.com \
    --cc=serge.ayoun@intel.com \
    --cc=shay.katz-zamir@intel.com \
    --cc=tglx@linutronix.de \
    --cc=x86@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Sgx Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-sgx/0 linux-sgx/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-sgx linux-sgx/ https://lore.kernel.org/linux-sgx \
		linux-sgx@vger.kernel.org linux-sgx@archiver.kernel.org
	public-inbox-index linux-sgx


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-sgx


AGPL code for this site: git clone https://public-inbox.org/ public-inbox