All of lore.kernel.org
 help / color / mirror / Atom feed
From: Dave Hansen <dave.hansen@linux.intel.com>
To: linux-mm@kvack.org
Cc: linux-kernel@vger.kernel.org,
	Dave Hansen <dave.hansen@linux.intel.com>,
	tim.gardner@canonical.com, jarkko@kernel.org,
	reinette.chatre@intel.com, shuah@kernel.org,
	linux-sgx@vger.kernel.org, linux-kselftest@vger.kernel.org
Subject: [PATCH] selftests/sgx: remove checks for file execute permissions
Date: Mon, 21 Jun 2021 12:05:56 -0700	[thread overview]
Message-ID: <20210621190556.4B5DCBB1@viggo.jf.intel.com> (raw)


The SGX selftests can fail for a bunch of non-obvious reasons
like 'noexec' permissions on /dev (which is the default *EVERYWHERE*
it seems).

A new test mistakenly also looked for +x permission on the
/dev/sgx_enclave.  File execute permissions really only apply to
the ability of execve() to work on a file, *NOT* on the ability
for an application to map the file with PROT_EXEC.  SGX needs to
mmap(PROT_EXEC), but doesn't need to execve() the device file.

Remove the check.

Fixes: 4284f7acb78b ("selftests/sgx: Improve error detection and messages")
Reported-by: Tim Gardner <tim.gardner@canonical.com>
Cc: Jarkko Sakkinen <jarkko@kernel.org>
Cc: Reinette Chatre <reinette.chatre@intel.com>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Shuah Khan <shuah@kernel.org>
Cc: linux-sgx@vger.kernel.org
Cc: linux-kselftest@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

---

 b/tools/testing/selftests/sgx/load.c |   16 +++-------------
 1 file changed, 3 insertions(+), 13 deletions(-)

diff -puN tools/testing/selftests/sgx/load.c~sgx-no-file-exec tools/testing/selftests/sgx/load.c
--- a/tools/testing/selftests/sgx/load.c~sgx-no-file-exec	2021-06-21 11:48:25.226294281 -0700
+++ b/tools/testing/selftests/sgx/load.c	2021-06-21 12:03:28.023292029 -0700
@@ -150,16 +150,6 @@ bool encl_load(const char *path, struct
 		goto err;
 	}
 
-	/*
-	 * This just checks if the /dev file has these permission
-	 * bits set.  It does not check that the current user is
-	 * the owner or in the owning group.
-	 */
-	if (!(sb.st_mode & (S_IXUSR | S_IXGRP | S_IXOTH))) {
-		fprintf(stderr, "no execute permissions on device file %s\n", device_path);
-		goto err;
-	}
-
 	ptr = mmap(NULL, PAGE_SIZE, PROT_READ, MAP_SHARED, fd, 0);
 	if (ptr == (void *)-1) {
 		perror("mmap for read");
@@ -169,13 +159,13 @@ bool encl_load(const char *path, struct
 
 #define ERR_MSG \
 "mmap() succeeded for PROT_READ, but failed for PROT_EXEC.\n" \
-" Check that current user has execute permissions on %s and \n" \
-" that /dev does not have noexec set: mount | grep \"/dev .*noexec\"\n" \
+" Check that /dev does not have noexec set:\n" \
+" \tmount | grep \"/dev .*noexec\"\n" \
 " If so, remount it executable: mount -o remount,exec /dev\n\n"
 
 	ptr = mmap(NULL, PAGE_SIZE, PROT_EXEC, MAP_SHARED, fd, 0);
 	if (ptr == (void *)-1) {
-		fprintf(stderr, ERR_MSG, device_path);
+		fprintf(stderr, ERR_MSG);
 		goto err;
 	}
 	munmap(ptr, PAGE_SIZE);
_

WARNING: multiple messages have this Message-ID (diff)
From: Dave Hansen <dave.hansen@linux.intel.com>
To: linux-mm@kvack.org
Cc: linux-kernel@vger.kernel.org,Dave Hansen
	<dave.hansen@linux.intel.com>,tim.gardner@canonical.com,jarkko@kernel.org,reinette.chatre@intel.com,shuah@kernel.org,linux-sgx@vger.kernel.org,linux-kselftest@vger.kernel.org
Subject: [PATCH] selftests/sgx: remove checks for file execute permissions
Date: Mon, 21 Jun 2021 12:05:56 -0700	[thread overview]
Message-ID: <20210621190556.4B5DCBB1@viggo.jf.intel.com> (raw)


The SGX selftests can fail for a bunch of non-obvious reasons
like 'noexec' permissions on /dev (which is the default *EVERYWHERE*
it seems).

A new test mistakenly also looked for +x permission on the
/dev/sgx_enclave.  File execute permissions really only apply to
the ability of execve() to work on a file, *NOT* on the ability
for an application to map the file with PROT_EXEC.  SGX needs to
mmap(PROT_EXEC), but doesn't need to execve() the device file.

Remove the check.

Fixes: 4284f7acb78b ("selftests/sgx: Improve error detection and messages")
Reported-by: Tim Gardner <tim.gardner@canonical.com>
Cc: Jarkko Sakkinen <jarkko@kernel.org>
Cc: Reinette Chatre <reinette.chatre@intel.com>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Shuah Khan <shuah@kernel.org>
Cc: linux-sgx@vger.kernel.org
Cc: linux-kselftest@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

---

 b/tools/testing/selftests/sgx/load.c |   16 +++-------------
 1 file changed, 3 insertions(+), 13 deletions(-)

diff -puN tools/testing/selftests/sgx/load.c~sgx-no-file-exec tools/testing/selftests/sgx/load.c
--- a/tools/testing/selftests/sgx/load.c~sgx-no-file-exec	2021-06-21 11:48:25.226294281 -0700
+++ b/tools/testing/selftests/sgx/load.c	2021-06-21 12:03:28.023292029 -0700
@@ -150,16 +150,6 @@ bool encl_load(const char *path, struct
 		goto err;
 	}
 
-	/*
-	 * This just checks if the /dev file has these permission
-	 * bits set.  It does not check that the current user is
-	 * the owner or in the owning group.
-	 */
-	if (!(sb.st_mode & (S_IXUSR | S_IXGRP | S_IXOTH))) {
-		fprintf(stderr, "no execute permissions on device file %s\n", device_path);
-		goto err;
-	}
-
 	ptr = mmap(NULL, PAGE_SIZE, PROT_READ, MAP_SHARED, fd, 0);
 	if (ptr == (void *)-1) {
 		perror("mmap for read");
@@ -169,13 +159,13 @@ bool encl_load(const char *path, struct
 
 #define ERR_MSG \
 "mmap() succeeded for PROT_READ, but failed for PROT_EXEC.\n" \
-" Check that current user has execute permissions on %s and \n" \
-" that /dev does not have noexec set: mount | grep \"/dev .*noexec\"\n" \
+" Check that /dev does not have noexec set:\n" \
+" \tmount | grep \"/dev .*noexec\"\n" \
 " If so, remount it executable: mount -o remount,exec /dev\n\n"
 
 	ptr = mmap(NULL, PAGE_SIZE, PROT_EXEC, MAP_SHARED, fd, 0);
 	if (ptr == (void *)-1) {
-		fprintf(stderr, ERR_MSG, device_path);
+		fprintf(stderr, ERR_MSG);
 		goto err;
 	}
 	munmap(ptr, PAGE_SIZE);
_


             reply	other threads:[~2021-06-21 19:06 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-21 19:05 Dave Hansen [this message]
2021-06-21 19:05 ` [PATCH] selftests/sgx: remove checks for file execute permissions Dave Hansen
2021-06-21 21:08 ` Reinette Chatre
2021-06-21 21:18   ` Dave Hansen
2021-06-24  0:40     ` Shuah Khan
2021-06-23 13:41 ` 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=20210621190556.4B5DCBB1@viggo.jf.intel.com \
    --to=dave.hansen@linux.intel.com \
    --cc=jarkko@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=linux-sgx@vger.kernel.org \
    --cc=reinette.chatre@intel.com \
    --cc=shuah@kernel.org \
    --cc=tim.gardner@canonical.com \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.