On Thu, Mar 18, 2021 at 12:43:01PM -0700, Dave Hansen wrote:
>
> From: Dave Hansen
>
> 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:
>
> 0x 0x2000 0x03
> 0x2000 0x1000 0x05
> 0x3000 0x3000 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
> Cc: Jarkko Sakkinen
> Cc: Shuah Khan
> Cc: Borislav Petkov
> Cc: x...@kernel.org
> Cc: linux-...@vger.kernel.org
> Cc: linux-kselft...@vger.kernel.org
Reviewed-by: Jarkko Sakkinen
> ---
>
> 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, );
> 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, );
> 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, );
> 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