* Re: [RFC PATCH v4 00/12] security: x86/sgx: SGX vs. LSM
From: Xing, Cedric @ 2019-07-12 0:08 UTC (permalink / raw)
To: Jarkko Sakkinen, Stephen Smalley
Cc: Sean Christopherson, linux-sgx, linux-security-module, selinux,
Bill Roberts, Casey Schaufler, James Morris, Dave Hansen,
Andy Lutomirski, Jethro Beekman, Dr . Greg Wettstein
In-Reply-To: <20190711175151.xwxdfbvxbm6esymk@linux.intel.com>
On 7/11/2019 10:51 AM, Jarkko Sakkinen wrote:
> On Thu, Jul 11, 2019 at 10:32:41AM -0400, Stephen Smalley wrote:
>> The existing permissions don't map cleanly to SGX but I think Sean and
>> Cedric were trying to make a best-effort approximation to the underlying
>> concepts in a manner that permits control over the introduction of
>> executable content.
>>
>> Sure, the existing EXECMOD check is only applied today when there is an
>> attempt to make executable a previously modified (detected based on COW
>> having occurred) private file mapping. But the general notion of
>> controlling the ability to execute modified content is still meaningful.
>
> OK to summarize EXECMOD does not connect with SGX in any possible way
> but SGX needs something that mimics EXECMOD behaviour?
Stephen may correct me if I'm wrong. EXECMOD is granted to files, to
indicate the bearer contains self-modifying code (or text relocation).
So if it applies the enclaves, there are two aspects of it:
(1) An enclave may be loaded from multiple image files, among which some
may contain self-modifying code and hence would require EXECMOD on each
of them. At runtime, W->X will be allowed/denied for pages loaded from
image files having/not having EXECMOD.
(2) But there are pages not loaded from any files - e.g. pages EAUG'ed
at runtime. We are trying to use the file containing SIGSTRUCT as the
"proxy file" - i.e. EXECMOD on the proxy file indicates the enclave may
load code into EAUG'ed pages at runtime.
(3) Well, this is not an aspect of EXECMOD. Yet there's another category
of pages - pages EADD'ed from anonymous memory. They are different than
EAUG'ed pages in that their contents are supplied by untrusted code. How
to control their accesses is still being debated. My argument was that
the source pages must be executable before they could be loaded
executable in EPC. Andy argued that SIGSTRUCT alone could be considered
sufficient validation on the contents in certain usages, so both Sean
and I had proposed PROCESS2__ENCLAVE_EXECANON as a new permission. But
for the 1st upstream I think we all agree to do just the minimum until
real requirements come up. After all, adding is generally easier than
taking away existing things.
^ permalink raw reply
* Re: [RFC PATCH v3 3/4] X86/sgx: Introduce EMA as a new LSM module
From: Xing, Cedric @ 2019-07-11 23:41 UTC (permalink / raw)
To: Stephen Smalley, Sean Christopherson
Cc: Casey Schaufler, linux-sgx, linux-security-module, selinux,
James Morris, Paul Moore
In-Reply-To: <47feea29-68c2-83ee-497a-83b616d2f90c@tycho.nsa.gov>
On 7/11/2019 9:32 AM, Stephen Smalley wrote:
> On 7/11/19 12:25 PM, Sean Christopherson wrote:
>> On Thu, Jul 11, 2019 at 12:11:06PM -0400, Stephen Smalley wrote:
>>> On 7/11/19 11:12 AM, Sean Christopherson wrote:
>>>> On Thu, Jul 11, 2019 at 09:51:19AM -0400, Stephen Smalley wrote:
>>>>> I'd also feel better if there was clear consensus among all of the
>>>>> @intel.com participants that this is the right approach. To date
>>>>> that has
>>>>> seemed elusive.
>>>>
>>>> That's a very kind way to phrase things :-)
>>>>
>>>> For initial upstreaming, we've agreed that there is no need to
>>>> extend the
>>>> uapi, i.e. we can punt on deciding between on-the-fly tracking and
>>>> having
>>>> userspace specify maximal permissions until we add SGX2 support.
>>>>
>>>> The last open (knock on wood) for initial upstreaming is whether
>>>> SELinux
>>>> would prefer to have new enclave specific permissions or reuse the
>>>> existing PROCESS__EXECMEM, FILE__EXECUTE and FILE__EXECMOD permissions.
>>>> My understanding is that enclave specific permissions are preferred.
>>>
>>> I was left unclear on this topic after the email exchanges with Cedric.
>>> There are at least three options:
>>>
>>> 1) Reuse the existing EXECMEM, EXECUTE, and EXECMOD permissions. Pros:
>>> Existing distro policies will be applied in the expected manner with
>>> respect
>>> to the introduction of executable code into the system, consistent
>>> control
>>> will be provided over the enclave and the host process, no change for
>>> users/documentation wrt policy. Cons: Existing permissions don't map
>>> exactly to SGX semantics, no ability to distinguish executable content
>>> within the enclave versus the host process at the LSM level (argued
>>> earlier
>>> by Cedric to be unnecessary and perhaps meaningless), need to allow
>>> FILE__EXECUTE or other checks on sigstruct files that may not actually
>>> contain code.
>>>
>>> 2) Define new permissions within existing security classes (e.g.
>>> process2,
>>> file). Pros: Can tailor permission names and definitions to SGX
>>> semantics,
>>> ability to distinguish enclave versus host process execute access, no
>>> need
>>> to grant FILE__EXECUTE to sigstruct files, class matches the target
>>> object,
>>> permissions computed and cached upon existing checks (i.e. when a
>>> process
>>> accesses a file, all of the permissions to that file are computed and
>>> then
>>> cached at once, including the enclave-related ones). Cons: Typical
>>> distro
>>> policies (unlike Android) allow unknown permissions by default for
>>> forward
>>> kernel compatibility reasons, so existing policies will permit these new
>>> permissions by default and enforcement will only truly take effect once
>>> policies are updated, adding new permissions to existing classes
>>> requires an
>>> update to the base policy (so they can't be shipped as a third party
>>> policy
>>> module alongside the SGX driver or installed as a local module by an
>>> admin,
>>> for example), documentation/user education required for new permissions.
>>>
>>> 3) Define new permissions in new security classes (e.g. enclave). Pros
>>> relative to #2: New classes and permissions can be defined and
>>> installed in
>>> third party or local policy module without requiring a change to the
>>> base
>>> policy. Cons relative to #2: Class won't correspond to the target
>>> object,
>>> permissions won't be computed and cached upon existing checks (only when
>>> performing the checks against the new classes).
>>>
>>> Combinations are also possible, of course.
>>
>> What's the impact on distros/ecosystems if we go with #1 for now and
>> later
>> decide to switch to #2 after upstreaming? I.e. can we take a minimal-ish
>> approach now without painting ourselves into a corner?
>
> Yes, I think that's fine.
I can't agree more on this. It's easier to add new things than to take
existing things out. We can just wait until usages come up that really
require new permissions.
^ permalink raw reply
* Re: possible deadlock in process_measurement
From: Mimi Zohar @ 2019-07-11 21:12 UTC (permalink / raw)
To: Eric Biggers
Cc: syzbot, dmitry.kasatkin, jmorris, linux-integrity, linux-kernel,
linux-security-module, serge, syzkaller-bugs, zohar
In-Reply-To: <1562869533.4014.56.camel@linux.ibm.com>
Hi Eric,
> > the existing dependency chain (in reverse order) is:
> >
> > -> #1 (&mm->mmap_sem#2){++++}:
> > down_read+0x3f/0x1e0 kernel/locking/rwsem.c:24
> > get_user_pages_unlocked+0xfc/0x4a0 mm/gup.c:1174
> > __gup_longterm_unlocked mm/gup.c:2193 [inline]
> > get_user_pages_fast+0x43f/0x530 mm/gup.c:2245
> > iov_iter_get_pages+0x2c2/0xf80 lib/iov_iter.c:1287
> > dio_refill_pages fs/direct-io.c:171 [inline]
> > dio_get_page fs/direct-io.c:215 [inline]
> > do_direct_IO fs/direct-io.c:983 [inline]
> > do_blockdev_direct_IO+0x3f7b/0x8e00 fs/direct-io.c:1336
> > __blockdev_direct_IO+0xa1/0xca fs/direct-io.c:1422
> > ext4_direct_IO_write fs/ext4/inode.c:3782 [inline]
> > ext4_direct_IO+0xaa7/0x1bb0 fs/ext4/inode.c:3909
> > generic_file_direct_write+0x20a/0x4a0 mm/filemap.c:3110
> > __generic_file_write_iter+0x2ee/0x630 mm/filemap.c:3293
> > ext4_file_write_iter+0x332/0x1070 fs/ext4/file.c:266
> > call_write_iter include/linux/fs.h:1870 [inline]
> > new_sync_write+0x4d3/0x770 fs/read_write.c:483
> > __vfs_write+0xe1/0x110 fs/read_write.c:496
> > vfs_write+0x268/0x5d0 fs/read_write.c:558
> > ksys_write+0x14f/0x290 fs/read_write.c:611
> > __do_sys_write fs/read_write.c:623 [inline]
> > __se_sys_write fs/read_write.c:620 [inline]
> > __x64_sys_write+0x73/0xb0 fs/read_write.c:620
> > do_syscall_64+0xfd/0x680 arch/x86/entry/common.c:301
> > entry_SYSCALL_64_after_hwframe+0x49/0xbe
> >
> > -> #0 (&sb->s_type->i_mutex_key#10){+.+.}:
> > lock_acquire+0x16f/0x3f0 kernel/locking/lockdep.c:4300
> > down_write+0x38/0xa0 kernel/locking/rwsem.c:66
> > inode_lock include/linux/fs.h:778 [inline]
> > process_measurement+0x15ae/0x15e0
> > security/integrity/ima/ima_main.c:228
> > ima_file_mmap+0x11a/0x130 security/integrity/ima/ima_main.c:370
> > security_file_mprotect+0xd5/0x100 security/security.c:1430
> > do_mprotect_pkey+0x537/0xa30 mm/mprotect.c:550
> > __do_sys_pkey_mprotect mm/mprotect.c:590 [inline]
> > __se_sys_pkey_mprotect mm/mprotect.c:587 [inline]
> > __x64_sys_pkey_mprotect+0x97/0xf0 mm/mprotect.c:587
> > do_syscall_64+0xfd/0x680 arch/x86/entry/common.c:301
> > entry_SYSCALL_64_after_hwframe+0x49/0xbe
> >
> > other info that might help us debug this:
> >
> > Possible unsafe locking scenario:
> >
> > CPU0 CPU1
> > ---- ----
> > lock(&mm->mmap_sem#2);
> > lock(&sb->s_type->i_mutex_key#10);
> > lock(&mm->mmap_sem#2);
> > lock(&sb->s_type->i_mutex_key#10);
> >
> > *** DEADLOCK ***
The locking on CPU1 shouldn't be nested. Only after the call to
security_file_mmap() would the mmap_sem be taken.
Mimi
> >
> > 1 lock held by syz-executor395/17373:
> > #0: 00000000e0714fc5 (&mm->mmap_sem#2){++++}, at:
> > do_mprotect_pkey+0x1f6/0xa30 mm/mprotect.c:485
> >
> > stack backtrace:
> > CPU: 1 PID: 17373 Comm: syz-executor395 Not tainted 5.2.0-rc2-next-20190531
> > #4
> > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
> > Google 01/01/2011
> > Call Trace:
> > __dump_stack lib/dump_stack.c:77 [inline]
> > dump_stack+0x172/0x1f0 lib/dump_stack.c:113
> > print_circular_bug.cold+0x1cc/0x28f kernel/locking/lockdep.c:1566
> > check_prev_add kernel/locking/lockdep.c:2311 [inline]
> > check_prevs_add kernel/locking/lockdep.c:2419 [inline]
> > validate_chain kernel/locking/lockdep.c:2801 [inline]
> > __lock_acquire+0x3755/0x5490 kernel/locking/lockdep.c:3790
> > lock_acquire+0x16f/0x3f0 kernel/locking/lockdep.c:4300
> > down_write+0x38/0xa0 kernel/locking/rwsem.c:66
> > inode_lock include/linux/fs.h:778 [inline]
> > process_measurement+0x15ae/0x15e0 security/integrity/ima/ima_main.c:228
> > ima_file_mmap+0x11a/0x130 security/integrity/ima/ima_main.c:370
> > security_file_mprotect+0xd5/0x100 security/security.c:1430
> > do_mprotect_pkey+0x537/0xa30 mm/mprotect.c:550
> > __do_sys_pkey_mprotect mm/mprotect.c:590 [inline]
> > __se_sys_pkey_mprotect mm/mprotect.c:587 [inline]
> > __x64_sys_pkey_mprotect+0x97/0xf0 mm/mprotect.c:587
> > do_syscall_64+0xfd/0x680 arch/x86/entry/common.c:301
> > entry_SYSCALL_64_after_hwframe+0x49/0xbe
> > RIP: 0033:0x440279
> > Code: 18 89 d0 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 00 48 89 f8 48 89 f7
> > 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff
> > ff 0f 83 fb 13 fc ff c3 66 2e 0f 1f 84 00 00 00 00
> > RSP: 002b:00007ffeec2f48d8 EFLAGS: 00000246 ORIG_RAX: 0000000000000149
> > RAX: ffffffffffffffda RBX: 00000000004002c8 RCX: 0000000000440279
> > RDX: 000000000000000
> >
>
^ permalink raw reply
* Re: possible deadlock in process_measurement
From: Eric Biggers @ 2019-07-11 19:50 UTC (permalink / raw)
To: Mimi Zohar
Cc: syzbot, dmitry.kasatkin, jmorris, linux-integrity, linux-kernel,
linux-security-module, serge, syzkaller-bugs, zohar
In-Reply-To: <1562854476.4014.47.camel@linux.ibm.com>
Hi Mimi,
On Thu, Jul 11, 2019 at 10:14:36AM -0400, Mimi Zohar wrote:
> Hi Eric,
>
> On Mon, 2019-06-03 at 09:35 -0700, syzbot wrote:
> > syzbot has found a reproducer for the following crash on:
> >
> > HEAD commit: 3c09c195 Add linux-next specific files for 20190531
> > git tree: linux-next
> > console output: https://syzkaller.appspot.com/x/log.txt?x=10f61a0ea00000
> > kernel config: https://syzkaller.appspot.com/x/.config?x=6cfb24468280cd5c
> > dashboard link: https://syzkaller.appspot.com/bug?extid=5ab61747675a87ea359d
> > compiler: gcc (GCC) 9.0.0 20181231 (experimental)
> > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=177c3d16a00000
> > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=14ec01baa00000
> >
>
> This reproducer seems like it is similar, but the cause is different
> than the original report. One has to do with overlayfs, while the
> other has to do with ext4, mprotect/mmap. I assume in both cases an
> IMA policy was required to trigger the locking bug. What type of IMA
> policy are you using?
>
> Do we need to differentiate the two reports? Is the "last occurred"
> notification for the overlay, for mprotect, or both? Please Cc the
> overlay mailing list on the overlay aspect.
AFAICS, syzbot boots all kernels with "ima_policy=tcb" on the command line.
And I don't think anything in userspace changes the IMA policy.
It's not unusual for multiple underlying bugs to get mixed into the same syzbot
bug. syzbot doesn't know that one "possible deadlock in process_measurement" is
different from another. "Last occurred" is for any crash that appeared as such.
This just needs to be handled the best we can. Sometimes all the bugs can be
fixed; sometimes they've already been fixed; or sometimes it's easiest to fix
just one and then mark the syzbot bug as fixed, and syzbot will report it again
it's still occurring for some other reason.
- Eric
^ permalink raw reply
* Re: [PATCH] KEYS: trusted: allow module init if TPM is inactive or deactivated
From: Jarkko Sakkinen @ 2019-07-11 19:48 UTC (permalink / raw)
To: Roberto Sassu
Cc: jejb, zohar, jgg, linux-integrity, linux-security-module,
keyrings, linux-kernel, crazyt2019+lml, tyhicks, nayna,
silviu.vlasceanu
In-Reply-To: <20190705163735.11539-1-roberto.sassu@huawei.com>
On Fri, Jul 05, 2019 at 06:37:35PM +0200, Roberto Sassu wrote:
> Commit c78719203fc6 ("KEYS: trusted: allow trusted.ko to initialize w/o a
> TPM") allows the trusted module to be loaded even a TPM is not found to
> avoid module dependency problems.
>
> Unfortunately, this does not completely solve the issue, as there could be
> a case where a TPM is found but is not functional (the TPM commands return
> an error). Specifically, after the tpm_chip structure is returned by
> tpm_default_chip() in init_trusted(), the execution terminates after
> init_digests() returns -EFAULT (due to the fact that tpm_get_random()
> returns a positive value, but less than TPM_MAX_DIGEST_SIZE).
>
> This patch fixes the issue by ignoring the TPM_ERR_DEACTIVATED and
> TPM_ERR_DISABLED errors.
Why allow trusted module to initialize if TPM is not functional?
Also:
err = tpm_transmit_cmd(chip, &buf,
offsetof(struct tpm2_get_random_out,
buffer),
"attempting get random");
if (err) {
if (err > 0)
err = -EIO;
goto out;
}
/Jarkko
^ permalink raw reply
* Re: [RFC/RFT] KEYS: trusted: Add generic trusted keys framework
From: Jarkko Sakkinen @ 2019-07-11 19:22 UTC (permalink / raw)
To: Sumit Garg
Cc: keyrings, linux-integrity, linux-crypto, linux-security-module,
dhowells, herbert, davem, jejb, zohar, jmorris, serge, casey,
ard.biesheuvel, daniel.thompson, linux-kernel, tee-dev
In-Reply-To: <1562337154-26376-1-git-send-email-sumit.garg@linaro.org>
On Fri, Jul 05, 2019 at 08:02:34PM +0530, Sumit Garg wrote:
> Current trusted keys framework is tightly coupled to use TPM device as
> an underlying implementation which makes it difficult for implementations
> like Trusted Execution Environment (TEE) etc. to provide trusked keys
> support in case platform doesn't posses a TPM device.
>
> So this patch tries to add generic trusted keys framework where underlying
> implemtations like TPM, TEE etc. could be easily plugged-in.
>
> Suggested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
1. Needs to be somehow dissected into digestable/reviewable pieces.
2. As a precursory step probably would make sense to move all
existing trusted keys code into one subsystem first.
/Jarkko
^ permalink raw reply
* Re: possible deadlock in process_measurement
From: Mimi Zohar @ 2019-07-11 18:25 UTC (permalink / raw)
To: Eric Biggers
Cc: syzbot, dmitry.kasatkin, jmorris, linux-integrity, linux-kernel,
linux-security-module, serge, syzkaller-bugs, zohar
In-Reply-To: <00000000000054e5d1058a6df2eb@google.com>
Hi Eric,
On Mon, 2019-06-03 at 09:35 -0700, syzbot wrote:
> syzbot has found a reproducer for the following crash on:
>
> HEAD commit: 3c09c195 Add linux-next specific files for 20190531
> git tree: linux-next
> console output: https://syzkaller.appspot.com/x/log.txt?x=10f61a0ea00000
> kernel config: https://syzkaller.appspot.com/x/.config?x=6cfb24468280cd5c
> dashboard link: https://syzkaller.appspot.com/bug?extid=5ab61747675a87ea359d
> compiler: gcc (GCC) 9.0.0 20181231 (experimental)
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=177c3d16a00000
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=14ec01baa00000
>
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+5ab61747675a87ea359d@syzkaller.appspotmail.com
>
> get_swap_device: Bad swap file entry 6000000000000001
> get_swap_device: Bad swap file entry 6400000000000001
> get_swap_device: Bad swap file entry 6800000000000001
> get_swap_device: Bad swap file entry 6c00000000000001
> get_swap_device: Bad swap file entry 7000000000000001
> get_swap_device: Bad swap file entry 7400000000000001
> ======================================================
> WARNING: possible circular locking dependency detected
> 5.2.0-rc2-next-20190531 #4 Not tainted
> ------------------------------------------------------
> syz-executor395/17373 is trying to acquire lock:
> 000000003d1a4a53 (&sb->s_type->i_mutex_key#10){+.+.}, at: inode_lock
> include/linux/fs.h:778 [inline]
> 000000003d1a4a53 (&sb->s_type->i_mutex_key#10){+.+.}, at:
> process_measurement+0x15ae/0x15e0 security/integrity/ima/ima_main.c:228
>
> but task is already holding lock:
> 00000000e0714fc5 (&mm->mmap_sem#2){++++}, at: do_mprotect_pkey+0x1f6/0xa30
> mm/mprotect.c:485
>
> which lock already depends on the new lock.
More information is needed here. Missing is the IMA policy. In
particular whether the IMA policy has "permit_directio" enabled? Is
it enabled for all files or particular ones. If I'm reading the log
properly, it looks like the IMA mmap policy rule does not exclude the
pemitted_directio files.
Do you have any suggestions on how to resolve this?
Mimi
>
>
> the existing dependency chain (in reverse order) is:
>
> -> #1 (&mm->mmap_sem#2){++++}:
> down_read+0x3f/0x1e0 kernel/locking/rwsem.c:24
> get_user_pages_unlocked+0xfc/0x4a0 mm/gup.c:1174
> __gup_longterm_unlocked mm/gup.c:2193 [inline]
> get_user_pages_fast+0x43f/0x530 mm/gup.c:2245
> iov_iter_get_pages+0x2c2/0xf80 lib/iov_iter.c:1287
> dio_refill_pages fs/direct-io.c:171 [inline]
> dio_get_page fs/direct-io.c:215 [inline]
> do_direct_IO fs/direct-io.c:983 [inline]
> do_blockdev_direct_IO+0x3f7b/0x8e00 fs/direct-io.c:1336
> __blockdev_direct_IO+0xa1/0xca fs/direct-io.c:1422
> ext4_direct_IO_write fs/ext4/inode.c:3782 [inline]
> ext4_direct_IO+0xaa7/0x1bb0 fs/ext4/inode.c:3909
> generic_file_direct_write+0x20a/0x4a0 mm/filemap.c:3110
> __generic_file_write_iter+0x2ee/0x630 mm/filemap.c:3293
> ext4_file_write_iter+0x332/0x1070 fs/ext4/file.c:266
> call_write_iter include/linux/fs.h:1870 [inline]
> new_sync_write+0x4d3/0x770 fs/read_write.c:483
> __vfs_write+0xe1/0x110 fs/read_write.c:496
> vfs_write+0x268/0x5d0 fs/read_write.c:558
> ksys_write+0x14f/0x290 fs/read_write.c:611
> __do_sys_write fs/read_write.c:623 [inline]
> __se_sys_write fs/read_write.c:620 [inline]
> __x64_sys_write+0x73/0xb0 fs/read_write.c:620
> do_syscall_64+0xfd/0x680 arch/x86/entry/common.c:301
> entry_SYSCALL_64_after_hwframe+0x49/0xbe
>
> -> #0 (&sb->s_type->i_mutex_key#10){+.+.}:
> lock_acquire+0x16f/0x3f0 kernel/locking/lockdep.c:4300
> down_write+0x38/0xa0 kernel/locking/rwsem.c:66
> inode_lock include/linux/fs.h:778 [inline]
> process_measurement+0x15ae/0x15e0
> security/integrity/ima/ima_main.c:228
> ima_file_mmap+0x11a/0x130 security/integrity/ima/ima_main.c:370
> security_file_mprotect+0xd5/0x100 security/security.c:1430
> do_mprotect_pkey+0x537/0xa30 mm/mprotect.c:550
> __do_sys_pkey_mprotect mm/mprotect.c:590 [inline]
> __se_sys_pkey_mprotect mm/mprotect.c:587 [inline]
> __x64_sys_pkey_mprotect+0x97/0xf0 mm/mprotect.c:587
> do_syscall_64+0xfd/0x680 arch/x86/entry/common.c:301
> entry_SYSCALL_64_after_hwframe+0x49/0xbe
>
> other info that might help us debug this:
>
> Possible unsafe locking scenario:
>
> CPU0 CPU1
> ---- ----
> lock(&mm->mmap_sem#2);
> lock(&sb->s_type->i_mutex_key#10);
> lock(&mm->mmap_sem#2);
> lock(&sb->s_type->i_mutex_key#10);
>
> *** DEADLOCK ***
>
> 1 lock held by syz-executor395/17373:
> #0: 00000000e0714fc5 (&mm->mmap_sem#2){++++}, at:
> do_mprotect_pkey+0x1f6/0xa30 mm/mprotect.c:485
>
> stack backtrace:
> CPU: 1 PID: 17373 Comm: syz-executor395 Not tainted 5.2.0-rc2-next-20190531
> #4
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
> Google 01/01/2011
> Call Trace:
> __dump_stack lib/dump_stack.c:77 [inline]
> dump_stack+0x172/0x1f0 lib/dump_stack.c:113
> print_circular_bug.cold+0x1cc/0x28f kernel/locking/lockdep.c:1566
> check_prev_add kernel/locking/lockdep.c:2311 [inline]
> check_prevs_add kernel/locking/lockdep.c:2419 [inline]
> validate_chain kernel/locking/lockdep.c:2801 [inline]
> __lock_acquire+0x3755/0x5490 kernel/locking/lockdep.c:3790
> lock_acquire+0x16f/0x3f0 kernel/locking/lockdep.c:4300
> down_write+0x38/0xa0 kernel/locking/rwsem.c:66
> inode_lock include/linux/fs.h:778 [inline]
> process_measurement+0x15ae/0x15e0 security/integrity/ima/ima_main.c:228
> ima_file_mmap+0x11a/0x130 security/integrity/ima/ima_main.c:370
> security_file_mprotect+0xd5/0x100 security/security.c:1430
> do_mprotect_pkey+0x537/0xa30 mm/mprotect.c:550
> __do_sys_pkey_mprotect mm/mprotect.c:590 [inline]
> __se_sys_pkey_mprotect mm/mprotect.c:587 [inline]
> __x64_sys_pkey_mprotect+0x97/0xf0 mm/mprotect.c:587
> do_syscall_64+0xfd/0x680 arch/x86/entry/common.c:301
> entry_SYSCALL_64_after_hwframe+0x49/0xbe
> RIP: 0033:0x440279
> Code: 18 89 d0 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 00 48 89 f8 48 89 f7
> 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff
> ff 0f 83 fb 13 fc ff c3 66 2e 0f 1f 84 00 00 00 00
> RSP: 002b:00007ffeec2f48d8 EFLAGS: 00000246 ORIG_RAX: 0000000000000149
> RAX: ffffffffffffffda RBX: 00000000004002c8 RCX: 0000000000440279
> RDX: 000000000000000
>
^ permalink raw reply
* Re: [RFC PATCH v4 00/12] security: x86/sgx: SGX vs. LSM
From: Jarkko Sakkinen @ 2019-07-11 17:51 UTC (permalink / raw)
To: Stephen Smalley
Cc: Xing, Cedric, Sean Christopherson, linux-sgx,
linux-security-module, selinux, Bill Roberts, Casey Schaufler,
James Morris, Dave Hansen, Andy Lutomirski, Jethro Beekman,
Dr . Greg Wettstein
In-Reply-To: <ef0c9052-d22e-aaa6-4915-b1823abea64c@tycho.nsa.gov>
On Thu, Jul 11, 2019 at 10:32:41AM -0400, Stephen Smalley wrote:
> The existing permissions don't map cleanly to SGX but I think Sean and
> Cedric were trying to make a best-effort approximation to the underlying
> concepts in a manner that permits control over the introduction of
> executable content.
>
> Sure, the existing EXECMOD check is only applied today when there is an
> attempt to make executable a previously modified (detected based on COW
> having occurred) private file mapping. But the general notion of
> controlling the ability to execute modified content is still meaningful.
OK to summarize EXECMOD does not connect with SGX in any possible way
but SGX needs something that mimics EXECMOD behaviour?
And the same probably goes for EXECMEM, which is also private mapping
related concept.
> In the case of regular files, having both FILE__WRITE and FILE__EXECUTE to
> the file is sufficient because that implies the ability to execute modified
> content. And those FILE__* checks predated the introduction of EXECMOD and
> EXECMEM.
Right.
> The mapping of /dev/sgx/enclave doesn't really fit existing categories
> because it doesn't provide the same semantics as a shared mapping of a
> regular file. Userspace will always need both FILE__WRITE and FILE__EXECUTE
> to /dev/sgx/enclave.
Right.
Yeah, that has been the confusing part for me that they've been shuffled
in the discussion together with SGX concepts and hooks like there was
any connection. Thank you for clarifying this!
/Jarkko
^ permalink raw reply
* Re: [RFC PATCH v3 3/4] X86/sgx: Introduce EMA as a new LSM module
From: Stephen Smalley @ 2019-07-11 16:32 UTC (permalink / raw)
To: Sean Christopherson
Cc: Xing, Cedric, Casey Schaufler, linux-sgx, linux-security-module,
selinux, James Morris, Paul Moore
In-Reply-To: <20190711162506.GF15067@linux.intel.com>
On 7/11/19 12:25 PM, Sean Christopherson wrote:
> On Thu, Jul 11, 2019 at 12:11:06PM -0400, Stephen Smalley wrote:
>> On 7/11/19 11:12 AM, Sean Christopherson wrote:
>>> On Thu, Jul 11, 2019 at 09:51:19AM -0400, Stephen Smalley wrote:
>>>> I'd also feel better if there was clear consensus among all of the
>>>> @intel.com participants that this is the right approach. To date that has
>>>> seemed elusive.
>>>
>>> That's a very kind way to phrase things :-)
>>>
>>> For initial upstreaming, we've agreed that there is no need to extend the
>>> uapi, i.e. we can punt on deciding between on-the-fly tracking and having
>>> userspace specify maximal permissions until we add SGX2 support.
>>>
>>> The last open (knock on wood) for initial upstreaming is whether SELinux
>>> would prefer to have new enclave specific permissions or reuse the
>>> existing PROCESS__EXECMEM, FILE__EXECUTE and FILE__EXECMOD permissions.
>>> My understanding is that enclave specific permissions are preferred.
>>
>> I was left unclear on this topic after the email exchanges with Cedric.
>> There are at least three options:
>>
>> 1) Reuse the existing EXECMEM, EXECUTE, and EXECMOD permissions. Pros:
>> Existing distro policies will be applied in the expected manner with respect
>> to the introduction of executable code into the system, consistent control
>> will be provided over the enclave and the host process, no change for
>> users/documentation wrt policy. Cons: Existing permissions don't map
>> exactly to SGX semantics, no ability to distinguish executable content
>> within the enclave versus the host process at the LSM level (argued earlier
>> by Cedric to be unnecessary and perhaps meaningless), need to allow
>> FILE__EXECUTE or other checks on sigstruct files that may not actually
>> contain code.
>>
>> 2) Define new permissions within existing security classes (e.g. process2,
>> file). Pros: Can tailor permission names and definitions to SGX semantics,
>> ability to distinguish enclave versus host process execute access, no need
>> to grant FILE__EXECUTE to sigstruct files, class matches the target object,
>> permissions computed and cached upon existing checks (i.e. when a process
>> accesses a file, all of the permissions to that file are computed and then
>> cached at once, including the enclave-related ones). Cons: Typical distro
>> policies (unlike Android) allow unknown permissions by default for forward
>> kernel compatibility reasons, so existing policies will permit these new
>> permissions by default and enforcement will only truly take effect once
>> policies are updated, adding new permissions to existing classes requires an
>> update to the base policy (so they can't be shipped as a third party policy
>> module alongside the SGX driver or installed as a local module by an admin,
>> for example), documentation/user education required for new permissions.
>>
>> 3) Define new permissions in new security classes (e.g. enclave). Pros
>> relative to #2: New classes and permissions can be defined and installed in
>> third party or local policy module without requiring a change to the base
>> policy. Cons relative to #2: Class won't correspond to the target object,
>> permissions won't be computed and cached upon existing checks (only when
>> performing the checks against the new classes).
>>
>> Combinations are also possible, of course.
>
> What's the impact on distros/ecosystems if we go with #1 for now and later
> decide to switch to #2 after upstreaming? I.e. can we take a minimal-ish
> approach now without painting ourselves into a corner?
Yes, I think that's fine.
> We can map quite closely to the existing intent of EXECUTE, EXECMOD and
> EXECMEM via a combination of checking protections at enclave load time and
> again at mmap()/mprotect(), e.g.:
>
> #ifdef CONFIG_INTEL_SGX
> static inline int enclave_has_perm(u32 sid, u32 requested)
> {
> return avc_has_perm(&selinux_state, sid, sid,
> SECCLASS_PROCESS, requested, NULL);
> }
>
> static int selinux_enclave_map(unsigned long prot)
> {
> const struct cred *cred = current_cred();
> u32 sid = cred_sid(cred);
>
> if ((prot & PROT_EXEC) && (prot & PROT_WRITE))
> return enclave_has_perm(sid, PROCESS__EXECMEM);
>
> return 0;
> }
>
> static int selinux_enclave_load(struct vm_area_struct *vma, unsigned long prot)
> {
> const struct cred *cred = current_cred();
> u32 sid = cred_sid(cred);
> int ret;
>
> /* Only executable enclave pages are restricted in any way. */
> if (!(prot & PROT_EXEC))
> return 0;
>
> if (!vma->vm_file || IS_PRIVATE(file_inode(vma->vm_file))) {
> ret = enclave_has_perm(sid, PROCESS__EXECMEM);
> } else {
> ret = file_has_perm(cred, vma->vm_file, FILE__EXECUTE);
> if (ret)
> goto out;
>
> /*
> * Load code from a modified private mapping or from a file
> * with the ability to do W->X within the enclave.
> */
> if (vma->anon_vma || (prot & PROT_WRITE))
> ret = file_has_perm(cred, vma->vm_file,
> FILE__EXECMOD);
> }
>
> out:
> return ret;
> }
> #endif
>
^ permalink raw reply
* Re: [RFC PATCH v3 3/4] X86/sgx: Introduce EMA as a new LSM module
From: Sean Christopherson @ 2019-07-11 16:25 UTC (permalink / raw)
To: Stephen Smalley
Cc: Xing, Cedric, Casey Schaufler, linux-sgx, linux-security-module,
selinux, James Morris, Paul Moore
In-Reply-To: <27e55a96-d5c4-69ed-a88d-7a3c26fb7f75@tycho.nsa.gov>
On Thu, Jul 11, 2019 at 12:11:06PM -0400, Stephen Smalley wrote:
> On 7/11/19 11:12 AM, Sean Christopherson wrote:
> >On Thu, Jul 11, 2019 at 09:51:19AM -0400, Stephen Smalley wrote:
> >>I'd also feel better if there was clear consensus among all of the
> >>@intel.com participants that this is the right approach. To date that has
> >>seemed elusive.
> >
> >That's a very kind way to phrase things :-)
> >
> >For initial upstreaming, we've agreed that there is no need to extend the
> >uapi, i.e. we can punt on deciding between on-the-fly tracking and having
> >userspace specify maximal permissions until we add SGX2 support.
> >
> >The last open (knock on wood) for initial upstreaming is whether SELinux
> >would prefer to have new enclave specific permissions or reuse the
> >existing PROCESS__EXECMEM, FILE__EXECUTE and FILE__EXECMOD permissions.
> >My understanding is that enclave specific permissions are preferred.
>
> I was left unclear on this topic after the email exchanges with Cedric.
> There are at least three options:
>
> 1) Reuse the existing EXECMEM, EXECUTE, and EXECMOD permissions. Pros:
> Existing distro policies will be applied in the expected manner with respect
> to the introduction of executable code into the system, consistent control
> will be provided over the enclave and the host process, no change for
> users/documentation wrt policy. Cons: Existing permissions don't map
> exactly to SGX semantics, no ability to distinguish executable content
> within the enclave versus the host process at the LSM level (argued earlier
> by Cedric to be unnecessary and perhaps meaningless), need to allow
> FILE__EXECUTE or other checks on sigstruct files that may not actually
> contain code.
>
> 2) Define new permissions within existing security classes (e.g. process2,
> file). Pros: Can tailor permission names and definitions to SGX semantics,
> ability to distinguish enclave versus host process execute access, no need
> to grant FILE__EXECUTE to sigstruct files, class matches the target object,
> permissions computed and cached upon existing checks (i.e. when a process
> accesses a file, all of the permissions to that file are computed and then
> cached at once, including the enclave-related ones). Cons: Typical distro
> policies (unlike Android) allow unknown permissions by default for forward
> kernel compatibility reasons, so existing policies will permit these new
> permissions by default and enforcement will only truly take effect once
> policies are updated, adding new permissions to existing classes requires an
> update to the base policy (so they can't be shipped as a third party policy
> module alongside the SGX driver or installed as a local module by an admin,
> for example), documentation/user education required for new permissions.
>
> 3) Define new permissions in new security classes (e.g. enclave). Pros
> relative to #2: New classes and permissions can be defined and installed in
> third party or local policy module without requiring a change to the base
> policy. Cons relative to #2: Class won't correspond to the target object,
> permissions won't be computed and cached upon existing checks (only when
> performing the checks against the new classes).
>
> Combinations are also possible, of course.
What's the impact on distros/ecosystems if we go with #1 for now and later
decide to switch to #2 after upstreaming? I.e. can we take a minimal-ish
approach now without painting ourselves into a corner?
We can map quite closely to the existing intent of EXECUTE, EXECMOD and
EXECMEM via a combination of checking protections at enclave load time and
again at mmap()/mprotect(), e.g.:
#ifdef CONFIG_INTEL_SGX
static inline int enclave_has_perm(u32 sid, u32 requested)
{
return avc_has_perm(&selinux_state, sid, sid,
SECCLASS_PROCESS, requested, NULL);
}
static int selinux_enclave_map(unsigned long prot)
{
const struct cred *cred = current_cred();
u32 sid = cred_sid(cred);
if ((prot & PROT_EXEC) && (prot & PROT_WRITE))
return enclave_has_perm(sid, PROCESS__EXECMEM);
return 0;
}
static int selinux_enclave_load(struct vm_area_struct *vma, unsigned long prot)
{
const struct cred *cred = current_cred();
u32 sid = cred_sid(cred);
int ret;
/* Only executable enclave pages are restricted in any way. */
if (!(prot & PROT_EXEC))
return 0;
if (!vma->vm_file || IS_PRIVATE(file_inode(vma->vm_file))) {
ret = enclave_has_perm(sid, PROCESS__EXECMEM);
} else {
ret = file_has_perm(cred, vma->vm_file, FILE__EXECUTE);
if (ret)
goto out;
/*
* Load code from a modified private mapping or from a file
* with the ability to do W->X within the enclave.
*/
if (vma->anon_vma || (prot & PROT_WRITE))
ret = file_has_perm(cred, vma->vm_file,
FILE__EXECMOD);
}
out:
return ret;
}
#endif
^ permalink raw reply
* Re: [RFC PATCH v3 3/4] X86/sgx: Introduce EMA as a new LSM module
From: Stephen Smalley @ 2019-07-11 16:11 UTC (permalink / raw)
To: Sean Christopherson
Cc: Xing, Cedric, Casey Schaufler, linux-sgx, linux-security-module,
selinux, James Morris, Paul Moore
In-Reply-To: <20190711151245.GD7645@linux.intel.com>
On 7/11/19 11:12 AM, Sean Christopherson wrote:
> On Thu, Jul 11, 2019 at 09:51:19AM -0400, Stephen Smalley wrote:
>> I'd also feel better if there was clear consensus among all of the
>> @intel.com participants that this is the right approach. To date that has
>> seemed elusive.
>
> That's a very kind way to phrase things :-)
>
> For initial upstreaming, we've agreed that there is no need to extend the
> uapi, i.e. we can punt on deciding between on-the-fly tracking and having
> userspace specify maximal permissions until we add SGX2 support.
>
> The last open (knock on wood) for initial upstreaming is whether SELinux
> would prefer to have new enclave specific permissions or reuse the
> existing PROCESS__EXECMEM, FILE__EXECUTE and FILE__EXECMOD permissions.
> My understanding is that enclave specific permissions are preferred.
I was left unclear on this topic after the email exchanges with Cedric.
There are at least three options:
1) Reuse the existing EXECMEM, EXECUTE, and EXECMOD permissions. Pros:
Existing distro policies will be applied in the expected manner with
respect to the introduction of executable code into the system,
consistent control will be provided over the enclave and the host
process, no change for users/documentation wrt policy. Cons: Existing
permissions don't map exactly to SGX semantics, no ability to
distinguish executable content within the enclave versus the host
process at the LSM level (argued earlier by Cedric to be unnecessary and
perhaps meaningless), need to allow FILE__EXECUTE or other checks on
sigstruct files that may not actually contain code.
2) Define new permissions within existing security classes (e.g.
process2, file). Pros: Can tailor permission names and definitions to
SGX semantics, ability to distinguish enclave versus host process
execute access, no need to grant FILE__EXECUTE to sigstruct files, class
matches the target object, permissions computed and cached upon existing
checks (i.e. when a process accesses a file, all of the permissions to
that file are computed and then cached at once, including the
enclave-related ones). Cons: Typical distro policies (unlike Android)
allow unknown permissions by default for forward kernel compatibility
reasons, so existing policies will permit these new permissions by
default and enforcement will only truly take effect once policies are
updated, adding new permissions to existing classes requires an update
to the base policy (so they can't be shipped as a third party policy
module alongside the SGX driver or installed as a local module by an
admin, for example), documentation/user education required for new
permissions.
3) Define new permissions in new security classes (e.g. enclave). Pros
relative to #2: New classes and permissions can be defined and installed
in third party or local policy module without requiring a change to the
base policy. Cons relative to #2: Class won't correspond to the target
object, permissions won't be computed and cached upon existing checks
(only when performing the checks against the new classes).
Combinations are also possible, of course.
^ permalink raw reply
* Re: [RFC PATCH v3 3/4] X86/sgx: Introduce EMA as a new LSM module
From: Sean Christopherson @ 2019-07-11 15:12 UTC (permalink / raw)
To: Stephen Smalley
Cc: Xing, Cedric, Casey Schaufler, linux-sgx, linux-security-module,
selinux, James Morris
In-Reply-To: <4c3e21dd-8890-e3cb-0db7-c154ac7201e1@tycho.nsa.gov>
On Thu, Jul 11, 2019 at 09:51:19AM -0400, Stephen Smalley wrote:
> I'd also feel better if there was clear consensus among all of the
> @intel.com participants that this is the right approach. To date that has
> seemed elusive.
That's a very kind way to phrase things :-)
For initial upstreaming, we've agreed that there is no need to extend the
uapi, i.e. we can punt on deciding between on-the-fly tracking and having
userspace specify maximal permissions until we add SGX2 support.
The last open (knock on wood) for initial upstreaming is whether SELinux
would prefer to have new enclave specific permissions or reuse the
existing PROCESS__EXECMEM, FILE__EXECUTE and FILE__EXECMOD permissions.
My understanding is that enclave specific permissions are preferred.
^ permalink raw reply
* Re: [RFC PATCH v4 00/12] security: x86/sgx: SGX vs. LSM
From: Stephen Smalley @ 2019-07-11 14:32 UTC (permalink / raw)
To: Jarkko Sakkinen, Xing, Cedric
Cc: Sean Christopherson, linux-sgx, linux-security-module, selinux,
Bill Roberts, Casey Schaufler, James Morris, Dave Hansen,
Andy Lutomirski, Jethro Beekman, Dr . Greg Wettstein
In-Reply-To: <20190711092609.jxe4uubwg5qbwfju@linux.intel.com>
On 7/11/19 5:26 AM, Jarkko Sakkinen wrote:
> On Wed, Jul 10, 2019 at 04:16:42PM -0700, Xing, Cedric wrote:
>>> Still puzzling with EXECMOD given that how it is documented in
>>> https://selinuxproject.org/page/ObjectClassesPerms. If anything in that
>>> document is out of date, would be nice if it was updated.
>>
>> If you search for "EXECMOD" in security/selinux/hooks.c in the latest
>> (Linux-5.2) master, you'll find only one occurrence - at line 3702.
>>
>> The logic over there, if translated into English, basically says
>> FILE__EXECMOD is required (on the backing file) if mprotect() is called to
>> request X on a private file mapping that has been modified by the calling
>> process. That's what Sean meant by "W->X".
>
> Looking at that part of code, there is this comment:
>
> /*
> * We are making executable a file mapping that has
> * had some COW done. Since pages might have been
> * written, check ability to execute the possibly
> * modified content. This typically should only
> * occur for text relocations.
> */
>
> There is no COW done with enclaves, never. Thus, EXECMOD does not
> connect in any possible way to SGX. OR, that comment is false.
>
> Which one is it?
>
> Also the official documentation for SELinux speaks only about COW
> mappings.
>
> Also the condition supports all this as a *private* file mapping ends up
> to the anon_vma list when it gets written. We have a *shared* file
> mapping
>
> Nothing that you say makes sense to me, sorry...
The existing permissions don't map cleanly to SGX but I think Sean and
Cedric were trying to make a best-effort approximation to the underlying
concepts in a manner that permits control over the introduction of
executable content.
Sure, the existing EXECMOD check is only applied today when there is an
attempt to make executable a previously modified (detected based on COW
having occurred) private file mapping. But the general notion of
controlling the ability to execute modified content is still meaningful.
In the case of regular files, having both FILE__WRITE and FILE__EXECUTE
to the file is sufficient because that implies the ability to execute
modified content. And those FILE__* checks predated the introduction of
EXECMOD and EXECMEM.
The mapping of /dev/sgx/enclave doesn't really fit existing categories
because it doesn't provide the same semantics as a shared mapping of a
regular file. Userspace will always need both FILE__WRITE and
FILE__EXECUTE to /dev/sgx/enclave.
^ permalink raw reply
* Re: possible deadlock in process_measurement
From: Mimi Zohar @ 2019-07-11 14:14 UTC (permalink / raw)
To: Eric Biggers
Cc: syzbot, dmitry.kasatkin, jmorris, linux-integrity, linux-kernel,
linux-security-module, serge, syzkaller-bugs, zohar
In-Reply-To: <00000000000054e5d1058a6df2eb@google.com>
Hi Eric,
On Mon, 2019-06-03 at 09:35 -0700, syzbot wrote:
> syzbot has found a reproducer for the following crash on:
>
> HEAD commit: 3c09c195 Add linux-next specific files for 20190531
> git tree: linux-next
> console output: https://syzkaller.appspot.com/x/log.txt?x=10f61a0ea00000
> kernel config: https://syzkaller.appspot.com/x/.config?x=6cfb24468280cd5c
> dashboard link: https://syzkaller.appspot.com/bug?extid=5ab61747675a87ea359d
> compiler: gcc (GCC) 9.0.0 20181231 (experimental)
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=177c3d16a00000
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=14ec01baa00000
>
This reproducer seems like it is similar, but the cause is different
than the original report. One has to do with overlayfs, while the
other has to do with ext4, mprotect/mmap. I assume in both cases an
IMA policy was required to trigger the locking bug. What type of IMA
policy are you using?
Do we need to differentiate the two reports? Is the "last occurred"
notification for the overlay, for mprotect, or both? Please Cc the
overlay mailing list on the overlay aspect.
thanks,
Mimi
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+5ab61747675a87ea359d@syzkaller.appspotmail.com
>
> get_swap_device: Bad swap file entry 6000000000000001
> get_swap_device: Bad swap file entry 6400000000000001
> get_swap_device: Bad swap file entry 6800000000000001
> get_swap_device: Bad swap file entry 6c00000000000001
> get_swap_device: Bad swap file entry 7000000000000001
> get_swap_device: Bad swap file entry 7400000000000001
> ======================================================
> WARNING: possible circular locking dependency detected
> 5.2.0-rc2-next-20190531 #4 Not tainted
> ------------------------------------------------------
> syz-executor395/17373 is trying to acquire lock:
> 000000003d1a4a53 (&sb->s_type->i_mutex_key#10){+.+.}, at: inode_lock
> include/linux/fs.h:778 [inline]
> 000000003d1a4a53 (&sb->s_type->i_mutex_key#10){+.+.}, at:
> process_measurement+0x15ae/0x15e0 security/integrity/ima/ima_main.c:228
>
> but task is already holding lock:
> 00000000e0714fc5 (&mm->mmap_sem#2){++++}, at: do_mprotect_pkey+0x1f6/0xa30
> mm/mprotect.c:485
>
> which lock already depends on the new lock.
>
>
> the existing dependency chain (in reverse order) is:
>
> -> #1 (&mm->mmap_sem#2){++++}:
> down_read+0x3f/0x1e0 kernel/locking/rwsem.c:24
> get_user_pages_unlocked+0xfc/0x4a0 mm/gup.c:1174
> __gup_longterm_unlocked mm/gup.c:2193 [inline]
> get_user_pages_fast+0x43f/0x530 mm/gup.c:2245
> iov_iter_get_pages+0x2c2/0xf80 lib/iov_iter.c:1287
> dio_refill_pages fs/direct-io.c:171 [inline]
> dio_get_page fs/direct-io.c:215 [inline]
> do_direct_IO fs/direct-io.c:983 [inline]
> do_blockdev_direct_IO+0x3f7b/0x8e00 fs/direct-io.c:1336
> __blockdev_direct_IO+0xa1/0xca fs/direct-io.c:1422
> ext4_direct_IO_write fs/ext4/inode.c:3782 [inline]
> ext4_direct_IO+0xaa7/0x1bb0 fs/ext4/inode.c:3909
> generic_file_direct_write+0x20a/0x4a0 mm/filemap.c:3110
> __generic_file_write_iter+0x2ee/0x630 mm/filemap.c:3293
> ext4_file_write_iter+0x332/0x1070 fs/ext4/file.c:266
> call_write_iter include/linux/fs.h:1870 [inline]
> new_sync_write+0x4d3/0x770 fs/read_write.c:483
> __vfs_write+0xe1/0x110 fs/read_write.c:496
> vfs_write+0x268/0x5d0 fs/read_write.c:558
> ksys_write+0x14f/0x290 fs/read_write.c:611
> __do_sys_write fs/read_write.c:623 [inline]
> __se_sys_write fs/read_write.c:620 [inline]
> __x64_sys_write+0x73/0xb0 fs/read_write.c:620
> do_syscall_64+0xfd/0x680 arch/x86/entry/common.c:301
> entry_SYSCALL_64_after_hwframe+0x49/0xbe
>
> -> #0 (&sb->s_type->i_mutex_key#10){+.+.}:
> lock_acquire+0x16f/0x3f0 kernel/locking/lockdep.c:4300
> down_write+0x38/0xa0 kernel/locking/rwsem.c:66
> inode_lock include/linux/fs.h:778 [inline]
> process_measurement+0x15ae/0x15e0
> security/integrity/ima/ima_main.c:228
> ima_file_mmap+0x11a/0x130 security/integrity/ima/ima_main.c:370
> security_file_mprotect+0xd5/0x100 security/security.c:1430
> do_mprotect_pkey+0x537/0xa30 mm/mprotect.c:550
> __do_sys_pkey_mprotect mm/mprotect.c:590 [inline]
> __se_sys_pkey_mprotect mm/mprotect.c:587 [inline]
> __x64_sys_pkey_mprotect+0x97/0xf0 mm/mprotect.c:587
> do_syscall_64+0xfd/0x680 arch/x86/entry/common.c:301
> entry_SYSCALL_64_after_hwframe+0x49/0xbe
>
> other info that might help us debug this:
>
> Possible unsafe locking scenario:
>
> CPU0 CPU1
> ---- ----
> lock(&mm->mmap_sem#2);
> lock(&sb->s_type->i_mutex_key#10);
> lock(&mm->mmap_sem#2);
> lock(&sb->s_type->i_mutex_key#10);
>
> *** DEADLOCK ***
>
> 1 lock held by syz-executor395/17373:
> #0: 00000000e0714fc5 (&mm->mmap_sem#2){++++}, at:
> do_mprotect_pkey+0x1f6/0xa30 mm/mprotect.c:485
>
> stack backtrace:
> CPU: 1 PID: 17373 Comm: syz-executor395 Not tainted 5.2.0-rc2-next-20190531
> #4
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
> Google 01/01/2011
> Call Trace:
> __dump_stack lib/dump_stack.c:77 [inline]
> dump_stack+0x172/0x1f0 lib/dump_stack.c:113
> print_circular_bug.cold+0x1cc/0x28f kernel/locking/lockdep.c:1566
> check_prev_add kernel/locking/lockdep.c:2311 [inline]
> check_prevs_add kernel/locking/lockdep.c:2419 [inline]
> validate_chain kernel/locking/lockdep.c:2801 [inline]
> __lock_acquire+0x3755/0x5490 kernel/locking/lockdep.c:3790
> lock_acquire+0x16f/0x3f0 kernel/locking/lockdep.c:4300
> down_write+0x38/0xa0 kernel/locking/rwsem.c:66
> inode_lock include/linux/fs.h:778 [inline]
> process_measurement+0x15ae/0x15e0 security/integrity/ima/ima_main.c:228
> ima_file_mmap+0x11a/0x130 security/integrity/ima/ima_main.c:370
> security_file_mprotect+0xd5/0x100 security/security.c:1430
> do_mprotect_pkey+0x537/0xa30 mm/mprotect.c:550
> __do_sys_pkey_mprotect mm/mprotect.c:590 [inline]
> __se_sys_pkey_mprotect mm/mprotect.c:587 [inline]
> __x64_sys_pkey_mprotect+0x97/0xf0 mm/mprotect.c:587
> do_syscall_64+0xfd/0x680 arch/x86/entry/common.c:301
> entry_SYSCALL_64_after_hwframe+0x49/0xbe
> RIP: 0033:0x440279
> Code: 18 89 d0 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 00 48 89 f8 48 89 f7
> 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff
> ff 0f 83 fb 13 fc ff c3 66 2e 0f 1f 84 00 00 00 00
> RSP: 002b:00007ffeec2f48d8 EFLAGS: 00000246 ORIG_RAX: 0000000000000149
> RAX: ffffffffffffffda RBX: 00000000004002c8 RCX: 0000000000440279
> RDX: 000000000000000
>
^ permalink raw reply
* Re: [RFC PATCH v3 3/4] X86/sgx: Introduce EMA as a new LSM module
From: Stephen Smalley @ 2019-07-11 13:51 UTC (permalink / raw)
To: Xing, Cedric, Casey Schaufler, linux-sgx, linux-security-module,
selinux, James Morris
In-Reply-To: <e2a0d952-b4d4-f8f3-ee58-eba63f30dc66@intel.com>
On 7/9/19 8:55 PM, Xing, Cedric wrote:
> On 7/9/2019 5:10 PM, Casey Schaufler wrote:
>> On 7/9/2019 3:13 PM, Xing, Cedric wrote:
>>> On 7/8/2019 4:53 PM, Casey Schaufler wrote:
>>>> On 7/8/2019 10:16 AM, Xing, Cedric wrote:
>>>>> On 7/8/2019 9:26 AM, Casey Schaufler wrote:
>>>>>> In this scheme you use an ema LSM to manage your ema data.
>>>>>> A quick sketch looks like:
>>>>>>
>>>>>> sgx_something_in() calls
>>>>>> security_enclave_load() calls
>>>>>> ema_enclave_load()
>>>>>> selinux_enclave_load()
>>>>>> otherlsm_enclave_load()
>>>>>>
>>>>>> Why is this better than:
>>>>>>
>>>>>> sgx_something_in() calls
>>>>>> ema_enclave_load()
>>>>>> security_enclave_load() calls
>>>>>> selinux_enclave_load()
>>>>>> otherlsm_enclave_load()
>>>>>
>>>>> Are you talking about moving EMA somewhere outside LSM?
>>>>
>>>> Yes. That's what I've been saying all along.
>>>>
>>>>> If so, where?
>>>>
>>>> I tried to make it obvious. Put the call to your EMA code
>>>> on the line before you call security_enclave_load().
>>>
>>> Sorry but I'm still confused.
>>>
>>> EMA code is used by LSMs only. Making it callable from other parts of
>>> the kernel IMHO is probably not a good idea. And more importantly I
>>> don't understand the motivation behind it. Would you please elaborate?
>>
>> LSM modules implement additional access control restrictions.
>> The EMA code does not do that, it provides management of data
>> that is used by security modules. It is not one itself. VFS
>> also performs this role, but no one would consider making VFS
>> a security module.
>
> You are right. EMA is more like a helper library than a real LSM. But
> the practical problem is, it has a piece of initialization code, to
> basically request some space in the file blob from the LSM
> infrastructure. That cannot be done by any LSMs at runtime. So it has to
> either be done in LSM infrastructure directly, or make itself an LSM to
> make its initialization function invoked by LSM infrastructure
> automatically. You have objected to the former, so I switched to the
> latter. Are you now objecting to the latter as well? Then what are you
> suggesting, really?
>
> VFS is a completely different story. It's the file system abstraction so
> it has a natural place to live in the kernel, and its initialization
> doesn't depend on the LSM infrastructure. EMA on the other hand, shall
> belong to LSM because it is both produced and consumed within LSM.
>
> And, Stephen, do you have an opinion on this?
I don't really understand Casey's position. I also wouldn't necessarily
view his opinion on the matter as necessarily authoritative since he is
not the LSM maintainer as far as I know although he has contributed a
lot of changes in recent years.
I understood the architecture of the original EMA implementation (i.e. a
support library to be used by the security modules to help them in
implementing their own access control policies), although I do have some
concerns about the complexity and lifecycle issues, and wonder if a
simpler model as suggested by Dr. Greg isn't feasible.
I'd also feel better if there was clear consensus among all of the
@intel.com participants that this is the right approach. To date that
has seemed elusive.
If there were consensus that the EMA approach was the right one and that
a simpler model is not feasible, and if the only obstacle to adopting
EMA was Casey's objections, then I'd say just put it directly into
SELinux and be done with it. I originally thought that was a mistake
but if other security modules don't want the support, so be it.
>
>>>>>>> +/**
>>>>>>> + * ema - Enclave Memory Area structure for LSM modules
>>>>>>
>>>>>> LSM modules is redundant. "LSM" or "LSMs" would be better.
>>>>>
>>>>> Noted
>>>>>
>>>>>>> diff --git a/security/Makefile b/security/Makefile
>>>>>>> index c598b904938f..b66d03a94853 100644
>>>>>>> --- a/security/Makefile
>>>>>>> +++ b/security/Makefile
>>>>>>> @@ -28,6 +28,7 @@ obj-$(CONFIG_SECURITY_YAMA) += yama/
>>>>>>> obj-$(CONFIG_SECURITY_LOADPIN) += loadpin/
>>>>>>> obj-$(CONFIG_SECURITY_SAFESETID) += safesetid/
>>>>>>> obj-$(CONFIG_CGROUP_DEVICE) += device_cgroup.o
>>>>>>> +obj-$(CONFIG_INTEL_SGX) += commonema.o
>>>>>>
>>>>>> The config option and the file name ought to match,
>>>>>> or at least be closer.
>>>>>
>>>>> Just trying to match file names as "capability" uses commoncap.c.
>>>>
>>>> Fine, then you should be using CONFIG_SECURITY_EMA.
>>>>
>>>>>
>>>>> Like I said, this feature could potentially be used by TEEs other
>>>>> than SGX. For now, SGX is the only user so it is tied to
>>>>> CONFIG_INTEL_SGX. I can rename it to ema.c or enclave.c. Do you
>>>>> have a preference?
>>>>
>>>> Make
>>>> CONFIG_SECURITY_EMA
>>>> depends on CONFIG_INTEL_SGX
>>>>
>>>> When another TEE (maybe MIPS_SSRPQ) comes along you can have
>>>>
>>>> CONFIG_SECURITY_EMA
>>>> depends on CONFIG_INTEL_SGX || CONFIG_MIPS_SSRPQ
>>>
>>> Your suggestions are reasonable. Given such config change wouldn't
>>> affect any code, can we do it later,
>>
>> That doesn't make the current options any less confusing,
>> and it will be easier to make the change now than at some
>> point in the future.
>>
>>> e.g., when additional TEEs come online and make use of these new
>>> hooks? After all, security_enclave_init() will need amendment anyway
>>> as one of its current parameters is of type 'struct sgx_sigstruct',
>>> which will need to be replaced with something more generic. At the
>>> time being, I'd like to keep things intuitive so as not to confuse
>>> reviewers.
>>
>> Reviewers (including me) are already confused by the inconsistency.
>
> OK. Let me make this change.
>
>>>>>
>>>>>>> diff --git a/security/commonema.c b/security/commonema.c
>>>>>>
>>>>>> Put this in a subdirectory. Please.
>>>>>
>>>>> Then why is commoncap.c located in this directory? I'm just trying
>>>>> to match the existing convention.
>>>>
>>>> commoncap is not optional. It is a base part of the
>>>> security subsystem. ema is optional.
>>>
>>> Alright. I'd move it into a sub-folder and rename it to ema.c. Would
>>> you be ok with that?
>>
>> Sounds fine.
>
> This is another part that confuses me. Per you comment here, I think you
> are OK with EMA being part of LSM (I mean, living somewhere under
> security/). But your other comment of calling ema_enclave_load()
> alongside security_enclave_load() made me think EMA and LSM were
> separate. What do you want really?
^ permalink raw reply
* Re: [RFC PATCH v2 1/3] x86/sgx: Add SGX specific LSM hooks
From: Dr. Greg @ 2019-07-11 10:22 UTC (permalink / raw)
To: Casey Schaufler
Cc: Xing, Cedric, Stephen Smalley, linux-sgx@vger.kernel.org,
linux-security-module@vger.kernel.org, selinux@vger.kernel.org,
Schaufler, Casey, jmorris@namei.org, luto@kernel.org,
jethro@fortanix.com, sds@tycho.nsa.gov,
jarkko.sakkinen@linux.intel.com, Christopherson, Sean J
In-Reply-To: <256013f7-292d-7014-9abb-61755f07eb25@schaufler-ca.com>
On Mon, Jul 08, 2019 at 05:02:00PM -0700, Casey Schaufler wrote:
> > On 7/7/2019 6:30 AM, Dr. Greg wrote:
> > All well taken points from an implementation perspective, but they
> > elide the point I was trying to make. Which is the fact that without
> > any semblance of a discussion regarding the requirements needed to
> > implement a security architecture around the concept of a TEE, this
> > entire process, despite Cedric's well intentioned efforts, amounts to
> > pounding a square solution into the round hole of a security problem.
> Lead with code. I love a good requirements document, but one of the
> few places where I agree with the agile folks is that working code
> speaks loudly.
>
> > Which, as I noted in my e-mail, is tantamount to security theater.
>
> Not buying that. Not rejecting it, either. Without code
> to judge it's kind of hard to say.
We tried the code approach.
By most accounts it seemed to go poorly, given that it ended up with
Jonathan Corbet writing an LWN feature article on the state of
dysfunction and chaos surrounding Linux SGX driver development.
So we are standing around and mumbling until we can figure out what
kind of code we need to write to make the new driver relevant to the
CISO's and security architects we need to defend SGX to.
Have a good week.
Dr. Greg
As always,
Dr. Greg Wettstein, Ph.D, Worker
IDfusion, LLC Implementing SGX secured and modeled
4206 N. 19th Ave. intelligent network endpoints.
Fargo, ND 58102
PH: 701-281-1686 EMAIL: greg@idfusion.net
------------------------------------------------------------------------------
"Five year projections, are you kidding me. We don't know what we are
supposed to be doing at the 4 o'clock meeting this afternoon."
-- Terry Wieland
Resurrection
^ permalink raw reply
* Re: [RFC PATCH v4 00/12] security: x86/sgx: SGX vs. LSM
From: Jarkko Sakkinen @ 2019-07-11 9:26 UTC (permalink / raw)
To: Xing, Cedric
Cc: Sean Christopherson, linux-sgx, linux-security-module, selinux,
Bill Roberts, Casey Schaufler, James Morris, Dave Hansen,
Andy Lutomirski, Jethro Beekman, Dr . Greg Wettstein,
Stephen Smalley
In-Reply-To: <38d3b0ee-be9c-cb1a-785a-325a3ade003b@intel.com>
On Wed, Jul 10, 2019 at 04:16:42PM -0700, Xing, Cedric wrote:
> > Still puzzling with EXECMOD given that how it is documented in
> > https://selinuxproject.org/page/ObjectClassesPerms. If anything in that
> > document is out of date, would be nice if it was updated.
>
> If you search for "EXECMOD" in security/selinux/hooks.c in the latest
> (Linux-5.2) master, you'll find only one occurrence - at line 3702.
>
> The logic over there, if translated into English, basically says
> FILE__EXECMOD is required (on the backing file) if mprotect() is called to
> request X on a private file mapping that has been modified by the calling
> process. That's what Sean meant by "W->X".
Looking at that part of code, there is this comment:
/*
* We are making executable a file mapping that has
* had some COW done. Since pages might have been
* written, check ability to execute the possibly
* modified content. This typically should only
* occur for text relocations.
*/
There is no COW done with enclaves, never. Thus, EXECMOD does not
connect in any possible way to SGX. OR, that comment is false.
Which one is it?
Also the official documentation for SELinux speaks only about COW
mappings.
Also the condition supports all this as a *private* file mapping ends up
to the anon_vma list when it gets written. We have a *shared* file
mapping
Nothing that you say makes sense to me, sorry...
/Jarkko
^ permalink raw reply
* Re: [RFC PATCH v4 00/12] security: x86/sgx: SGX vs. LSM
From: Jarkko Sakkinen @ 2019-07-11 9:06 UTC (permalink / raw)
To: Sean Christopherson
Cc: linux-sgx, linux-security-module, selinux, Bill Roberts,
Casey Schaufler, James Morris, Dave Hansen, Cedric Xing,
Andy Lutomirski, Jethro Beekman, Dr . Greg Wettstein,
Stephen Smalley
In-Reply-To: <20190710203104.GH4348@linux.intel.com>
On Wed, Jul 10, 2019 at 01:31:04PM -0700, Sean Christopherson wrote:
> On Wed, Jul 10, 2019 at 11:19:30PM +0300, Jarkko Sakkinen wrote:
> > On Tue, Jul 09, 2019 at 10:09:17AM -0700, Sean Christopherson wrote:
> > > On Tue, Jul 09, 2019 at 07:22:03PM +0300, Jarkko Sakkinen wrote:
> > > > On Mon, Jul 08, 2019 at 10:29:30AM -0700, Sean Christopherson wrote:
> > > > > On Fri, Jul 05, 2019 at 07:05:49PM +0300, Jarkko Sakkinen wrote:
> > > > > > On Wed, Jun 19, 2019 at 03:23:49PM -0700, Sean Christopherson wrote:
> > > > > >
> > > > > > I still don't get why we need this whole mess and do not simply admit
> > > > > > that there are two distinct roles:
> > > > > >
> > > > > > 1. Creator
> > > > > > 2. User
> > > > >
> > > > > Because SELinux has existing concepts of EXECMEM and EXECMOD.
> > > >
> > > > What is the official documentation for those? I've only found some
> > > > explanations from discussions and some RHEL sysadmin guides.
> > >
> > > No clue. My knowledge was gleaned from the code and from Stephen's
> > > feedback.
> >
> > OK, thanks for elaboration. Got nailed some details I was missing :-)
> >
> > Anyway, to accompany your code changes I'm eager to document this not
> > least because it is a good peer test that this all make sense (you
> > cannot "unit test" a security model so that is the next best thing).
> >
> > Still, we need a documentation reference to reflect the narrative
> > for these changes, seriously. It cannot be that SELinux is widely
> > deployed and it completely lacks documentation for its basic
> > objects, can it?
>
> The vast majority of documentation I've found is on *using* SELinux,
> e.g. writing policies and whatnot. I haven't found anything on its
> internal details, although I admitedly haven't looked all that hard.
I think these are the best references.
Object classes:
https://selinuxproject.org/page/ObjectClassesPerms
Background/concepts:
https://selinuxproject.org/page/Category:Notebook#Notebook_Sections
/Jarkko
^ permalink raw reply
* Re: [PATCH V34 10/29] hibernate: Disable when the kernel is locked down
From: joeyli @ 2019-07-11 4:11 UTC (permalink / raw)
To: Jiri Kosina
Cc: Pavel Machek, Matthew Garrett, jmorris, linux-security-module,
linux-kernel, linux-api, Josh Boyer, David Howells,
Matthew Garrett, rjw, linux-pm
In-Reply-To: <nycvar.YFH.7.76.1906241520500.27227@cbobk.fhfr.pm>
Hi experts,
On Mon, Jun 24, 2019 at 03:21:23PM +0200, Jiri Kosina wrote:
> On Sat, 22 Jun 2019, Pavel Machek wrote:
>
> > > There is currently no way to verify the resume image when returning
> > > from hibernate. This might compromise the signed modules trust model,
> > > so until we can work with signed hibernate images we disable it when the
> > > kernel is locked down.
> >
> > I keep getting these...
> >
> > IIRC suse has patches to verify the images.
>
> Yeah, Joey Lee is taking care of those. CCing.
>
The last time that I sent for hibernation encryption and authentication is
here:
https://lkml.org/lkml/2019/1/3/281
It needs some big changes after review:
- Simplify the design: remove keyring dependency and trampoline.
- Encrypted whole snapshot image instead of only data pages.
- Using TPM:
- Direct use TPM API in hibernation instead of keyring
- Localities (suggested by James Bottomley)
I am still finding enough time to implement those changes, especial TPM
parts.
Thanks
Joey Lee
^ permalink raw reply
* Re: [GIT PULL] Keys: Set 4 - Key ACLs for 5.3
From: Mimi Zohar @ 2019-07-11 3:07 UTC (permalink / raw)
To: Linus Torvalds, David Howells, James Morris, keyrings, Netdev,
linux-nfs, CIFS, linux-afs, linux-fsdevel, linux-integrity,
LSM List, Linux List Kernel Mailing
In-Reply-To: <CAHk-=wiFti6=K2fyAYhx-PSX9ovQPJUNp0FMdV0pDaO_pSx9MQ@mail.gmail.com>
Hi Linus,
On Wed, 2019-07-10 at 18:59 -0700, Linus Torvalds wrote:
> Anyway, since it does seem like David is offline, I've just reverted
> this from my tree, and will be continuing my normal merge window pull
> requests (the other issues I have seen have fixes in their respective
> trees).
Sorry for the delay. An exception is needed for loading builtin keys
"KEY_ALLOC_BUILT_IN" onto a keyring that is not writable by userspace.
The following works, but probably is not how David would handle the
exception.
diff --git a/security/keys/key.c b/security/keys/key.c
index 519211a996e7..a99332c1e014 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -896,7 +896,7 @@ key_ref_t key_create_or_update(key_ref_t keyring_ref,
/* if we're going to allocate a new key, we're going to have
* to modify the keyring */
ret = key_permission(keyring_ref, KEY_NEED_WRITE);
- if (ret < 0) {
+ if (ret < 0 && !(flags & KEY_ALLOC_BUILT_IN)) {
key_ref = ERR_PTR(ret);
goto error_link_end;
}
Mimi
^ permalink raw reply related
* Re: [GIT PULL] Keys: Set 4 - Key ACLs for 5.3
From: Linus Torvalds @ 2019-07-11 1:59 UTC (permalink / raw)
To: Linus Torvalds, David Howells, James Morris, keyrings, Netdev,
linux-nfs, CIFS, linux-afs, linux-fsdevel, linux-integrity,
LSM List, Linux List Kernel Mailing
In-Reply-To: <20190710201552.GB83443@gmail.com>
On Wed, Jul 10, 2019 at 1:15 PM Eric Biggers <ebiggers@kernel.org> wrote:
>
> Also worth noting that the key ACL patches were only in linux-next for 9 days
> before the pull request was sent.
Yes. I was not entirely happy with the whole key subsystem situation.
See my concerns in
https://lore.kernel.org/lkml/CAHk-=wjEowdfG7v_4ttu3xhf9gqopj1+q1nGG86+mGfGDTEBBg@mail.gmail.com/
for more. That was before I realized it was buggy.
So it really would be good to have more people involved, and more
structure to the keys development (and, I suspect, much else under
security/)
Anyway, since it does seem like David is offline, I've just reverted
this from my tree, and will be continuing my normal merge window pull
requests (the other issues I have seen have fixes in their respective
trees).
Linus
^ permalink raw reply
* Re: [RFC PATCH v4 00/12] security: x86/sgx: SGX vs. LSM
From: Xing, Cedric @ 2019-07-10 23:16 UTC (permalink / raw)
To: Jarkko Sakkinen, Sean Christopherson
Cc: linux-sgx, linux-security-module, selinux, Bill Roberts,
Casey Schaufler, James Morris, Dave Hansen, Andy Lutomirski,
Jethro Beekman, Dr . Greg Wettstein, Stephen Smalley
In-Reply-To: <20190710221638.bwnwtcozpv44ojdg@linux.intel.com>
On 7/10/2019 3:16 PM, Jarkko Sakkinen wrote:
> Just some questions on these.
>
> On Tue, Jul 09, 2019 at 10:09:17AM -0700, Sean Christopherson wrote:
>> - FILE__ENCLAVE_EXECUTE: equivalent to FILE__EXECUTE, required to gain X
>> on an enclave page loaded from a regular file
>
> One thing that I have hard time to perceive is that whether the process
> or the target object has them. So would this be in the files extended
> attribute or does process need to possess this or both?
The target object.
>> - PROCESS2__ENCLAVE_EXECDIRTY: hybrid of EXECMOD and EXECUTE+WRITE,
>> required to gain W->X on an enclave page
>
> Still puzzling with EXECMOD given that how it is documented in
> https://selinuxproject.org/page/ObjectClassesPerms. If anything in that
> document is out of date, would be nice if it was updated.
If you search for "EXECMOD" in security/selinux/hooks.c in the latest
(Linux-5.2) master, you'll find only one occurrence - at line 3702.
The logic over there, if translated into English, basically says
FILE__EXECMOD is required (on the backing file) if mprotect() is called
to request X on a private file mapping that has been modified by the
calling process. That's what Sean meant by "W->X".
EXCLAVE_EXECDIRTY is similar to EXECMOD but because of his "maximal
protection" model, LSM couldn't distinguish between "W->X" and "X->W",
hence those two are collapsed into a single case - WX in "maximal
protection".
>> - PROCESS2__ENCLAVE_EXECANON: subset of EXECMEM, required to gain X on
>> an enclave page that is loaded from an
>> anonymous mapping
>>
>> - PROCESS2__ENCLAVE_MAPWX: subset of EXECMEM, required to gain WX on an
>> enclave page
>
> I guess these three belong to the process and are not attached to file.
Correct.
ENCLAVE_EXECANON basically means the calling process doesn't care what
permissions given to enclave pages as the SIGSTRUCT alone is considered
sufficient validation. This has a security impact process wide so shall
be a process permission.
ENCLAVE_{EXECDIRTY|MAPWX} express enclave specific
requirements/behaviors and IMO shall be enclave permissions, probably
manifested as file permissions on the file containing SIGSTRUCT. Sean
was taking a shortcut to make them process scope in order to avoid
keeping the SIGSTRUCT file around, which was what I criticized as
"illogical".
> How in SELinux anyway process in the first place acquires any SELinux
> permissions? I guess getty or whatever login process can set its perms
> before setuid() et al somehow (I don't know how) because they run as
> root?
>
> /Jarkko
>
^ permalink raw reply
* Re: [RFC PATCH v4 00/12] security: x86/sgx: SGX vs. LSM
From: Jarkko Sakkinen @ 2019-07-10 22:16 UTC (permalink / raw)
To: Sean Christopherson
Cc: linux-sgx, linux-security-module, selinux, Bill Roberts,
Casey Schaufler, James Morris, Dave Hansen, Cedric Xing,
Andy Lutomirski, Jethro Beekman, Dr . Greg Wettstein,
Stephen Smalley
In-Reply-To: <20190709170917.GD25369@linux.intel.com>
Just some questions on these.
On Tue, Jul 09, 2019 at 10:09:17AM -0700, Sean Christopherson wrote:
> - FILE__ENCLAVE_EXECUTE: equivalent to FILE__EXECUTE, required to gain X
> on an enclave page loaded from a regular file
One thing that I have hard time to perceive is that whether the process
or the target object has them. So would this be in the files extended
attribute or does process need to possess this or both?
> - PROCESS2__ENCLAVE_EXECDIRTY: hybrid of EXECMOD and EXECUTE+WRITE,
> required to gain W->X on an enclave page
Still puzzling with EXECMOD given that how it is documented in
https://selinuxproject.org/page/ObjectClassesPerms. If anything in that
document is out of date, would be nice if it was updated.
> - PROCESS2__ENCLAVE_EXECANON: subset of EXECMEM, required to gain X on
> an enclave page that is loaded from an
> anonymous mapping
>
> - PROCESS2__ENCLAVE_MAPWX: subset of EXECMEM, required to gain WX on an
> enclave page
I guess these three belong to the process and are not attached to file.
How in SELinux anyway process in the first place acquires any SELinux
permissions? I guess getty or whatever login process can set its perms
before setuid() et al somehow (I don't know how) because they run as
root?
/Jarkko
^ permalink raw reply
* Re: [RFC PATCH v4 00/12] security: x86/sgx: SGX vs. LSM
From: Jarkko Sakkinen @ 2019-07-10 22:00 UTC (permalink / raw)
To: Sean Christopherson
Cc: linux-sgx, linux-security-module, selinux, Bill Roberts,
Casey Schaufler, James Morris, Dave Hansen, Cedric Xing,
Andy Lutomirski, Jethro Beekman, Dr . Greg Wettstein,
Stephen Smalley
In-Reply-To: <20190710201930.ly7ykediuy35cjze@linux.intel.com>
On Wed, Jul 10, 2019 at 11:19:30PM +0300, Jarkko Sakkinen wrote:
> Still, we need a documentation reference to reflect the narrative
> for these changes, seriously. It cannot be that SELinux is widely
> deployed and it completely lacks documentation for its basic
> objects, can it?
I found one good reference:
https://selinuxpTroject.org/page/ObjectClassesPerms
It describes EXECMOD as:
"Make executable a file mapping that has been modified by copy-on-write.
(Text relocation)"
This makes me wonder how EXECMOD even connects to this discussion?
Enclave is never a COW mapping. Seems like there is a huge diff on
how SELinux's official documentation describes it and how it is
described here...
/Jarkko
^ permalink raw reply
* Re: [RFC PATCH v3 3/4] X86/sgx: Introduce EMA as a new LSM module
From: Casey Schaufler @ 2019-07-10 21:14 UTC (permalink / raw)
To: Xing, Cedric, linux-sgx, linux-security-module, selinux, casey
In-Reply-To: <e2a0d952-b4d4-f8f3-ee58-eba63f30dc66@intel.com>
On 7/9/2019 5:55 PM, Xing, Cedric wrote:
> On 7/9/2019 5:10 PM, Casey Schaufler wrote:
>> On 7/9/2019 3:13 PM, Xing, Cedric wrote:
>>> On 7/8/2019 4:53 PM, Casey Schaufler wrote:
>>>> On 7/8/2019 10:16 AM, Xing, Cedric wrote:
>>>>> On 7/8/2019 9:26 AM, Casey Schaufler wrote:
>>>>>> In this scheme you use an ema LSM to manage your ema data.
>>>>>> A quick sketch looks like:
>>>>>>
>>>>>> sgx_something_in() calls
>>>>>> security_enclave_load() calls
>>>>>> ema_enclave_load()
>>>>>> selinux_enclave_load()
>>>>>> otherlsm_enclave_load()
>>>>>>
>>>>>> Why is this better than:
>>>>>>
>>>>>> sgx_something_in() calls
>>>>>> ema_enclave_load()
>>>>>> security_enclave_load() calls
>>>>>> selinux_enclave_load()
>>>>>> otherlsm_enclave_load()
>>>>>
>>>>> Are you talking about moving EMA somewhere outside LSM?
>>>>
>>>> Yes. That's what I've been saying all along.
>>>>
>>>>> If so, where?
>>>>
>>>> I tried to make it obvious. Put the call to your EMA code
>>>> on the line before you call security_enclave_load().
>>>
>>> Sorry but I'm still confused.
>>>
>>> EMA code is used by LSMs only. Making it callable from other parts of the kernel IMHO is probably not a good idea. And more importantly I don't understand the motivation behind it. Would you please elaborate?
>>
>> LSM modules implement additional access control restrictions.
>> The EMA code does not do that, it provides management of data
>> that is used by security modules. It is not one itself. VFS
>> also performs this role, but no one would consider making VFS
>> a security module.
>
> You are right.
So far, so good ...
> EMA is more like a helper library than a real LSM.
Then you should use it as such.
> But the practical problem is, it has a piece of initialization code, to basically request some space in the file blob from the LSM infrastructure.
The security modules that want to use EMA should
request that space.
> That cannot be done by any LSMs at runtime.
Sure it can. And it has to. What if you don't
have any security modules that use the EMA data?
Surely you don't want to be allocating blob space
for EMA data if no one is going to use it.
> So it has to either be done in LSM infrastructure directly, or make itself an LSM to make its initialization function invoked by LSM infrastructure automatically.
That is not true. The security module that wants to use
the EMA data can call whatever allocation function you use.
Or, the call can be made from the code just before you call
the security hook, which would be identical to calling it
as a "first" hook.
> You have objected to the former, so I switched to the latter. Are you now objecting to the latter as well? Then what are you suggesting, really?
Call your allocation function just before you call
security_enclave_load(). There is no way that selinux_enclave_load()
could tell the difference.
> VFS is a completely different story. It's the file system abstraction so it has a natural place to live in the kernel, and its initialization doesn't depend on the LSM infrastructure. EMA on the other hand, shall belong to LSM because it is both produced and consumed within LSM.
And this is the enclave abstraction, or rather, should be
according to at least half the people joining in on the
thread. It does not belong in the LSM infrastructure because
it is it's own thing, with its own state and data, which it
needs to maintain in its own way and place. It needs interfaces
so that security modules can use that information appropriately.
It needs a hook or two so that the enclave abstraction can ask
the security modules to make decisions.
>
> And, Stephen, do you have an opinion on this?
>
>>>>>>> +/**
>>>>>>> + * ema - Enclave Memory Area structure for LSM modules
>>>>>>
>>>>>> LSM modules is redundant. "LSM" or "LSMs" would be better.
>>>>>
>>>>> Noted
>>>>>
>>>>>>> diff --git a/security/Makefile b/security/Makefile
>>>>>>> index c598b904938f..b66d03a94853 100644
>>>>>>> --- a/security/Makefile
>>>>>>> +++ b/security/Makefile
>>>>>>> @@ -28,6 +28,7 @@ obj-$(CONFIG_SECURITY_YAMA) += yama/
>>>>>>> obj-$(CONFIG_SECURITY_LOADPIN) += loadpin/
>>>>>>> obj-$(CONFIG_SECURITY_SAFESETID) += safesetid/
>>>>>>> obj-$(CONFIG_CGROUP_DEVICE) += device_cgroup.o
>>>>>>> +obj-$(CONFIG_INTEL_SGX) += commonema.o
>>>>>>
>>>>>> The config option and the file name ought to match,
>>>>>> or at least be closer.
>>>>>
>>>>> Just trying to match file names as "capability" uses commoncap.c.
>>>>
>>>> Fine, then you should be using CONFIG_SECURITY_EMA.
>>>>
>>>>>
>>>>> Like I said, this feature could potentially be used by TEEs other than SGX. For now, SGX is the only user so it is tied to CONFIG_INTEL_SGX. I can rename it to ema.c or enclave.c. Do you have a preference?
>>>>
>>>> Make
>>>> CONFIG_SECURITY_EMA
>>>> depends on CONFIG_INTEL_SGX
>>>>
>>>> When another TEE (maybe MIPS_SSRPQ) comes along you can have
>>>>
>>>> CONFIG_SECURITY_EMA
>>>> depends on CONFIG_INTEL_SGX || CONFIG_MIPS_SSRPQ
>>>
>>> Your suggestions are reasonable. Given such config change wouldn't affect any code, can we do it later,
>>
>> That doesn't make the current options any less confusing,
>> and it will be easier to make the change now than at some
>> point in the future.
>>
>>> e.g., when additional TEEs come online and make use of these new hooks? After all, security_enclave_init() will need amendment anyway as one of its current parameters is of type 'struct sgx_sigstruct', which will need to be replaced with something more generic. At the time being, I'd like to keep things intuitive so as not to confuse reviewers.
>>
>> Reviewers (including me) are already confused by the inconsistency.
>
> OK. Let me make this change.
Thank you.
>>>>>
>>>>>>> diff --git a/security/commonema.c b/security/commonema.c
>>>>>>
>>>>>> Put this in a subdirectory. Please.
>>>>>
>>>>> Then why is commoncap.c located in this directory? I'm just trying to match the existing convention.
>>>>
>>>> commoncap is not optional. It is a base part of the
>>>> security subsystem. ema is optional.
>>>
>>> Alright. I'd move it into a sub-folder and rename it to ema.c. Would you be ok with that?
>>
>> Sounds fine.
>
> This is another part that confuses me. Per you comment here, I think you are OK with EMA being part of LSM
Ah. Being in the security directory does not mean it's
a part of the LSM system. Keys and integrity are security
subsystems that are related to, but not part of, the LSM
sub-system.
> (I mean, living somewhere under security/). But your other comment of calling ema_enclave_load() alongside security_enclave_load() made me think EMA and LSM were separate. What do you want really?
Please stop asking the same question over and over.
You're not going to get a different answer from what
you've gotten already. Look back at it what's already
been said.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox