* Re: [PATCH] selftests/sgx: improve error detection and messages
2021-03-18 19:43 [PATCH] selftests/sgx: improve error detection and messages Dave Hansen
@ 2021-03-19 5:37 ` Jarkko Sakkinen
2021-03-19 11:38 ` [tip: x86/sgx] selftests/sgx: Improve " tip-bot2 for Dave Hansen
2021-03-19 18:45 ` tip-bot2 for Dave Hansen
2 siblings, 0 replies; 7+ messages in thread
From: Jarkko Sakkinen @ 2021-03-19 5:37 UTC (permalink / raw)
To: Dave Hansen; +Cc: linux-kernel, shuah, bp, x86, linux-sgx, linux-kselftest
On Thu, Mar 18, 2021 at 12:43:01PM -0700, Dave Hansen wrote:
>
> From: Dave Hansen <dave.hansen@linux.intel.com>
>
> The SGX device file (/dev/sgx_enclave) is unusual in that it requires
> execute permissions. It has to be both "chmod +x" *and* be on a
> filesystem without 'noexec'.
>
> In the future, udev and systemd should get updates to set up systems
> automatically. But, for now, nobody's systems do this automatically,
> and everybody gets error messages like this when running ./test_sgx:
>
> 0x0000000000000000 0x0000000000002000 0x03
> 0x0000000000002000 0x0000000000001000 0x05
> 0x0000000000003000 0x0000000000003000 0x03
> mmap() failed, errno=1.
>
> That isn't very user friendly, even for forgetful kernel developers.
>
> Further, the test case is rather haphazard about its use of fprintf()
> versus perror().
>
> Improve the error messages. Use perror() where possible. Lastly,
> do some sanity checks on opening and mmap()ing the device file so
> that we can get a decent error message out to the user.
>
> Now, if your user doesn't have permission, you'll get the following:
>
> $ ls -l /dev/sgx_enclave
> crw------- 1 root root 10, 126 Mar 18 11:29 /dev/sgx_enclave
> $ ./test_sgx
> Unable to open /dev/sgx_enclave: Permission denied
>
> If you then 'chown dave:dave /dev/sgx_enclave' (or whatever), but
> you leave execute permissions off, you'll get:
>
> $ ls -l /dev/sgx_enclave
> crw------- 1 dave dave 10, 126 Mar 18 11:29 /dev/sgx_enclave
> $ ./test_sgx
> no execute permissions on device file
>
> If you fix that with "chmod ug+x /dev/sgx" but you leave /dev as
> noexec, you'll get this:
>
> $ mount | grep "/dev .*noexec"
> udev on /dev type devtmpfs (rw,nosuid,noexec,...)
> $ ./test_sgx
> ERROR: mmap for exec: Operation not permitted
> mmap() succeeded for PROT_READ, but failed for PROT_EXEC
> check that user has execute permissions on /dev/sgx_enclave and
> that /dev does not have noexec set: 'mount | grep "/dev .*noexec"'
>
> That can be fixed with:
>
> mount -o remount,noexec /devESC
>
> Hopefully, the combination of better error messages and the search
> engines indexing this message will help people fix their systems
> until we do this properly.
>
> Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: Jarkko Sakkinen <jarkko@kernel.org>
> Cc: Shuah Khan <shuah@kernel.org>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: x86@kernel.org
> Cc: linux-sgx@vger.kernel.org
> Cc: linux-kselftest@vger.kernel.org
Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
> ---
>
> b/tools/testing/selftests/sgx/load.c | 66 +++++++++++++++++++++++++++--------
> b/tools/testing/selftests/sgx/main.c | 2 -
> 2 files changed, 53 insertions(+), 15 deletions(-)
>
> diff -puN tools/testing/selftests/sgx/load.c~sgx-selftest-err-rework tools/testing/selftests/sgx/load.c
> --- a/tools/testing/selftests/sgx/load.c~sgx-selftest-err-rework 2021-03-18 12:18:38.649828215 -0700
> +++ b/tools/testing/selftests/sgx/load.c 2021-03-18 12:40:46.388824904 -0700
> @@ -45,19 +45,19 @@ static bool encl_map_bin(const char *pat
>
> fd = open(path, O_RDONLY);
> if (fd == -1) {
> - perror("open()");
> + perror("enclave executable open()");
> return false;
> }
>
> ret = stat(path, &sb);
> if (ret) {
> - perror("stat()");
> + perror("enclave executable stat()");
> goto err;
> }
>
> bin = mmap(NULL, sb.st_size, PROT_READ, MAP_PRIVATE, fd, 0);
> if (bin == MAP_FAILED) {
> - perror("mmap()");
> + perror("enclave executable mmap()");
> goto err;
> }
>
> @@ -90,8 +90,7 @@ static bool encl_ioc_create(struct encl
> ioc.src = (unsigned long)secs;
> rc = ioctl(encl->fd, SGX_IOC_ENCLAVE_CREATE, &ioc);
> if (rc) {
> - fprintf(stderr, "SGX_IOC_ENCLAVE_CREATE failed: errno=%d\n",
> - errno);
> + perror("SGX_IOC_ENCLAVE_CREATE failed");
> munmap((void *)secs->base, encl->encl_size);
> return false;
> }
> @@ -116,31 +115,69 @@ static bool encl_ioc_add_pages(struct en
>
> rc = ioctl(encl->fd, SGX_IOC_ENCLAVE_ADD_PAGES, &ioc);
> if (rc < 0) {
> - fprintf(stderr, "SGX_IOC_ENCLAVE_ADD_PAGES failed: errno=%d.\n",
> - errno);
> + perror("SGX_IOC_ENCLAVE_ADD_PAGES failed");
> return false;
> }
>
> return true;
> }
>
> +
> +
> bool encl_load(const char *path, struct encl *encl)
> {
> + const char device_path[] = "/dev/sgx_enclave";
> Elf64_Phdr *phdr_tbl;
> off_t src_offset;
> Elf64_Ehdr *ehdr;
> + struct stat sb;
> + void *ptr;
> int i, j;
> int ret;
> + int fd = -1;
>
> memset(encl, 0, sizeof(*encl));
>
> - ret = open("/dev/sgx_enclave", O_RDWR);
> - if (ret < 0) {
> - fprintf(stderr, "Unable to open /dev/sgx_enclave\n");
> + fd = open(device_path, O_RDWR);
> + if (fd < 0) {
> + perror("Unable to open /dev/sgx_enclave");
> + goto err;
> + }
> +
> + ret = stat(device_path, &sb);
> + if (ret) {
> + perror("device file stat()");
> + 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\n");
> + goto err;
> + }
> +
> + ptr = mmap(NULL, PAGE_SIZE, PROT_READ, MAP_SHARED, fd, 0);
> + if (ptr == (void *)-1) {
> + perror("mmap for read");
> + goto err;
> + }
> + munmap(ptr, PAGE_SIZE);
> +
> + ptr = mmap(NULL, PAGE_SIZE, PROT_EXEC, MAP_SHARED, fd, 0);
> + if (ptr == (void *)-1) {
> + perror("ERROR: mmap for exec");
> + fprintf(stderr, "mmap() succeeded for PROT_READ, but failed for PROT_EXEC\n");
> + fprintf(stderr, "check that user has execute permissions on %s and\n", device_path);
> + fprintf(stderr, "that /dev does not have noexec set: 'mount | grep \"/dev .*noexec\"'\n");
> goto err;
> }
> + munmap(ptr, PAGE_SIZE);
>
> - encl->fd = ret;
> + encl->fd = fd;
>
> if (!encl_map_bin(path, encl))
> goto err;
> @@ -217,6 +254,8 @@ bool encl_load(const char *path, struct
> return true;
>
> err:
> + if (fd != -1)
> + close(fd);
> encl_delete(encl);
> return false;
> }
> @@ -229,7 +268,7 @@ static bool encl_map_area(struct encl *e
> area = mmap(NULL, encl_size * 2, PROT_NONE,
> MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
> if (area == MAP_FAILED) {
> - perror("mmap");
> + perror("reservation mmap()");
> return false;
> }
>
> @@ -268,8 +307,7 @@ bool encl_build(struct encl *encl)
> ioc.sigstruct = (uint64_t)&encl->sigstruct;
> ret = ioctl(encl->fd, SGX_IOC_ENCLAVE_INIT, &ioc);
> if (ret) {
> - fprintf(stderr, "SGX_IOC_ENCLAVE_INIT failed: errno=%d\n",
> - errno);
> + perror("SGX_IOC_ENCLAVE_INIT failed");
> return false;
> }
>
> diff -puN tools/testing/selftests/sgx/main.c~sgx-selftest-err-rework tools/testing/selftests/sgx/main.c
> --- a/tools/testing/selftests/sgx/main.c~sgx-selftest-err-rework 2021-03-18 12:18:38.652828215 -0700
> +++ b/tools/testing/selftests/sgx/main.c 2021-03-18 12:18:38.657828215 -0700
> @@ -195,7 +195,7 @@ int main(int argc, char *argv[], char *e
> addr = mmap((void *)encl.encl_base + seg->offset, seg->size,
> seg->prot, MAP_SHARED | MAP_FIXED, encl.fd, 0);
> if (addr == MAP_FAILED) {
> - fprintf(stderr, "mmap() failed, errno=%d.\n", errno);
> + perror("mmap() segment failed");
> exit(KSFT_FAIL);
> }
> }
> _
>
^ permalink raw reply [flat|nested] 7+ messages in thread* [tip: x86/sgx] selftests/sgx: Improve error detection and messages
2021-03-18 19:43 [PATCH] selftests/sgx: improve error detection and messages Dave Hansen
2021-03-19 5:37 ` Jarkko Sakkinen
@ 2021-03-19 11:38 ` tip-bot2 for Dave Hansen
2021-03-19 14:58 ` Borislav Petkov
2021-03-19 18:45 ` tip-bot2 for Dave Hansen
2 siblings, 1 reply; 7+ messages in thread
From: tip-bot2 for Dave Hansen @ 2021-03-19 11:38 UTC (permalink / raw)
To: linux-tip-commits
Cc: Dave Hansen, Ingo Molnar, Jarkko Sakkinen, x86, linux-kernel
The following commit has been merged into the x86/sgx branch of tip:
Commit-ID: 79713a1fa1b9cd9d650b1ff0657ddbadc5dbbeaa
Gitweb: https://git.kernel.org/tip/79713a1fa1b9cd9d650b1ff0657ddbadc5dbbeaa
Author: Dave Hansen <dave.hansen@linux.intel.com>
AuthorDate: Thu, 18 Mar 2021 12:43:01 -07:00
Committer: Ingo Molnar <mingo@kernel.org>
CommitterDate: Fri, 19 Mar 2021 12:18:21 +01:00
selftests/sgx: Improve error detection and messages
The SGX device file (/dev/sgx_enclave) is unusual in that it requires
execute permissions. It has to be both "chmod +x" *and* be on a
filesystem without 'noexec'.
In the future, udev and systemd should get updates to set up systems
automatically. But, for now, nobody's systems do this automatically,
and everybody gets error messages like this when running ./test_sgx:
0x0000000000000000 0x0000000000002000 0x03
0x0000000000002000 0x0000000000001000 0x05
0x0000000000003000 0x0000000000003000 0x03
mmap() failed, errno=1.
That isn't very user friendly, even for forgetful kernel developers.
Further, the test case is rather haphazard about its use of fprintf()
versus perror().
Improve the error messages. Use perror() where possible. Lastly,
do some sanity checks on opening and mmap()ing the device file so
that we can get a decent error message out to the user.
Now, if your user doesn't have permission, you'll get the following:
$ ls -l /dev/sgx_enclave
crw------- 1 root root 10, 126 Mar 18 11:29 /dev/sgx_enclave
$ ./test_sgx
Unable to open /dev/sgx_enclave: Permission denied
If you then 'chown dave:dave /dev/sgx_enclave' (or whatever), but
you leave execute permissions off, you'll get:
$ ls -l /dev/sgx_enclave
crw------- 1 dave dave 10, 126 Mar 18 11:29 /dev/sgx_enclave
$ ./test_sgx
no execute permissions on device file
If you fix that with "chmod ug+x /dev/sgx" but you leave /dev as
noexec, you'll get this:
$ mount | grep "/dev .*noexec"
udev on /dev type devtmpfs (rw,nosuid,noexec,...)
$ ./test_sgx
ERROR: mmap for exec: Operation not permitted
mmap() succeeded for PROT_READ, but failed for PROT_EXEC
check that user has execute permissions on /dev/sgx_enclave and
that /dev does not have noexec set: 'mount | grep "/dev .*noexec"'
That can be fixed with:
mount -o remount,noexec /devESC
Hopefully, the combination of better error messages and the search
engines indexing this message will help people fix their systems
until we do this properly.
Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
Link: https://lore.kernel.org/r/20210318194301.11D9A984@viggo.jf.intel.com
---
tools/testing/selftests/sgx/load.c | 66 ++++++++++++++++++++++-------
tools/testing/selftests/sgx/main.c | 2 +-
2 files changed, 53 insertions(+), 15 deletions(-)
diff --git a/tools/testing/selftests/sgx/load.c b/tools/testing/selftests/sgx/load.c
index 9d43b75..4c149f4 100644
--- a/tools/testing/selftests/sgx/load.c
+++ b/tools/testing/selftests/sgx/load.c
@@ -45,19 +45,19 @@ static bool encl_map_bin(const char *path, struct encl *encl)
fd = open(path, O_RDONLY);
if (fd == -1) {
- perror("open()");
+ perror("enclave executable open()");
return false;
}
ret = stat(path, &sb);
if (ret) {
- perror("stat()");
+ perror("enclave executable stat()");
goto err;
}
bin = mmap(NULL, sb.st_size, PROT_READ, MAP_PRIVATE, fd, 0);
if (bin == MAP_FAILED) {
- perror("mmap()");
+ perror("enclave executable mmap()");
goto err;
}
@@ -90,8 +90,7 @@ static bool encl_ioc_create(struct encl *encl)
ioc.src = (unsigned long)secs;
rc = ioctl(encl->fd, SGX_IOC_ENCLAVE_CREATE, &ioc);
if (rc) {
- fprintf(stderr, "SGX_IOC_ENCLAVE_CREATE failed: errno=%d\n",
- errno);
+ perror("SGX_IOC_ENCLAVE_CREATE failed");
munmap((void *)secs->base, encl->encl_size);
return false;
}
@@ -116,31 +115,69 @@ static bool encl_ioc_add_pages(struct encl *encl, struct encl_segment *seg)
rc = ioctl(encl->fd, SGX_IOC_ENCLAVE_ADD_PAGES, &ioc);
if (rc < 0) {
- fprintf(stderr, "SGX_IOC_ENCLAVE_ADD_PAGES failed: errno=%d.\n",
- errno);
+ perror("SGX_IOC_ENCLAVE_ADD_PAGES failed");
return false;
}
return true;
}
+
+
bool encl_load(const char *path, struct encl *encl)
{
+ const char device_path[] = "/dev/sgx_enclave";
Elf64_Phdr *phdr_tbl;
off_t src_offset;
Elf64_Ehdr *ehdr;
+ struct stat sb;
+ void *ptr;
int i, j;
int ret;
+ int fd = -1;
memset(encl, 0, sizeof(*encl));
- ret = open("/dev/sgx_enclave", O_RDWR);
- if (ret < 0) {
- fprintf(stderr, "Unable to open /dev/sgx_enclave\n");
+ fd = open(device_path, O_RDWR);
+ if (fd < 0) {
+ perror("Unable to open /dev/sgx_enclave");
+ goto err;
+ }
+
+ ret = stat(device_path, &sb);
+ if (ret) {
+ perror("device file stat()");
+ 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\n");
+ goto err;
+ }
+
+ ptr = mmap(NULL, PAGE_SIZE, PROT_READ, MAP_SHARED, fd, 0);
+ if (ptr == (void *)-1) {
+ perror("mmap for read");
+ goto err;
+ }
+ munmap(ptr, PAGE_SIZE);
+
+ ptr = mmap(NULL, PAGE_SIZE, PROT_EXEC, MAP_SHARED, fd, 0);
+ if (ptr == (void *)-1) {
+ perror("ERROR: mmap for exec");
+ fprintf(stderr, "mmap() succeeded for PROT_READ, but failed for PROT_EXEC\n");
+ fprintf(stderr, "check that user has execute permissions on %s and\n", device_path);
+ fprintf(stderr, "that /dev does not have noexec set: 'mount | grep \"/dev .*noexec\"'\n");
goto err;
}
+ munmap(ptr, PAGE_SIZE);
- encl->fd = ret;
+ encl->fd = fd;
if (!encl_map_bin(path, encl))
goto err;
@@ -217,6 +254,8 @@ bool encl_load(const char *path, struct encl *encl)
return true;
err:
+ if (fd != -1)
+ close(fd);
encl_delete(encl);
return false;
}
@@ -229,7 +268,7 @@ static bool encl_map_area(struct encl *encl)
area = mmap(NULL, encl_size * 2, PROT_NONE,
MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
if (area == MAP_FAILED) {
- perror("mmap");
+ perror("reservation mmap()");
return false;
}
@@ -268,8 +307,7 @@ bool encl_build(struct encl *encl)
ioc.sigstruct = (uint64_t)&encl->sigstruct;
ret = ioctl(encl->fd, SGX_IOC_ENCLAVE_INIT, &ioc);
if (ret) {
- fprintf(stderr, "SGX_IOC_ENCLAVE_INIT failed: errno=%d\n",
- errno);
+ perror("SGX_IOC_ENCLAVE_INIT failed");
return false;
}
diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c
index 724cec7..b117bb8 100644
--- a/tools/testing/selftests/sgx/main.c
+++ b/tools/testing/selftests/sgx/main.c
@@ -195,7 +195,7 @@ int main(int argc, char *argv[], char *envp[])
addr = mmap((void *)encl.encl_base + seg->offset, seg->size,
seg->prot, MAP_SHARED | MAP_FIXED, encl.fd, 0);
if (addr == MAP_FAILED) {
- fprintf(stderr, "mmap() failed, errno=%d.\n", errno);
+ perror("mmap() segment failed");
exit(KSFT_FAIL);
}
}
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [tip: x86/sgx] selftests/sgx: Improve error detection and messages
2021-03-19 11:38 ` [tip: x86/sgx] selftests/sgx: Improve " tip-bot2 for Dave Hansen
@ 2021-03-19 14:58 ` Borislav Petkov
2021-03-19 15:20 ` Jarkko Sakkinen
2021-03-19 15:33 ` Dave Hansen
0 siblings, 2 replies; 7+ messages in thread
From: Borislav Petkov @ 2021-03-19 14:58 UTC (permalink / raw)
To: Dave Hansen
Cc: linux-kernel, linux-tip-commits, Ingo Molnar, Jarkko Sakkinen, x86
On Fri, Mar 19, 2021 at 11:38:44AM -0000, tip-bot2 for Dave Hansen wrote:
> tools/testing/selftests/sgx/load.c | 66 ++++++++++++++++++++++-------
> tools/testing/selftests/sgx/main.c | 2 +-
> 2 files changed, 53 insertions(+), 15 deletions(-)
Anything against some more tweaks ontop?
---
diff --git a/tools/testing/selftests/sgx/load.c b/tools/testing/selftests/sgx/load.c
index 4c149f46d798..f441ac34b4d4 100644
--- a/tools/testing/selftests/sgx/load.c
+++ b/tools/testing/selftests/sgx/load.c
@@ -156,7 +156,7 @@ bool encl_load(const char *path, struct encl *encl)
* 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\n");
+ fprintf(stderr, "no execute permissions on device file %s\n", device_path);
goto err;
}
@@ -167,12 +167,15 @@ bool encl_load(const char *path, struct encl *encl)
}
munmap(ptr, PAGE_SIZE);
+#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" \
+" 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) {
- perror("ERROR: mmap for exec");
- fprintf(stderr, "mmap() succeeded for PROT_READ, but failed for PROT_EXEC\n");
- fprintf(stderr, "check that user has execute permissions on %s and\n", device_path);
- fprintf(stderr, "that /dev does not have noexec set: 'mount | grep \"/dev .*noexec\"'\n");
+ fprintf(stderr, ERR_MSG, device_path);
goto err;
}
munmap(ptr, PAGE_SIZE);
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [tip: x86/sgx] selftests/sgx: Improve error detection and messages
2021-03-19 14:58 ` Borislav Petkov
@ 2021-03-19 15:20 ` Jarkko Sakkinen
2021-03-19 15:33 ` Dave Hansen
1 sibling, 0 replies; 7+ messages in thread
From: Jarkko Sakkinen @ 2021-03-19 15:20 UTC (permalink / raw)
To: Borislav Petkov
Cc: Dave Hansen, linux-kernel, linux-tip-commits, Ingo Molnar, x86
On Fri, Mar 19, 2021 at 03:58:07PM +0100, Borislav Petkov wrote:
> On Fri, Mar 19, 2021 at 11:38:44AM -0000, tip-bot2 for Dave Hansen wrote:
> > tools/testing/selftests/sgx/load.c | 66 ++++++++++++++++++++++-------
> > tools/testing/selftests/sgx/main.c | 2 +-
> > 2 files changed, 53 insertions(+), 15 deletions(-)
>
> Anything against some more tweaks ontop?
Nope :-)
> ---
> diff --git a/tools/testing/selftests/sgx/load.c b/tools/testing/selftests/sgx/load.c
> index 4c149f46d798..f441ac34b4d4 100644
> --- a/tools/testing/selftests/sgx/load.c
> +++ b/tools/testing/selftests/sgx/load.c
> @@ -156,7 +156,7 @@ bool encl_load(const char *path, struct encl *encl)
> * 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\n");
> + fprintf(stderr, "no execute permissions on device file %s\n", device_path);
> goto err;
> }
>
> @@ -167,12 +167,15 @@ bool encl_load(const char *path, struct encl *encl)
> }
> munmap(ptr, PAGE_SIZE);
>
> +#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" \
> +" 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) {
> - perror("ERROR: mmap for exec");
> - fprintf(stderr, "mmap() succeeded for PROT_READ, but failed for PROT_EXEC\n");
> - fprintf(stderr, "check that user has execute permissions on %s and\n", device_path);
> - fprintf(stderr, "that /dev does not have noexec set: 'mount | grep \"/dev .*noexec\"'\n");
> + fprintf(stderr, ERR_MSG, device_path);
> goto err;
> }
> munmap(ptr, PAGE_SIZE);
>
>
> --
> Regards/Gruss,
> Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette
>
Printing device path is a good sanity thing to have, thanks.
/Jarkko
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [tip: x86/sgx] selftests/sgx: Improve error detection and messages
2021-03-19 14:58 ` Borislav Petkov
2021-03-19 15:20 ` Jarkko Sakkinen
@ 2021-03-19 15:33 ` Dave Hansen
1 sibling, 0 replies; 7+ messages in thread
From: Dave Hansen @ 2021-03-19 15:33 UTC (permalink / raw)
To: Borislav Petkov, Dave Hansen
Cc: linux-kernel, linux-tip-commits, Ingo Molnar, Jarkko Sakkinen, x86
On 3/19/21 7:58 AM, Borislav Petkov wrote:
> On Fri, Mar 19, 2021 at 11:38:44AM -0000, tip-bot2 for Dave Hansen wrote:
>> tools/testing/selftests/sgx/load.c | 66 ++++++++++++++++++++++-------
>> tools/testing/selftests/sgx/main.c | 2 +-
>> 2 files changed, 53 insertions(+), 15 deletions(-)
> Anything against some more tweaks ontop?
Looks good to me.
^ permalink raw reply [flat|nested] 7+ messages in thread
* [tip: x86/sgx] selftests/sgx: Improve error detection and messages
2021-03-18 19:43 [PATCH] selftests/sgx: improve error detection and messages Dave Hansen
2021-03-19 5:37 ` Jarkko Sakkinen
2021-03-19 11:38 ` [tip: x86/sgx] selftests/sgx: Improve " tip-bot2 for Dave Hansen
@ 2021-03-19 18:45 ` tip-bot2 for Dave Hansen
2 siblings, 0 replies; 7+ messages in thread
From: tip-bot2 for Dave Hansen @ 2021-03-19 18:45 UTC (permalink / raw)
To: linux-tip-commits
Cc: Dave Hansen, Ingo Molnar, Borislav Petkov, Jarkko Sakkinen, x86,
linux-kernel
The following commit has been merged into the x86/sgx branch of tip:
Commit-ID: 4284f7acb78bfb0e0c26a2b78e2b2c3d68fccd6f
Gitweb: https://git.kernel.org/tip/4284f7acb78bfb0e0c26a2b78e2b2c3d68fccd6f
Author: Dave Hansen <dave.hansen@linux.intel.com>
AuthorDate: Thu, 18 Mar 2021 12:43:01 -07:00
Committer: Borislav Petkov <bp@suse.de>
CommitterDate: Fri, 19 Mar 2021 19:23:41 +01:00
selftests/sgx: Improve error detection and messages
The SGX device file (/dev/sgx_enclave) is unusual in that it requires
execute permissions. It has to be both "chmod +x" *and* be on a
filesystem without 'noexec'.
In the future, udev and systemd should get updates to set up systems
automatically. But, for now, nobody's systems do this automatically,
and everybody gets error messages like this when running ./test_sgx:
0x0000000000000000 0x0000000000002000 0x03
0x0000000000002000 0x0000000000001000 0x05
0x0000000000003000 0x0000000000003000 0x03
mmap() failed, errno=1.
That isn't very user friendly, even for forgetful kernel developers.
Further, the test case is rather haphazard about its use of fprintf()
versus perror().
Improve the error messages. Use perror() where possible. Lastly,
do some sanity checks on opening and mmap()ing the device file so
that we can get a decent error message out to the user.
Now, if your user doesn't have permission, you'll get the following:
$ ls -l /dev/sgx_enclave
crw------- 1 root root 10, 126 Mar 18 11:29 /dev/sgx_enclave
$ ./test_sgx
Unable to open /dev/sgx_enclave: Permission denied
If you then 'chown dave:dave /dev/sgx_enclave' (or whatever), but
you leave execute permissions off, you'll get:
$ ls -l /dev/sgx_enclave
crw------- 1 dave dave 10, 126 Mar 18 11:29 /dev/sgx_enclave
$ ./test_sgx
no execute permissions on device file
If you fix that with "chmod ug+x /dev/sgx" but you leave /dev as
noexec, you'll get this:
$ mount | grep "/dev .*noexec"
udev on /dev type devtmpfs (rw,nosuid,noexec,...)
$ ./test_sgx
ERROR: mmap for exec: Operation not permitted
mmap() succeeded for PROT_READ, but failed for PROT_EXEC
check that user has execute permissions on /dev/sgx_enclave and
that /dev does not have noexec set: 'mount | grep "/dev .*noexec"'
That can be fixed with:
mount -o remount,noexec /devESC
Hopefully, the combination of better error messages and the search
engines indexing this message will help people fix their systems
until we do this properly.
[ bp: Improve error messages more. ]
Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Borislav Petkov <bp@suse.de>
Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
Link: https://lore.kernel.org/r/20210318194301.11D9A984@viggo.jf.intel.com
---
tools/testing/selftests/sgx/load.c | 69 +++++++++++++++++++++++------
tools/testing/selftests/sgx/main.c | 2 +-
2 files changed, 56 insertions(+), 15 deletions(-)
diff --git a/tools/testing/selftests/sgx/load.c b/tools/testing/selftests/sgx/load.c
index 9d43b75..f441ac3 100644
--- a/tools/testing/selftests/sgx/load.c
+++ b/tools/testing/selftests/sgx/load.c
@@ -45,19 +45,19 @@ static bool encl_map_bin(const char *path, struct encl *encl)
fd = open(path, O_RDONLY);
if (fd == -1) {
- perror("open()");
+ perror("enclave executable open()");
return false;
}
ret = stat(path, &sb);
if (ret) {
- perror("stat()");
+ perror("enclave executable stat()");
goto err;
}
bin = mmap(NULL, sb.st_size, PROT_READ, MAP_PRIVATE, fd, 0);
if (bin == MAP_FAILED) {
- perror("mmap()");
+ perror("enclave executable mmap()");
goto err;
}
@@ -90,8 +90,7 @@ static bool encl_ioc_create(struct encl *encl)
ioc.src = (unsigned long)secs;
rc = ioctl(encl->fd, SGX_IOC_ENCLAVE_CREATE, &ioc);
if (rc) {
- fprintf(stderr, "SGX_IOC_ENCLAVE_CREATE failed: errno=%d\n",
- errno);
+ perror("SGX_IOC_ENCLAVE_CREATE failed");
munmap((void *)secs->base, encl->encl_size);
return false;
}
@@ -116,31 +115,72 @@ static bool encl_ioc_add_pages(struct encl *encl, struct encl_segment *seg)
rc = ioctl(encl->fd, SGX_IOC_ENCLAVE_ADD_PAGES, &ioc);
if (rc < 0) {
- fprintf(stderr, "SGX_IOC_ENCLAVE_ADD_PAGES failed: errno=%d.\n",
- errno);
+ perror("SGX_IOC_ENCLAVE_ADD_PAGES failed");
return false;
}
return true;
}
+
+
bool encl_load(const char *path, struct encl *encl)
{
+ const char device_path[] = "/dev/sgx_enclave";
Elf64_Phdr *phdr_tbl;
off_t src_offset;
Elf64_Ehdr *ehdr;
+ struct stat sb;
+ void *ptr;
int i, j;
int ret;
+ int fd = -1;
memset(encl, 0, sizeof(*encl));
- ret = open("/dev/sgx_enclave", O_RDWR);
- if (ret < 0) {
- fprintf(stderr, "Unable to open /dev/sgx_enclave\n");
+ fd = open(device_path, O_RDWR);
+ if (fd < 0) {
+ perror("Unable to open /dev/sgx_enclave");
+ goto err;
+ }
+
+ ret = stat(device_path, &sb);
+ if (ret) {
+ perror("device file stat()");
+ 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");
+ goto err;
+ }
+ munmap(ptr, PAGE_SIZE);
+
+#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" \
+" 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);
goto err;
}
+ munmap(ptr, PAGE_SIZE);
- encl->fd = ret;
+ encl->fd = fd;
if (!encl_map_bin(path, encl))
goto err;
@@ -217,6 +257,8 @@ bool encl_load(const char *path, struct encl *encl)
return true;
err:
+ if (fd != -1)
+ close(fd);
encl_delete(encl);
return false;
}
@@ -229,7 +271,7 @@ static bool encl_map_area(struct encl *encl)
area = mmap(NULL, encl_size * 2, PROT_NONE,
MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
if (area == MAP_FAILED) {
- perror("mmap");
+ perror("reservation mmap()");
return false;
}
@@ -268,8 +310,7 @@ bool encl_build(struct encl *encl)
ioc.sigstruct = (uint64_t)&encl->sigstruct;
ret = ioctl(encl->fd, SGX_IOC_ENCLAVE_INIT, &ioc);
if (ret) {
- fprintf(stderr, "SGX_IOC_ENCLAVE_INIT failed: errno=%d\n",
- errno);
+ perror("SGX_IOC_ENCLAVE_INIT failed");
return false;
}
diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c
index 724cec7..b117bb8 100644
--- a/tools/testing/selftests/sgx/main.c
+++ b/tools/testing/selftests/sgx/main.c
@@ -195,7 +195,7 @@ int main(int argc, char *argv[], char *envp[])
addr = mmap((void *)encl.encl_base + seg->offset, seg->size,
seg->prot, MAP_SHARED | MAP_FIXED, encl.fd, 0);
if (addr == MAP_FAILED) {
- fprintf(stderr, "mmap() failed, errno=%d.\n", errno);
+ perror("mmap() segment failed");
exit(KSFT_FAIL);
}
}
^ permalink raw reply related [flat|nested] 7+ messages in thread