| Message ID | 20201117223630.17355-1-jarkko@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | selftests/x86: Fix malformed src_offset initialization | expand |
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.
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
> 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
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
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
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
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;
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(-)