* Re: [RFC PATCH v3 3/4] X86/sgx: Introduce EMA as a new LSM module
From: Casey Schaufler @ 2019-07-08 23:53 UTC (permalink / raw)
To: Xing, Cedric, linux-sgx, linux-security-module, selinux, casey
In-Reply-To: <ce4dcce2-88fb-ccec-f173-fc567d9ca006@intel.com>
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().
>
>>
>> If you did really want ema to behave like an LSM
>> you would put the file data that SELinux is managing
>> into the ema portion of the blob and provide interfaces
>> for the SELinux (or whoever) to use that. Also, it's
>> an abomination (as I've stated before) for ema to
>> rely on SELinux to provide a file_free() hook for
>> ema's data. If you continue down the LSM route, you
>> need to provide an ema_file_free() hook. You can't
>> count on SELinux to do it for you. If there are multiple
>> LSMs (coming soon!) that use the ema data, they'll all
>> try to free it, and then Bad Things can happen.
>
> I'm afraid you have misunderstood the code. What is kept open and gets closed in selinux_file_free() is the sigstruct file. SELinux uses it to determine the page permissions for enclave pages from anonymous sources. It is a policy choice made inside SELinux and has nothing to do with EMA.
OK.
>
> There's indeed an ema_file_free_security() to free the EMA map for enclaves being closed. EMA does *NOT* rely on any other LSMs to free data for it. The only exception is when an LSM fails enclave_load(), it has to call ema_remove_range() to remove the range being added, which was *not* required originally in v2.
OK.
>
>>> +/**
>>> + * 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
>
>>> 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.
>
>>> +static struct lsm_blob_sizes ema_blob_sizes __lsm_ro_after_init = {
>>> + .lbs_file = sizeof(atomic_long_t),
>>> +};
>>
>> If this is ema's data ema must manage it. You *must* have
>> a file_free().
>
> There is one indeed - ema_file_free_security().
I see it now.
>
>>
>>> +
>>> +static atomic_long_t *_map_file(struct file *encl)
>>> +{
>>> + return (void *)((char *)(encl->f_security) + ema_blob_sizes.lbs_file);
>>
>> I don't trust all the casting going on here, especially since
>> you don't end up with the type you should be returning.
>
> Will change.
>
>>> +}
>>> +
>>> +static struct ema_map *_alloc_map(void)
>>
>> Function header comments, please.
>
> Will add.
^ permalink raw reply
* Re: [RFC PATCH v2 1/3] x86/sgx: Add SGX specific LSM hooks
From: Casey Schaufler @ 2019-07-09 0:02 UTC (permalink / raw)
To: Dr. Greg, casey
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: <20190707133023.GA4521@wind.enjellic.com>
On 7/7/2019 6:30 AM, Dr. Greg wrote:
> On Wed, Jul 03, 2019 at 08:32:10AM -0700, Casey Schaufler wrote:
>
> Good morning, I hope the weekend has been enjoyable for everyone.
>
>>>> On 7/2/2019 12:42 AM, Xing, Cedric wrote:
>>>>> ...
>>>>> Guess this discussion will never end if we don't get into
>>>>> code. Guess it'd be more productive to talk over phone then come back
>>>>> to this thread with a conclusion. Will that be ok with you?
>>>> I don't think that a phone call is going to help. Talking code
>>>> issues tends to muddle them in my brain. If you can give me a few
>>>> days I will propose a rough version of how I think your code should
>>>> be integrated into the LSM environment. I'm spending more time
>>>> trying (unsuccessfully :( ) to discribe the issues in English than
>>>> it will probably take in C.
>>> While Casey is off writing his rosetta stone,
>> I'd hardly call it that. More of an effort to round the corners on
>> the square peg. And Cedric has some ideas on how to approach that.
> Should we infer from this comment that, of the two competing
> strategies, Cedric's is the favored architecture?
With Cedric's latest patches I'd say there's only one
strategy. There's still some refinement to do, but we're
getting there.
>>> let me suggest that the
>>> most important thing we need to do is to take a little time, step back
>>> and look at the big picture with respect to what we are trying to
>>> accomplish and if we are going about it in a way that makes any sense
>>> from an engineering perspective.
>>>
>>> This conversation shouldn't be about SGX, it should be about the best
>>> way for the kernel/LSM to discipline a Trusted Execution Environment
>>> (TEE). As I have noted previously, a TEE is a 'blackbox' that, by
>>> design, is intended to allow execution of code and processing of data
>>> in a manner that is resistant to manipulation or inspection by
>>> untrusted userspace, the kernel and/or the hardware itself.
>>>
>>> Given that fact, if we are to be intellectually honest, we need to ask
>>> ourselves how effective we believe we can be in controlling any TEE
>>> with kernel based mechanisms. This is particularly the case if the
>>> author of any code running in the TEE has adversarial intent.
>>>
>>> Here is the list of controls that we believe an LSM can, effectively,
>>> implement against a TEE:
>>>
>>> 1.) Code provenance and origin.
>>>
>>> 2.) Cryptographic verification of dynamically executable content.
>>>
>>> 2.) The ability of a TEE to implement anonymous executable content.
>>>
>>> If people are in agreement with this concept, it is difficult to
>>> understand why we should be implementing complex state machines and
>>> the like, whether it is in the driver or the LSM. Security code has
>>> to be measured with a metric of effectiveness, otherwise we are
>>> engaging in security theater.
>>>
>>> I believe that if we were using this lens, we would already have a
>>> mainline SGX driver, since we seem to have most of the needed LSM
>>> infrastructure and any additional functionality would be a straight
>>> forward implementation. Most importantly, the infrastructure would
>>> not be SGX specific, which would seem to be a desirable political
>>> concept.
>> Generality introduced in the absence of multiple instances
>> often results in unnecessary complexity, unused interfaces
>> and feature compromise. Guessing what other TEE systems might
>> do, and constraining SGX to those models (or the other way around)
>> is a well established road to ruin. The LSM infrastructure is
>> a fine example. For the first ten years the "general" mechanism
>> had a single user. I'd say to hold off on the general until there
>> is more experience with the specific. It's easier to construct
>> a general mechanism around things that work than to fit things
>> that need to work into some preconceived notion of generality.
> 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.
> Everyone wants to see this driver upstream. If we would have had a
> reasoned discussion regarding what it means to implement proper
> controls around a TEE, when we started to bring these issues forward
> last November, we could have possibly been on the road to having a
> driver with reasoned security controls and one that actually delivers
> the security guarantees the hardware was designed to deliver.
>
> Best wishes for a productive week to everyone.
>
> Dr. Greg
>
> As always,
> Dr. G.W. Wettstein, Ph.D. Enjellic Systems Development, LLC.
> 4206 N. 19th Ave. Specializing in information infra-structure
> Fargo, ND 58102 development.
> PH: 701-281-1686 EMAIL: greg@enjellic.com
> ------------------------------------------------------------------------------
> "Any intelligent fool can make things bigger and more complex... It
> takes a touch of genius - and a lot of courage to move in the opposite
> direction."
> -- Albert Einstein
^ permalink raw reply
* Re: [PATCH 02/10] vfs: syscall: Add move_mount(2) to move mounts around
From: Eric W. Biederman @ 2019-07-09 0:13 UTC (permalink / raw)
To: Al Viro
Cc: Tetsuo Handa, David Howells, linux-api, linux-fsdevel, torvalds,
linux-security-module
In-Reply-To: <20190708202124.GX17978@ZenIV.linux.org.uk>
Al Viro <viro@zeniv.linux.org.uk> writes:
> On Mon, Jul 08, 2019 at 07:01:32PM +0100, Al Viro wrote:
>> On Mon, Jul 08, 2019 at 12:12:21PM -0500, Eric W. Biederman wrote:
>>
>> > Al you do realize that the TOCTOU you are talking about comes the system
>> > call API. TOMOYO can only be faulted for not playing in their own
>> > sandbox and not reaching out and fixing the vfs implementation details.
>
> PS: the fact that mount(2) has been overloaded to hell and back (including
> MS_MOVE, which goes back to v2.5.0.5) predates the introduction of ->sb_mount()
> and LSM in general (2.5.27). MS_BIND is 2.4.0-test9pre2.
>
> In all the years since the introduction of ->sb_mount() I've seen zero
> questions from LSM folks regarding a sane place for those checks. What I have
> seen was "we want it immediately upon the syscall entry, let the module
> figure out what to do" in reply to several times I tried to tell them "folks,
> it's called in a bad place; you want the checks applied to objects, not to
> raw string arguments".
>
> As it is, we have easily bypassable checks on mount(2) (by way of ->sb_mount();
> there are other hooks also in the game for remounts and new mounts).
>
> I see no point whatsoever trying to duplicate ->sb_mount() on the top level
> of move_mount(2). When and if sane checks are agreed upon for that thing,
> they certainly should be used both for MS_MOVE case of mount(2) and for
> move_mount(2). And that'll come for free from calling those in do_move_mount().
> They won't be the first thing called in mount(2) - we demultiplex first,
> decide that we have a move and do pathname resolution on source. And that's
> precisely what we need to avoid the TOCTOU there.
>
> I'm sorry, but this "run the hook at the very beginning, the modules know
> better what they want, just give them as close to syscall arguments as
> possible before even looking at the flags" model is wrong, plain and simple.
>
> As for the MS_MOVE checks, the arguments are clear enough (two struct path *,
> same as what we pass to do_move_mount()). AFAICS, only tomoyo and
> apparmor are trying to do anything for MS_MOVE in ->sb_mount(), and both
> look like it should be easy enough to implement what said checks intend
> to do (probably - assuming that aa_move_mount() doesn't depend upon
> having its kern_path() inside the __begin_current_label_crit_section()/
> __end_current_label_crit_section() pair; looks like it shouldn't be,
> but that's for apparmor folks to decide).
>
> That's really for LSM folks, though - I've given up on convincing
> (or even getting a rationale out of) them on anything related to hook
> semantics years ago.
I have found the LSM folks in recent years to be rather reasonable,
especially when something concrete has been proposed.
A quick look suggests that the new security_mount_move is a reasonable
hook for the mount_move check.
Tetsuo, do you think you can implement the checks you need for Tomoyo
for mount_move on top of the new security_mount_move?
Al is proposing that similar hooks be added for the other subcases of
mount so that less racy hooks can be implemented. Tetsuo do you have
any problem with that?
Tetsuo whatever the virtues of this patchset getting merged into Linus's
tree it is merged now, so the only thing we can do now is roll up our
sleeves go through everything and fix the regressions/bugs/issues that
have emerged with the new mount api.
Eric
^ permalink raw reply
* Re: [RFC PATCH v3 2/4] x86/64: Call LSM hooks from SGX subsystem/module
From: Sean Christopherson @ 2019-07-09 1:03 UTC (permalink / raw)
To: Cedric Xing; +Cc: linux-sgx, linux-security-module, selinux
In-Reply-To: <a1a4f6a4f7be05ce1635b48a80cea86c27ee14cc.1562542383.git.cedric.xing@intel.com>
On Sun, Jul 07, 2019 at 04:41:32PM -0700, Cedric Xing wrote:
...
> @@ -575,6 +576,46 @@ static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
> return ret;
> }
>
> +static int sgx_encl_prepare_page(struct file *filp, unsigned long dst,
> + unsigned long src, void *buf)
> +{
> + struct vm_area_struct *vma;
> + unsigned long prot;
> + int rc;
> +
> + if (dst & ~PAGE_SIZE)
> + return -EINVAL;
> +
> + rc = down_read_killable(¤t->mm->mmap_sem);
> + if (rc)
> + return rc;
> +
> + vma = find_vma(current->mm, dst);
> + if (vma && dst >= vma->vm_start)
> + prot = _calc_vm_trans(vma->vm_flags, VM_READ, PROT_READ) |
> + _calc_vm_trans(vma->vm_flags, VM_WRITE, PROT_WRITE) |
> + _calc_vm_trans(vma->vm_flags, VM_EXEC, PROT_EXEC);
> + else
> + prot = 0;
> +
> + vma = find_vma(current->mm, src);
> + if (!vma || src < vma->vm_start || src + PAGE_SIZE > vma->vm_end)
> + rc = -EFAULT;
> +
> + if (!rc && !(vma->vm_flags & VM_MAYEXEC))
> + rc = -EACCES;
Disallowing loading enclave *data* from a noexec file system is an arbitrary
and weird restriction.
> +
> + if (!rc && copy_from_user(buf, (void __user *)src, PAGE_SIZE))
> + rc = -EFAULT;
> +
> + if (!rc)
> + rc = security_enclave_load(filp, dst, PAGE_SIZE, prot, vma);
> +
> + up_read(¤t->mm->mmap_sem);
> +
> + return rc;
> +}
^ permalink raw reply
* Re: [RFC PATCH v3 4/4] x86/sgx: Implement SGX specific hooks in SELinux
From: Sean Christopherson @ 2019-07-09 1:33 UTC (permalink / raw)
To: Cedric Xing; +Cc: linux-sgx, linux-security-module, selinux
In-Reply-To: <3a9efc8d3c27490dbcfe802ce3facddd62f47872.1562542383.git.cedric.xing@intel.com>
On Sun, Jul 07, 2019 at 04:41:34PM -0700, Cedric Xing wrote:
> +static int enclave_mprotect(struct vm_area_struct *vma, size_t prot)
> +{
> + struct ema_map *m;
> + int rc;
> +
> + /* is vma an enclave vma ? */
> + if (!vma->vm_file)
> + return 0;
> + m = ema_get_map(vma->vm_file);
> + if (!m)
> + return 0;
> +
> + /* WX requires EXECMEM */
> + if ((prot && PROT_WRITE) && (prot & PROT_EXEC)) {
> + rc = avc_has_perm(&selinux_state, current_sid(), current_sid(),
> + SECCLASS_PROCESS, PROCESS__EXECMEM, NULL);
> + if (rc)
> + return rc;
> + }
> +
> + rc = ema_lock_map(m);
> + if (rc)
> + return rc;
> +
> + if ((prot & PROT_EXEC) && !(vma->vm_flags & VM_EXEC))
> + rc = ema_apply_to_range(m, vma->vm_start, vma->vm_end,
> + ema__chk_X_cb, vma->vm_file);
> + if (!rc && (prot & PROT_WRITE) && !(vma->vm_flags & VM_WRITE))
> + rc = ema_apply_to_range(m, vma->vm_start, vma->vm_end,
> + ema__set_M_cb, NULL);
Not tracking whether a page has been mapped X and having ema__chk_W_cb()
allows an application to circumvent W^X policies by spinning up a helper
process.
Ignoring that issue, this approach suffers from the same race condition I
pointed out a while back[1]. If process A maps a page W and process B
maps the same page X, then the result of ema__chk_X_cb() depends on the
order of mprotect() calls between A and B.
[1] https://lore.kernel.org/linux-security-module/20190614200123.GA32570@linux.intel.com/
> + ema_unlock_map(m);
> +
> + return rc;
> +}
^ permalink raw reply
* Re: [RFC PATCH v2 1/3] x86/sgx: Add SGX specific LSM hooks
From: Sean Christopherson @ 2019-07-09 1:52 UTC (permalink / raw)
To: Casey Schaufler
Cc: Dr. Greg, 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
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:
> > On Wed, Jul 03, 2019 at 08:32:10AM -0700, Casey Schaufler wrote:
> >
> > Good morning, I hope the weekend has been enjoyable for everyone.
> >
> >>>> On 7/2/2019 12:42 AM, Xing, Cedric wrote:
> >>>>> ...
> >>>>> Guess this discussion will never end if we don't get into
> >>>>> code. Guess it'd be more productive to talk over phone then come back
> >>>>> to this thread with a conclusion. Will that be ok with you?
> >>>> I don't think that a phone call is going to help. Talking code
> >>>> issues tends to muddle them in my brain. If you can give me a few
> >>>> days I will propose a rough version of how I think your code should
> >>>> be integrated into the LSM environment. I'm spending more time
> >>>> trying (unsuccessfully :( ) to discribe the issues in English than
> >>>> it will probably take in C.
> >>> While Casey is off writing his rosetta stone,
> >> I'd hardly call it that. More of an effort to round the corners on
> >> the square peg. And Cedric has some ideas on how to approach that.
> > Should we infer from this comment that, of the two competing
> > strategies, Cedric's is the favored architecture?
>
> With Cedric's latest patches I'd say there's only one
> strategy. There's still some refinement to do, but we're
> getting there.
Dynamic tracking has an unsolvable race condition. If process A maps a
page W and process B maps the same page X, then the result of W^X checks
depends on the order of mprotect() calls between A and B.
If we're ok saying "don't do that" then I can get behind dynamic tracking
as a whole. Even if we settle on dynamic tracking, where that tracking
code lives is still an open IMO.
^ permalink raw reply
* Re: [GIT PULL] SELinux patches for v5.3
From: pr-tracker-bot @ 2019-07-09 3:15 UTC (permalink / raw)
To: Paul Moore; +Cc: Linus Torvalds, selinux, linux-security-module, linux-kernel
In-Reply-To: <CAHC9VhSERNCM2d42y8fBT236D62mco=B_ZM_vytEoBP1qicvCA@mail.gmail.com>
The pull request you sent on Tue, 2 Jul 2019 13:28:37 -0400:
> git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git tags/selinux-pr-20190702
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/7c0f89634892693fc0b46f25e0a6d57bd6dd5698
Thank you!
--
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker
^ permalink raw reply
* Re: [GIT PULL] Keys: Set 1 - Miscellany for 5.3
From: pr-tracker-bot @ 2019-07-09 3:15 UTC (permalink / raw)
To: David Howells
Cc: torvalds, dhowells, jmorris, keyrings, linux-security-module,
linux-kernel
In-Reply-To: <26702.1562360619@warthog.procyon.org.uk>
The pull request you sent on Fri, 05 Jul 2019 22:03:39 +0100:
> git://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git tags/keys-misc-20190619
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/504b69eb3c95180bc59f1ae9096ad4b10bbbf254
Thank you!
--
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker
^ permalink raw reply
* Re: [GIT PULL] Keys: Set 2 - request_key() improvements for 5.3
From: pr-tracker-bot @ 2019-07-09 3:15 UTC (permalink / raw)
To: David Howells
Cc: torvalds, dhowells, jmorris, keyrings, linux-security-module,
linux-kernel
In-Reply-To: <27327.1562361214@warthog.procyon.org.uk>
The pull request you sent on Fri, 05 Jul 2019 22:13:34 +0100:
> git://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git tags/keys-request-20190626
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/f771fde82051976a6fc0fd570f8b86de4a92124b
Thank you!
--
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker
^ permalink raw reply
* Re: [GIT PULL] Keys: Set 4 - Key ACLs for 5.3
From: pr-tracker-bot @ 2019-07-09 3:15 UTC (permalink / raw)
To: David Howells
Cc: torvalds, dhowells, jmorris, keyrings, netdev, linux-nfs,
linux-cifs, linux-afs, linux-fsdevel, linux-integrity,
linux-security-module, linux-kernel
In-Reply-To: <28477.1562362239@warthog.procyon.org.uk>
The pull request you sent on Fri, 05 Jul 2019 22:30:39 +0100:
> git://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git tags/keys-acl-20190703
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/0f75ef6a9cff49ff612f7ce0578bced9d0b38325
Thank you!
--
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker
^ permalink raw reply
* Re: [GIT PULL] tpmdd updates for Linux v5.3
From: pr-tracker-bot @ 2019-07-09 3:15 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: torvalds, linux-kernel, linux-security-module, linux-integrity,
jmorris, mjg59
In-Reply-To: <2595b4f6ce49cc3d413c75f86a63ad9d26f0f1fd.camel@linux.intel.com>
The pull request you sent on Thu, 27 Jun 2019 01:50:47 +0300:
> git://git.infradead.org/users/jjs/linux-tpmdd.git tags/tpmdd-next-20190625
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/884922591e2b58fd7f1018701f957446d1ffac4d
Thank you!
--
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker
^ permalink raw reply
* Re: [GIT PULL] Keys: Set 3 - Keyrings namespacing for 5.3
From: pr-tracker-bot @ 2019-07-09 3:15 UTC (permalink / raw)
To: David Howells
Cc: torvalds, dhowells, jmorris, ebiederm, dwalsh, keyrings, netdev,
linux-nfs, linux-cifs, linux-afs, linux-security-module,
linux-kernel
In-Reply-To: <27850.1562361644@warthog.procyon.org.uk>
The pull request you sent on Fri, 05 Jul 2019 22:20:44 +0100:
> git://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git tags/keys-namespace-20190627
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/2e12256b9a76584fa3a6da19210509d4775aee36
Thank you!
--
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker
^ permalink raw reply
* Re: keyrings pull requests for the next merge window
From: Linus Torvalds @ 2019-07-09 3:25 UTC (permalink / raw)
To: David Howells; +Cc: LSM List, Linux List Kernel Mailing, James Morris, keyrings
In-Reply-To: <10976.1562256893@warthog.procyon.org.uk>
[ Adding a few mailing lists, since the thrust of my email is about
more people being around and involved, and the pull requests
themselves were indeed cc'd to the mailing lists too ]
On Thu, Jul 4, 2019 at 9:15 AM David Howells <dhowells@redhat.com> wrote:
>
> I have a bunch of keyrings patches to be pulled in during the merge window. I
> believe you want security patches to go directly to you rather than through
> James now?
>
> I've divided these patches into four logical sets, though due to conflicting
> changes the sets are in a sequence, built one upon another.
>
> How do you want them presenting? Do you want a pull request for each set, one
> for all of them or would you prefer they go through James's security tree?
So I was traveling when this email came in, but in the meantime you
sent the four pull requests and I have now pulled them all. You should
have gotten the pr-tracker-bot notification already (or it will happen
soon).
An initial very positive comment: the pull requests themselves with
all the explanations were very good. That part of the process worked
very well, I think.
I felt like I got an explanation of what I pulled, and I think the
merge commits themselves are the better for it, so that the
explanation now remains in the git history, and other people too can
see what got merged and why.
HOWEVER.
There are parts I really didn't much like was when I look at all the
individual commits themselves.
Again, the commit messages there are good and that part all looks fine.
BUT.
The history itself looks questionable. The dates don't make sense, and
the different branches were obviously all done together as a linear
history, rebased, and worked on as one single branch, . Fine - it was
then at least partitioned into sensible parts, and sometimes this is
how it really ends up working, but I did get the feeling that this was
all very artificial and more importantly I get the feeling that none
of the commits had any real-life exposure.
That lack of real-life exposure also shows in the almost complete lack
of any reviews, any commentary from other people, and absolutely
nobody else seems to have been involved. Not as an author, but not in
any other capacity either. There were a couple of initial commits that
had reviewed-by's, but apart from that there really was *no* sign of
any outside involvement at all.
I looked up a couple of the patches on patchwork too, and saw zero
discussion. Maybe the ones I picked just happened to have none, but I
really get the feeling that pretty much none of this had any external
input what-so-ever.
And that makes me unhappy.
In other words: the pull requests, the explanations, everything looked
very good and I enjoyed that part. I don't see any new warnings, and
everything built fine at every stage. I have no real technical
complaints from that angle.
But I absolutely abhor how this seems to all have been developed in a
complete and utter vacuum. That just fundamentally worries me. I
can't point to anything being bad, but the lack of any kind of work
from anybody else just makes me antsy.
Is there really nobody else working or caring about this at all?
This is not new, and I do note that your afs work tends to have the
same pattern (but honestly, when it comes to one particular odd
filesystem or driver, it's not something I react to). It's just
perhaps more noticeable to me now that I pull directly, and it's much
more noticeable when it's a _subsystem_ rather than something like a
end-point driver/filesystem. I think the pulls themselves worked, and
I don't mind the direct pulling, but I *do* notice that I end up
minding the fact that now with the direct pulls, there's even _less_
of a "at least somebody else looked and cared".
Put another way: I'd like other people to be involved. Either as
reviewers, or as intermediate people, or _something_. The "David
Howells lives in his own world and nobody else looks at it and then he
sends it directly to Linus" model makes me somewhat unhappy.
Again, I'd like to stress that the pull requests themselves were fine,
and I have no complaints on that side and I have (at least as of yet)
no reason to worry about the code itself. It's really the "lone
developer sends directly to me" that stands out as not happening
elsewhere that I'm worried about.
Hmm?
Linus
^ permalink raw reply
* Re: [GIT PULL] integrity subsystem updates for v5.3
From: pr-tracker-bot @ 2019-07-09 3:50 UTC (permalink / raw)
To: Mimi Zohar
Cc: Linus Torvalds, linux-security-module, linux-integrity,
linux-kernel
In-Reply-To: <1562590738.11461.9.camel@linux.ibm.com>
The pull request you sent on Mon, 08 Jul 2019 08:58:58 -0400:
> git://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git next-integrity
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/8b68150883ca466a23e90902dd4113b22e692f04
Thank you!
--
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker
^ permalink raw reply
* Re: [PATCH v5 06/12] S.A.R.A.: WX protection
From: Kees Cook @ 2019-07-09 4:51 UTC (permalink / raw)
To: Salvatore Mesoraca
Cc: Al Viro, linux-kernel, Kernel Hardening, linux-mm,
linux-security-module, Brad Spengler, Casey Schaufler,
Christoph Hellwig, Jann Horn, PaX Team, Serge E. Hallyn,
Thomas Gleixner, James Morris
In-Reply-To: <CAJHCu1+JYWN7mEHprmCc+osP=K4qGA9xB3Jxg53_K4kwo4J6dA@mail.gmail.com>
On Sun, Jul 07, 2019 at 05:49:35PM +0200, Salvatore Mesoraca wrote:
> Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > On Sat, Jul 06, 2019 at 12:54:47PM +0200, Salvatore Mesoraca wrote:
> >
> > > +#define sara_warn_or_return(err, msg) do { \
> > > + if ((sara_wxp_flags & SARA_WXP_VERBOSE)) \
> > > + pr_wxp(msg); \
> > > + if (!(sara_wxp_flags & SARA_WXP_COMPLAIN)) \
> > > + return -err; \
> > > +} while (0)
> > > +
> > > +#define sara_warn_or_goto(label, msg) do { \
> > > + if ((sara_wxp_flags & SARA_WXP_VERBOSE)) \
> > > + pr_wxp(msg); \
> > > + if (!(sara_wxp_flags & SARA_WXP_COMPLAIN)) \
> > > + goto label; \
> > > +} while (0)
> >
> > No. This kind of "style" has no place in the kernel.
> >
> > Don't hide control flow. It's nasty enough to reviewers,
> > but it's pure hell on anyone who strays into your code while
> > chasing a bug or doing general code audit. In effect, you
> > are creating your oh-so-private C dialect and assuming that
> > everyone who ever looks at your code will start with learning
> > that *AND* incorporating it into their mental C parser.
> > I'm sorry, but you are not that important.
> >
> > If it looks like a function call, a casual reader will assume
> > that this is exactly what it is. And when one is scanning
> > through a function (e.g. to tell if handling of some kind
> > of refcounts is correct, with twentieth grep through the
> > tree having brought something in your code into the view),
> > the last thing one wants is to switch between the area-specific
> > C dialects. Simply because looking at yours is sandwiched
> > between digging through some crap in drivers/target/ and that
> > weird thing in kernel/tracing/, hopefully staying limited
> > to 20 seconds of glancing through several functions in your
> > code.
> >
> > Don't Do That. Really.
>
> I understand your concerns.
> The first version of SARA didn't use these macros,
> they were added because I was asked[1] to do so.
>
> I have absolutely no problems in reverting this change.
> I just want to make sure that there is agreement on this matter.
> Maybe Kees can clarify his stance.
>
> Thank you for your suggestions.
>
> [1] https://lkml.kernel.org/r/CAGXu5jJuQx2qOt_aDqDQDcqGOZ5kmr5rQ9Zjv=MRRCJ65ERfGw@mail.gmail.com
I just didn't like how difficult it was to review the repeated checking.
I thought then (and still think now) it's worth the unusual style to
improve the immediate readability. Obviously Al disagrees. I'm not
against dropping my suggestion; it's just a pain to review it and it
seems like an area that would be highly prone to subtle typos. Perhaps
some middle ground:
#define sara_warn(msg) ({ \
if ((sara_wxp_flags & SARA_WXP_VERBOSE)) \
pr_wxp(msg); \
!(sara_wxp_flags & SARA_WXP_COMPLAIN); \
})
...
if (unlikely(sara_wxp_flags & SARA_WXP_WXORX &&
vm_flags & VM_WRITE &&
vm_flags & VM_EXEC &&
sara_warn("W^X")))
return -EPERM;
that way the copy/pasting isn't present but the control flow is visible?
--
Kees Cook
^ permalink raw reply
* Re: [RFC 3/7] tee: add private login method for kernel clients
From: Sumit Garg @ 2019-07-09 5:56 UTC (permalink / raw)
To: Jens Wiklander
Cc: keyrings, linux-integrity, linux-security-module, corbet,
dhowells, jejb, Jarkko Sakkinen, Mimi Zohar, jmorris, serge,
Ard Biesheuvel, Daniel Thompson, linux-doc,
Linux Kernel Mailing List, tee-dev
In-Reply-To: <20190708153908.GA28253@jax>
Thanks Jens for your comments.
On Mon, 8 Jul 2019 at 21:09, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> Hi Sumit,
>
> On Thu, Jun 13, 2019 at 04:00:29PM +0530, Sumit Garg wrote:
> > There are use-cases where user-space shouldn't be allowed to communicate
> > directly with a TEE device which is dedicated to provide a specific
> > service for a kernel client. So add a private login method for kernel
> > clients and disallow user-space to open-session using this login method.
> >
> > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> > ---
> > drivers/tee/tee_core.c | 6 ++++++
> > include/uapi/linux/tee.h | 2 ++
> > 2 files changed, 8 insertions(+)
> >
> > diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
> > index 0f16d9f..4581bd1 100644
> > --- a/drivers/tee/tee_core.c
> > +++ b/drivers/tee/tee_core.c
> > @@ -334,6 +334,12 @@ static int tee_ioctl_open_session(struct tee_context *ctx,
> > goto out;
> > }
> >
> > + if (arg.clnt_login == TEE_IOCTL_LOGIN_REE_KERNEL) {
> TEE_IOCTL_LOGIN_REE_KERNEL is defined as 0x80000000 which is in the
> range specified and implementation defined by the GP spec. I wonder if
> we shouldn't filter the entire implementation defined range instead of
> just this value.
Agree. Will rather check for entire implementation defined range:
0x80000000 - 0xFFFFFFFF.
>
> > + pr_err("login method not allowed for user-space client\n");
> pr_debug(), if it's needed at all.
>
Ok will use pr_debug() instead.
> > + rc = -EPERM;
> > + goto out;
> > + }
> > +
> > rc = ctx->teedev->desc->ops->open_session(ctx, &arg, params);
> > if (rc)
> > goto out;
> > diff --git a/include/uapi/linux/tee.h b/include/uapi/linux/tee.h
> > index 4b9eb06..f33c69c 100644
> > --- a/include/uapi/linux/tee.h
> > +++ b/include/uapi/linux/tee.h
> > @@ -172,6 +172,8 @@ struct tee_ioctl_buf_data {
> > #define TEE_IOCTL_LOGIN_APPLICATION 4
> > #define TEE_IOCTL_LOGIN_USER_APPLICATION 5
> > #define TEE_IOCTL_LOGIN_GROUP_APPLICATION 6
> > +/* Private login method for REE kernel clients */
> It's worth noting that this is filtered by the TEE framework, compared
> to everything else which is treated opaquely.
>
IIUC, you are referring to login filter in optee_os. Change to prevent
filter for this login method is part of this PR [1].
[1] https://github.com/OP-TEE/optee_os/pull/3082
-Sumit
> > +#define TEE_IOCTL_LOGIN_REE_KERNEL 0x80000000
> >
> > /**
> > * struct tee_ioctl_param - parameter
> > --
> > 2.7.4
> >
>
> Thanks,
> Jens
^ permalink raw reply
* Re: [RFC 0/7] Introduce TEE based Trusted Keys support
From: Sumit Garg @ 2019-07-09 5:58 UTC (permalink / raw)
To: Jens Wiklander
Cc: corbet, dhowells, jejb, Jarkko Sakkinen, Mimi Zohar, jmorris,
serge, Ard Biesheuvel, Daniel Thompson, linux-doc,
Linux Kernel Mailing List, tee-dev, keyrings,
linux-security-module, linux-integrity
In-Reply-To: <20190708163140.GB28253@jax>
On Mon, 8 Jul 2019 at 22:01, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> Hi Sumit,
>
> On Mon, Jul 08, 2019 at 06:11:39PM +0530, Sumit Garg wrote:
> > Hi Jens,
> >
> > On Thu, 13 Jun 2019 at 16:01, Sumit Garg <sumit.garg@linaro.org> wrote:
> > >
> > > Add support for TEE based trusted keys where TEE provides the functionality
> > > to seal and unseal trusted keys using hardware unique key. Also, this is
> > > an alternative in case platform doesn't possess a TPM device.
> > >
> > > This series also adds some TEE features like:
> > >
> > > Patch #1, #2 enables support for registered kernel shared memory with TEE.
> > >
> >
> > Would you like to pick up Patch #1, #2 separately? I think both these
> > patches add independent functionality and also got reviewed-by tags
> > too.
>
> I think it makes more sense to keep them together in the same patch
> series or could end up with dependencies between trees.
>
I understand your point. Let me keep this patch-set together to avoid
any dependencies.
-Sumit
> If you don't think dependencies will be an issue then I don't mind
> picking them up, in that case they'd likely sit in an arm-soc branch
> until next merge window. However, I think that #3 (support for private
> kernel login method) should be included too and that one isn't ready
> yet.
>
> Thanks,
> Jens
>
> >
> >
> > -Sumit
> >
> > > Patch #3 enables support for private kernel login method required for
> > > cases like trusted keys where we don't wan't user-space to directly access
> > > TEE service to retrieve trusted key contents.
> > >
> > > Rest of the patches from #4 to #7 adds support for TEE based trusted keys.
> > >
> > > This patch-set has been tested with OP-TEE based pseudo TA which can be
> > > found here [1].
> > >
> > > Looking forward to your valuable feedback/suggestions.
> > >
> > > [1] https://github.com/OP-TEE/optee_os/pull/3082
> > >
> > > Sumit Garg (7):
> > > tee: optee: allow kernel pages to register as shm
> > > tee: enable support to register kernel memory
> > > tee: add private login method for kernel clients
> > > KEYS: trusted: Introduce TEE based Trusted Keys
> > > KEYS: encrypted: Allow TEE based trusted master keys
> > > doc: keys: Document usage of TEE based Trusted Keys
> > > MAINTAINERS: Add entry for TEE based Trusted Keys
> > >
> > > Documentation/security/keys/tee-trusted.rst | 93 +++++
> > > MAINTAINERS | 9 +
> > > drivers/tee/optee/call.c | 7 +
> > > drivers/tee/tee_core.c | 6 +
> > > drivers/tee/tee_shm.c | 16 +-
> > > include/keys/tee_trusted.h | 84 ++++
> > > include/keys/trusted-type.h | 1 +
> > > include/linux/tee_drv.h | 1 +
> > > include/uapi/linux/tee.h | 2 +
> > > security/keys/Kconfig | 3 +
> > > security/keys/Makefile | 3 +
> > > security/keys/encrypted-keys/masterkey_trusted.c | 10 +-
> > > security/keys/tee_trusted.c | 506 +++++++++++++++++++++++
> > > 13 files changed, 737 insertions(+), 4 deletions(-)
> > > create mode 100644 Documentation/security/keys/tee-trusted.rst
> > > create mode 100644 include/keys/tee_trusted.h
> > > create mode 100644 security/keys/tee_trusted.c
> > >
> > > --
> > > 2.7.4
> > >
^ permalink raw reply
* Re: [RFC 3/7] tee: add private login method for kernel clients
From: Jens Wiklander @ 2019-07-09 7:03 UTC (permalink / raw)
To: Sumit Garg
Cc: keyrings, linux-integrity, linux-security-module, corbet,
dhowells, jejb, Jarkko Sakkinen, Mimi Zohar, jmorris, serge,
Ard Biesheuvel, Daniel Thompson, linux-doc,
Linux Kernel Mailing List, tee-dev
In-Reply-To: <CAFA6WYNzs=RErreWaa5BmF-P03Vf9nzQjvY_JpMckw87k9z12w@mail.gmail.com>
On Tue, Jul 09, 2019 at 11:26:19AM +0530, Sumit Garg wrote:
> Thanks Jens for your comments.
>
> On Mon, 8 Jul 2019 at 21:09, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> >
> > Hi Sumit,
> >
> > On Thu, Jun 13, 2019 at 04:00:29PM +0530, Sumit Garg wrote:
> > > There are use-cases where user-space shouldn't be allowed to communicate
> > > directly with a TEE device which is dedicated to provide a specific
> > > service for a kernel client. So add a private login method for kernel
> > > clients and disallow user-space to open-session using this login method.
> > >
> > > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> > > ---
> > > drivers/tee/tee_core.c | 6 ++++++
> > > include/uapi/linux/tee.h | 2 ++
> > > 2 files changed, 8 insertions(+)
> > >
> > > diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
> > > index 0f16d9f..4581bd1 100644
> > > --- a/drivers/tee/tee_core.c
> > > +++ b/drivers/tee/tee_core.c
> > > @@ -334,6 +334,12 @@ static int tee_ioctl_open_session(struct tee_context *ctx,
> > > goto out;
> > > }
> > >
> > > + if (arg.clnt_login == TEE_IOCTL_LOGIN_REE_KERNEL) {
> > TEE_IOCTL_LOGIN_REE_KERNEL is defined as 0x80000000 which is in the
> > range specified and implementation defined by the GP spec. I wonder if
> > we shouldn't filter the entire implementation defined range instead of
> > just this value.
>
> Agree. Will rather check for entire implementation defined range:
> 0x80000000 - 0xFFFFFFFF.
>
> >
> > > + pr_err("login method not allowed for user-space client\n");
> > pr_debug(), if it's needed at all.
> >
>
> Ok will use pr_debug() instead.
>
> > > + rc = -EPERM;
> > > + goto out;
> > > + }
> > > +
> > > rc = ctx->teedev->desc->ops->open_session(ctx, &arg, params);
> > > if (rc)
> > > goto out;
> > > diff --git a/include/uapi/linux/tee.h b/include/uapi/linux/tee.h
> > > index 4b9eb06..f33c69c 100644
> > > --- a/include/uapi/linux/tee.h
> > > +++ b/include/uapi/linux/tee.h
> > > @@ -172,6 +172,8 @@ struct tee_ioctl_buf_data {
> > > #define TEE_IOCTL_LOGIN_APPLICATION 4
> > > #define TEE_IOCTL_LOGIN_USER_APPLICATION 5
> > > #define TEE_IOCTL_LOGIN_GROUP_APPLICATION 6
> > > +/* Private login method for REE kernel clients */
> > It's worth noting that this is filtered by the TEE framework, compared
> > to everything else which is treated opaquely.
> >
>
> IIUC, you are referring to login filter in optee_os. Change to prevent
> filter for this login method is part of this PR [1].
>
> [1] https://github.com/OP-TEE/optee_os/pull/3082
No, I was referring to the changes in tee_ioctl_open_session() above.
It's relevant for user space to know since it will be prevented from
using that range of login identifiers. This will restrict the user space
API, but I think the risk of breakage is minimal as OP-TEE is the only
in-tree driver registering in the TEE framework. I'm not aware of any
out-of-tree drivers registering.
Thanks,
Jens
>
> -Sumit
>
> > > +#define TEE_IOCTL_LOGIN_REE_KERNEL 0x80000000
> > >
> > > /**
> > > * struct tee_ioctl_param - parameter
> > > --
> > > 2.7.4
> > >
> >
> > Thanks,
> > Jens
^ permalink raw reply
* Re: [RFC 3/7] tee: add private login method for kernel clients
From: Sumit Garg @ 2019-07-09 9:36 UTC (permalink / raw)
To: Jens Wiklander
Cc: keyrings, linux-integrity, linux-security-module, corbet,
dhowells, jejb, Jarkko Sakkinen, Mimi Zohar, jmorris, serge,
Ard Biesheuvel, Daniel Thompson, linux-doc,
Linux Kernel Mailing List, tee-dev
In-Reply-To: <20190709070354.GA5791@jax>
On Tue, 9 Jul 2019 at 12:33, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> On Tue, Jul 09, 2019 at 11:26:19AM +0530, Sumit Garg wrote:
> > Thanks Jens for your comments.
> >
> > On Mon, 8 Jul 2019 at 21:09, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> > >
> > > Hi Sumit,
> > >
> > > On Thu, Jun 13, 2019 at 04:00:29PM +0530, Sumit Garg wrote:
> > > > There are use-cases where user-space shouldn't be allowed to communicate
> > > > directly with a TEE device which is dedicated to provide a specific
> > > > service for a kernel client. So add a private login method for kernel
> > > > clients and disallow user-space to open-session using this login method.
> > > >
> > > > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> > > > ---
> > > > drivers/tee/tee_core.c | 6 ++++++
> > > > include/uapi/linux/tee.h | 2 ++
> > > > 2 files changed, 8 insertions(+)
> > > >
> > > > diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
> > > > index 0f16d9f..4581bd1 100644
> > > > --- a/drivers/tee/tee_core.c
> > > > +++ b/drivers/tee/tee_core.c
> > > > @@ -334,6 +334,12 @@ static int tee_ioctl_open_session(struct tee_context *ctx,
> > > > goto out;
> > > > }
> > > >
> > > > + if (arg.clnt_login == TEE_IOCTL_LOGIN_REE_KERNEL) {
> > > TEE_IOCTL_LOGIN_REE_KERNEL is defined as 0x80000000 which is in the
> > > range specified and implementation defined by the GP spec. I wonder if
> > > we shouldn't filter the entire implementation defined range instead of
> > > just this value.
> >
> > Agree. Will rather check for entire implementation defined range:
> > 0x80000000 - 0xFFFFFFFF.
> >
I had a second thought on this. It would be more restrictive for
user-space TEE client library which may need to use implementation
defined login method. So either we could define specific ranges for
kernel and user-space or we can start with single login method
reserved for kernel.
> > >
> > > > + pr_err("login method not allowed for user-space client\n");
> > > pr_debug(), if it's needed at all.
> > >
> >
> > Ok will use pr_debug() instead.
> >
> > > > + rc = -EPERM;
> > > > + goto out;
> > > > + }
> > > > +
> > > > rc = ctx->teedev->desc->ops->open_session(ctx, &arg, params);
> > > > if (rc)
> > > > goto out;
> > > > diff --git a/include/uapi/linux/tee.h b/include/uapi/linux/tee.h
> > > > index 4b9eb06..f33c69c 100644
> > > > --- a/include/uapi/linux/tee.h
> > > > +++ b/include/uapi/linux/tee.h
> > > > @@ -172,6 +172,8 @@ struct tee_ioctl_buf_data {
> > > > #define TEE_IOCTL_LOGIN_APPLICATION 4
> > > > #define TEE_IOCTL_LOGIN_USER_APPLICATION 5
> > > > #define TEE_IOCTL_LOGIN_GROUP_APPLICATION 6
> > > > +/* Private login method for REE kernel clients */
> > > It's worth noting that this is filtered by the TEE framework, compared
> > > to everything else which is treated opaquely.
> > >
> >
> > IIUC, you are referring to login filter in optee_os. Change to prevent
> > filter for this login method is part of this PR [1].
> >
> > [1] https://github.com/OP-TEE/optee_os/pull/3082
>
> No, I was referring to the changes in tee_ioctl_open_session() above.
> It's relevant for user space to know since it will be prevented from
> using that range of login identifiers.
Ok, so you mean to extend the comment here for user-space to know that
this login method/range is filtered by the TEE framework. Will do
that.
> This will restrict the user space
> API, but I think the risk of breakage is minimal as OP-TEE is the only
> in-tree driver registering in the TEE framework. I'm not aware of any
> out-of-tree drivers registering.
I am not sure if I follow you here. How do you expect this change to
break out-of-tree TEE driver registration?
-Sumit
>
> Thanks,
> Jens
>
> >
> > -Sumit
> >
> > > > +#define TEE_IOCTL_LOGIN_REE_KERNEL 0x80000000
> > > >
> > > > /**
> > > > * struct tee_ioctl_param - parameter
> > > > --
> > > > 2.7.4
> > > >
> > >
> > > Thanks,
> > > Jens
^ permalink raw reply
* Re: [PATCH 02/10] vfs: syscall: Add move_mount(2) to move mounts around
From: Tetsuo Handa @ 2019-07-09 10:51 UTC (permalink / raw)
To: Eric W. Biederman, Al Viro
Cc: David Howells, linux-api, linux-fsdevel, torvalds,
linux-security-module, John Johansen
In-Reply-To: <87pnmkhxoy.fsf@xmission.com>
On 2019/07/09 9:13, Eric W. Biederman wrote:
> Tetsuo, do you think you can implement the checks you need for Tomoyo
> for mount_move on top of the new security_mount_move?
I hope "Yes", for I'm not aware what will become possible with the new mount
syscalls. For example, if it becomes possible to use multiple source device
files or directories, TOMOYO is not ready to check multiple source device
files or directories, for currently TOMOYO uses
file mount $DEVICE $MOUNTPOINT $FILESYSTEM $OPTIONS
( https://tomoyo.osdn.jp/2.6/policy-specification/domain-policy-syntax.html#file_mount )
syntax. If only optional part (any arguments which were previously passed via
"const void *data" of mount() syscall) differs, current syntax can be reused.
>
> Al is proposing that similar hooks be added for the other subcases of
> mount so that less racy hooks can be implemented. Tetsuo do you have
> any problem with that?
I welcome improvements that get rid of calling kern_path() from LSM hooks.
In the past, I incorrectly implemented which MS_* flag should be checked
(commit df91e49477a9be15 ("TOMOYO: Fix mount flags checking order.")).
If LSM hooks for mount manipulation are divided into multiple LSM hooks
(along with changing the argument from "const char *" to "const struct path *"),
such mistakes can be avoided.
> Tetsuo whatever the virtues of this patchset getting merged into Linus's
> tree it is merged now, so the only thing we can do now is roll up our
> sleeves go through everything and fix the regressions/bugs/issues that
> have emerged with the new mount api.
For now, can we apply this patch for 5.2-stable ?
From dd62fab0592e02580fd5a34222a2d11bfc179f61 Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Tue, 9 Jul 2019 19:05:49 +0900
Subject: [PATCH] LSM: Disable move_mount() syscall when TOMOYO or AppArmor is enabled.
Commit 2db154b3ea8e14b0 ("vfs: syscall: Add move_mount(2) to move mounts
around") introduced security_move_mount() LSM hook, but we missed that
TOMOYO and AppArmor did not implement hooks for checking move_mount(2).
For pathname based access controls like TOMOYO and AppArmor, unchecked
mount manipulation is not acceptable. Therefore, until TOMOYO and AppArmor
implement hooks, in order to avoid unchecked mount manipulation, pretend
as if move_mount(2) is unavailable when either TOMOYO or AppArmor is
enabled.
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Fixes: 2db154b3ea8e14b0 ("vfs: syscall: Add move_mount(2) to move mounts around")
Cc: stable@vger.kernel.org # 5.2
---
include/linux/lsm_hooks.h | 6 ++++++
security/apparmor/lsm.c | 1 +
security/tomoyo/tomoyo.c | 1 +
3 files changed, 8 insertions(+)
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 47f58cf..cd411b7 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -2142,4 +2142,10 @@ static inline void security_delete_hooks(struct security_hook_list *hooks,
extern int lsm_inode_alloc(struct inode *inode);
+static inline int no_move_mount(const struct path *from_path,
+ const struct path *to_path)
+{
+ return -ENOSYS;
+}
+
#endif /* ! __LINUX_LSM_HOOKS_H */
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index ec3a928..5cdf63b 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -1158,6 +1158,7 @@ struct lsm_blob_sizes apparmor_blob_sizes __lsm_ro_after_init = {
LSM_HOOK_INIT(capable, apparmor_capable),
LSM_HOOK_INIT(sb_mount, apparmor_sb_mount),
+ LSM_HOOK_INIT(move_mount, no_move_mount),
LSM_HOOK_INIT(sb_umount, apparmor_sb_umount),
LSM_HOOK_INIT(sb_pivotroot, apparmor_sb_pivotroot),
diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
index 716c92e..be1b1a1 100644
--- a/security/tomoyo/tomoyo.c
+++ b/security/tomoyo/tomoyo.c
@@ -558,6 +558,7 @@ static void tomoyo_task_free(struct task_struct *task)
LSM_HOOK_INIT(path_chown, tomoyo_path_chown),
LSM_HOOK_INIT(path_chroot, tomoyo_path_chroot),
LSM_HOOK_INIT(sb_mount, tomoyo_sb_mount),
+ LSM_HOOK_INIT(move_mount, no_move_mount),
LSM_HOOK_INIT(sb_umount, tomoyo_sb_umount),
LSM_HOOK_INIT(sb_pivotroot, tomoyo_sb_pivotroot),
LSM_HOOK_INIT(socket_bind, tomoyo_socket_bind),
--
1.8.3.1
^ permalink raw reply related
* [GIT PULL] LSM: capabilities updates for v5.3
From: James Morris @ 2019-07-09 11:32 UTC (permalink / raw)
To: Linus Torvalds; +Cc: linux-security-module, linux-kernel
Please pull these minor fixes for capabilities:
o Update the commoncap.c code to utilize XATTR_SECURITY_PREFIX_LEN,
from Carmeli tamir.
o Make the capability hooks static, from Yue Haibing.
---
The following changes since commit e93c9c99a629c61837d5a7fc2120cd2b6c70dbdd:
Linux 5.1 (2019-05-05 17:42:58 -0700)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git next-lsm
for you to fetch changes up to c5eaab1d131d0a6272df7d55a971a67400d63f56:
security/commoncap: Use xattr security prefix len (2019-07-07 14:55:54 +1200)
----------------------------------------------------------------
Carmeli Tamir (1):
security/commoncap: Use xattr security prefix len
YueHaibing (1):
security: Make capability_hooks static
security/commoncap.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
^ permalink raw reply
* Re: [RFC PATCH v4 01/12] x86/sgx: Use mmu_notifier.release() instead of per-vma refcounting
From: Jarkko Sakkinen @ 2019-07-09 16:18 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: <20190708145707.GC20433@linux.intel.com>
On Mon, Jul 08, 2019 at 07:57:07AM -0700, Sean Christopherson wrote:
> On Fri, Jun 21, 2019 at 12:03:36AM +0300, Jarkko Sakkinen wrote:
> > On Wed, Jun 19, 2019 at 03:23:50PM -0700, Sean Christopherson wrote:
> > > Using per-vma refcounting to track mm_structs associated with an enclave
> > > requires hooking .vm_close(), which in turn prevents the mm from merging
> > > vmas (precisely to allow refcounting).
> >
> > Why having sgx_vma_close() prevents that? I do not understand the
> > problem statement.
>
> vmas that define .vm_close() cannot be merged.
Ugh, did not know that :-) Thank you.
/Jarkko
^ permalink raw reply
* Re: [RFC PATCH v4 00/12] security: x86/sgx: SGX vs. LSM
From: Jarkko Sakkinen @ 2019-07-09 16:22 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: <20190708172930.GA20791@linux.intel.com>
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.
> That being said, we can do so without functional changes to the SGX uapi,
> e.g. add reserved fields so that the initial uapi can be extended *if* we
> decide to go with the "userspace provides maximal protections" path, and
> use the EPCM permissions as the maximal protections for the initial
> upstreaming.
>
> That'd give us a minimal implemenation for initial upstreaming and would
> eliminate Cedric's blocking complaint. The "whole mess" of whitelisting,
> blacklisting and SGX2 support would be deferred until post-upstreaming.
I'd like that approach more too.
/Jarkko
^ permalink raw reply
* Re: [PATCH] KEYS: trusted: allow module init if TPM is inactive or deactivated
From: Jarkko Sakkinen @ 2019-07-09 16:24 UTC (permalink / raw)
To: James Bottomley
Cc: Roberto Sassu, zohar, jgg, linux-integrity, linux-security-module,
keyrings, linux-kernel, crazyt2019+lml, tyhicks, nayna,
silviu.vlasceanu
In-Reply-To: <1562618099.20748.13.camel@linux.ibm.com>
On Mon, Jul 08, 2019 at 01:34:59PM -0700, James Bottomley wrote:
> Not a criticism of your patch, but can we please stop doing this.
> Single random number sources are horrendously bad practice because it
> gives an attacker a single target to subvert. We should ensure the TPM
> is plugged into the kernel RNG as a source and then take randomness
> from the mixed pool so it's harder for an attacker because they have to
> subvert all our sources to predict what came out.
It is and I agree.
/Jarkko
^ permalink raw reply
* Re: [PATCH] KEYS: trusted: allow module init if TPM is inactive or deactivated
From: Mimi Zohar @ 2019-07-09 16:31 UTC (permalink / raw)
To: Jarkko Sakkinen, James Bottomley
Cc: Roberto Sassu, jgg, linux-integrity, linux-security-module,
keyrings, linux-kernel, crazyt2019+lml, tyhicks, nayna,
silviu.vlasceanu
In-Reply-To: <20190709162458.f4fjteokcmidv7w6@linux.intel.com>
On Tue, 2019-07-09 at 19:24 +0300, Jarkko Sakkinen wrote:
> On Mon, Jul 08, 2019 at 01:34:59PM -0700, James Bottomley wrote:
> > Not a criticism of your patch, but can we please stop doing this.
> > Single random number sources are horrendously bad practice because it
> > gives an attacker a single target to subvert. We should ensure the TPM
> > is plugged into the kernel RNG as a source and then take randomness
> > from the mixed pool so it's harder for an attacker because they have to
> > subvert all our sources to predict what came out.
>
> It is and I agree.
I still haven't quite figured out why the digests need to be
initialized to anything other than 0.
Mimi
^ 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