Subject: [PATCH 0/1] SGX self test fails

Subject: [PATCH 0/1] SGX self test fails

[Tim Gardner]

I'm just starting my learning curve on SGX, so I don't know if I've missed
some setup for the SGX device entries. After looking at arch/x86/kernel/cpu/sgx/driver.c
I see that there is no mode value for either sgx_dev_enclave or sgx_dev_provision.

With this patch I can get the SGX self test to complete:

sudo ./test_sgx
Warning: no execute permissions on device file /dev/sgx_enclave
0x0000000000000000 0x0000000000002000 0x03
0x0000000000002000 0x0000000000001000 0x05
0x0000000000003000 0x0000000000003000 0x03
SUCCESS

Is the warning even necessary ?

Tim

[PATCH] selftests/sgx: Defeat execute permissions test

[Tim Gardner]

The permissions check on /dev/sgx_enclave appears to be an oversight
in that it will not allow the test to continue. Skipping this test
with a warning allows the test to proceed.

Cc: Jarkko Sakkinen <jarkko@kernel.org>
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
Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
---
 tools/testing/selftests/sgx/load.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/sgx/load.c b/tools/testing/selftests/sgx/load.c
index f441ac34b4d4..e5bcaca1c372 100644
--- a/tools/testing/selftests/sgx/load.c
+++ b/tools/testing/selftests/sgx/load.c
@@ -155,10 +155,8 @@ bool encl_load(const char *path, struct encl *encl)
 	 * 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;
-	}
+	if (!(sb.st_mode & (S_IXUSR | S_IXGRP | S_IXOTH)))
+		fprintf(stderr, "Warning: no execute permissions on device file %s\n", device_path);
 
 	ptr = mmap(NULL, PAGE_SIZE, PROT_READ, MAP_SHARED, fd, 0);
 	if (ptr == (void *)-1) {
-- 
2.17.1

Re: Subject: [PATCH 0/1] SGX self test fails

[Dave Hansen]

On 4/29/21 11:39 AM, Tim Gardner wrote:
> I'm just starting my learning curve on SGX, so I don't know if I've missed
> some setup for the SGX device entries. After looking at arch/x86/kernel/cpu/sgx/driver.c
> I see that there is no mode value for either sgx_dev_enclave or sgx_dev_provision.
> 
> With this patch I can get the SGX self test to complete:
> 
> sudo ./test_sgx
> Warning: no execute permissions on device file /dev/sgx_enclave
> 0x0000000000000000 0x0000000000002000 0x03
> 0x0000000000002000 0x0000000000001000 0x05
> 0x0000000000003000 0x0000000000003000 0x03
> SUCCESS
> 
> Is the warning even necessary ?

Dang, I just added that warning.  I thought it was necessary, but I
guess not:

$ ls -l /dev/sgx_enclave
crw------- 1 dave dave 10, 125 Apr 28 11:32 /dev/sgx_enclave
$ ./test_sgx
0x0000000000000000 0x0000000000002000 0x03
0x0000000000002000 0x0000000000001000 0x05
0x0000000000003000 0x0000000000003000 0x03
SUCCESS

*But*, is that OK?  Should we be happily creating a PROT_EXEC mapping on
a ugo-x file?  Why were we respecting noexec on the filesystem but not
ugo-x on the file?

Re: Subject: [PATCH 0/1] SGX self test fails

[Dr. Greg]

On Thu, Apr 29, 2021 at 11:55:23AM -0700, Dave Hansen wrote:

Good morning, I hope the end of the week is going well for everyone.

> On 4/29/21 11:39 AM, Tim Gardner wrote:
> > I'm just starting my learning curve on SGX, so I don't know if
> > I've missed some setup for the SGX device entries. After looking
> > at arch/x86/kernel/cpu/sgx/driver.c I see that there is no mode
> > value for either sgx_dev_enclave or sgx_dev_provision.
> >
> > With this patch I can get the SGX self test to complete:
> > 
> > sudo ./test_sgx
> > Warning: no execute permissions on device file /dev/sgx_enclave
> > 0x0000000000000000 0x0000000000002000 0x03
> > 0x0000000000002000 0x0000000000001000 0x05
> > 0x0000000000003000 0x0000000000003000 0x03
> > SUCCESS
> > 
> > Is the warning even necessary ?
> 
> Dang, I just added that warning.  I thought it was necessary, but I
> guess not:
> 
> $ ls -l /dev/sgx_enclave
> crw------- 1 dave dave 10, 125 Apr 28 11:32 /dev/sgx_enclave
> $ ./test_sgx
> 0x0000000000000000 0x0000000000002000 0x03
> 0x0000000000002000 0x0000000000001000 0x05
> 0x0000000000003000 0x0000000000003000 0x03
> SUCCESS
> 
> *But*, is that OK?  Should we be happily creating a PROT_EXEC mapping on
> a ugo-x file?  Why were we respecting noexec on the filesystem but not
> ugo-x on the file?

Because no one placed any explicit executable mode bit checks on the
inode that is underlying the character device file in
arch/x86/kernel/cpu/sgx/driver.c:sgx_open() and the controls on
executable virtual memory are implemented in the mm/mmap.c:do_mmap()
path when the mmap system call is executed.

The notion of using discretionary access controls, and arguably MAC's,
since the true identity of a file is its inode label, to gate
executable permissions by a user on the contents of a file are a
function of the exec* system calls.

The notion of whether or not it is 'OK' to not allow system
administrators the ability to control SGX usage with file level exec
privileges would seem to be more philosophical then practical.  The
SGX device node does not represent an executable entity, it represents
a gateway to the right to create and then populate anonymous
executable memory.

From the standpoint of current systems administration practice, the
notion and need for executable bits being important on device nodes is
foreign, and violates the concept of least surprise.  As we have
already seen with the issue of noexec on /dev, the historical notion
of security has been that executable files should not be allowed in
the /dev heirarchy.

With that mindset, the notion of gating SGX permissions with only the
write bit on the character device would seem to make conceptual sense.
It does, however, limit the utility of using the bprm* LSM hooks to
implement whatever LSM controls over SGX that are envisioned for the
future.

At the risk of fanning dormant embers into flame, the relevance of all
of this needs to be considered in a future that will include Enclave
Dynamic Memory Management (EDMM).  At that point in time, access to
the SGX device node, gated through either discretionary or mandatory
access controls, means that an eligible entity will have unrestricted
rights to load completely anonymous, in fact cryptographically
anonymous, text into memory and execute it.

The utility of anything but yes/no decisions needs to be made with
that concept in mind.

To avoid the risk of being classified as a blatherer, I will return to
my work on maintaining support for cryptographic access controls on
all of this... :-)

Best wishes for a pleasant spring weekend to everyone.

Dr. Greg

As always,
Dr. Greg Wettstein, Ph.D, Worker      Autonomously self-defensive
Enjellic Systems Development, LLC     IOT platforms and edge devices.
4206 N. 19th Ave.
Fargo, ND  58102
PH: 701-281-1686                      EMAIL: greg@enjellic.com
------------------------------------------------------------------------------
"Everything should be made as simple as possible, but not simpler."
                                -- Albert Einstein

Re: Subject: [PATCH 0/1] SGX self test fails

[Jarkko Sakkinen]

On Thu, Apr 29, 2021 at 11:55:23AM -0700, Dave Hansen wrote:
> On 4/29/21 11:39 AM, Tim Gardner wrote:
> > I'm just starting my learning curve on SGX, so I don't know if I've missed
> > some setup for the SGX device entries. After looking at arch/x86/kernel/cpu/sgx/driver.c
> > I see that there is no mode value for either sgx_dev_enclave or sgx_dev_provision.
> > 
> > With this patch I can get the SGX self test to complete:
> > 
> > sudo ./test_sgx
> > Warning: no execute permissions on device file /dev/sgx_enclave
> > 0x0000000000000000 0x0000000000002000 0x03
> > 0x0000000000002000 0x0000000000001000 0x05
> > 0x0000000000003000 0x0000000000003000 0x03
> > SUCCESS
> > 
> > Is the warning even necessary ?
> 
> Dang, I just added that warning.  I thought it was necessary, but I
> guess not:
> 
> $ ls -l /dev/sgx_enclave
> crw------- 1 dave dave 10, 125 Apr 28 11:32 /dev/sgx_enclave
> $ ./test_sgx
> 0x0000000000000000 0x0000000000002000 0x03
> 0x0000000000002000 0x0000000000001000 0x05
> 0x0000000000003000 0x0000000000003000 0x03
> SUCCESS
> 
> *But*, is that OK?  Should we be happily creating a PROT_EXEC mapping on
> a ugo-x file?  Why were we respecting noexec on the filesystem but not
> ugo-x on the file?

Yeah, this supports my earlier response:

"EPERM  The prot argument asks for PROT_EXEC but the mapped area
 belongs to a file on a filesystem that was mounted no-exec."
https://man7.org/linux/man-pages/man2/mmap.2.html

I guess the right model is to think just as "anonymous memory"
with equivalent access control semantics after succesfully
opened for read and write.

BTW, this is good material for the man page :-)

/Jarkko

Re: Subject: [PATCH 0/1] SGX self test fails

[Dave Hansen]

On 5/3/21 8:41 AM, Jarkko Sakkinen wrote:
>> $ ls -l /dev/sgx_enclave
>> crw------- 1 dave dave 10, 125 Apr 28 11:32 /dev/sgx_enclave
>> $ ./test_sgx
>> 0x0000000000000000 0x0000000000002000 0x03
>> 0x0000000000002000 0x0000000000001000 0x05
>> 0x0000000000003000 0x0000000000003000 0x03
>> SUCCESS
>>
>> *But*, is that OK?  Should we be happily creating a PROT_EXEC mapping on
>> a ugo-x file?  Why were we respecting noexec on the filesystem but not
>> ugo-x on the file?
> Yeah, this supports my earlier response:
> 
> "EPERM  The prot argument asks for PROT_EXEC but the mapped area
>  belongs to a file on a filesystem that was mounted no-exec."
> https://man7.org/linux/man-pages/man2/mmap.2.html
> 
> I guess the right model is to think just as "anonymous memory"
> with equivalent access control semantics after succesfully
> opened for read and write.

I guess I'll answer my own question: The "x" bit on file permissions
really only controls the ability for the file to be execve()'d, but has
no bearing on the ability for an executable *mapping* to be created.
This is existing VFS behavior and is not specific to SGX.

I think I'll just send a patch to pull that warning out.

Re: Subject: [PATCH 0/1] SGX self test fails

[Jarkko Sakkinen]

On Mon, May 03, 2021 at 09:39:05AM -0700, Dave Hansen wrote:
> On 5/3/21 8:41 AM, Jarkko Sakkinen wrote:
> >> $ ls -l /dev/sgx_enclave
> >> crw------- 1 dave dave 10, 125 Apr 28 11:32 /dev/sgx_enclave
> >> $ ./test_sgx
> >> 0x0000000000000000 0x0000000000002000 0x03
> >> 0x0000000000002000 0x0000000000001000 0x05
> >> 0x0000000000003000 0x0000000000003000 0x03
> >> SUCCESS
> >>
> >> *But*, is that OK?  Should we be happily creating a PROT_EXEC mapping on
> >> a ugo-x file?  Why were we respecting noexec on the filesystem but not
> >> ugo-x on the file?
> > Yeah, this supports my earlier response:
> > 
> > "EPERM  The prot argument asks for PROT_EXEC but the mapped area
> >  belongs to a file on a filesystem that was mounted no-exec."
> > https://man7.org/linux/man-pages/man2/mmap.2.html
> > 
> > I guess the right model is to think just as "anonymous memory"
> > with equivalent access control semantics after succesfully
> > opened for read and write.
> 
> I guess I'll answer my own question: The "x" bit on file permissions
> really only controls the ability for the file to be execve()'d, but has
> no bearing on the ability for an executable *mapping* to be created.
> This is existing VFS behavior and is not specific to SGX.

Yeah, that's nicely put it into one sentence :-)

> I think I'll just send a patch to pull that warning out.

/Jarkko

Re: Subject: [PATCH 0/1] SGX self test fails

[Jarkko Sakkinen]

On Thu, Apr 29, 2021 at 12:39:51PM -0600, Tim Gardner wrote:
> I'm just starting my learning curve on SGX, so I don't know if I've missed
> some setup for the SGX device entries. After looking at arch/x86/kernel/cpu/sgx/driver.c
> I see that there is no mode value for either sgx_dev_enclave or sgx_dev_provision.

Take a look at

https://github.com/systemd/systemd/pull/18944

> With this patch I can get the SGX self test to complete:
> 
> sudo ./test_sgx
> Warning: no execute permissions on device file /dev/sgx_enclave
> 0x0000000000000000 0x0000000000002000 0x03
> 0x0000000000002000 0x0000000000001000 0x05
> 0x0000000000003000 0x0000000000003000 0x03
> SUCCESS
> 
> Is the warning even necessary ?
> 
> Tim

With a quick look, I think that check is too strict. AFAIK, mmap(PROT_EXEC)
can be done, as long as the file is not in a noexec FS.

Dave?

For reference:

https://lore.kernel.org/linux-sgx/20210318194301.11D9A984@viggo.jf.intel.com/

/Jarkko