[PATCH] selftests/x86: Fix malformed src_offset initialization

[PATCH] selftests/x86: Fix malformed src_offset initialization

[Jarkko Sakkinen]

Assign src_offset just to the p_offset, when first initialized.
This has been probably copy-pasting accident (at least looks like
it).

Cc: Borislav Petkov <bp@alien8.de>
Cc: Shuah Khan <shuah@kernel.org>
Cc: linux-kselftest@vger.kernel.org
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
 tools/testing/selftests/sgx/load.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/sgx/load.c b/tools/testing/selftests/sgx/load.c
index 07988de6b767..64976f266bae 100644
--- a/tools/testing/selftests/sgx/load.c
+++ b/tools/testing/selftests/sgx/load.c
@@ -185,7 +185,7 @@ bool encl_load(const char *path, struct encl *encl)
 		}
 
 		if (j == 0) {
-			src_offset = (phdr->p_offset & PAGE_MASK) - src_offset;
+			src_offset = (phdr->p_offset & PAGE_MASK);
 
 			seg->prot = PROT_READ | PROT_WRITE;
 			seg->flags = SGX_PAGE_TYPE_TCS << 8;
-- 
2.27.0

Re: [PATCH] selftests/x86: Fix malformed src_offset initialization

[Borislav Petkov]

On Wed, Nov 18, 2020 at 12:36:30AM +0200, Jarkko Sakkinen wrote:
> Assign src_offset just to the p_offset, when first initialized.
> This has been probably copy-pasting accident (at least looks like
> it).
> 
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Shuah Khan <shuah@kernel.org>
> Cc: linux-kselftest@vger.kernel.org
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> ---
>  tools/testing/selftests/sgx/load.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/sgx/load.c b/tools/testing/selftests/sgx/load.c
> index 07988de6b767..64976f266bae 100644
> --- a/tools/testing/selftests/sgx/load.c
> +++ b/tools/testing/selftests/sgx/load.c
> @@ -185,7 +185,7 @@ bool encl_load(const char *path, struct encl *encl)
>  		}
>  
>  		if (j == 0) {
> -			src_offset = (phdr->p_offset & PAGE_MASK) - src_offset;
> +			src_offset = (phdr->p_offset & PAGE_MASK);
>  
>  			seg->prot = PROT_READ | PROT_WRITE;
>  			seg->flags = SGX_PAGE_TYPE_TCS << 8;
> -- 

Still no joy:

0x0000000000000000 0x0000000000002000 0x03
0x0000000000002000 0x0000000000001000 0x05
0x0000000000003000 0x0000000000003000 0x03
encl_load: encl->nr_segments: 3
encl_load: seg2 offset: 0x3000, seg2 size: 12288
encl_load: encl_size: 32768, src_size: 24576
encl_map_area: encl_size: 32768
encl_map_area: area: 0x0x7f7ec8dd8000
encl_map_area: encl_base: 0x7f7ec8dd8000
mapping segment 0, seg->prot: (read write )
base: 0x7f7ec8dd8000, offset 0x0, size: 8192
mapping segment 1, seg->prot: (read  exec)
base: 0x7f7ec8dd8000, offset 0x2000, size: 4096
mmap() failed, errno=1.
mmap: Operation not permitted

That second segment is PROT_EXEC and mmap(2) manpage says:

       EPERM  The prot argument asks for PROT_EXEC but the mapped  area  belongs  to  a
              file on a filesystem that was mounted no-exec.

       EPERM  The operation was prevented by a file seal; see fcntl(2).

I don't see fcntl() calls in the test and the fs I'm running it from is
not mapped "no-exec":

/dev/nvme0n1p2 on / type ext4 (rw,relatime,errors=remount-ro)

so something else is missing.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] selftests/x86: Fix malformed src_offset initialization

[Jethro Beekman]

[-- Attachment #1: Type: text/plain, Size: 2423 bytes --]

On 2020-11-18 12:11, Borislav Petkov wrote:
> On Wed, Nov 18, 2020 at 12:36:30AM +0200, Jarkko Sakkinen wrote:
>> Assign src_offset just to the p_offset, when first initialized.
>> This has been probably copy-pasting accident (at least looks like
>> it).
>>
>> Cc: Borislav Petkov <bp@alien8.de>
>> Cc: Shuah Khan <shuah@kernel.org>
>> Cc: linux-kselftest@vger.kernel.org
>> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
>> ---
>>  tools/testing/selftests/sgx/load.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tools/testing/selftests/sgx/load.c b/tools/testing/selftests/sgx/load.c
>> index 07988de6b767..64976f266bae 100644
>> --- a/tools/testing/selftests/sgx/load.c
>> +++ b/tools/testing/selftests/sgx/load.c
>> @@ -185,7 +185,7 @@ bool encl_load(const char *path, struct encl *encl)
>>  		}
>>  
>>  		if (j == 0) {
>> -			src_offset = (phdr->p_offset & PAGE_MASK) - src_offset;
>> +			src_offset = (phdr->p_offset & PAGE_MASK);
>>  
>>  			seg->prot = PROT_READ | PROT_WRITE;
>>  			seg->flags = SGX_PAGE_TYPE_TCS << 8;
>> -- 
> 
> Still no joy:
> 
> 0x0000000000000000 0x0000000000002000 0x03
> 0x0000000000002000 0x0000000000001000 0x05
> 0x0000000000003000 0x0000000000003000 0x03
> encl_load: encl->nr_segments: 3
> encl_load: seg2 offset: 0x3000, seg2 size: 12288
> encl_load: encl_size: 32768, src_size: 24576
> encl_map_area: encl_size: 32768
> encl_map_area: area: 0x0x7f7ec8dd8000
> encl_map_area: encl_base: 0x7f7ec8dd8000
> mapping segment 0, seg->prot: (read write )
> base: 0x7f7ec8dd8000, offset 0x0, size: 8192
> mapping segment 1, seg->prot: (read  exec)
> base: 0x7f7ec8dd8000, offset 0x2000, size: 4096
> mmap() failed, errno=1.
> mmap: Operation not permitted
> 
> That second segment is PROT_EXEC and mmap(2) manpage says:
> 
>        EPERM  The prot argument asks for PROT_EXEC but the mapped  area  belongs  to  a
>               file on a filesystem that was mounted no-exec.
> 
>        EPERM  The operation was prevented by a file seal; see fcntl(2).
> 
> I don't see fcntl() calls in the test and the fs I'm running it from is
> not mapped "no-exec":
> 
> /dev/nvme0n1p2 on / type ext4 (rw,relatime,errors=remount-ro)
> 
> so something else is missing.

Just to double check, since you didn't show the /dev mount line: that is also not mounted noexec?

--
Jethro Beekman | Fortanix




[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4490 bytes --]

Re: [PATCH] selftests/x86: Fix malformed src_offset initialization

[Borislav Petkov]

On Wed, Nov 18, 2020 at 12:18:00PM +0100, Jethro Beekman wrote:
> Just to double check, since you didn't show the /dev mount line: that
> is also not mounted noexec?

Yes:

udev on /dev type devtmpfs (rw,nosuid,noexec,relatime,size=8021332k,nr_inodes=2005333,mode=755,inode64)

but this is how udev mounts it by default. Now I did:

# mount /dev -o remount,exec

and I got

udev on /dev type devtmpfs (rw,nosuid,relatime,size=8021332k,nr_inodes=2005333,mode=755,inode64)

and now it fails differently:

0x0000000000000000 0x0000000000002000 0x03
0x0000000000002000 0x0000000000001000 0x05
0x0000000000003000 0x0000000000003000 0x03
encl_load: encl->nr_segments: 3
encl_load: seg2 offset: 0x3000, seg2 size: 12288
encl_load: encl_size: 32768, src_size: 24576
encl_map_area: encl_size: 32768
encl_map_area: area: 0x0x7feae0db2000
encl_map_area: encl_base: 0x7feae0db8000
SGX_IOC_ENCLAVE_INIT failed: errno=1

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] selftests/x86: Fix malformed src_offset initialization

[Borislav Petkov]

On Wed, Nov 18, 2020 at 12:44:44PM +0100, Borislav Petkov wrote:
> 0x0000000000000000 0x0000000000002000 0x03
> 0x0000000000002000 0x0000000000001000 0x05
> 0x0000000000003000 0x0000000000003000 0x03
> encl_load: encl->nr_segments: 3
> encl_load: seg2 offset: 0x3000, seg2 size: 12288
> encl_load: encl_size: 32768, src_size: 24576
> encl_map_area: encl_size: 32768
> encl_map_area: area: 0x0x7feae0db2000
> encl_map_area: encl_base: 0x7feae0db8000
> SGX_IOC_ENCLAVE_INIT failed: errno=1

Running that same thing again succeeded this time:

0x0000000000000000 0x0000000000002000 0x03
0x0000000000002000 0x0000000000001000 0x05
0x0000000000003000 0x0000000000003000 0x03
encl_load: encl->nr_segments: 3
encl_load: seg2 offset: 0x3000, seg2 size: 12288
encl_load: encl_size: 32768, src_size: 24576
encl_map_area: encl_size: 32768
encl_map_area: area: 0x0x7f846bec0000
encl_map_area: encl_base: 0x7f846bec0000
mapping segment 0, seg->prot: (read write )
base: 0x7f846bec0000, offset 0x0, size: 8192
mapping segment 1, seg->prot: (read  exec)
base: 0x7f846bec0000, offset 0x2000, size: 4096
mapping segment 2, seg->prot: (read write )
base: 0x7f846bec0000, offset 0x3000, size: 12288
SUCCESS

then I did a couple of successful runs and the next one failed again:

0x0000000000000000 0x0000000000002000 0x03
0x0000000000002000 0x0000000000001000 0x05
0x0000000000003000 0x0000000000003000 0x03
encl_load: encl->nr_segments: 3
encl_load: seg2 offset: 0x3000, seg2 size: 12288
encl_load: encl_size: 32768, src_size: 24576
encl_map_area: encl_size: 32768
encl_map_area: area: 0x0x7fb09d4a0000
encl_map_area: encl_base: 0x7fb09d4a0000
SGX_IOC_ENCLAVE_INIT failed: errno=1

Fun.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] selftests/x86: Fix malformed src_offset initialization

[Borislav Petkov]

Looks like 10% failure rate:

for i in $(seq 0 100); do ./test_sgx | grep failed; done
SGX_IOC_ENCLAVE_INIT failed: errno=1
SGX_IOC_ENCLAVE_INIT failed: errno=1
SGX_IOC_ENCLAVE_INIT failed: errno=1
SGX_IOC_ENCLAVE_INIT failed: errno=1
SGX_IOC_ENCLAVE_INIT failed: errno=1
SGX_IOC_ENCLAVE_INIT failed: errno=1
SGX_IOC_ENCLAVE_INIT failed: errno=1
SGX_IOC_ENCLAVE_INIT failed: errno=1
SGX_IOC_ENCLAVE_INIT failed: errno=1

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] selftests/x86: Fix malformed src_offset initialization

[Jarkko Sakkinen]

On Wed, Nov 18, 2020 at 12:47:03PM +0100, Borislav Petkov wrote:
> On Wed, Nov 18, 2020 at 12:44:44PM +0100, Borislav Petkov wrote:
> > 0x0000000000000000 0x0000000000002000 0x03
> > 0x0000000000002000 0x0000000000001000 0x05
> > 0x0000000000003000 0x0000000000003000 0x03
> > encl_load: encl->nr_segments: 3
> > encl_load: seg2 offset: 0x3000, seg2 size: 12288
> > encl_load: encl_size: 32768, src_size: 24576
> > encl_map_area: encl_size: 32768
> > encl_map_area: area: 0x0x7feae0db2000
> > encl_map_area: encl_base: 0x7feae0db8000
> > SGX_IOC_ENCLAVE_INIT failed: errno=1
> 
> Running that same thing again succeeded this time:
> 
> 0x0000000000000000 0x0000000000002000 0x03
> 0x0000000000002000 0x0000000000001000 0x05
> 0x0000000000003000 0x0000000000003000 0x03
> encl_load: encl->nr_segments: 3
> encl_load: seg2 offset: 0x3000, seg2 size: 12288
> encl_load: encl_size: 32768, src_size: 24576
> encl_map_area: encl_size: 32768
> encl_map_area: area: 0x0x7f846bec0000
> encl_map_area: encl_base: 0x7f846bec0000
> mapping segment 0, seg->prot: (read write )
> base: 0x7f846bec0000, offset 0x0, size: 8192
> mapping segment 1, seg->prot: (read  exec)
> base: 0x7f846bec0000, offset 0x2000, size: 4096
> mapping segment 2, seg->prot: (read write )
> base: 0x7f846bec0000, offset 0x3000, size: 12288
> SUCCESS
> 
> then I did a couple of successful runs and the next one failed again:
> 
> 0x0000000000000000 0x0000000000002000 0x03
> 0x0000000000002000 0x0000000000001000 0x05
> 0x0000000000003000 0x0000000000003000 0x03
> encl_load: encl->nr_segments: 3
> encl_load: seg2 offset: 0x3000, seg2 size: 12288
> encl_load: encl_size: 32768, src_size: 24576
> encl_map_area: encl_size: 32768
> encl_map_area: area: 0x0x7fb09d4a0000
> encl_map_area: encl_base: 0x7fb09d4a0000
> SGX_IOC_ENCLAVE_INIT failed: errno=1
> 
> Fun.

If you adjust log level, then you should probably see this from
sgx_enclave_init():

	} else if (ret) {
		pr_debug("EINIT returned %d\n", ret);
		ret = -EPERM;
	}

EINIT fails with big certainty because SIGSTRUCT is malformed. The only
dynamic thing in that process is RSA key generation sigstruct.c.
Otherwise, everything is static between the runs. That's why I'm quite
confident that key generation is the issue. Given how the issue behaves
I'd guess it eats the entropy pool.

So what I would propose is that I fix this by adding a static 3072-bit
key and remove the generation code

I found a patch that I can use to revert dynamic generation:

https://lore.kernel.org/linux-sgx/20200319023306.6875-1-jarkko.sakkinen@linux.intel.com/

> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette

/Jarkko

Re: [PATCH] selftests/x86: Fix malformed src_offset initialization

[Jarkko Sakkinen]

On Wed, Nov 18, 2020 at 05:24:38PM +0200, Jarkko Sakkinen wrote:
> On Wed, Nov 18, 2020 at 12:47:03PM +0100, Borislav Petkov wrote:
> > On Wed, Nov 18, 2020 at 12:44:44PM +0100, Borislav Petkov wrote:
> > > 0x0000000000000000 0x0000000000002000 0x03
> > > 0x0000000000002000 0x0000000000001000 0x05
> > > 0x0000000000003000 0x0000000000003000 0x03
> > > encl_load: encl->nr_segments: 3
> > > encl_load: seg2 offset: 0x3000, seg2 size: 12288
> > > encl_load: encl_size: 32768, src_size: 24576
> > > encl_map_area: encl_size: 32768
> > > encl_map_area: area: 0x0x7feae0db2000
> > > encl_map_area: encl_base: 0x7feae0db8000
> > > SGX_IOC_ENCLAVE_INIT failed: errno=1
> > 
> > Running that same thing again succeeded this time:
> > 
> > 0x0000000000000000 0x0000000000002000 0x03
> > 0x0000000000002000 0x0000000000001000 0x05
> > 0x0000000000003000 0x0000000000003000 0x03
> > encl_load: encl->nr_segments: 3
> > encl_load: seg2 offset: 0x3000, seg2 size: 12288
> > encl_load: encl_size: 32768, src_size: 24576
> > encl_map_area: encl_size: 32768
> > encl_map_area: area: 0x0x7f846bec0000
> > encl_map_area: encl_base: 0x7f846bec0000
> > mapping segment 0, seg->prot: (read write )
> > base: 0x7f846bec0000, offset 0x0, size: 8192
> > mapping segment 1, seg->prot: (read  exec)
> > base: 0x7f846bec0000, offset 0x2000, size: 4096
> > mapping segment 2, seg->prot: (read write )
> > base: 0x7f846bec0000, offset 0x3000, size: 12288
> > SUCCESS
> > 
> > then I did a couple of successful runs and the next one failed again:
> > 
> > 0x0000000000000000 0x0000000000002000 0x03
> > 0x0000000000002000 0x0000000000001000 0x05
> > 0x0000000000003000 0x0000000000003000 0x03
> > encl_load: encl->nr_segments: 3
> > encl_load: seg2 offset: 0x3000, seg2 size: 12288
> > encl_load: encl_size: 32768, src_size: 24576
> > encl_map_area: encl_size: 32768
> > encl_map_area: area: 0x0x7fb09d4a0000
> > encl_map_area: encl_base: 0x7fb09d4a0000
> > SGX_IOC_ENCLAVE_INIT failed: errno=1
> > 
> > Fun.
> 
> If you adjust log level, then you should probably see this from
> sgx_enclave_init():
> 
> 	} else if (ret) {
> 		pr_debug("EINIT returned %d\n", ret);
> 		ret = -EPERM;
> 	}
> 
> EINIT fails with big certainty because SIGSTRUCT is malformed. The only
> dynamic thing in that process is RSA key generation sigstruct.c.
> Otherwise, everything is static between the runs. That's why I'm quite
> confident that key generation is the issue. Given how the issue behaves
> I'd guess it eats the entropy pool.
> 
> So what I would propose is that I fix this by adding a static 3072-bit
> key and remove the generation code
> 
> I found a patch that I can use to revert dynamic generation:
> 
> https://lore.kernel.org/linux-sgx/20200319023306.6875-1-jarkko.sakkinen@linux.intel.com/

Not going to use at is. Just replace gen_sign_key(). Will be quite
localized fix.

/Jarkko

Re: [PATCH] selftests/x86: Fix malformed src_offset initialization

[Jethro Beekman]

[-- Attachment #1: Type: text/plain, Size: 1425 bytes --]

On 2020-11-18 12:44, Borislav Petkov wrote:
> On Wed, Nov 18, 2020 at 12:18:00PM +0100, Jethro Beekman wrote:
>> Just to double check, since you didn't show the /dev mount line: that
>> is also not mounted noexec?
> 
> Yes:
> 
> udev on /dev type devtmpfs (rw,nosuid,noexec,relatime,size=8021332k,nr_inodes=2005333,mode=755,inode64)
> 
> but this is how udev mounts it by default. Now I did:

On some distros, unfortunately, yes, and this breaks SGX. See https://www.spinics.net/lists/linux-sgx/msg02562.html and https://www.spinics.net/lists/linux-sgx/msg02617.html

> 
> # mount /dev -o remount,exec
> 
> and I got
> 
> udev on /dev type devtmpfs (rw,nosuid,relatime,size=8021332k,nr_inodes=2005333,mode=755,inode64)
> 
> and now it fails differently:
> 
> 0x0000000000000000 0x0000000000002000 0x03
> 0x0000000000002000 0x0000000000001000 0x05
> 0x0000000000003000 0x0000000000003000 0x03
> encl_load: encl->nr_segments: 3
> encl_load: seg2 offset: 0x3000, seg2 size: 12288
> encl_load: encl_size: 32768, src_size: 24576
> encl_map_area: encl_size: 32768
> encl_map_area: area: 0x0x7feae0db2000
> encl_map_area: encl_base: 0x7feae0db8000
> SGX_IOC_ENCLAVE_INIT failed: errno=1
> 

I think that means SGX_INVALID_SIG_STRUCT, which is a very odd error to get. It basically means the file header is wrong. Maybe some concurrency/fflush issue in the test?

--
Jethro Beekman | Fortanix


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4490 bytes --]

Re: [PATCH] selftests/x86: Fix malformed src_offset initialization

[Borislav Petkov]

On Wed, Nov 18, 2020 at 01:00:30PM +0100, Jethro Beekman wrote:
> On some distros, unfortunately, yes, and this breaks SGX. See
> https://www.spinics.net/lists/linux-sgx/msg02562.html and
> https://www.spinics.net/lists/linux-sgx/msg02617.html

I guess anonymous inode doesn't sound all too bad, albeit Sean calls it
an ugly hack.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] selftests/x86: Fix malformed src_offset initialization

[Jarkko Sakkinen]

On Wed, Nov 18, 2020 at 01:15:50PM +0100, Borislav Petkov wrote:
> On Wed, Nov 18, 2020 at 01:00:30PM +0100, Jethro Beekman wrote:
> > On some distros, unfortunately, yes, and this breaks SGX. See
> > https://www.spinics.net/lists/linux-sgx/msg02562.html and
> > https://www.spinics.net/lists/linux-sgx/msg02617.html
> 
> I guess anonymous inode doesn't sound all too bad, albeit Sean calls it
> an ugly hack.
> 
> Thx.

LOL, I did not even remember that I had created patches for it at some
point.

I'll send the fix for the sigstruct issue. It's not a good idea to
generate big rsa keys on fly I guess.

> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette

/Jarkko

Re: [PATCH] selftests/x86: Fix malformed src_offset initialization

[Jarkko Sakkinen]

On Wed, Nov 18, 2020 at 01:00:30PM +0100, Jethro Beekman wrote:
> On 2020-11-18 12:44, Borislav Petkov wrote:
> > On Wed, Nov 18, 2020 at 12:18:00PM +0100, Jethro Beekman wrote:
> >> Just to double check, since you didn't show the /dev mount line: that
> >> is also not mounted noexec?
> > 
> > Yes:
> > 
> > udev on /dev type devtmpfs (rw,nosuid,noexec,relatime,size=8021332k,nr_inodes=2005333,mode=755,inode64)
> > 
> > but this is how udev mounts it by default. Now I did:
> 
> On some distros, unfortunately, yes, and this breaks SGX. See https://www.spinics.net/lists/linux-sgx/msg02562.html and https://www.spinics.net/lists/linux-sgx/msg02617.html
> 
> > 
> > # mount /dev -o remount,exec
> > 
> > and I got
> > 
> > udev on /dev type devtmpfs (rw,nosuid,relatime,size=8021332k,nr_inodes=2005333,mode=755,inode64)
> > 
> > and now it fails differently:
> > 
> > 0x0000000000000000 0x0000000000002000 0x03
> > 0x0000000000002000 0x0000000000001000 0x05
> > 0x0000000000003000 0x0000000000003000 0x03
> > encl_load: encl->nr_segments: 3
> > encl_load: seg2 offset: 0x3000, seg2 size: 12288
> > encl_load: encl_size: 32768, src_size: 24576
> > encl_map_area: encl_size: 32768
> > encl_map_area: area: 0x0x7feae0db2000
> > encl_map_area: encl_base: 0x7feae0db8000
> > SGX_IOC_ENCLAVE_INIT failed: errno=1
> > 
> 
> I think that means SGX_INVALID_SIG_STRUCT, which is a very odd error
> to get. It basically means the file header is wrong. Maybe some
> concurrency/fflush issue in the test?

Sent a fix:

https://lore.kernel.org/linux-sgx/20201118170640.39629-1-jarkko@kernel.org/T/#u

> --
> Jethro Beekman | Fortanix
> 

/Jarkko

Re: [PATCH] selftests/x86: Fix malformed src_offset initialization

[Jarkko Sakkinen]

On Wed, Nov 18, 2020 at 12:11:23PM +0100, Borislav Petkov wrote:
> On Wed, Nov 18, 2020 at 12:36:30AM +0200, Jarkko Sakkinen wrote:
> > Assign src_offset just to the p_offset, when first initialized.
> > This has been probably copy-pasting accident (at least looks like
> > it).
> > 
> > Cc: Borislav Petkov <bp@alien8.de>
> > Cc: Shuah Khan <shuah@kernel.org>
> > Cc: linux-kselftest@vger.kernel.org
> > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > ---
> >  tools/testing/selftests/sgx/load.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tools/testing/selftests/sgx/load.c b/tools/testing/selftests/sgx/load.c
> > index 07988de6b767..64976f266bae 100644
> > --- a/tools/testing/selftests/sgx/load.c
> > +++ b/tools/testing/selftests/sgx/load.c
> > @@ -185,7 +185,7 @@ bool encl_load(const char *path, struct encl *encl)
> >  		}
> >  
> >  		if (j == 0) {
> > -			src_offset = (phdr->p_offset & PAGE_MASK) - src_offset;
> > +			src_offset = (phdr->p_offset & PAGE_MASK);
> >  
> >  			seg->prot = PROT_READ | PROT_WRITE;
> >  			seg->flags = SGX_PAGE_TYPE_TCS << 8;
> > -- 
> 
> Still no joy:
> 
> 0x0000000000000000 0x0000000000002000 0x03
> 0x0000000000002000 0x0000000000001000 0x05
> 0x0000000000003000 0x0000000000003000 0x03
> encl_load: encl->nr_segments: 3
> encl_load: seg2 offset: 0x3000, seg2 size: 12288
> encl_load: encl_size: 32768, src_size: 24576
> encl_map_area: encl_size: 32768
> encl_map_area: area: 0x0x7f7ec8dd8000
> encl_map_area: encl_base: 0x7f7ec8dd8000
> mapping segment 0, seg->prot: (read write )
> base: 0x7f7ec8dd8000, offset 0x0, size: 8192
> mapping segment 1, seg->prot: (read  exec)
> base: 0x7f7ec8dd8000, offset 0x2000, size: 4096
> mmap() failed, errno=1.
> mmap: Operation not permitted
> 
> That second segment is PROT_EXEC and mmap(2) manpage says:
> 
>        EPERM  The prot argument asks for PROT_EXEC but the mapped  area  belongs  to  a
>               file on a filesystem that was mounted no-exec.
> 
>        EPERM  The operation was prevented by a file seal; see fcntl(2).
> 
> I don't see fcntl() calls in the test and the fs I'm running it from is
> not mapped "no-exec":
> 
> /dev/nvme0n1p2 on / type ext4 (rw,relatime,errors=remount-ro)
> 
> so something else is missing.

What about "/dev"?

I.e. I have

udev on /dev type devtmpfs (rw,nosuid,relatime,size=1878244k,nr_inodes=469561,mode=755)

> Thx.
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette

/Jarkko

Re: [PATCH] selftests/x86: Fix malformed src_offset initialization

[Borislav Petkov]

On Wed, Nov 18, 2020 at 12:36:30AM +0200, Jarkko Sakkinen wrote:
> Assign src_offset just to the p_offset, when first initialized.
> This has been probably copy-pasting accident (at least looks like
> it).
> 
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Shuah Khan <shuah@kernel.org>
> Cc: linux-kselftest@vger.kernel.org
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> ---
>  tools/testing/selftests/sgx/load.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/sgx/load.c b/tools/testing/selftests/sgx/load.c
> index 07988de6b767..64976f266bae 100644
> --- a/tools/testing/selftests/sgx/load.c
> +++ b/tools/testing/selftests/sgx/load.c
> @@ -185,7 +185,7 @@ bool encl_load(const char *path, struct encl *encl)
>  		}
>  
>  		if (j == 0) {
> -			src_offset = (phdr->p_offset & PAGE_MASK) - src_offset;
> +			src_offset = (phdr->p_offset & PAGE_MASK);
>  
>  			seg->prot = PROT_READ | PROT_WRITE;
>  			seg->flags = SGX_PAGE_TYPE_TCS << 8;
> -- 

Folded in after removing the brackets and pushed the whole thing out.
Phew, finally!

From now on, only fixes ontop pls and testing tip:x86/sgx would be of
great help.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] selftests/x86: Fix malformed src_offset initialization

[Jarkko Sakkinen]

On Wed, Nov 18, 2020 at 06:19:00PM +0100, Borislav Petkov wrote:
> On Wed, Nov 18, 2020 at 12:36:30AM +0200, Jarkko Sakkinen wrote:
> > Assign src_offset just to the p_offset, when first initialized.
> > This has been probably copy-pasting accident (at least looks like
> > it).
> > 
> > Cc: Borislav Petkov <bp@alien8.de>
> > Cc: Shuah Khan <shuah@kernel.org>
> > Cc: linux-kselftest@vger.kernel.org
> > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > ---
> >  tools/testing/selftests/sgx/load.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tools/testing/selftests/sgx/load.c b/tools/testing/selftests/sgx/load.c
> > index 07988de6b767..64976f266bae 100644
> > --- a/tools/testing/selftests/sgx/load.c
> > +++ b/tools/testing/selftests/sgx/load.c
> > @@ -185,7 +185,7 @@ bool encl_load(const char *path, struct encl *encl)
> >  		}
> >  
> >  		if (j == 0) {
> > -			src_offset = (phdr->p_offset & PAGE_MASK) - src_offset;
> > +			src_offset = (phdr->p_offset & PAGE_MASK);
> >  
> >  			seg->prot = PROT_READ | PROT_WRITE;
> >  			seg->flags = SGX_PAGE_TYPE_TCS << 8;
> > -- 
> 
> Folded in after removing the brackets and pushed the whole thing out.
> Phew, finally!
> 
> From now on, only fixes ontop pls and testing tip:x86/sgx would be of
> great help.
> 
> Thx.

Duh, I sent the fix for the selftest before seeing this.

Thank you, I'll separately clone tip and test it.

> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette

/Jarkko

Re: [PATCH] selftests/x86: Fix malformed src_offset initialization

[Borislav Petkov]

On Wed, Nov 18, 2020 at 07:58:50PM +0200, Jarkko Sakkinen wrote:
> Duh, I sent the fix for the selftest before seeing this.

All, good - your static key fix is queued now too. The only thing that's
needs work now is the anon inode thing but that can come later.

> Thank you, I'll separately clone tip and test it.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] selftests/x86: Fix malformed src_offset initialization

[Jarkko Sakkinen]

On Wed, Nov 18, 2020 at 07:04:50PM +0100, Borislav Petkov wrote:
> On Wed, Nov 18, 2020 at 07:58:50PM +0200, Jarkko Sakkinen wrote:
> > Duh, I sent the fix for the selftest before seeing this.
> 
> All, good - your static key fix is queued now too. The only thing that's
> needs work now is the anon inode thing but that can come later.

Just checking that I got this right: you want me to port my anon inode
changes from March to be applied on top of tip and send them?

> > Thank you, I'll separately clone tip and test it.
> 
> Thx.
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette

/Jarkko

Re: [PATCH] selftests/x86: Fix malformed src_offset initialization

[Borislav Petkov]

On Wed, Nov 18, 2020 at 11:37:55PM +0200, Jarkko Sakkinen wrote:
> Just checking that I got this right: you want me to port my anon inode
> changes from March to be applied on top of tip and send them?

Well, we need to somehow address the issue when some distros map /dev
noexec and that is conflicting with SGX due to it needing to mmap with
executable permissions but /dev/sgx_enclave is noexec...

I guess the first thing that needs figuring out is why are some distros
mounting /dev noexec.

I mean, you can always do the easiest thing: somewhere in the SGX
docs say that one of the steps towards running SGX enclaves on such
distros is for the admin to map /dev exec. However, does that have other
security implications which would make such exec mounting a security
hazard?

If so, then the SGX code would need changing...

Questions like those.

HTH.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] selftests/x86: Fix malformed src_offset initialization

[Andy Lutomirski]

> On Nov 18, 2020, at 1:54 PM, Borislav Petkov <bp@alien8.de> wrote:
> 
> On Wed, Nov 18, 2020 at 11:37:55PM +0200, Jarkko Sakkinen wrote:
>> Just checking that I got this right: you want me to port my anon inode
>> changes from March to be applied on top of tip and send them?
> 
> Well, we need to somehow address the issue when some distros map /dev
> noexec and that is conflicting with SGX due to it needing to mmap with
> executable permissions but /dev/sgx_enclave is noexec...
> 
> I guess the first thing that needs figuring out is why are some distros
> mounting /dev noexec.
> 
> I mean, you can always do the easiest thing: somewhere in the SGX
> docs say that one of the steps towards running SGX enclaves on such
> distros is for the admin to map /dev exec. However, does that have other
> security implications which would make such exec mounting a security
> hazard?
> 
> If so, then the SGX code would need changing...
> 
> Questions like those.

I thought we had determined that this was solvable entirely in userspace.  Udev can handle this, no?

> 
> HTH.
> 
> -- 
> Regards/Gruss,
>    Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] selftests/x86: Fix malformed src_offset initialization

[Jarkko Sakkinen]

On Wed, Nov 18, 2020 at 05:16:53PM -0800, Andy Lutomirski wrote:
> 
> 
> > On Nov 18, 2020, at 1:54 PM, Borislav Petkov <bp@alien8.de> wrote:
> > 
> > On Wed, Nov 18, 2020 at 11:37:55PM +0200, Jarkko Sakkinen wrote:
> >> Just checking that I got this right: you want me to port my anon inode
> >> changes from March to be applied on top of tip and send them?
> > 
> > Well, we need to somehow address the issue when some distros map /dev
> > noexec and that is conflicting with SGX due to it needing to mmap with
> > executable permissions but /dev/sgx_enclave is noexec...
> > 
> > I guess the first thing that needs figuring out is why are some distros
> > mounting /dev noexec.
> > 
> > I mean, you can always do the easiest thing: somewhere in the SGX
> > docs say that one of the steps towards running SGX enclaves on such
> > distros is for the admin to map /dev exec. However, does that have other
> > security implications which would make such exec mounting a security
> > hazard?
> > 
> > If so, then the SGX code would need changing...
> > 
> > Questions like those.
> 
> I thought we had determined that this was solvable entirely in
> userspace.  Udev can handle this, no?

Check my response to Boris.

In that seame thread you said that you would post to udev mailing list
about the matter? Did you ever got around? Not sure if it matters
though. I think we have reasonable information that this is the right
solution.

https://lore.kernel.org/linux-sgx/CALCETrVBO8ceeT8qXw2rDQgdzJH8U-YLpYNMDGC0VudD4VgTCQ@mail.gmail.com/


> > 
> > HTH.
> > 
> > -- 
> > Regards/Gruss,
> >    Boris.
> > 
> > https://people.kernel.org/tglx/notes-about-netiquette

/Jarkko

Re: [PATCH] selftests/x86: Fix malformed src_offset initialization

[Jarkko Sakkinen]

On Wed, Nov 18, 2020 at 10:54:46PM +0100, Borislav Petkov wrote:
> On Wed, Nov 18, 2020 at 11:37:55PM +0200, Jarkko Sakkinen wrote:
> > Just checking that I got this right: you want me to port my anon inode
> > changes from March to be applied on top of tip and send them?
> 
> Well, we need to somehow address the issue when some distros map /dev
> noexec and that is conflicting with SGX due to it needing to mmap with
> executable permissions but /dev/sgx_enclave is noexec...
> 
> I guess the first thing that needs figuring out is why are some distros
> mounting /dev noexec.
> 
> I mean, you can always do the easiest thing: somewhere in the SGX
> docs say that one of the steps towards running SGX enclaves on such
> distros is for the admin to map /dev exec. However, does that have other
> security implications which would make such exec mounting a security
> hazard?
> 
> If so, then the SGX code would need changing...
> 
> Questions like those.
> 
> HTH.

OK I did re-read the whole thing:

https://lore.kernel.org/linux-sgx/20200331114432.7593-1-jarkko.sakkinen@linux.intel.com/

As Topi (the Debian developer who did this change) stated in the thread,
the threat scenario is that someone could create executable files to
/dev if it isn't noexec. It's not about the permissions of device files
per se.

The problem with anonymous inode is that LSM's cannot label it easily
like you can a non-anonymous inode.

There's a patch set for secure anonymous inodes but it hasn't landed yet
and I think it would make the whole security model somewhat more
complicated [1]. What I mean is that secure anonymous inodes would make
sense in a context where anonymous inodes are used for other reasons to
begin with.

The route of using sgxfs was discarded because it results static
permissions and is also quite big hammer for the purpose [2].

In that thread there were two valid routes to move forward:

1. You can always create a separate tmpfs and create enclave nodes
   and even bind mount them (proposed by Andy) to /dev.
2. It would be possible (given how Topi described the threat scenario)
   implement "noexec_dev" mount option that would allow 'x' for dev's
   but not for anything else (proposed by me).

To summarize, I would not change the SGX code for this but apply either
(1) and (2) when deploying SGX.

> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette

[1] https://lore.kernel.org/linux-security-module/20201011082936.4131726-1-lokeshgidra@google.com/
[2] https://lore.kernel.org/linux-sgx/CALCETrWkaUS2RU61_4KoNhn3RkW-J+SWiCQTZ623wu4b7snJ0w@mail.gmail.com/

/Jarkko

Re: [PATCH] selftests/x86: Fix malformed src_offset initialization

[Borislav Petkov]

On Thu, Nov 19, 2020 at 06:41:44PM +0200, Jarkko Sakkinen wrote:
> As Topi (the Debian developer who did this change) stated in the thread,
> the threat scenario is that someone could create executable files to
> /dev if it isn't noexec. It's not about the permissions of device files
> per se.

Aha, makes sense.

> The problem with anonymous inode is that LSM's cannot label it easily
> like you can a non-anonymous inode.
> 
> There's a patch set for secure anonymous inodes but it hasn't landed yet
> and I think it would make the whole security model somewhat more
> complicated [1].

More complicated is never good.

> 1. You can always create a separate tmpfs and create enclave nodes
>    and even bind mount them (proposed by Andy) to /dev.
> 2. It would be possible (given how Topi described the threat scenario)
>    implement "noexec_dev" mount option that would allow 'x' for dev's
>    but not for anything else (proposed by me).
> 
> To summarize, I would not change the SGX code for this but apply either
> (1) and (2) when deploying SGX.

Both sound ok to me, especially 1 since you can do that now, without any
changes needed.

Anything else means adding more code which for distros and people using
older kernels means, more backporting and more verif.

But WTH do I know...

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] selftests/x86: Fix malformed src_offset initialization

[Dr. Greg]

On Wed, Nov 18, 2020 at 11:37:55PM +0200, Jarkko Sakkinen wrote:

Good evening, I hope the week is going well for everyone.

> On Wed, Nov 18, 2020 at 07:04:50PM +0100, Borislav Petkov wrote:
> > On Wed, Nov 18, 2020 at 07:58:50PM +0200, Jarkko Sakkinen wrote:
> > > Duh, I sent the fix for the selftest before seeing this.
> > 
> > All, good - your static key fix is queued now too. The only thing that's
> > needs work now is the anon inode thing but that can come later.

> Just checking that I got this right: you want me to port my anon
> inode changes from March to be applied on top of tip and send them?

Given this issue, I would submit that you also need to consider the
patch that I sent over the weekend that unconditionally blocks
mmap/mprotect on an initialized enclave.

The issue with a noexec /dev filesystem goes on to confirm that the
page permission callback architecture, while certainly elegant, won't
work given the current architecture of the driver and the SGX hardware
itself.

The stashed page permissions are derived from the enclave permissions
set by the enclave author.  To be useful for the JIT model that Andy
described, the 'maximal' permissions would need to include WX.

Setting these types of permissions is problematic, not only from the
perspective of a noexec filesystem, which will presumably get fixed by
the anonymous inode, but it also triggers the very LSM issues that
started the re-design of all this a year ago.

> /Jarkko

Have a good evening.

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
------------------------------------------------------------------------------
"Opportunity is missed by most people because it is dressed in overalls
 and looks like work."
                                -- Thomas Edison