linux-sgx.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Sean Christopherson <sean.j.christopherson@intel.com>
To: Dave Hansen <dave.hansen@intel.com>
Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>,
	x86@kernel.org, linux-sgx@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	Jethro Beekman <jethro@fortanix.com>,
	Haitao Huang <haitao.huang@linux.intel.com>,
	Chunyang Hui <sanqian.hcy@antfin.com>,
	Jordan Hand <jorhand@linux.microsoft.com>,
	Nathaniel McCallum <npmccallum@redhat.com>,
	Seth Moore <sethmo@google.com>,
	Darren Kenny <darren.kenny@oracle.com>,
	Suresh Siddha <suresh.b.siddha@intel.com>,
	akpm@linux-foundation.org, andriy.shevchenko@linux.intel.com,
	asapek@google.com, bp@alien8.de, cedric.xing@intel.com,
	chenalexchen@google.com, conradparker@google.com,
	cyhanish@google.com, haitao.huang@intel.com, kai.huang@intel.com,
	kai.svahn@intel.com, kmoy@google.com, ludloff@google.com,
	luto@kernel.org, nhorman@redhat.com, puiterwijk@redhat.com,
	rientjes@google.com, tglx@linutronix.de, yaozhangx@google.com,
	mikko.ylinen@intel.com
Subject: Re: [PATCH v39 13/24] x86/sgx: Add SGX_IOC_ENCLAVE_ADD_PAGES
Date: Mon, 19 Oct 2020 14:15:26 -0700	[thread overview]
Message-ID: <20201019211525.GC23072@linux.intel.com> (raw)
In-Reply-To: <516a1b7a-38cc-adde-833b-b661cbee97f2@intel.com>

On Mon, Oct 19, 2020 at 01:48:32PM -0700, Dave Hansen wrote:
> On 10/17/20 10:03 PM, Jarkko Sakkinen wrote:
> >>> +		if (ret) {
> >>> +			if (encls_failed(ret))
> >>> +				ENCLS_WARN(ret, "EEXTEND");
> >>> +			return -EIO;
> >>
> >> How frequent should we expect these to be?  Can users cause them?  You
> >> should *proably* call it ENCLS_WARN_ONCE() if it's implemented that way.

Ya, it's implemented using WARN_ONCE.  It doesn't append _ONCE mostly to avoid
unnecessary verbosity, e.g. there's no existing SGX code that uses vanilla
WARN, nor does it seem likely that there will ever be a case where using WARN
is justified.

> > If power cycle happens.
> 
> So, we get one warning per power cycle?  Practically, do you mean a
> suspend/resume cycle, or is this more like hibernation-to-disk-resume?
> 
> In any case, if this is normal system operation (which closing my laptop
> lid qualifies as), it should produce zero warnings.

encls_failed() filters out EPCM faults (which, by the by, is why the kernel
cares about that #GP vs. #PF erratum).  So what you describe is the implemented
behavior, i.e. WARNs are triggerable if and only if there is a hardware or
kernel bug.

FWIW, I prefer burying the encls_failed() logic in ENCLS_WARN.  encls_failed()
is only used to gate ENCLS_WARN, and ENCLS_WARN is always wrapped with
encls_failed() excepted for EREMOVE.  The only downside to applying the logic
to EREMOVE is that it could theoretically suppress kernel bugs on CPUs that are
subject to the #GP instead of #PF errata.
 
> >>> +static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long src,
> >>> +			     unsigned long offset, struct sgx_secinfo *secinfo,
> >>> +			     unsigned long flags)
> >>> +{
> >>> +	struct sgx_encl_page *encl_page;
> >>> +	struct sgx_epc_page *epc_page;
> >>> +	int ret;
> >>> +
> >>> +	encl_page = sgx_encl_page_alloc(encl, offset, secinfo->flags);
> >>> +	if (IS_ERR(encl_page))
> >>> +		return PTR_ERR(encl_page);
> >>> +
> >>> +	epc_page = __sgx_alloc_epc_page();
> >>> +	if (IS_ERR(epc_page)) {
> >>> +		kfree(encl_page);
> >>> +		return PTR_ERR(epc_page);
> >>> +	}
> >>
> >> Looking at these, I'm forgetting why we need to both allocate an
> >> encl_page and an epc_page.  Commends might remind me.  So would better
> >> names.
> > 
> > Should the struct names be renamed?
> > 
> > Like sgx_phys_epc_page and sgx_virt_epc_page?
> 
> "epc" is additional acronym nonsense and redundant with "sgx" and "page"
> anyway.
> 
> I'd probably call then 'sgx_phys_page' and 'sgx_virt_slot' or something.

I don't too much deeply about whether or not sgx_encl_page is renamed, but I
would very strongly prefer keeping sgx_epc_page.  Nearly all of the SGX
literature refers to the physical pages residing in the EPC as "EPC pages".
IMO, the "sgx" is the somewhat superfluous part that is tacked on to add
namespacing in case of collisions with "epc".

> >>> +	mmap_read_lock(current->mm);
> >>> +	mutex_lock(&encl->lock);
> >>> +
> >>> +	/*
> >>> +	 * Insert prior to EADD in case of OOM.
> >>> +	if (copy_from_user(&addp, arg, sizeof(addp)))
> >>> +		return -EFAULT;
> >>> +
> >>> +	if (!IS_ALIGNED(addp.offset, PAGE_SIZE) ||
> >>> +	    !IS_ALIGNED(addp.src, PAGE_SIZE))
> >>> +		return -EINVAL;
> >>> +
> >>> +	if (!(access_ok(addp.src, PAGE_SIZE)))
> >>> +		return -EFAULT;
> >>
> >> This worries me.  You're doing an access_ok() check on addp.src because
> >> you evidently don't trust it.  But, below, it looks to be accessed
> >> directly with an offset, bound by addp.length, which I think can be
> >>> PAGE_SIZE.
> >>
> >> I'd feel a lot better if addp.src's value was being passed around as a
> >> __user pointer.
> > 
> > I'm not sure if that call is even needed. Each page is pinned with
> > get_user_pages(). AFAIK, it should be enough. This must be legacy cruft.
> 
> get_user_pages() and access_ok() do *very* different things.  Even if
> the pages are pinned, you might still be tricked into referencing off
> the end of the page, or up into the kernel address space.
> 
> >>> +	if (addp.length & (PAGE_SIZE - 1))
> >>> +		return -EINVAL;
> >>> +
> >>> +	if (addp.offset + addp.length - PAGE_SIZE >= encl->size)
> >>> +		return -EINVAL;
> >>> +
> >>> +	if (copy_from_user(&secinfo, (void __user *)addp.secinfo,
> >>> +			   sizeof(secinfo)))
> >>> +		return -EFAULT;
> >>> +
> >>> +	if (sgx_validate_secinfo(&secinfo))
> >>> +		return -EINVAL;
> >>> +
> >>> +	for (c = 0 ; c < addp.length; c += PAGE_SIZE) {
> >>> +		if (signal_pending(current)) {
> >>> +			if (!c)
> >>> +				ret = -ERESTARTSYS;
> >>> +
> >>> +			break;
> >>> +		}
> >>> +
> >>> +		if (c == SGX_MAX_ADD_PAGES_LENGTH)
> >>> +			break;
> >>> +
> >>> +		if (need_resched())
> >>> +			cond_resched();
> >>> +
> >>> +		ret = sgx_encl_add_page(encl, addp.src + c, addp.offset + c,
> >>> +					&secinfo, addp.flags);
> >>
> >> Yeah...  Don't we need to do another access_ok() check here, if we
> >> needed one above since we are moving away from addrp.src?
> > 
> > I don't think so because the page is pinned with get_user_pages().
> 
> No, get_user_pages() is orthogonal.
> 
> Looking at this again, you _might_ be OK since you validated addp.length
> against encl->size.  But, it's all very convoluted and doesn't look very
> organized or obviously right.

The easiest fix would be to have the existing access_ok() check the entire
range, no?  Or am I missing something obvious?

  reply	other threads:[~2020-10-19 21:15 UTC|newest]

Thread overview: 117+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-03  4:50 [PATCH v39 00/24] Intel SGX foundations Jarkko Sakkinen
2020-10-03  4:50 ` [PATCH v39 01/24] x86/cpufeatures: x86/msr: Add Intel SGX hardware bits Jarkko Sakkinen
2020-10-19 14:10   ` Dave Hansen
2020-10-19 17:49     ` Sean Christopherson
2020-10-03  4:50 ` [PATCH v39 02/24] x86/cpufeatures: x86/msr: Add Intel SGX Launch Control " Jarkko Sakkinen
2020-10-03  4:50 ` [PATCH v39 03/24] x86/mm: x86/sgx: Signal SIGSEGV with PF_SGX Jarkko Sakkinen
2020-10-03  4:50 ` [PATCH v39 04/24] x86/sgx: Add SGX microarchitectural data structures Jarkko Sakkinen
2020-10-03  4:50 ` [PATCH v39 05/24] x86/sgx: Add wrappers for ENCLS leaf functions Jarkko Sakkinen
2020-10-19 14:30   ` Dave Hansen
2020-10-19 17:38     ` Sean Christopherson
2020-10-19 17:48       ` Dave Hansen
2020-10-19 17:53         ` Sean Christopherson
2020-10-19 17:58           ` Dave Hansen
2020-10-03  4:50 ` [PATCH v39 06/24] x86/cpu/intel: Detect SGX support Jarkko Sakkinen
2020-10-03  4:50 ` [PATCH v39 07/24] x86/cpu/intel: Add nosgx kernel parameter Jarkko Sakkinen
2020-10-03  4:50 ` [PATCH v39 08/24] x86/sgx: Initialize metadata for Enclave Page Cache (EPC) sections Jarkko Sakkinen
2020-10-19  8:45   ` Jarkko Sakkinen
2020-10-19 12:39     ` Borislav Petkov
2020-10-23  9:01       ` Jarkko Sakkinen
2020-10-19 13:40     ` Dave Hansen
2020-10-23  9:03       ` Jarkko Sakkinen
2020-10-03  4:50 ` [PATCH v39 09/24] x86/sgx: Add __sgx_alloc_epc_page() and sgx_free_epc_page() Jarkko Sakkinen
2020-10-03  4:50 ` [PATCH v39 10/24] mm: Add 'mprotect' hook to struct vm_operations_struct Jarkko Sakkinen
2020-10-03  4:50 ` [PATCH v39 11/24] x86/sgx: Add SGX enclave driver Jarkko Sakkinen
2020-10-03 14:39   ` Greg KH
2020-10-04 14:32     ` Jarkko Sakkinen
2020-10-04 15:01       ` Jarkko Sakkinen
2020-10-05  9:42       ` Greg KH
2020-10-05 12:42         ` Jarkko Sakkinen
2020-10-07 18:09           ` Haitao Huang
2020-10-07 19:26             ` Greg KH
2020-10-09  6:44               ` Jarkko Sakkinen
2020-10-14 20:16                 ` Dave Hansen
2020-10-05  8:45     ` Christoph Hellwig
2020-10-05 11:42       ` Jarkko Sakkinen
2020-10-05 11:50         ` Greg KH
2020-10-05 14:23           ` Jarkko Sakkinen
2020-10-05 15:02             ` Greg KH
2020-10-05 16:40               ` Dave Hansen
2020-10-05 20:02                 ` Jarkko Sakkinen
2020-10-09  7:10     ` Pavel Machek
2020-10-09  7:21       ` Greg KH
2020-10-09  8:21         ` Pavel Machek
2020-10-03 19:54   ` Matthew Wilcox
2020-10-04 21:50     ` Jarkko Sakkinen
2020-10-04 22:02       ` Jarkko Sakkinen
2020-10-04 22:27       ` Matthew Wilcox
2020-10-04 23:41         ` Jarkko Sakkinen
2020-10-05  1:30           ` Matthew Wilcox
2020-10-05  3:06             ` Jarkko Sakkinen
2020-10-03  4:50 ` [PATCH v39 12/24] x86/sgx: Add SGX_IOC_ENCLAVE_CREATE Jarkko Sakkinen
2020-10-16 17:07   ` Dave Hansen
2020-10-18  4:26     ` Jarkko Sakkinen
2020-10-19 20:21       ` Dave Hansen
2020-10-19 20:48         ` Sean Christopherson
2020-10-03  4:50 ` [PATCH v39 13/24] x86/sgx: Add SGX_IOC_ENCLAVE_ADD_PAGES Jarkko Sakkinen
2020-10-16 21:25   ` Dave Hansen
2020-10-18  5:03     ` Jarkko Sakkinen
2020-10-19  7:03       ` Jarkko Sakkinen
2020-10-19 20:48       ` Dave Hansen
2020-10-19 21:15         ` Sean Christopherson [this message]
2020-10-19 21:44           ` Dave Hansen
2020-10-23 10:11             ` Jarkko Sakkinen
2020-10-03  4:50 ` [PATCH v39 14/24] x86/sgx: Add SGX_IOC_ENCLAVE_INIT Jarkko Sakkinen
2020-10-03  4:50 ` [PATCH v39 15/24] x86/sgx: Add SGX_IOC_ENCLAVE_PROVISION Jarkko Sakkinen
2020-10-20 15:48   ` Dave Hansen
2020-10-23 10:14     ` Jarkko Sakkinen
2020-10-20 21:19   ` Dave Hansen
2020-10-23 10:17     ` Jarkko Sakkinen
2020-10-23 14:19       ` Dave Hansen
2020-10-24 11:34         ` Jarkko Sakkinen
2020-10-24 15:47           ` Andy Lutomirski
2020-10-24 20:23             ` Jarkko Sakkinen
2020-10-27 10:38               ` Dr. Greg
2020-10-23 14:23       ` Jethro Beekman
2020-10-24 11:40         ` Jarkko Sakkinen
2020-10-03  4:50 ` [PATCH v39 16/24] x86/sgx: Add a page reclaimer Jarkko Sakkinen
2020-10-03  5:22   ` Haitao Huang
2020-10-03 13:32     ` Jarkko Sakkinen
2020-10-03 18:23       ` Haitao Huang
2020-10-04 22:39         ` Jarkko Sakkinen
2020-10-07 17:25           ` Jarkko Sakkinen
2020-10-03  4:50 ` [PATCH v39 17/24] x86/sgx: Add ptrace() support for the SGX driver Jarkko Sakkinen
2020-10-03  4:50 ` [PATCH v39 18/24] x86/vdso: Add support for exception fixup in vDSO functions Jarkko Sakkinen
2020-10-03  4:50 ` [PATCH v39 19/24] x86/fault: Add helper function to sanitize error code Jarkko Sakkinen
2020-10-03  4:50 ` [PATCH v39 20/24] x86/traps: Attempt to fixup exceptions in vDSO before signaling Jarkko Sakkinen
2020-10-03  4:50 ` [PATCH v39 21/24] x86/vdso: Implement a vDSO for Intel SGX enclave call Jarkko Sakkinen
2020-10-06  2:57   ` Sean Christopherson
2020-10-06  8:30     ` Jethro Beekman
2020-10-06 15:15       ` Sean Christopherson
2020-10-06 17:28         ` Jarkko Sakkinen
2020-10-06 23:21           ` Sean Christopherson
2020-10-07  0:22             ` Jarkko Sakkinen
2020-10-07  1:17               ` Sean Christopherson
2020-10-07  3:14                 ` Jarkko Sakkinen
2020-10-07  4:34                   ` Sean Christopherson
2020-10-07  7:39                     ` Jarkko Sakkinen
2020-10-07  8:04                       ` Jarkko Sakkinen
2020-10-07 15:25                       ` Sean Christopherson
2020-10-07 17:08                         ` Jarkko Sakkinen
2020-10-07 17:13                           ` Jarkko Sakkinen
2020-10-06 15:49       ` Jarkko Sakkinen
2020-10-06 15:36     ` Jarkko Sakkinen
2020-10-06 21:39     ` Jarkko Sakkinen
2020-10-07  0:23       ` Jarkko Sakkinen
2020-10-17  1:48   ` Andy Lutomirski
2020-10-17 21:02     ` Jarkko Sakkinen
2020-10-03  4:50 ` [PATCH v39 22/24] selftests/x86: Add a selftest for SGX Jarkko Sakkinen
2020-10-12 16:50   ` Jarkko Sakkinen
2020-10-03  4:50 ` [PATCH v39 23/24] docs: x86/sgx: Document SGX micro architecture and kernel internals Jarkko Sakkinen
2020-10-03  4:50 ` [PATCH v39 24/24] x86/sgx: Update MAINTAINERS Jarkko Sakkinen
2020-10-16 21:04   ` Dave Hansen
2020-10-18  4:27     ` Jarkko Sakkinen
2020-10-03 14:32 ` [PATCH v39 00/24] Intel SGX foundations Greg KH
2020-10-03 14:53   ` Jarkko Sakkinen
2020-10-15 19:06 ` Dave Hansen
2020-10-17 20:43   ` Jarkko Sakkinen

Reply instructions:

You may reply publicly 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=20201019211525.GC23072@linux.intel.com \
    --to=sean.j.christopherson@intel.com \
    --cc=akpm@linux-foundation.org \
    --cc=andriy.shevchenko@linux.intel.com \
    --cc=asapek@google.com \
    --cc=bp@alien8.de \
    --cc=cedric.xing@intel.com \
    --cc=chenalexchen@google.com \
    --cc=conradparker@google.com \
    --cc=cyhanish@google.com \
    --cc=darren.kenny@oracle.com \
    --cc=dave.hansen@intel.com \
    --cc=haitao.huang@intel.com \
    --cc=haitao.huang@linux.intel.com \
    --cc=jarkko.sakkinen@linux.intel.com \
    --cc=jethro@fortanix.com \
    --cc=jorhand@linux.microsoft.com \
    --cc=kai.huang@intel.com \
    --cc=kai.svahn@intel.com \
    --cc=kmoy@google.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-sgx@vger.kernel.org \
    --cc=ludloff@google.com \
    --cc=luto@kernel.org \
    --cc=mikko.ylinen@intel.com \
    --cc=nhorman@redhat.com \
    --cc=npmccallum@redhat.com \
    --cc=puiterwijk@redhat.com \
    --cc=rientjes@google.com \
    --cc=sanqian.hcy@antfin.com \
    --cc=sethmo@google.com \
    --cc=suresh.b.siddha@intel.com \
    --cc=tglx@linutronix.de \
    --cc=x86@kernel.org \
    --cc=yaozhangx@google.com \
    --subject='Re: [PATCH v39 13/24] x86/sgx: Add SGX_IOC_ENCLAVE_ADD_PAGES' \
    /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

This is a public inbox, see mirroring instructions
on how to clone and mirror all data and code used for this inbox