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, 5 Apr 2022 10:03:41 +0300
From:   Jarkko Sakkinen <jarkko@kernel.org>
To:     Reinette Chatre <reinette.chatre@intel.com>
Cc:     dave.hansen@linux.intel.com, tglx@linutronix.de, bp@alien8.de,
        luto@kernel.org, mingo@redhat.com, linux-sgx@vger.kernel.org,
        x86@kernel.org, seanjc@google.com, kai.huang@intel.com,
        cathy.zhang@intel.com, cedric.xing@intel.com,
        haitao.huang@intel.com, mark.shanahan@intel.com, hpa@zytor.com,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH V3 21/30] selftests/sgx: Add test for EPCM permission
 changes
Message-ID: <YkvpzedC8JSnDyaJ@kernel.org>
References: <cover.1648847675.git.reinette.chatre@intel.com>
 <b2304ac0322d945b4bfdce36d3aae654f6ed35a0.1648847675.git.reinette.chatre@intel.com>
 <Ykvpi6HRL9cLorw6@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Ykvpi6HRL9cLorw6@kernel.org>
Precedence: bulk

On Tue, Apr 05, 2022 at 10:02:38AM +0300, Jarkko Sakkinen wrote:
> On Mon, Apr 04, 2022 at 09:49:29AM -0700, Reinette Chatre wrote:
> > EPCM permission changes could be made from within (to relax
> > permissions) or out (to restrict permissions) the enclave. Kernel
> > support is needed when permissions are restricted to be able to
> > call the privileged ENCLS[EMODPR] instruction. EPCM permissions
> > can be relaxed via ENCLU[EMODPE] from within the enclave but the
> > enclave still depends on the kernel to install PTEs with the needed
> > permissions.
> > 
> > Add a test that exercises a few of the enclave page permission flows:
> > 1) Test starts with a RW (from enclave and kernel perspective)
> >    enclave page that is mapped via a RW VMA.
> > 2) Use the SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS ioctl() to restrict
> >    the enclave (EPCM) page permissions to read-only.
> > 3) Run ENCLU[EACCEPT] from within the enclave to accept the new page
> >    permissions.
> > 4) Attempt to write to the enclave page from within the enclave - this
> >    should fail with a page fault on the EPCM permissions since the page
> >    table entry continues to allow RW access.
> > 5) Restore EPCM permissions to RW by running ENCLU[EMODPE] from within
> >    the enclave.
> > 6) Attempt to write to the enclave page from within the enclave - this
> >    should succeed since both EPCM and PTE permissions allow this access.
> > 
> > Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
> > ---
> > Changes since V2:
> > - Modify test to support separation between EPCM and PTE/VMA permissions
> >   - Fix changelog and comments to reflect new relationship between
> >     EPCM and PTE/VMA permissions.
> >   - With EPCM permissions controlling access instead of PTE permissions,
> >     check for SGX error code now encountered in page fault.
> >   - Stop calling SGX_IOC_ENCLAVE_RELAX_PERMISSIONS and ensure that
> >     only calling ENCLU[EMODPE] from within enclave is necessary to restore
> >     access to the enclave page.
> > - Update to use new struct name struct sgx_enclave_restrict_perm -> struct
> >   sgx_enclave_restrict_permissions. (Jarkko)
> > 
> > Changes since V1:
> > - Adapt test to the kernel interface changes: the ioctl() name change
> >   and providing entire secinfo as parameter.
> > - Remove the ENCLU[EACCEPT] call after permissions are relaxed since
> >   the new flow no longer results in the EPCM PR bit being set.
> > - Rewrite error path to reduce line lengths.
> > 
> >  tools/testing/selftests/sgx/defines.h   |  15 ++
> >  tools/testing/selftests/sgx/main.c      | 218 ++++++++++++++++++++++++
> >  tools/testing/selftests/sgx/test_encl.c |  38 +++++
> >  3 files changed, 271 insertions(+)
> > 
> > diff --git a/tools/testing/selftests/sgx/defines.h b/tools/testing/selftests/sgx/defines.h
> > index 02d775789ea7..b638eb98c80c 100644
> > --- a/tools/testing/selftests/sgx/defines.h
> > +++ b/tools/testing/selftests/sgx/defines.h
> > @@ -24,6 +24,8 @@ enum encl_op_type {
> >  	ENCL_OP_PUT_TO_ADDRESS,
> >  	ENCL_OP_GET_FROM_ADDRESS,
> >  	ENCL_OP_NOP,
> > +	ENCL_OP_EACCEPT,
> > +	ENCL_OP_EMODPE,
> >  	ENCL_OP_MAX,
> >  };
> >  
> > @@ -53,4 +55,17 @@ struct encl_op_get_from_addr {
> >  	uint64_t addr;
> >  };
> >  
> > +struct encl_op_eaccept {
> > +	struct encl_op_header header;
> > +	uint64_t epc_addr;
> > +	uint64_t flags;
> > +	uint64_t ret;
> > +};
> > +
> > +struct encl_op_emodpe {
> > +	struct encl_op_header header;
> > +	uint64_t epc_addr;
> > +	uint64_t flags;
> > +};
> > +
> >  #endif /* DEFINES_H */
> > diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c
> > index dd74fa42302e..0e0bd1c4d702 100644
> > --- a/tools/testing/selftests/sgx/main.c
> > +++ b/tools/testing/selftests/sgx/main.c
> > @@ -25,6 +25,18 @@ static const uint64_t MAGIC = 0x1122334455667788ULL;
> >  static const uint64_t MAGIC2 = 0x8877665544332211ULL;
> >  vdso_sgx_enter_enclave_t vdso_sgx_enter_enclave;
> >  
> > +/*
> > + * Security Information (SECINFO) data structure needed by a few SGX
> > + * instructions (eg. ENCLU[EACCEPT] and ENCLU[EMODPE]) holds meta-data
> > + * about an enclave page. &enum sgx_secinfo_page_state specifies the
> > + * secinfo flags used for page state.
> > + */
> > +enum sgx_secinfo_page_state {
> > +	SGX_SECINFO_PENDING = (1 << 3),
> > +	SGX_SECINFO_MODIFIED = (1 << 4),
> > +	SGX_SECINFO_PR = (1 << 5),
> > +};
> > +
> >  struct vdso_symtab {
> >  	Elf64_Sym *elf_symtab;
> >  	const char *elf_symstrtab;
> > @@ -555,4 +567,210 @@ TEST_F(enclave, pte_permissions)
> >  	EXPECT_EQ(self->run.exception_addr, 0);
> >  }
> >  
> > +/*
> > + * Enclave page permission test.
> > + *
> > + * Modify and restore enclave page's EPCM (enclave) permissions from
> > + * outside enclave (ENCLS[EMODPR] via kernel) as well as from within
> > + * enclave (via ENCLU[EMODPE]). Check for page fault if
> > + * VMA allows access but EPCM permissions do not.
> > + */
> > +TEST_F(enclave, epcm_permissions)
> > +{
> > +	struct sgx_enclave_restrict_permissions restrict_ioc;
> > +	struct encl_op_get_from_addr get_addr_op;
> > +	struct encl_op_put_to_addr put_addr_op;
> > +	struct encl_op_eaccept eaccept_op;
> > +	struct encl_op_emodpe emodpe_op;
> > +	struct sgx_secinfo secinfo;
> > +	unsigned long data_start;
> > +	int ret, errno_save;
> > +
> > +	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata));
> > +
> > +	memset(&self->run, 0, sizeof(self->run));
> > +	self->run.tcs = self->encl.encl_base;
> > +
> > +	/*
> > +	 * Ensure kernel supports needed ioctl() and system supports needed
> > +	 * commands.
> > +	 */
> > +	memset(&restrict_ioc, 0, sizeof(restrict_ioc));
> > +	memset(&secinfo, 0, sizeof(secinfo));
> > +
> > +	ret = ioctl(self->encl.fd, SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS,
> > +		    &restrict_ioc);
> > +	errno_save = ret == -1 ? errno : 0;
> > +
> > +	/*
> > +	 * Invalid parameters were provided during sanity check,
> > +	 * expect command to fail.
> > +	 */
> > +	ASSERT_EQ(ret, -1);
> > +
> > +	/* ret == -1 */
> > +	if (errno_save == ENOTTY)
> > +		SKIP(return,
> > +		     "Kernel does not support SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS ioctl()");
> > +	else if (errno_save == ENODEV)
> > +		SKIP(return, "System does not support SGX2");
> > +
> > +	/*
> > +	 * Page that will have its permissions changed is the second data
> > +	 * page in the .data segment. This forms part of the local encl_buffer
> > +	 * within the enclave.
> > +	 *
> > +	 * At start of test @data_start should have EPCM as well as PTE and
> > +	 * VMA permissions of RW.
> > +	 */
> > +
> > +	data_start = self->encl.encl_base +
> > +		     encl_get_data_offset(&self->encl) + PAGE_SIZE;
> > +
> > +	/*
> > +	 * Sanity check that page at @data_start is writable before making
> > +	 * any changes to page permissions.
> > +	 *
> > +	 * Start by writing MAGIC to test page.
> > +	 */
> > +	put_addr_op.value = MAGIC;
> > +	put_addr_op.addr = data_start;
> > +	put_addr_op.header.type = ENCL_OP_PUT_TO_ADDRESS;
> > +
> > +	EXPECT_EQ(ENCL_CALL(&put_addr_op, &self->run, true), 0);
> > +
> > +	EXPECT_EEXIT(&self->run);
> > +	EXPECT_EQ(self->run.exception_vector, 0);
> > +	EXPECT_EQ(self->run.exception_error_code, 0);
> > +	EXPECT_EQ(self->run.exception_addr, 0);
> > +
> > +	/*
> > +	 * Read memory that was just written to, confirming that
> > +	 * page is writable.
> > +	 */
> > +	get_addr_op.value = 0;
> > +	get_addr_op.addr = data_start;
> > +	get_addr_op.header.type = ENCL_OP_GET_FROM_ADDRESS;
> > +
> > +	EXPECT_EQ(ENCL_CALL(&get_addr_op, &self->run, true), 0);
> > +
> > +	EXPECT_EQ(get_addr_op.value, MAGIC);
> > +	EXPECT_EEXIT(&self->run);
> > +	EXPECT_EQ(self->run.exception_vector, 0);
> > +	EXPECT_EQ(self->run.exception_error_code, 0);
> > +	EXPECT_EQ(self->run.exception_addr, 0);
> > +
> > +	/*
> > +	 * Change EPCM permissions to read-only. Kernel still considers
> > +	 * the page writable.
> > +	 */
> > +	memset(&restrict_ioc, 0, sizeof(restrict_ioc));
> > +	memset(&secinfo, 0, sizeof(secinfo));
> > +
> > +	secinfo.flags = PROT_READ;
> > +	restrict_ioc.offset = encl_get_data_offset(&self->encl) + PAGE_SIZE;
> > +	restrict_ioc.length = PAGE_SIZE;
> > +	restrict_ioc.secinfo = (unsigned long)&secinfo;
> > +
> > +	ret = ioctl(self->encl.fd, SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS,
> > +		    &restrict_ioc);
> > +	errno_save = ret == -1 ? errno : 0;
> > +
> > +	EXPECT_EQ(ret, 0);
> > +	EXPECT_EQ(errno_save, 0);
> > +	EXPECT_EQ(restrict_ioc.result, 0);
> > +	EXPECT_EQ(restrict_ioc.count, 4096);
> > +
> > +	/*
> > +	 * EPCM permissions changed from kernel, need to EACCEPT from enclave.
> > +	 */
> > +	eaccept_op.epc_addr = data_start;
> > +	eaccept_op.flags = PROT_READ | SGX_SECINFO_REG | SGX_SECINFO_PR;
> > +	eaccept_op.ret = 0;
> > +	eaccept_op.header.type = ENCL_OP_EACCEPT;
> > +
> > +	EXPECT_EQ(ENCL_CALL(&eaccept_op, &self->run, true), 0);
> > +
> > +	EXPECT_EEXIT(&self->run);
> > +	EXPECT_EQ(self->run.exception_vector, 0);
> > +	EXPECT_EQ(self->run.exception_error_code, 0);
> > +	EXPECT_EQ(self->run.exception_addr, 0);
> > +	EXPECT_EQ(eaccept_op.ret, 0);
> > +
> > +	/*
> > +	 * EPCM permissions of page is now read-only, expect #PF
> > +	 * on EPCM when attempting to write to page from within enclave.
> > +	 */
> > +	put_addr_op.value = MAGIC2;
> > +
> > +	EXPECT_EQ(ENCL_CALL(&put_addr_op, &self->run, true), 0);
> > +
> > +	EXPECT_EQ(self->run.function, ERESUME);
> > +	EXPECT_EQ(self->run.exception_vector, 14);
> > +	EXPECT_EQ(self->run.exception_error_code, 0x8007);
> > +	EXPECT_EQ(self->run.exception_addr, data_start);
> > +
> > +	self->run.exception_vector = 0;
> > +	self->run.exception_error_code = 0;
> > +	self->run.exception_addr = 0;
> > +
> > +	/*
> > +	 * Received AEX but cannot return to enclave at same entrypoint,
> > +	 * need different TCS from where EPCM permission can be made writable
> > +	 * again.
> > +	 */
> > +	self->run.tcs = self->encl.encl_base + PAGE_SIZE;
> > +
> > +	/*
> > +	 * Enter enclave at new TCS to change EPCM permissions to be
> > +	 * writable again and thus fix the page fault that triggered the
> > +	 * AEX.
> > +	 */
> > +
> > +	emodpe_op.epc_addr = data_start;
> > +	emodpe_op.flags = PROT_READ | PROT_WRITE;
> > +	emodpe_op.header.type = ENCL_OP_EMODPE;
> > +
> > +	EXPECT_EQ(ENCL_CALL(&emodpe_op, &self->run, true), 0);
> > +
> > +	EXPECT_EEXIT(&self->run);
> > +	EXPECT_EQ(self->run.exception_vector, 0);
> > +	EXPECT_EQ(self->run.exception_error_code, 0);
> > +	EXPECT_EQ(self->run.exception_addr, 0);
> > +
> > +	/*
> > +	 * Attempt to return to main TCS to resume execution at faulting
> > +	 * instruction, PTE should continue to allow writing to the page.
> > +	 */
> > +	self->run.tcs = self->encl.encl_base;
> > +
> > +	/*
> > +	 * Wrong page permissions that caused original fault has
> > +	 * now been fixed via EPCM permissions.
> > +	 * Resume execution in main TCS to re-attempt the memory access.
> > +	 */
> > +	self->run.tcs = self->encl.encl_base;
> > +
> > +	EXPECT_EQ(vdso_sgx_enter_enclave((unsigned long)&put_addr_op, 0, 0,
> > +					 ERESUME, 0, 0,
> > +					 &self->run),
> > +		  0);
> > +
> > +	EXPECT_EEXIT(&self->run);
> > +	EXPECT_EQ(self->run.exception_vector, 0);
> > +	EXPECT_EQ(self->run.exception_error_code, 0);
> > +	EXPECT_EQ(self->run.exception_addr, 0);
> > +
> > +	get_addr_op.value = 0;
> > +
> > +	EXPECT_EQ(ENCL_CALL(&get_addr_op, &self->run, true), 0);
> > +
> > +	EXPECT_EQ(get_addr_op.value, MAGIC2);
> > +	EXPECT_EEXIT(&self->run);
> > +	EXPECT_EQ(self->run.user_data, 0);
> > +	EXPECT_EQ(self->run.exception_vector, 0);
> > +	EXPECT_EQ(self->run.exception_error_code, 0);
> > +	EXPECT_EQ(self->run.exception_addr, 0);
> > +}
> > +
> >  TEST_HARNESS_MAIN
> > diff --git a/tools/testing/selftests/sgx/test_encl.c b/tools/testing/selftests/sgx/test_encl.c
> > index 4fca01cfd898..5b6c65331527 100644
> > --- a/tools/testing/selftests/sgx/test_encl.c
> > +++ b/tools/testing/selftests/sgx/test_encl.c
> > @@ -11,6 +11,42 @@
> >   */
> >  static uint8_t encl_buffer[8192] = { 1 };
> >  
> > +enum sgx_enclu_function {
> > +	EACCEPT = 0x5,
> > +	EMODPE = 0x6,
> > +};
> > +
> > +static void do_encl_emodpe(void *_op)
> > +{
> > +	struct sgx_secinfo secinfo __aligned(sizeof(struct sgx_secinfo)) = {0};
> > +	struct encl_op_emodpe *op = _op;
> > +
> > +	secinfo.flags = op->flags;
> > +
> > +	asm volatile(".byte 0x0f, 0x01, 0xd7"
> > +				:
> > +				: "a" (EMODPE),
> > +				  "b" (&secinfo),
> > +				  "c" (op->epc_addr));
> > +}
> > +
> > +static void do_encl_eaccept(void *_op)
> > +{
> > +	struct sgx_secinfo secinfo __aligned(sizeof(struct sgx_secinfo)) = {0};
> > +	struct encl_op_eaccept *op = _op;
> > +	int rax;
> > +
> > +	secinfo.flags = op->flags;
> > +
> > +	asm volatile(".byte 0x0f, 0x01, 0xd7"
> > +				: "=a" (rax)
> > +				: "a" (EACCEPT),
> > +				  "b" (&secinfo),
> > +				  "c" (op->epc_addr));
> > +
> > +	op->ret = rax;
> > +}
> > +
> >  static void *memcpy(void *dest, const void *src, size_t n)
> >  {
> >  	size_t i;
> > @@ -62,6 +98,8 @@ void encl_body(void *rdi,  void *rsi)
> >  		do_encl_op_put_to_addr,
> >  		do_encl_op_get_from_addr,
> >  		do_encl_op_nop,
> > +		do_encl_eaccept,
> > +		do_encl_emodpe,
> >  	};
> >  
> >  	struct encl_op_header *op = (struct encl_op_header *)rdi;
> > -- 
> > 2.25.1
> > 
> 
> Lacking:
> 
> KERNEL SELFTEST FRAMEWORK
> M:	Shuah Khan <shuah@kernel.org>
> M:	Shuah Khan <skhan@linuxfoundation.org>
> L:	linux-kselftest@vger.kernel.org
> S:	Maintained
> Q:	https://patchwork.kernel.org/project/linux-kselftest/list/
> T:	git git://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git
> F:	Documentation/dev-tools/kselftest*
> F:	tools/testing/selftests/
> 
> BR, Jarkko

Anyway, you can put to all selftests:

Acked-by: Jarkko Sakkinen <jarkko@kernel.org>

BR, Jarkko