Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 1 Mar 2022 14:26:48 +0100
From:   Jarkko Sakkinen <jarkko@kernel.org>
To:     Dave Hansen <dave.hansen@intel.com>
Cc:     "Dhanraj, Vijay" <vijay.dhanraj@intel.com>,
        "Chatre, Reinette" <reinette.chatre@intel.com>,
        "dave.hansen@linux.intel.com" <dave.hansen@linux.intel.com>,
        "tglx@linutronix.de" <tglx@linutronix.de>,
        "bp@alien8.de" <bp@alien8.de>,
        "Lutomirski, Andy" <luto@kernel.org>,
        "mingo@redhat.com" <mingo@redhat.com>,
        "linux-sgx@vger.kernel.org" <linux-sgx@vger.kernel.org>,
        "x86@kernel.org" <x86@kernel.org>,
        "Christopherson,, Sean" <seanjc@google.com>,
        "Huang, Kai" <kai.huang@intel.com>,
        "Zhang, Cathy" <cathy.zhang@intel.com>,
        "Xing, Cedric" <cedric.xing@intel.com>,
        "Huang, Haitao" <haitao.huang@intel.com>,
        "Shanahan, Mark" <mark.shanahan@intel.com>,
        "hpa@zytor.com" <hpa@zytor.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH V2 16/32] x86/sgx: Support restricting of enclave page
 permissions
Message-ID: <Yh4fGORDaJyVrJQW@iki.fi>
References: <cover.1644274683.git.reinette.chatre@intel.com>
 <4ce06608b5351f65f4e6bc6fc87c88a71215a2e7.1644274683.git.reinette.chatre@intel.com>
 <YhLhoMFPyOFZ2fsX@iki.fi>
 <DM8PR11MB55917F499CDF4CC7D426B0A7F63C9@DM8PR11MB5591.namprd11.prod.outlook.com>
 <Yhy/GvJegnTqYdq6@iki.fi>
 <f6a256a6-a7d7-a8e7-c9a8-e232203c63af@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <f6a256a6-a7d7-a8e7-c9a8-e232203c63af@intel.com>
Precedence: bulk

On Mon, Feb 28, 2022 at 07:16:22AM -0800, Dave Hansen wrote:
> On 2/28/22 04:24, Jarkko Sakkinen wrote:
> >> Regarding the recent update of splitting the page permissions change
> >> request into two IOCTLS (RELAX and RESTRICT), can we combine them into
> >> one? That is, revert to how it was done in the v1 version?
> > They are logically separate complex functionalities:
> > 
> > 1. "restrict" calls EMODPR and requires EACCEPT
> > 2. "relax" increases permissions up to vetted ("EADD") and could be
> >     combined with EMODPE called inside enclave.
> 
> It would be great to have a _slightly_ better justification than that.
> Existing permission interfaces like chmod or mprotect() don't have this
> asymmetry.
> 
> I think you're saying that the underlying hardware implementation is
> asymmetric, so the interface should be too.  I don't find that argument
> very convincing.  If the hardware interface is arcane and we can make it
> look more sane in the ioctl() layer, we should that, asymmetry or not.

That is my argument, yes.

> If we can't make it any more sane, let's say why the ioctl() must or
> should be asymmetric.

Perhaps underling this asymmetry in kdoc would be enough.

> The SGX2 page permission mechanism is horribly counter intuitive.
> *Everybody* that looks at it thinks that it's wrong.  That means that we
> have a lot of work ahead of us to explain the interfaces that get
> layered on top.

I fully agree on this :-)

With EACCEPTCOPY (kudos to Mark S. for reminding me of this version of
EACCEPT @ chat.enarx.dev) it is possible to make R and RX pages but
obviously new RX pages are now out of the picture:


	/*
	 * Adding a regular page that is architecturally allowed to only
	 * be created with RW permissions.
	 * TBD: Interface with user space policy to support max permissions
	 * of RWX.
	 */
	prot = PROT_READ | PROT_WRITE;
	encl_page->vm_run_prot_bits = calc_vm_prot_bits(prot, 0);
	encl_page->vm_max_prot_bits = encl_page->vm_run_prot_bits;

If that TBD is left out to the final version the page augmentation has a
risk of a API bottleneck, and that risk can realize then also in the page
permission ioctls.

I.e. now any review comment is based on not fully known territory, we have
one known unknown, and some unknown unknowns from unpredictable effect to
future API changes.

BR, Jarkko