* Re: [PATCH 2/3] Teach SELinux about anonymous inodes
From: Stephen Smalley @ 2020-02-14 16:39 UTC (permalink / raw)
To: Daniel Colascione, timmurray, selinux, linux-security-module,
linux-fsdevel, linux-kernel, kvm, viro, paul, nnk, lokeshgidra
In-Reply-To: <20200214032635.75434-3-dancol@google.com>
On 2/13/20 10:26 PM, Daniel Colascione wrote:
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 1659b59fb5d7..6de0892620b3 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -2915,6 +2915,62 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
> return 0;
> }
>
> +static int selinux_inode_init_security_anon(struct inode *inode,
> + const struct qstr *name,
> + const struct file_operations *fops,
> + const struct inode *context_inode)
> +{
> + const struct task_security_struct *tsec = selinux_cred(current_cred());
> + struct common_audit_data ad;
> + struct inode_security_struct *isec;
> + int rc;
> +
> + if (unlikely(IS_PRIVATE(inode)))
> + return 0;
This is not possible since the caller clears S_PRIVATE before calling
and it would be a bug to call the hook on an inode that was intended to
be private, so we shouldn't check it here.
> +
> + if (unlikely(!selinux_state.initialized))
> + return 0;
Are we doing this to defer initialization until selinux_complete_init()
- that's normally why we bail in the !initialized case? Not entirely
sure what will happen in such a situation since we won't have the
context_inode or the allocating task information at that time, so we
certainly won't get the same result - probably they would all be labeled
with whatever anon_inodefs is assigned via genfscon or
SECINITSID_UNLABELED by default. If we instead just drop this test and
proceed, we'll inherit the context inode SID if specified or we'll call
security_transition_sid(), which in the !initialized case will just
return the tsid i.e. tsec->sid, so it will be labeled with the creating
task SID (SECINITSID_KERNEL prior to initialization). Then the
avc_has_perm() call will pass because everything gets allowed until
initialization. So you could drop this check and userfaultfds created
before policy load would get the kernel SID, or you can keep it and they
will get the unlabeled SID. Preference?
> +
> + isec = selinux_inode(inode);
> +
> + /*
> + * We only get here once per ephemeral inode. The inode has
> + * been initialized via inode_alloc_security but is otherwise
> + * untouched.
> + */
> +
> + if (context_inode) {
> + struct inode_security_struct *context_isec =
> + selinux_inode(context_inode);
> + if (IS_ERR(context_isec))
> + return PTR_ERR(context_isec);
This isn't possible AFAICT so you don't need to test for it or handle
it. In fact, even the test for NULL in selinux_inode() is bogus and
should get dropped AFAICT; we always allocate an inode security blob
even before policy load so it would be a bug if we ever had a NULL there.
> + isec->sid = context_isec->sid;
> + } else {
> + rc = security_transition_sid(
> + &selinux_state, tsec->sid, tsec->sid,
> + SECCLASS_FILE, name, &isec->sid);
> + if (rc)
> + return rc;
> + }
Since you switched to using security_transition_sid(), you are not using
the fops parameter anymore nor comparing with userfaultfd_fops, so you
could drop the parameter from the hook and leave the latter static in
the first patch.
That's assuming you are ok with having to define these type_transition
rules for the userfaultfd case instead of having your own separate
security class. Wondering how many different anon inode names/classes
there are in the kernel today and how much they change over time; for a
small, relatively stable set, separate classes might be ok; for a large,
dynamic set, type transitions should scale better. We might still need
to create a mapping table in SELinux from the names to some stable
identifier for the policy lookup if we can't rely on the names being stable.
^ permalink raw reply
* Re: [PATCH v8 07/11] proc: flush task dcache entries from all procfs instances
From: Eric W. Biederman @ 2020-02-14 14:15 UTC (permalink / raw)
To: Linus Torvalds
Cc: Al Viro, LKML, Kernel Hardening, Linux API, Linux FS Devel,
Linux Security Module, Akinobu Mita, Alexey Dobriyan,
Andrew Morton, Andy Lutomirski, Daniel Micay, Djalal Harouni,
Dmitry V . Levin, Greg Kroah-Hartman, Ingo Molnar,
J . Bruce Fields, Jeff Layton, Jonathan Corbet, Kees Cook,
Oleg Nesterov, Solar Designer
In-Reply-To: <CAHk-=wjePLiQqUfQGCrNb0wp+EtgRddQbcK-pHH=6rxbdYNNOA@mail.gmail.com>
Linus Torvalds <torvalds@linux-foundation.org> writes:
> I guess a lot of readdir users end up doing a stat on it immediately
> afterwards. I think right now we do it to get the inode number, and
> maybe that is a basic requirement (even if I don't think it's really
> stable - an inode could be evicted and then the ino changes, no?)
All I know is proc_fill_cache seemed like a good idea at the time.
I may have been to clever.
While I think proc_fill_cache probably exacerbates the issue
it isn't the reason we have the flushing logic. The proc
flushing logic was introduced in around 2.5.9 much earlier
than the other proc things.
commit 0030633355db2bba32d97655df73b04215018ab9
Author: Alexander Viro <viro@math.psu.edu>
Date: Sun Apr 21 23:03:37 2002 -0700
[PATCH] (3/5) sane procfs/dcache interaction
- sane dentry retention. Namely, we don't kill /proc/<pid> dentries at the
first opportunity (as the current tree does). Instead we do the following:
* ->d_delete() kills it only if process is already dead.
* all ->lookup() in proc/base.c end with checking if process is still
alive and unhash if it isn't.
* proc_pid_lookup() (lookup for /proc/<pid>) caches reference to dentry
in task_struct. It's _not_ counted in ->d_count.
* ->d_iput() resets said reference to NULL.
* release_task() (burying a zombie) checks if there is a cached
reference and if there is - shrinks the subtree.
* tasklist_lock is used for exclusion.
That way we are guaranteed that after release_task() all dentries in
/proc/<pid> will go away as soon as possible; OTOH, before release_task()
we have normal retention policy - they go away under memory pressure with
the same rules as for dentries on any other fs.
Tracking down when this logic was introduced I also see that this code
has broken again and again any time proc changes (like now). So it is
definitely subtle and fragile.
Eric
^ permalink raw reply
* Re: [PATCH v26 10/22] x86/sgx: Linux Enclave Driver
From: Jethro Beekman @ 2020-02-14 9:24 UTC (permalink / raw)
To: Sean Christopherson
Cc: Jarkko Sakkinen, linux-kernel, x86, linux-sgx, akpm, dave.hansen,
nhorman, npmccallum, haitao.huang, andriy.shevchenko, tglx,
kai.svahn, bp, josh, luto, kai.huang, rientjes, cedric.xing,
puiterwijk, linux-security-module, Haitao Huang
In-Reply-To: <20200213180737.GC18610@linux.intel.com>
[-- Attachment #1: Type: text/plain, Size: 2979 bytes --]
On 2020-02-13 19:07, Sean Christopherson wrote:
> On Thu, Feb 13, 2020 at 02:59:52PM +0100, Jethro Beekman wrote:
>> On 2020-02-09 22:25, Jarkko Sakkinen wrote:
>>> +/**
>>> + * struct sgx_enclave_add_pages - parameter structure for the
>>> + * %SGX_IOC_ENCLAVE_ADD_PAGE ioctl
>>> + * @src: start address for the page data
>>> + * @offset: starting page offset
>>> + * @length: length of the data (multiple of the page size)
>>> + * @secinfo: address for the SECINFO data
>>> + * @flags: page control flags
>>> + * @count: number of bytes added (multiple of the page size)
>>> + */
>>> +struct sgx_enclave_add_pages {
>>> + __u64 src;
>>> + __u64 offset;
>>> + __u64 length;
>>> + __u64 secinfo;
>>> + __u64 flags;
>>> + __u64 count;
>>> +};
>>
>> Compared to the last time I looked at the patch set, this API removes the
>> ability to measure individual pages chunks. That is not acceptable.
>
> Why is it not acceptable? E.g. what specific use case do you have that
> _requires_ on measuring partial 4k pages of an enclave?
The use case is someone gives me an enclave and I want to load it. If I don't load it exactly as the enclave author specified, the enclave hash will be different, and it won't work.
>> On 2019-10-11 16:37, Sean Christopherson wrote:
>>> Hiding the 256-byte granualarity from userspace is a good idea as it's not
>>> intrinsically tied to the SGX architecture and exists only because of
>>> latency requirements.
>>
>> What do you mean by "it's not intrinsically tied to the SGX architecture"?
>> This is a fundamental part of the SGX instruction set. This is the
>> instruction definition from the SDM: "EEXTEND—Extend Uninitialized Enclave
>> Measurement by 256 Bytes".
>
> SGX fundamentally works at a 4k granularity. EEXTEND is special cased
> because extending the measurement is a slow operation, i.e. EEXTEND on more
> than 256 byte chunks, *with the current implementation*, would exceeded
> latency requirements, e.g. block interrupts for too long and hose the
> kernel.
>
> A future implementation of SGX could change the latency of extending the
> measurement, e.g. a different algorithm that is slower/faster, and so could
> introduce EEXTEND2 which would work at a different granularity than EEXTEND.
>
> EEXTEND could have avoided the latency problems via other methods, e.g. by
> being interruptible a la EINIT and/or by being restartable. But that ship
> has sailed, so to avoid future complication in the kernel's ABI we're
> proposing/advocating supporting only measuring at a 4k granularity.
It doesn't really matter what the reason for the current EEXTEND implementation is. It's there now in the ISA, it needs to be supported. If EEXTEND2 (or whatever) is added to the ISA, it will likely influence the enclave hash, so userspace would need to specify what instruction is used for measuring anyway.
--
Jethro Beekman | Fortanix
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4054 bytes --]
^ permalink raw reply
* Re: [PATCH v8 07/11] proc: flush task dcache entries from all procfs instances
From: Eric W. Biederman @ 2020-02-14 3:48 UTC (permalink / raw)
To: Al Viro
Cc: Linus Torvalds, LKML, Kernel Hardening, Linux API, Linux FS Devel,
Linux Security Module, Akinobu Mita, Alexey Dobriyan,
Andrew Morton, Andy Lutomirski, Daniel Micay, Djalal Harouni,
Dmitry V . Levin, Greg Kroah-Hartman, Ingo Molnar,
J . Bruce Fields, Jeff Layton, Jonathan Corbet, Kees Cook,
Oleg Nesterov, Solar Designer
In-Reply-To: <20200213055527.GS23230@ZenIV.linux.org.uk>
Al Viro <viro@zeniv.linux.org.uk> writes:
> On Wed, Feb 12, 2020 at 10:37:52PM -0600, Eric W. Biederman wrote:
>
>> I think I have an alternate idea that could work. Add some extra code
>> into proc_task_readdir, that would look for dentries that no longer
>> point to tasks and d_invalidate them. With the same logic probably
>> being called from a few more places as well like proc_pid_readdir,
>> proc_task_lookup, and proc_pid_lookup.
>>
>> We could even optimize it and have a process died flag we set in the
>> superblock.
>>
>> That would would batch up the freeing work until the next time someone
>> reads from proc in a way that would create more dentries. So it would
>> prevent dentries from reaped zombies from growing without bound.
>>
>> Hmm. Given the existence of proc_fill_cache it would really be a good
>> idea if readdir and lookup performed some of the freeing work as well.
>> As on readdir we always populate the dcache for all of the directory
>> entries.
>
> First of all, that won't do a damn thing when nobody is accessing
> given superblock. What's more, readdir in root of that procfs instance
> is not enough - you need it in task/ of group leader.
It should give a rough bound on the number of stale dentries a
superblock can have. The same basic concept has been used very
successfully in many incremental garbage collectors. In those malloc
(or the equivalent) does a finite amount of garbage collection work to
roughly balance out the amount of memory allocated. I am proposing
something similar for proc instances.
Further if no one is accessing a superblock we don't have a problem
either.
> What I don't understand is the insistence on getting those dentries
> via dcache lookups. _IF_ we are willing to live with cacheline
> contention (on ->d_lock of root dentry, if nothing else), why not
> do the following:
No insistence from this side.
I was not seeing atomic_inc_not_zero(sb->s_active) from rcu
context as option earlier. But it is an option.
> * put all dentries of such directories ([0-9]* and [0-9]*/task/*)
> into a list anchored in task_struct; have non-counting reference to
> task_struct stored in them (might simplify part of get_proc_task() users,
> BTW - avoids pid-to-task_struct lookups if we have a dentry and not just
> the inode; many callers do)
> * have ->d_release() remove from it (protecting per-task_struct lock
> nested outside of all ->d_lock)
> * on exit:
> lock the (per-task_struct) list
> while list is non-empty
> pick the first dentry
> remove from the list
> sb = dentry->d_sb
> try to bump sb->s_active (if non-zero, that is).
> if failed
> continue // move on to the next one - nothing to do here
> grab ->d_lock
> res = handle_it(dentry, &temp_list)
> drop ->d_lock
> unlock the list
> if (!list_empty(&temp_list))
> shrink_dentry_list(&temp_list)
> if (res)
> d_invalidate(dentry)
> dput(dentry)
> deactivate_super(sb)
> lock the list
> unlock the list
>
> handle_it(dentry, temp_list) // ->d_lock held; that one should be in dcache.c
> if ->d_count is negative // unlikely
> return 0;
> if ->d_count is positive,
> increment ->d_count
> return 1;
> // OK, it's still alive, but ->d_count is 0
> __d_drop // equivalent of d_invalidate in this case
> if not on a shrink list // otherwise it's not our headache
> if on lru list
> d_lru_del
> d_shrink_add dentry to temp_list
> return 0;
>
> And yeah, that'll dirty ->s_active for each procfs superblock that
> has dentry for our process present in dcache. On exit()...
I would thread the whole thing through the proc_inode instead of coming
up with a new allocation per dentry so an extra memory allocation isn't
needed. We already have i_dentry. So going from the vfs_inode to
the dentry is trivial.
But truthfully I don't like proc_flush_task.
The problem is that proc_flush_task is a layering violation and magic
code that pretty much no one understands. We have some very weird
cases where dput or d_invalidate wound up triggering ext3 code. It has
been fixed for a long time now, but it wasy crazy weird unexpected
stuff.
Al your logic above just feels very clever, and like many pieces of the
kernel have to know how other pieces of the kernel work. If we can find
something stupid and simple that also solves the problem I would be much
happier. Than anyone could understand and fix it if something goes
wrong.
Eric
^ permalink raw reply
* Re: [PATCH v8 07/11] proc: flush task dcache entries from all procfs instances
From: Eric W. Biederman @ 2020-02-14 3:49 UTC (permalink / raw)
To: Al Viro
Cc: Linus Torvalds, LKML, Kernel Hardening, Linux API, Linux FS Devel,
Linux Security Module, Akinobu Mita, Alexey Dobriyan,
Andrew Morton, Andy Lutomirski, Daniel Micay, Djalal Harouni,
Dmitry V . Levin, Greg Kroah-Hartman, Ingo Molnar,
J . Bruce Fields, Jeff Layton, Jonathan Corbet, Kees Cook,
Oleg Nesterov, Solar Designer
In-Reply-To: <20200212203833.GQ23230@ZenIV.linux.org.uk>
Al Viro <viro@zeniv.linux.org.uk> writes:
> On Wed, Feb 12, 2020 at 12:35:04PM -0800, Linus Torvalds wrote:
>> On Wed, Feb 12, 2020 at 12:03 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>> >
>> > What's to prevent racing with fs shutdown while you are doing the second part?
>>
>> I was thinking that only the proc_flush_task() code would do this.
>>
>> And that holds a ref to the vfsmount through upid->ns.
>>
>> So I wasn't suggesting doing this in general - just splitting up the
>> implementation of d_invalidate() so that proc_flush_task_mnt() could
>> delay the complex part to after having traversed the RCU-protected
>> list.
>>
>> But hey - I missed this part of the problem originally, so maybe I'm
>> just missing something else this time. Wouldn't be the first time.
>
> Wait, I thought the whole point of that had been to allow multiple
> procfs instances for the same userns? Confused...
Multiple procfs instances for the same pidns. Exactly.
Which would let people have their own set of procfs mount
options without having to worry about stomping on someone else.
The fundamental problem with multiple procfs instances per pidns
is there isn't an obvous place to put a vfs mount.
...
Which means we need some way to keep the file system from going away
while anyone in the kernel is running proc_flush_task.
One was I can see to solve this that would give us cheap readers, is to
have a percpu count of the number of processes in proc_flush_task.
That would work something like mnt_count.
Then forbid proc_kill_sb from removing any super block from the list
or otherwise making progress until the proc_flush_task_count goes
to zero.
f we wanted cheap readers and an expensive writer
kind of flag that proc_kill_sb can
Thinking out loud perhaps we have add a list_head on task_struct
and a list_head in proc_inode. That would let us find the inodes
and by extention the dentries we care about quickly.
Then in evict_inode we could remove the proc_inode from the list.
Eric
^ permalink raw reply
* [PATCH 0/3] SELinux support for anonymous inodes and UFFD
From: Daniel Colascione @ 2020-02-14 3:26 UTC (permalink / raw)
To: timmurray, selinux, linux-security-module, linux-fsdevel,
linux-kernel, kvm, viro, paul, nnk, sds, lokeshgidra
Cc: Daniel Colascione
In-Reply-To: <20200211225547.235083-1-dancol@google.com>
Userfaultfd in unprivileged contexts could be potentially very
useful. We'd like to harden userfaultfd to make such unprivileged use
less risky. This patch series allows SELinux to manage userfaultfd
file descriptors and in the future, other kinds of
anonymous-inode-based file descriptor. SELinux policy authors can
apply policy types to anonymous inodes by providing name-based
transition rules keyed off the anonymous inode internal name (
"[userfaultfd]" in the case of userfaultfd(2) file descriptors) and
applying policy to the new SIDs thus produced.
Inside the kernel, a pair of new anon_inodes interface,
anon_inode_getfile_secure and anon_inode_getfd_secure, allow callers
to opt into this SELinux management. In this new "secure" mode,
anon_inodes creates new ephemeral inodes for anonymous file objects
instead of reusing the normal anon_inodes singleton dummy inode. A new
LSM hook gives security modules an opportunity to configure and veto
these ephemeral inodes.
This patch series is one of two fork of [1] and is an
alternative to [2].
The primary difference between the two patch series is that this
partch series creates a unique inode for each "secure" anonymous
inode, while the other patch series ([2]) continues using the
singleton dummy anonymous inode and adds a way to attach SELinux
security information directly to file objects.
I prefer the approach in this patch series because 1) it's a smaller
patch than [2], and 2) it produces a more regular security
architecture: in this patch series, secure anonymous inodes aren't
S_PRIVATE and they maintain the SELinux property that the label for a
file is in its inode. We do need an additional inode per anonymous
file, but per-struct-file inode creation doesn't seem to be a problem
for pipes and sockets.
The previous version of this feature ([1]) created a new SELinux
security class for userfaultfd file descriptors. This version adopts
the generic transition-based approach of [2].
This patch series also differs from [2] in that it doesn't affect all
anonymous inodes right away --- instead requiring anon_inodes callers
to opt in --- but this difference isn't one of basic approach. The
important question to resolve is whether we should be creating new
inodes or enhancing per-file data.
[1] https://lore.kernel.org/lkml/20200211225547.235083-1-dancol@google.com/
[2] https://lore.kernel.org/linux-fsdevel/20200213194157.5877-1-sds@tycho.nsa.gov/
Daniel Colascione (3):
Add a new LSM-supporting anonymous inode interface
Teach SELinux about anonymous inodes
Wire UFFD up to SELinux
fs/anon_inodes.c | 196 ++++++++++++++++++++++++++++--------
fs/userfaultfd.c | 34 +++++--
include/linux/anon_inodes.h | 13 +++
include/linux/lsm_hooks.h | 9 ++
include/linux/security.h | 4 +
security/security.c | 10 ++
security/selinux/hooks.c | 57 +++++++++++
7 files changed, 274 insertions(+), 49 deletions(-)
--
2.25.0.265.gbab2e86ba0-goog
^ permalink raw reply
* [PATCH 3/3] Wire UFFD up to SELinux
From: Daniel Colascione @ 2020-02-14 3:26 UTC (permalink / raw)
To: timmurray, selinux, linux-security-module, linux-fsdevel,
linux-kernel, kvm, viro, paul, nnk, sds, lokeshgidra
Cc: Daniel Colascione
In-Reply-To: <20200214032635.75434-1-dancol@google.com>
This change gives userfaultfd file descriptors a real security
context, allowing policy to act on them.
Signed-off-by: Daniel Colascione <dancol@google.com>
---
fs/userfaultfd.c | 30 ++++++++++++++++++++++++++----
1 file changed, 26 insertions(+), 4 deletions(-)
diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
index 07b0f6e03849..06e92697aba4 100644
--- a/fs/userfaultfd.c
+++ b/fs/userfaultfd.c
@@ -76,6 +76,8 @@ struct userfaultfd_ctx {
bool mmap_changing;
/* mm with one ore more vmas attached to this userfaultfd_ctx */
struct mm_struct *mm;
+ /* The inode that owns this context --- not a strong reference. */
+ const struct inode *owner;
};
struct userfaultfd_fork_ctx {
@@ -1020,8 +1022,10 @@ static int resolve_userfault_fork(struct userfaultfd_ctx *ctx,
{
int fd;
- fd = anon_inode_getfd("[userfaultfd]", &userfaultfd_fops, new,
- O_RDWR | (new->flags & UFFD_SHARED_FCNTL_FLAGS));
+ fd = anon_inode_getfd_secure(
+ "[userfaultfd]", &userfaultfd_fops, new,
+ O_RDWR | (new->flags & UFFD_SHARED_FCNTL_FLAGS),
+ ctx->owner);
if (fd < 0)
return fd;
@@ -1943,6 +1947,7 @@ static void init_once_userfaultfd_ctx(void *mem)
SYSCALL_DEFINE1(userfaultfd, int, flags)
{
+ struct file *file;
struct userfaultfd_ctx *ctx;
int fd;
@@ -1972,8 +1977,25 @@ SYSCALL_DEFINE1(userfaultfd, int, flags)
/* prevent the mm struct to be freed */
mmgrab(ctx->mm);
- fd = anon_inode_getfd("[userfaultfd]", &userfaultfd_fops, ctx,
- O_RDWR | (flags & UFFD_SHARED_FCNTL_FLAGS));
+ file = anon_inode_getfile_secure(
+ "[userfaultfd]", &userfaultfd_fops, ctx,
+ O_RDWR | (flags & UFFD_SHARED_FCNTL_FLAGS),
+ NULL);
+ if (IS_ERR(file)) {
+ fd = PTR_ERR(file);
+ goto out;
+ }
+
+ fd = get_unused_fd_flags(O_RDONLY | O_CLOEXEC);
+ if (fd < 0) {
+ fput(file);
+ goto out;
+ }
+
+ ctx->owner = file_inode(file);
+ fd_install(fd, file);
+
+out:
if (fd < 0) {
mmdrop(ctx->mm);
kmem_cache_free(userfaultfd_ctx_cachep, ctx);
--
2.25.0.265.gbab2e86ba0-goog
^ permalink raw reply related
* [PATCH 2/3] Teach SELinux about anonymous inodes
From: Daniel Colascione @ 2020-02-14 3:26 UTC (permalink / raw)
To: timmurray, selinux, linux-security-module, linux-fsdevel,
linux-kernel, kvm, viro, paul, nnk, sds, lokeshgidra
Cc: Daniel Colascione
In-Reply-To: <20200214032635.75434-1-dancol@google.com>
This change uses the anon_inodes and LSM infrastructure introduced in
the previous patch to give SELinux the ability to control
anonymous-inode files that are created using the new _secure()
anon_inodes functions.
A SELinux policy author detects and controls these anonymous inodes by
adding a name-based type_transition rule that assigns a new security
type to anonymous-inode files created in some domain. The name used
for the name-based transition is the name associated with the
anonymous inode for file listings --- e.g., "[userfaultfd]" or
"[perf_event]".
Example:
type uffd_t;
type_transition sysadm_t sysadm_t : file uffd_t "[userfaultfd]";
allow sysadm_t uffd_t:file { create };
(The next patch in this series is necessary for making userfaultfd
support this new interface. The example above is just
for exposition.)
Signed-off-by: Daniel Colascione <dancol@google.com>
---
security/selinux/hooks.c | 57 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 57 insertions(+)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 1659b59fb5d7..6de0892620b3 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2915,6 +2915,62 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
return 0;
}
+static int selinux_inode_init_security_anon(struct inode *inode,
+ const struct qstr *name,
+ const struct file_operations *fops,
+ const struct inode *context_inode)
+{
+ const struct task_security_struct *tsec = selinux_cred(current_cred());
+ struct common_audit_data ad;
+ struct inode_security_struct *isec;
+ int rc;
+
+ if (unlikely(IS_PRIVATE(inode)))
+ return 0;
+
+ if (unlikely(!selinux_state.initialized))
+ return 0;
+
+ isec = selinux_inode(inode);
+
+ /*
+ * We only get here once per ephemeral inode. The inode has
+ * been initialized via inode_alloc_security but is otherwise
+ * untouched.
+ */
+
+ if (context_inode) {
+ struct inode_security_struct *context_isec =
+ selinux_inode(context_inode);
+ if (IS_ERR(context_isec))
+ return PTR_ERR(context_isec);
+ isec->sid = context_isec->sid;
+ } else {
+ rc = security_transition_sid(
+ &selinux_state, tsec->sid, tsec->sid,
+ SECCLASS_FILE, name, &isec->sid);
+ if (rc)
+ return rc;
+ }
+
+ isec->initialized = LABEL_INITIALIZED;
+
+ /*
+ * Now that we've initialized security, check whether we're
+ * allowed to actually create this type of anonymous inode.
+ */
+
+ ad.type = LSM_AUDIT_DATA_INODE;
+ ad.u.inode = inode;
+
+ return avc_has_perm(&selinux_state,
+ tsec->sid,
+ isec->sid,
+ isec->sclass,
+ FILE__CREATE,
+ &ad);
+}
+
static int selinux_inode_create(struct inode *dir, struct dentry *dentry, umode_t mode)
{
return may_create(dir, dentry, SECCLASS_FILE);
@@ -6923,6 +6979,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
LSM_HOOK_INIT(inode_free_security, selinux_inode_free_security),
LSM_HOOK_INIT(inode_init_security, selinux_inode_init_security),
+ LSM_HOOK_INIT(inode_init_security_anon, selinux_inode_init_security_anon),
LSM_HOOK_INIT(inode_create, selinux_inode_create),
LSM_HOOK_INIT(inode_link, selinux_inode_link),
LSM_HOOK_INIT(inode_unlink, selinux_inode_unlink),
--
2.25.0.265.gbab2e86ba0-goog
^ permalink raw reply related
* [PATCH 1/3] Add a new LSM-supporting anonymous inode interface
From: Daniel Colascione @ 2020-02-14 3:26 UTC (permalink / raw)
To: timmurray, selinux, linux-security-module, linux-fsdevel,
linux-kernel, kvm, viro, paul, nnk, sds, lokeshgidra
Cc: Daniel Colascione
In-Reply-To: <20200214032635.75434-1-dancol@google.com>
This change adds two new functions, anon_inode_getfile_secure and
anon_inode_getfd_secure, that create anonymous-node files with
individual non-S_PRIVATE inodes to which security modules can apply
policy. Existing callers continue using the original singleton-inode
kind of anonymous-inode file. We can transition anonymous inode users
to the new kind of anonymous inode in individual patches for the sake
of bisection and review.
The new functions accept an optional context_inode parameter that
callers can use to provide additional contextual information to
security modules, e.g., indicating that one anonymous struct file is a
logical child of another, allowing a security model to propagate
security information from one to the other.
Signed-off-by: Daniel Colascione <dancol@google.com>
---
fs/anon_inodes.c | 196 ++++++++++++++++++++++++++++--------
fs/userfaultfd.c | 4 +-
include/linux/anon_inodes.h | 13 +++
include/linux/lsm_hooks.h | 9 ++
include/linux/security.h | 4 +
security/security.c | 10 ++
6 files changed, 191 insertions(+), 45 deletions(-)
diff --git a/fs/anon_inodes.c b/fs/anon_inodes.c
index 89714308c25b..114a04fc1db4 100644
--- a/fs/anon_inodes.c
+++ b/fs/anon_inodes.c
@@ -55,75 +55,135 @@ static struct file_system_type anon_inode_fs_type = {
.kill_sb = kill_anon_super,
};
-/**
- * anon_inode_getfile - creates a new file instance by hooking it up to an
- * anonymous inode, and a dentry that describe the "class"
- * of the file
- *
- * @name: [in] name of the "class" of the new file
- * @fops: [in] file operations for the new file
- * @priv: [in] private data for the new file (will be file's private_data)
- * @flags: [in] flags
- *
- * Creates a new file by hooking it on a single inode. This is useful for files
- * that do not need to have a full-fledged inode in order to operate correctly.
- * All the files created with anon_inode_getfile() will share a single inode,
- * hence saving memory and avoiding code duplication for the file/inode/dentry
- * setup. Returns the newly created file* or an error pointer.
- */
-struct file *anon_inode_getfile(const char *name,
- const struct file_operations *fops,
- void *priv, int flags)
+static struct inode *anon_inode_make_secure_inode(
+ const char *name,
+ const struct inode *context_inode,
+ const struct file_operations *fops)
+{
+ struct inode *inode;
+ const struct qstr qname = QSTR_INIT(name, strlen(name));
+ int error;
+
+ inode = alloc_anon_inode(anon_inode_mnt->mnt_sb);
+ if (IS_ERR(inode))
+ return ERR_PTR(PTR_ERR(inode));
+ inode->i_flags &= ~S_PRIVATE;
+ error = security_inode_init_security_anon(
+ inode, &qname, fops, context_inode);
+ if (error) {
+ iput(inode);
+ return ERR_PTR(error);
+ }
+ return inode;
+}
+
+struct file *_anon_inode_getfile(const char *name,
+ const struct file_operations *fops,
+ void *priv, int flags,
+ const struct inode *context_inode,
+ bool secure)
{
+ struct inode *inode;
struct file *file;
- if (IS_ERR(anon_inode_inode))
- return ERR_PTR(-ENODEV);
+ if (secure) {
+ inode = anon_inode_make_secure_inode(
+ name, context_inode, fops);
+ if (IS_ERR(inode))
+ return ERR_PTR(PTR_ERR(inode));
+ } else {
+ inode = anon_inode_inode;
+ if (IS_ERR(inode))
+ return ERR_PTR(-ENODEV);
+ /*
+ * We know the anon_inode inode count is always
+ * greater than zero, so ihold() is safe.
+ */
+ ihold(inode);
+ }
- if (fops->owner && !try_module_get(fops->owner))
- return ERR_PTR(-ENOENT);
+ if (fops->owner && !try_module_get(fops->owner)) {
+ file = ERR_PTR(-ENOENT);
+ goto err;
+ }
- /*
- * We know the anon_inode inode count is always greater than zero,
- * so ihold() is safe.
- */
- ihold(anon_inode_inode);
- file = alloc_file_pseudo(anon_inode_inode, anon_inode_mnt, name,
+ file = alloc_file_pseudo(inode, anon_inode_mnt, name,
flags & (O_ACCMODE | O_NONBLOCK), fops);
if (IS_ERR(file))
goto err;
- file->f_mapping = anon_inode_inode->i_mapping;
+ file->f_mapping = inode->i_mapping;
file->private_data = priv;
return file;
err:
- iput(anon_inode_inode);
+ iput(inode);
module_put(fops->owner);
return file;
}
-EXPORT_SYMBOL_GPL(anon_inode_getfile);
/**
- * anon_inode_getfd - creates a new file instance by hooking it up to an
- * anonymous inode, and a dentry that describe the "class"
- * of the file
+ * anon_inode_getfile_secure - creates a new file instance by hooking
+ * it up to a new anonymous inode and a
+ * dentry that describe the "class" of the
+ * file. Make it possible to use security
+ * modules to control access to the
+ * new file.
*
* @name: [in] name of the "class" of the new file
* @fops: [in] file operations for the new file
* @priv: [in] private data for the new file (will be file's private_data)
- * @flags: [in] flags
+ * @flags: [in] flags for the file
+ * @anon_inode_flags: [in] flags for anon_inode*
+ *
+ * Creates a new file by hooking it on an unspecified inode. This is
+ * useful for files that do not need to have a full-fledged inode in
+ * order to operate correctly. All the files created with
+ * anon_inode_getfile_secure() will have distinct inodes, avoiding
+ * code duplication for the file/inode/dentry setup. Returns the
+ * newly created file* or an error pointer.
+ */
+struct file *anon_inode_getfile_secure(const char *name,
+ const struct file_operations *fops,
+ void *priv, int flags,
+ const struct inode *context_inode)
+{
+ return _anon_inode_getfile(
+ name, fops, priv, flags, context_inode, true);
+}
+EXPORT_SYMBOL_GPL(anon_inode_getfile_secure);
+
+/**
+ * anon_inode_getfile - creates a new file instance by hooking it up
+ * to an anonymous inode and a dentry that
+ * describe the "class" of the file.
+ *
+ * @name: [in] name of the "class" of the new file
+ * @fops: [in] file operations for the new file
+ * @priv: [in] private data for the new file (will be file's private_data)
+ * @flags: [in] flags for the file
*
- * Creates a new file by hooking it on a single inode. This is useful for files
+ * Creates a new file by hooking it on an unspecified inode. This is useful for files
* that do not need to have a full-fledged inode in order to operate correctly.
- * All the files created with anon_inode_getfd() will share a single inode,
+ * All the files created with anon_inode_getfile() will share a single inode,
* hence saving memory and avoiding code duplication for the file/inode/dentry
- * setup. Returns new descriptor or an error code.
+ * setup. Returns the newly created file* or an error pointer.
*/
-int anon_inode_getfd(const char *name, const struct file_operations *fops,
- void *priv, int flags)
+struct file *anon_inode_getfile(const char *name,
+ const struct file_operations *fops,
+ void *priv, int flags)
+{
+ return _anon_inode_getfile(name, fops, priv, flags, NULL, false);
+}
+EXPORT_SYMBOL_GPL(anon_inode_getfile);
+
+static int _anon_inode_getfd(const char *name,
+ const struct file_operations *fops,
+ void *priv, int flags,
+ const struct inode *context_inode,
+ bool secure)
{
int error, fd;
struct file *file;
@@ -133,7 +193,8 @@ int anon_inode_getfd(const char *name, const struct file_operations *fops,
return error;
fd = error;
- file = anon_inode_getfile(name, fops, priv, flags);
+ file = _anon_inode_getfile(name, fops, priv, flags, context_inode,
+ secure);
if (IS_ERR(file)) {
error = PTR_ERR(file);
goto err_put_unused_fd;
@@ -146,6 +207,57 @@ int anon_inode_getfd(const char *name, const struct file_operations *fops,
put_unused_fd(fd);
return error;
}
+
+/**
+ * anon_inode_getfd_secure - creates a new file instance by hooking it
+ * up to a new anonymous inode and a dentry
+ * that describe the "class" of the file.
+ * Make it possible to use security modules
+ * to control access to the new file.
+ *
+ * @name: [in] name of the "class" of the new file
+ * @fops: [in] file operations for the new file
+ * @priv: [in] private data for the new file (will be file's private_data)
+ * @flags: [in] flags
+ *
+ * Creates a new file by hooking it on an unspecified inode. This is
+ * useful for files that do not need to have a full-fledged inode in
+ * order to operate correctly. All the files created with
+ * anon_inode_getfile_secure() will have distinct inodes, avoiding
+ * code duplication for the file/inode/dentry setup. Returns a newly
+ * created file descriptor or an error code.
+ */
+int anon_inode_getfd_secure(const char *name, const struct file_operations *fops,
+ void *priv, int flags,
+ const struct inode *context_inode)
+{
+ return _anon_inode_getfd(name, fops, priv, flags,
+ context_inode, true);
+}
+EXPORT_SYMBOL_GPL(anon_inode_getfd_secure);
+
+/**
+ * anon_inode_getfd - creates a new file instance by hooking it up to
+ * an anonymous inode and a dentry that describe
+ * the "class" of the file
+ *
+ * @name: [in] name of the "class" of the new file
+ * @fops: [in] file operations for the new file
+ * @priv: [in] private data for the new file (will be file's private_data)
+ * @flags: [in] flags
+ *
+ * Creates a new file by hooking it on an unspecified inode. This is
+ * useful for files that do not need to have a full-fledged inode in
+ * order to operate correctly. All the files created with
+ * anon_inode_getfile() will use the same singleton inode, reducing
+ * memory use and avoiding code duplication for the file/inode/dentry
+ * setup. Returns a newly created file descriptor or an error code.
+ */
+int anon_inode_getfd(const char *name, const struct file_operations *fops,
+ void *priv, int flags)
+{
+ return _anon_inode_getfd(name, fops, priv, flags, NULL, false);
+}
EXPORT_SYMBOL_GPL(anon_inode_getfd);
static int __init anon_inode_init(void)
diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
index 37df7c9eedb1..07b0f6e03849 100644
--- a/fs/userfaultfd.c
+++ b/fs/userfaultfd.c
@@ -1014,8 +1014,6 @@ static __poll_t userfaultfd_poll(struct file *file, poll_table *wait)
}
}
-static const struct file_operations userfaultfd_fops;
-
static int resolve_userfault_fork(struct userfaultfd_ctx *ctx,
struct userfaultfd_ctx *new,
struct uffd_msg *msg)
@@ -1920,7 +1918,7 @@ static void userfaultfd_show_fdinfo(struct seq_file *m, struct file *f)
}
#endif
-static const struct file_operations userfaultfd_fops = {
+const struct file_operations userfaultfd_fops = {
#ifdef CONFIG_PROC_FS
.show_fdinfo = userfaultfd_show_fdinfo,
#endif
diff --git a/include/linux/anon_inodes.h b/include/linux/anon_inodes.h
index d0d7d96261ad..67bd85d92dca 100644
--- a/include/linux/anon_inodes.h
+++ b/include/linux/anon_inodes.h
@@ -10,12 +10,25 @@
#define _LINUX_ANON_INODES_H
struct file_operations;
+struct inode;
+
+struct file *anon_inode_getfile_secure(const char *name,
+ const struct file_operations *fops,
+ void *priv, int flags,
+ const struct inode *context_inode);
struct file *anon_inode_getfile(const char *name,
const struct file_operations *fops,
void *priv, int flags);
+
+int anon_inode_getfd_secure(const char *name,
+ const struct file_operations *fops,
+ void *priv, int flags,
+ const struct inode *context_inode);
+
int anon_inode_getfd(const char *name, const struct file_operations *fops,
void *priv, int flags);
+
#endif /* _LINUX_ANON_INODES_H */
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 20d8cf194fb7..de5d37e388df 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -215,6 +215,10 @@
* Returns 0 if @name and @value have been successfully set,
* -EOPNOTSUPP if no security attribute is needed, or
* -ENOMEM on memory allocation failure.
+ * @inode_init_security_anon:
+ * Set up a secure anonymous inode.
+ * Returns 0 on success. Returns -EPERM if the security module denies
+ * the creation of this inode.
* @inode_create:
* Check permission to create a regular file.
* @dir contains inode structure of the parent of the new file.
@@ -1552,6 +1556,10 @@ union security_list_options {
const struct qstr *qstr,
const char **name, void **value,
size_t *len);
+ int (*inode_init_security_anon)(struct inode *inode,
+ const struct qstr *name,
+ const struct file_operations *fops,
+ const struct inode *context_inode);
int (*inode_create)(struct inode *dir, struct dentry *dentry,
umode_t mode);
int (*inode_link)(struct dentry *old_dentry, struct inode *dir,
@@ -1884,6 +1892,7 @@ struct security_hook_heads {
struct hlist_head inode_alloc_security;
struct hlist_head inode_free_security;
struct hlist_head inode_init_security;
+ struct hlist_head inode_init_security_anon;
struct hlist_head inode_create;
struct hlist_head inode_link;
struct hlist_head inode_unlink;
diff --git a/include/linux/security.h b/include/linux/security.h
index 64b19f050343..8ea76af0be7a 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -320,6 +320,10 @@ void security_inode_free(struct inode *inode);
int security_inode_init_security(struct inode *inode, struct inode *dir,
const struct qstr *qstr,
initxattrs initxattrs, void *fs_data);
+int security_inode_init_security_anon(struct inode *inode,
+ const struct qstr *name,
+ const struct file_operations *fops,
+ const struct inode *context_inode);
int security_old_inode_init_security(struct inode *inode, struct inode *dir,
const struct qstr *qstr, const char **name,
void **value, size_t *len);
diff --git a/security/security.c b/security/security.c
index 565bc9b67276..d06f3969c030 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1033,6 +1033,16 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
}
EXPORT_SYMBOL(security_inode_init_security);
+int
+security_inode_init_security_anon(struct inode *inode,
+ const struct qstr *name,
+ const struct file_operations *fops,
+ const struct inode *context_inode)
+{
+ return call_int_hook(inode_init_security_anon, 0, inode, name,
+ fops, context_inode);
+}
+
int security_old_inode_init_security(struct inode *inode, struct inode *dir,
const struct qstr *qstr, const char **name,
void **value, size_t *len)
--
2.25.0.265.gbab2e86ba0-goog
^ permalink raw reply related
* Re: [PATCH v8 07/11] proc: flush task dcache entries from all procfs instances
From: Linus Torvalds @ 2020-02-13 22:47 UTC (permalink / raw)
To: Al Viro
Cc: Eric W. Biederman, LKML, Kernel Hardening, Linux API,
Linux FS Devel, Linux Security Module, Akinobu Mita,
Alexey Dobriyan, Andrew Morton, Andy Lutomirski, Daniel Micay,
Djalal Harouni, Dmitry V . Levin, Greg Kroah-Hartman, Ingo Molnar,
J . Bruce Fields, Jeff Layton, Jonathan Corbet, Kees Cook,
Oleg Nesterov, Solar Designer
In-Reply-To: <20200213222350.GU23230@ZenIV.linux.org.uk>
On Thu, Feb 13, 2020 at 2:23 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> I'd been thinking of ->d_fsdata pointing to a structure with list_head
> and a (non-counting) task_struct pointer for those guys. Allocated
> on lookup, of course (as well as readdir ;-/) and put on the list
> at the same time.
Hmm. That smells like potentially a lot of small allocations, and
making readdir() even nastier.
Do we really want to create the dentries at readdir time? We do now
(with proc_fill_cache()) but do we actually _need_ to?
I guess a lot of readdir users end up doing a stat on it immediately
afterwards. I think right now we do it to get the inode number, and
maybe that is a basic requirement (even if I don't think it's really
stable - an inode could be evicted and then the ino changes, no?)
Ho humm. This all doesn't make me happy. But I guess the proof is in
the pudding - and if you come up with a good patch, I won't complain.
Linus
^ permalink raw reply
* Re: [PATCH v8 07/11] proc: flush task dcache entries from all procfs instances
From: Al Viro @ 2020-02-13 22:23 UTC (permalink / raw)
To: Linus Torvalds
Cc: Eric W. Biederman, LKML, Kernel Hardening, Linux API,
Linux FS Devel, Linux Security Module, Akinobu Mita,
Alexey Dobriyan, Andrew Morton, Andy Lutomirski, Daniel Micay,
Djalal Harouni, Dmitry V . Levin, Greg Kroah-Hartman, Ingo Molnar,
J . Bruce Fields, Jeff Layton, Jonathan Corbet, Kees Cook,
Oleg Nesterov, Solar Designer
In-Reply-To: <CAHk-=wgQnNHYxV7-SyRP=g9vTHyNAK9g1juLLB=eho4=DHVZEQ@mail.gmail.com>
On Thu, Feb 13, 2020 at 01:30:11PM -0800, Linus Torvalds wrote:
> On Wed, Feb 12, 2020 at 9:55 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > What I don't understand is the insistence on getting those dentries
> > via dcache lookups.
>
> I don't think that's an "insistence", it's more of a "historical
> behavior" together with "several changes over the years to deal with
> dentry-level cleanups and updates".
>
> > _IF_ we are willing to live with cacheline
> > contention (on ->d_lock of root dentry, if nothing else), why not
> > do the following:
> > * put all dentries of such directories ([0-9]* and [0-9]*/task/*)
> > into a list anchored in task_struct; have non-counting reference to
> > task_struct stored in them (might simplify part of get_proc_task() users,
>
> Hmm.
>
> Right now I don't think we actually create any dentries at all for the
> short-lived process case.
>
> Wouldn't your suggestion make fork/exit rather worse?
>
> Or would you create the dentries dynamically still at lookup time, and
> then attach them to the process at that point?
>
> What list would you use for the dentry chaining? Would you play games
> with the dentry hashing, and "hash" them off the process, and never
> hit in the lookup cache?
I'd been thinking of ->d_fsdata pointing to a structure with list_head
and a (non-counting) task_struct pointer for those guys. Allocated
on lookup, of course (as well as readdir ;-/) and put on the list
at the same time.
IOW, for short-lived process we simply have an empty (h)list anchored
in task_struct and that's it.
^ permalink raw reply
* Re: [PATCH v8 07/11] proc: flush task dcache entries from all procfs instances
From: Linus Torvalds @ 2020-02-13 21:30 UTC (permalink / raw)
To: Al Viro
Cc: Eric W. Biederman, LKML, Kernel Hardening, Linux API,
Linux FS Devel, Linux Security Module, Akinobu Mita,
Alexey Dobriyan, Andrew Morton, Andy Lutomirski, Daniel Micay,
Djalal Harouni, Dmitry V . Levin, Greg Kroah-Hartman, Ingo Molnar,
J . Bruce Fields, Jeff Layton, Jonathan Corbet, Kees Cook,
Oleg Nesterov, Solar Designer
In-Reply-To: <20200213055527.GS23230@ZenIV.linux.org.uk>
On Wed, Feb 12, 2020 at 9:55 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> What I don't understand is the insistence on getting those dentries
> via dcache lookups.
I don't think that's an "insistence", it's more of a "historical
behavior" together with "several changes over the years to deal with
dentry-level cleanups and updates".
> _IF_ we are willing to live with cacheline
> contention (on ->d_lock of root dentry, if nothing else), why not
> do the following:
> * put all dentries of such directories ([0-9]* and [0-9]*/task/*)
> into a list anchored in task_struct; have non-counting reference to
> task_struct stored in them (might simplify part of get_proc_task() users,
Hmm.
Right now I don't think we actually create any dentries at all for the
short-lived process case.
Wouldn't your suggestion make fork/exit rather worse?
Or would you create the dentries dynamically still at lookup time, and
then attach them to the process at that point?
What list would you use for the dentry chaining? Would you play games
with the dentry hashing, and "hash" them off the process, and never
hit in the lookup cache?
Am I misunderstanding what you suggest?
Linus
^ permalink raw reply
* Re: [PATCH v2] ima: export the measurement list when needed
From: Ken Goldman @ 2020-02-13 20:11 UTC (permalink / raw)
To: david.safford, Mimi Zohar, linux-integrity, linux-security-module
Cc: monty.wiseman
In-Reply-To: <fab03a0b8cc9dc93f2d0db51071521ce82e2b96b.camel@gmail.com>
On 2/11/2020 11:10 AM, david.safford@gmail.com wrote:
> There is no reason to resend the same data for every attestation,
> nor is there any reason to store already attested measurements anywhere
> on the client. By versioning the log file names, userspace gets a
> simple way to know what has and has not been attested, and for small
> embedded devices we don't need to waste memory or filesystem space
> on the data already attested.
Yes ... no.
There isn't any reason to resend measurements that a verifier has
already received. In fact, the logic I coded also said "if PCR 10
didn't change, there's no need to even ask for the measurement log".
However, don't you have to save already attested measurements for
the case where there are multiple verifiers? A different verifier would
have to receive all measurements.
^ permalink raw reply
* Re: [RFC PATCH] security,anon_inodes,kvm: enable security support for anon inodes
From: Stephen Smalley @ 2020-02-13 19:47 UTC (permalink / raw)
To: selinux; +Cc: linux-security-module, linux-fsdevel, kvm, viro, paul, dancol,
nnk
In-Reply-To: <20200213194157.5877-1-sds@tycho.nsa.gov>
[-- Attachment #1: Type: text/plain, Size: 1210 bytes --]
On 2/13/20 2:41 PM, Stephen Smalley wrote:
> An example of a sample program and policy will follow in a follow-up
> to this patch to demonstrate the effect on userfaultfd and kvm.
Attached are example test programs and policies to demonstrate the
change in behavior before and after this RFC patch for userfaultfd and
kvm. The test policies can be edited to selectively allow specific
permissions for testing various scenarios, but with the defaults in
them, one should see the following behavior:
sudo semodule -i kvm.cil userfaultfd.cil
make kvm userfaultfd
Before:
(no labeling/access control applied by SELinux to userfaultfd files or
to anon inodes created by kvm)
$ ./userfaultfd
api: 170
features: 510
ioctls: 9223372036854775811
read: Resource temporarily unavailable
$ ./kvm
api version: 12
created vm
created vcpu
rax: 0
rbx: 0
rcx: 0
rdx: 1536
rdi: 0
rsi: 0
rsp: 0
rbp: 0
r8: 0
r9: 0
r10: 0
r11: 0
r12: 0
r13: 0
r14: 0
r15: 0
rip: 65520
rflags: 2
created device
checked device attr
After:
(SELinux ioctl whitelisting used to selectively deny access)
./userfaultfd
UFFDIO_API: Permission denied
$ ./kvm
api version: 12
created vm
created vcpu
KVM_GET_REGS: Permission denied
[-- Attachment #2: kvm.cil --]
[-- Type: application/vnd.ms-artgalry, Size: 1115 bytes --]
[-- Attachment #3: userfaultfd.cil --]
[-- Type: application/vnd.ms-artgalry, Size: 621 bytes --]
[-- Attachment #4: kvm.c --]
[-- Type: text/x-csrc, Size: 2234 bytes --]
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <stdio.h>
#include <stdlib.h>
#include <sys/ioctl.h>
#include <linux/kvm.h>
void print_regs(const struct kvm_regs *regs)
{
printf("rax: %llu\n", regs->rax);
printf("rbx: %llu\n", regs->rbx);
printf("rcx: %llu\n", regs->rcx);
printf("rdx: %llu\n", regs->rdx);
printf("rdi: %llu\n", regs->rdi);
printf("rsi: %llu\n", regs->rsi);
printf("rsp: %llu\n", regs->rsp);
printf("rbp: %llu\n", regs->rbp);
printf("r8: %llu\n", regs->r8);
printf("r9: %llu\n", regs->r9);
printf("r10: %llu\n", regs->r10);
printf("r11: %llu\n", regs->r11);
printf("r12: %llu\n", regs->r12);
printf("r13: %llu\n", regs->r13);
printf("r14: %llu\n", regs->r14);
printf("r15: %llu\n", regs->r15);
printf("rip: %llu\n", regs->rip);
printf("rflags: %llu\n", regs->rflags);
printf("\n");
}
void print_device_attr(const struct kvm_device_attr *dev_attr)
{
printf("flags: %u\n", dev_attr->flags);
printf("group: %u\n", dev_attr->group);
printf("attr: %llu\n", dev_attr->attr);
printf("addr: %llu\n", dev_attr->addr);
printf("\n");
}
int main(void)
{
int fd = open("/dev/kvm", O_RDWR);
if (fd < 0) {
perror("/dev/kvm");
return -1;
}
int ret = ioctl(fd, KVM_GET_API_VERSION, 0);
if (ret < 0) {
perror("KVM_GET_API_VERSION");
return -1;
}
printf("api version: %d\n\n", ret);
int vmfd = ioctl(fd, KVM_CREATE_VM, 0);
if (vmfd < 0) {
perror("KVM_CREATE_VM");
return -1;
}
printf("created vm\n\n");
int vcpufd = ioctl(vmfd, KVM_CREATE_VCPU, 0);
if (vcpufd < 0) {
perror("KVM_CREATE_VCPU");
return -1;
}
printf("created vcpu\n\n");
struct kvm_regs regs;
if (ioctl(vcpufd, KVM_GET_REGS, ®s) < 0) {
perror("KVM_GET_REGS");
return -1;
}
print_regs(®s);
struct kvm_create_device dev = {0};
dev.type = KVM_DEV_TYPE_VFIO;
if (ioctl(vmfd, KVM_CREATE_DEVICE, &dev) < 0) {
perror("KVM_CREATE_DEVICE");
return -1;
}
printf("created device\n\n");
struct kvm_device_attr dev_attr = {0};
dev_attr.group = KVM_DEV_VFIO_GROUP;
dev_attr.attr = KVM_DEV_VFIO_GROUP_ADD;
if (ioctl(dev.fd, KVM_HAS_DEVICE_ATTR, &dev_attr) < 0) {
perror("KVM_HAS_DEVICE_ATTR");
return -1;
}
printf("checked device attr\n\n");
return 0;
}
[-- Attachment #5: userfaultfd.c --]
[-- Type: text/x-csrc, Size: 870 bytes --]
#include <stdio.h>
#include <fcntl.h>
#include <unistd.h>
#include <sys/types.h>
#include <sys/ioctl.h>
#include <sys/syscall.h>
#include <linux/userfaultfd.h>
void print_api(const struct uffdio_api *api)
{
printf("api: %llu\n", api->api);
printf("features: %llu\n", api->features);
printf("ioctls: %llu\n", api->ioctls);
printf("\n");
}
int main(void)
{
long uffd = syscall(__NR_userfaultfd, O_CLOEXEC | O_NONBLOCK);
if (uffd < 0) {
perror("syscall(userfaultfd)");
return -1;
}
struct uffdio_api api = {0};
api.api = UFFD_API;
if (ioctl(uffd, UFFDIO_API, &api) < 0) {
perror("UFFDIO_API");
return -1;
}
print_api(&api);
struct uffd_msg msg = {0};
ssize_t count = read(uffd, &msg, sizeof(msg));
if (count < 0) {
perror("read");
return -1;
} else if (count == 0) {
printf("read EOF\n\n");
}
printf("read uffd\n\n");
return 0;
}
^ permalink raw reply
* [RFC PATCH] security,anon_inodes,kvm: enable security support for anon inodes
From: Stephen Smalley @ 2020-02-13 19:41 UTC (permalink / raw)
To: selinux
Cc: linux-security-module, linux-fsdevel, kvm, viro, paul, dancol,
nnk, Stephen Smalley
Add support for labeling and controlling access to files attached to anon
inodes. Introduce extended interfaces for creating such files to permit
passing a related file as an input to decide how to label the anon
inode. Define a security hook for initializing the anon inode security
attributes. Security attributes are either inherited from a related file
or determined based on some combination of the creating task and policy
(in the case of SELinux, using type_transition rules). As an
example user of the inheritance support, convert kvm to use the new
interface for passing the related file so that the anon inode can inherit
the security attributes of /dev/kvm and provide consistent access control
for subsequent ioctl operations. Other users of anon inodes, including
userfaultfd, will default to the transition-based mechanism instead.
Compared to the series in
https://lore.kernel.org/selinux/20200211225547.235083-1-dancol@google.com/,
this approach differs in that it does not require creation of a separate
anonymous inode for each file (instead storing the per-instance security
information in the file security blob), it applies labeling and control
to all users of anonymous inodes rather than requiring opt-in via a new
flag, it supports labeling based on a related inode if provided,
it relies on type transitions to compute the label of the anon inode
when there is no related inode, and it does not require introducing a new
security class for each user of anonymous inodes.
On the other hand, the approach in this patch does expose the name passed
by the creator of the anon inode to the policy (an indirect mapping could
be provided within SELinux if these names aren't considered to be stable),
requires the definition of type_transition rules to distinguish userfaultfd
inodes from proc inodes based on type since they share the same class,
doesn't support denying the creation of anonymous inodes (making the hook
added by this patch return something other than void is problematic due to
it being called after the file is already allocated and error handling in
the callers can't presently account for this scenario and end up calling
release methods multiple times), and may be more expensive
(security_transition_sid overhead on each anon inode allocation).
We are primarily posting this RFC patch now so that the two different
approaches can be concretely compared. We anticipate a hybrid of the
two approaches being the likely outcome in the end. In particular
if support for allocating a separate inode for each of these files
is acceptable, then we would favor storing the security information
in the inode security blob and using it instead of the file security
blob.
This patch only converts kvm to use a related inode (/dev/kvm) for the
creation of anon inodes as an example user. We would look to
incrementally convert other subsystems where applicable. This could
potentially cause policy breakage if policies are written for a subsystem
using the type_transition rules and then the subsystem is later converted
to use a related inode, so some means of compatible evolution would be
required.
There are a number of file permission checks in SELinux that do not
currently use the file_has_perm() helper and therefore do not pick up
the logic for handling these anonymous inodes. These will need to be looked
at further to see whether they are relevant to anonymous inodes and should
be updated with the new logic.
Any hooks that are directly passed a dentry or inode and not a file will
still skip permission checking by virtue of the S_ISPRIVATE() checks in
security/security.c and in security/selinux/hooks.c. At some point they
need to be audited to determine whether they are relevant to anonymous
inodes.
This change may create compatibility issues since it unconditionally
enables labeling and access control for all anonymous inodes; it is
known to break two selinux-testsuite bpf tests without adjusting its
policy. It will likely need to be wrapped by a SELinux policy
capability before being merged to prevent breaking existing policies.
An example of a sample program and policy will follow in a follow-up
to this patch to demonstrate the effect on userfaultfd and kvm.
Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
---
fs/anon_inodes.c | 53 +++++++++++++++++-------
include/linux/anon_inodes.h | 7 ++++
include/linux/lsm_hooks.h | 11 +++++
include/linux/security.h | 8 ++++
security/security.c | 7 ++++
security/selinux/hooks.c | 69 ++++++++++++++++++++++++++++---
security/selinux/include/objsec.h | 4 ++
virt/kvm/kvm_main.c | 27 +++++++-----
8 files changed, 155 insertions(+), 31 deletions(-)
diff --git a/fs/anon_inodes.c b/fs/anon_inodes.c
index 89714308c25b..4579913cb33e 100644
--- a/fs/anon_inodes.c
+++ b/fs/anon_inodes.c
@@ -21,6 +21,7 @@
#include <linux/magic.h>
#include <linux/anon_inodes.h>
#include <linux/pseudo_fs.h>
+#include <linux/security.h>
#include <linux/uaccess.h>
@@ -55,15 +56,25 @@ static struct file_system_type anon_inode_fs_type = {
.kill_sb = kill_anon_super,
};
+struct file *anon_inode_getfile(const char *name,
+ const struct file_operations *fops,
+ void *priv, int flags)
+{
+ return anon_inode_getfile_inherit(name, fops, priv, flags, NULL);
+}
+EXPORT_SYMBOL_GPL(anon_inode_getfile);
+
/**
- * anon_inode_getfile - creates a new file instance by hooking it up to an
- * anonymous inode, and a dentry that describe the "class"
- * of the file
+ * anon_inode_getfile_inherit - creates a new file instance by hooking it up to
+ * an anonymous inode, and a dentry that describe
+ * the "class" of the file
*
* @name: [in] name of the "class" of the new file
* @fops: [in] file operations for the new file
* @priv: [in] private data for the new file (will be file's private_data)
* @flags: [in] flags
+ * @related: [in optional] a related file that security attributes will be
+ * inherited from.
*
* Creates a new file by hooking it on a single inode. This is useful for files
* that do not need to have a full-fledged inode in order to operate correctly.
@@ -71,9 +82,10 @@ static struct file_system_type anon_inode_fs_type = {
* hence saving memory and avoiding code duplication for the file/inode/dentry
* setup. Returns the newly created file* or an error pointer.
*/
-struct file *anon_inode_getfile(const char *name,
- const struct file_operations *fops,
- void *priv, int flags)
+struct file *anon_inode_getfile_inherit(const char *name,
+ const struct file_operations *fops,
+ void *priv, int flags,
+ const struct file *related)
{
struct file *file;
@@ -97,6 +109,8 @@ struct file *anon_inode_getfile(const char *name,
file->private_data = priv;
+ security_anon_file_init(file, related, name);
+
return file;
err:
@@ -104,17 +118,27 @@ struct file *anon_inode_getfile(const char *name,
module_put(fops->owner);
return file;
}
-EXPORT_SYMBOL_GPL(anon_inode_getfile);
+EXPORT_SYMBOL_GPL(anon_inode_getfile_inherit);
+
+int anon_inode_getfd(const char *name,
+ const struct file_operations *fops, void *priv,
+ int flags)
+{
+ return anon_inode_getfd_inherit(name, fops, priv, flags, NULL);
+}
+EXPORT_SYMBOL_GPL(anon_inode_getfd);
/**
- * anon_inode_getfd - creates a new file instance by hooking it up to an
- * anonymous inode, and a dentry that describe the "class"
- * of the file
+ * anon_inode_getfd_inherit - creates and installs a new file instance by
+ * hooking it up to an anonymous inode, and a dentry
+ * that describe the "class" of the file
*
* @name: [in] name of the "class" of the new file
* @fops: [in] file operations for the new file
* @priv: [in] private data for the new file (will be file's private_data)
* @flags: [in] flags
+ * @related: [in optional] a related file that security attributes will be
+ * inherited from.
*
* Creates a new file by hooking it on a single inode. This is useful for files
* that do not need to have a full-fledged inode in order to operate correctly.
@@ -122,8 +146,9 @@ EXPORT_SYMBOL_GPL(anon_inode_getfile);
* hence saving memory and avoiding code duplication for the file/inode/dentry
* setup. Returns new descriptor or an error code.
*/
-int anon_inode_getfd(const char *name, const struct file_operations *fops,
- void *priv, int flags)
+int anon_inode_getfd_inherit(const char *name,
+ const struct file_operations *fops, void *priv,
+ int flags, const struct file *related)
{
int error, fd;
struct file *file;
@@ -133,7 +158,7 @@ int anon_inode_getfd(const char *name, const struct file_operations *fops,
return error;
fd = error;
- file = anon_inode_getfile(name, fops, priv, flags);
+ file = anon_inode_getfile_inherit(name, fops, priv, flags, related);
if (IS_ERR(file)) {
error = PTR_ERR(file);
goto err_put_unused_fd;
@@ -146,7 +171,7 @@ int anon_inode_getfd(const char *name, const struct file_operations *fops,
put_unused_fd(fd);
return error;
}
-EXPORT_SYMBOL_GPL(anon_inode_getfd);
+EXPORT_SYMBOL_GPL(anon_inode_getfd_inherit);
static int __init anon_inode_init(void)
{
diff --git a/include/linux/anon_inodes.h b/include/linux/anon_inodes.h
index d0d7d96261ad..c385eff5d852 100644
--- a/include/linux/anon_inodes.h
+++ b/include/linux/anon_inodes.h
@@ -14,8 +14,15 @@ struct file_operations;
struct file *anon_inode_getfile(const char *name,
const struct file_operations *fops,
void *priv, int flags);
+struct file *anon_inode_getfile_inherit(const char *name,
+ const struct file_operations *fops,
+ void *priv, int flags,
+ const struct file *related);
int anon_inode_getfd(const char *name, const struct file_operations *fops,
void *priv, int flags);
+int anon_inode_getfd_inherit(const char *name,
+ const struct file_operations *fops, void *priv,
+ int flags, const struct file *related);
#endif /* _LINUX_ANON_INODES_H */
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 20d8cf194fb7..253ecffeda0b 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1454,6 +1454,14 @@
* code execution in kernel space should be permitted.
*
* @what: kernel feature being accessed
+ *
+ * @anon_file_init:
+ * Initialize the security attributes of a file attached to an anonymous
+ * inode.
+ * @file contains the file to initialize
+ * @related contains a related file that security attributes will be
+ * inherited from.
+ * @name contains the name of the "class" of the file
*/
union security_list_options {
int (*binder_set_context_mgr)(struct task_struct *mgr);
@@ -1818,6 +1826,8 @@ union security_list_options {
void (*bpf_prog_free_security)(struct bpf_prog_aux *aux);
#endif /* CONFIG_BPF_SYSCALL */
int (*locked_down)(enum lockdown_reason what);
+ void (*anon_file_init)(struct file *file, const struct file *related,
+ const char *name);
#ifdef CONFIG_PERF_EVENTS
int (*perf_event_open)(struct perf_event_attr *attr, int type);
int (*perf_event_alloc)(struct perf_event *event);
@@ -2068,6 +2078,7 @@ struct security_hook_heads {
struct hlist_head bpf_prog_free_security;
#endif /* CONFIG_BPF_SYSCALL */
struct hlist_head locked_down;
+ struct hlist_head anon_file_init;
#ifdef CONFIG_PERF_EVENTS
struct hlist_head perf_event_open;
struct hlist_head perf_event_alloc;
diff --git a/include/linux/security.h b/include/linux/security.h
index 64b19f050343..6ecef70566c1 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -447,6 +447,8 @@ int security_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen);
int security_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen);
int security_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen);
int security_locked_down(enum lockdown_reason what);
+void security_anon_file_init(struct file *file, const struct file *related,
+ const char *name);
#else /* CONFIG_SECURITY */
static inline int call_blocking_lsm_notifier(enum lsm_event event, void *data)
@@ -1274,6 +1276,12 @@ static inline int security_locked_down(enum lockdown_reason what)
{
return 0;
}
+static inline void security_anon_file_init(struct file *file,
+ const struct file *related,
+ const char *name);
+{
+ return 0;
+}
#endif /* CONFIG_SECURITY */
#ifdef CONFIG_SECURITY_NETWORK
diff --git a/security/security.c b/security/security.c
index 565bc9b67276..ce5d24c6cae8 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2437,6 +2437,13 @@ int security_locked_down(enum lockdown_reason what)
}
EXPORT_SYMBOL(security_locked_down);
+void security_anon_file_init(struct file *file, const struct file *related,
+ const char *name)
+{
+ call_void_hook(anon_file_init, file, related, name);
+}
+EXPORT_SYMBOL(security_anon_file_init);
+
#ifdef CONFIG_PERF_EVENTS
int security_perf_event_open(struct perf_event_attr *attr, int type)
{
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 44f6f4e20cba..5193b93fae4f 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -1725,8 +1725,13 @@ static int file_has_perm(const struct cred *cred,
/* av is zero if only checking access to the descriptor. */
rc = 0;
- if (av)
- rc = inode_has_perm(cred, inode, av, &ad);
+ if (av) {
+ if (unlikely(fsec->anon))
+ rc = avc_has_perm(&selinux_state, sid, fsec->isid,
+ fsec->anon_sclass, av, &ad);
+ else
+ rc = inode_has_perm(cred, inode, av, &ad);
+ }
out:
return rc;
@@ -3558,6 +3563,8 @@ static int ioctl_has_perm(const struct cred *cred, struct file *file,
struct inode_security_struct *isec;
struct lsm_ioctlop_audit ioctl;
u32 ssid = cred_sid(cred);
+ u32 tsid;
+ u16 tclass;
int rc;
u8 driver = cmd >> 8;
u8 xperm = cmd & 0xff;
@@ -3577,12 +3584,21 @@ static int ioctl_has_perm(const struct cred *cred, struct file *file,
goto out;
}
- if (unlikely(IS_PRIVATE(inode)))
- return 0;
+ if (unlikely(fsec->anon)) {
+ tsid = fsec->isid;
+ tclass = fsec->anon_sclass;
+ } else {
+ if (unlikely(IS_PRIVATE(inode)))
+ return 0;
+
+ isec = inode_security(inode);
+
+ tsid = isec->sid;
+ tclass = isec->sclass;
+ }
- isec = inode_security(inode);
rc = avc_has_extended_perms(&selinux_state,
- ssid, isec->sid, isec->sclass,
+ ssid, tsid, tclass,
requested, driver, xperm, &ad);
out:
return rc;
@@ -6805,6 +6821,46 @@ static int selinux_lockdown(enum lockdown_reason what)
LOCKDOWN__CONFIDENTIALITY, &ad);
}
+static void selinux_anon_file_init(struct file *file,
+ const struct file *related, const char *name)
+{
+ struct file_security_struct *fsec = selinux_file(file);
+ const struct file_security_struct *pfsec;
+ const struct inode *pinode;
+ const struct inode_security_struct *pisec;
+ u32 sid;
+ struct qstr qname = {{{0}}, name};
+ int error;
+
+ fsec->anon = true;
+
+ if (related) {
+ pfsec = selinux_file(related);
+
+ if (pfsec->anon) {
+ fsec->isid = pfsec->isid;
+ fsec->anon_sclass = pfsec->anon_sclass;
+ } else {
+ pinode = file_inode(related);
+ pisec = selinux_inode(pinode);
+
+ fsec->isid = pisec->sid;
+ fsec->anon_sclass = pisec->sclass;
+ }
+ } else {
+ fsec->isid = SECINITSID_UNLABELED;
+ fsec->anon_sclass = SECCLASS_FILE;
+
+ sid = cred_sid(current_cred());
+
+ error = security_transition_sid(&selinux_state, sid, sid,
+ SECCLASS_FILE, &qname,
+ &fsec->isid);
+ if (error)
+ fsec->isid = SECINITSID_UNLABELED;
+ }
+}
+
struct lsm_blob_sizes selinux_blob_sizes __lsm_ro_after_init = {
.lbs_cred = sizeof(struct task_security_struct),
.lbs_file = sizeof(struct file_security_struct),
@@ -7108,6 +7164,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
#endif
LSM_HOOK_INIT(locked_down, selinux_lockdown),
+ LSM_HOOK_INIT(anon_file_init, selinux_anon_file_init),
/*
* PUT "CLONING" (ACCESSING + ALLOCATING) HOOKS HERE
diff --git a/security/selinux/include/objsec.h b/security/selinux/include/objsec.h
index 330b7b6d44e0..8aa620e40a46 100644
--- a/security/selinux/include/objsec.h
+++ b/security/selinux/include/objsec.h
@@ -57,6 +57,10 @@ struct file_security_struct {
u32 sid; /* SID of open file description */
u32 fown_sid; /* SID of file owner (for SIGIO) */
u32 isid; /* SID of inode at the time of file open */
+ bool anon; /* Is file connected to an anonymous inode */
+ u16 anon_sclass; /* inode security class if connected
+ * to an anonymous inode
+ */
u32 pseqno; /* Policy seqno at the time of file open */
};
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 67ae2d5c37b2..ad9161de6273 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2756,12 +2756,13 @@ static struct file_operations kvm_vcpu_fops = {
/*
* Allocates an inode for the vcpu.
*/
-static int create_vcpu_fd(struct kvm_vcpu *vcpu)
+static int create_vcpu_fd(struct kvm_vcpu *vcpu, const struct file *filp)
{
char name[8 + 1 + ITOA_MAX_LEN + 1];
snprintf(name, sizeof(name), "kvm-vcpu:%d", vcpu->vcpu_id);
- return anon_inode_getfd(name, &kvm_vcpu_fops, vcpu, O_RDWR | O_CLOEXEC);
+ return anon_inode_getfd_inherit(name, &kvm_vcpu_fops, vcpu,
+ O_RDWR | O_CLOEXEC, filp);
}
static void kvm_create_vcpu_debugfs(struct kvm_vcpu *vcpu)
@@ -2783,7 +2784,8 @@ static void kvm_create_vcpu_debugfs(struct kvm_vcpu *vcpu)
/*
* Creates some virtual cpus. Good luck creating more than one.
*/
-static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, u32 id)
+static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, u32 id,
+ const struct file *filp)
{
int r;
struct kvm_vcpu *vcpu;
@@ -2838,7 +2840,7 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, u32 id)
/* Now it's all set up, let userspace reach it */
kvm_get_kvm(kvm);
- r = create_vcpu_fd(vcpu);
+ r = create_vcpu_fd(vcpu, filp);
if (r < 0) {
kvm_put_kvm_no_destroy(kvm);
goto unlock_vcpu_destroy;
@@ -3239,7 +3241,8 @@ void kvm_unregister_device_ops(u32 type)
}
static int kvm_ioctl_create_device(struct kvm *kvm,
- struct kvm_create_device *cd)
+ struct kvm_create_device *cd,
+ const struct file *filp)
{
const struct kvm_device_ops *ops = NULL;
struct kvm_device *dev;
@@ -3279,7 +3282,8 @@ static int kvm_ioctl_create_device(struct kvm *kvm,
ops->init(dev);
kvm_get_kvm(kvm);
- ret = anon_inode_getfd(ops->name, &kvm_device_fops, dev, O_RDWR | O_CLOEXEC);
+ ret = anon_inode_getfd_inherit(ops->name, &kvm_device_fops, dev,
+ O_RDWR | O_CLOEXEC, filp);
if (ret < 0) {
kvm_put_kvm_no_destroy(kvm);
mutex_lock(&kvm->lock);
@@ -3369,7 +3373,7 @@ static long kvm_vm_ioctl(struct file *filp,
return -EIO;
switch (ioctl) {
case KVM_CREATE_VCPU:
- r = kvm_vm_ioctl_create_vcpu(kvm, arg);
+ r = kvm_vm_ioctl_create_vcpu(kvm, arg, filp);
break;
case KVM_ENABLE_CAP: {
struct kvm_enable_cap cap;
@@ -3526,7 +3530,7 @@ static long kvm_vm_ioctl(struct file *filp,
if (copy_from_user(&cd, argp, sizeof(cd)))
goto out;
- r = kvm_ioctl_create_device(kvm, &cd);
+ r = kvm_ioctl_create_device(kvm, &cd, filp);
if (r)
goto out;
@@ -3595,7 +3599,7 @@ static struct file_operations kvm_vm_fops = {
KVM_COMPAT(kvm_vm_compat_ioctl),
};
-static int kvm_dev_ioctl_create_vm(unsigned long type)
+static int kvm_dev_ioctl_create_vm(unsigned long type, const struct file *filp)
{
int r;
struct kvm *kvm;
@@ -3613,7 +3617,8 @@ static int kvm_dev_ioctl_create_vm(unsigned long type)
if (r < 0)
goto put_kvm;
- file = anon_inode_getfile("kvm-vm", &kvm_vm_fops, kvm, O_RDWR);
+ file = anon_inode_getfile_inherit("kvm-vm", &kvm_vm_fops, kvm, O_RDWR,
+ filp);
if (IS_ERR(file)) {
put_unused_fd(r);
r = PTR_ERR(file);
@@ -3653,7 +3658,7 @@ static long kvm_dev_ioctl(struct file *filp,
r = KVM_API_VERSION;
break;
case KVM_CREATE_VM:
- r = kvm_dev_ioctl_create_vm(arg);
+ r = kvm_dev_ioctl_create_vm(arg, filp);
break;
case KVM_CHECK_EXTENSION:
r = kvm_vm_ioctl_check_extension_generic(NULL, arg);
--
2.24.1
^ permalink raw reply related
* Re: [PATCH v26 10/22] x86/sgx: Linux Enclave Driver
From: Sean Christopherson @ 2020-02-13 18:07 UTC (permalink / raw)
To: Jethro Beekman
Cc: Jarkko Sakkinen, linux-kernel, x86, linux-sgx, akpm, dave.hansen,
nhorman, npmccallum, haitao.huang, andriy.shevchenko, tglx,
kai.svahn, bp, josh, luto, kai.huang, rientjes, cedric.xing,
puiterwijk, linux-security-module, Suresh Siddha, Haitao Huang
In-Reply-To: <d17c50a7-6900-731b-43a2-d6e49b8eb44d@fortanix.com>
On Thu, Feb 13, 2020 at 02:59:52PM +0100, Jethro Beekman wrote:
> On 2020-02-09 22:25, Jarkko Sakkinen wrote:
> > +/**
> > + * struct sgx_enclave_add_pages - parameter structure for the
> > + * %SGX_IOC_ENCLAVE_ADD_PAGE ioctl
> > + * @src: start address for the page data
> > + * @offset: starting page offset
> > + * @length: length of the data (multiple of the page size)
> > + * @secinfo: address for the SECINFO data
> > + * @flags: page control flags
> > + * @count: number of bytes added (multiple of the page size)
> > + */
> > +struct sgx_enclave_add_pages {
> > + __u64 src;
> > + __u64 offset;
> > + __u64 length;
> > + __u64 secinfo;
> > + __u64 flags;
> > + __u64 count;
> > +};
>
> Compared to the last time I looked at the patch set, this API removes the
> ability to measure individual pages chunks. That is not acceptable.
Why is it not acceptable? E.g. what specific use case do you have that
_requires_ on measuring partial 4k pages of an enclave?
> On 2019-10-11 16:37, Sean Christopherson wrote:
> > Hiding the 256-byte granualarity from userspace is a good idea as it's not
> > intrinsically tied to the SGX architecture and exists only because of
> > latency requirements.
>
> What do you mean by "it's not intrinsically tied to the SGX architecture"?
> This is a fundamental part of the SGX instruction set. This is the
> instruction definition from the SDM: "EEXTEND—Extend Uninitialized Enclave
> Measurement by 256 Bytes".
SGX fundamentally works at a 4k granularity. EEXTEND is special cased
because extending the measurement is a slow operation, i.e. EEXTEND on more
than 256 byte chunks, *with the current implementation*, would exceeded
latency requirements, e.g. block interrupts for too long and hose the
kernel.
A future implementation of SGX could change the latency of extending the
measurement, e.g. a different algorithm that is slower/faster, and so could
introduce EEXTEND2 which would work at a different granularity than EEXTEND.
EEXTEND could have avoided the latency problems via other methods, e.g. by
being interruptible a la EINIT and/or by being restartable. But that ship
has sailed, so to avoid future complication in the kernel's ABI we're
proposing/advocating supporting only measuring at a 4k granularity.
> The exact sequence of EADD/EEXTEND calls is part of the enclave hash. The OS
> mustn't arbitrarily restrict how an enclave may be loaded. If the enclave
It's not arbitrary, there are good reasons for wanting to work with 4k
granularity. Regardless, there are many examples of the kernel arbitrarily
restricting what can be done relative to what is physically possible in
hardware.
> loader were to follows OS-specific restrictions, that would result in
> effectively different enclaves. Because of these interoperability concerns,
> 256-byte granularity *must* be exposed through the UAPI.
Interoperability with what? Other OSes?
> Besides only partially measuring a page, there are some other fringe cases
> that are technically possible, although I haven't seen any toolchains that do
> that. These include not interleaving EADD and EEXTEND, not using logical
> ordering for the EEXTENDs, and call EEXTEND multiple times on the same chunk.
> Maximum interoperability would require supporting any EADD/EEXTEND sequence.
Same interoperability question as above.
> Maybe we should just add an EEXTEND@offset ioctl? This would give
> fine-grained control when needed (one could set flags=0 in the add pages
> ioctl and interleave with EEXTEND as needed). If you're ok adding an EEXTEND
> ioctl I don't think this issue needs to block landing the driver in its
> current form, in which case:
We've also discussed an EEXTEND ioctl(), but ultimately couldn't come up
with a use case that _required_ partial page measurement.
> Tested-by: Jethro Beekman <jethro@fortanix.com>
>
> Sorry for being super late with this, I know you asked me for feedback about
> this specific point in October. However, I did previously mention several
> times that being able to measure individual 256-byte chunks is necessary.
>
> -- Jethro Beekman | Fortanix
^ permalink raw reply
* Re: [RFC PATCH 0/2] ima: uncompressed module appraisal support
From: Eric Snowberg @ 2020-02-13 15:32 UTC (permalink / raw)
To: Nayna
Cc: Mimi Zohar, dmitry.kasatkin, jmorris, serge, dhowells, geert,
gregkh, nayna, tglx, bauerman, mpe, linux-integrity,
linux-security-module, linux-kernel, Roberto Sassu
In-Reply-To: <eae8aa2d-1874-ba72-0452-a55bc811bd3d@linux.vnet.ibm.com>
> On Feb 12, 2020, at 7:04 AM, Nayna <nayna@linux.vnet.ibm.com> wrote:
>
>
> On 2/11/20 12:33 PM, Eric Snowberg wrote:
>>> On Feb 10, 2020, at 1:33 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>>
>>> On Mon, 2020-02-10 at 12:24 -0700, Eric Snowberg wrote:
>>>>> On Feb 10, 2020, at 10:09 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>>>>> Ok, understood, “modsig” refers to strictly kernel module appended signatures
>>>>>> without regard to the keyring that verifies it. Since there are inconsistencies
>>>>>> here, would you consider something like my first patch? It will verify an
>>>>>> uncompressed kernel module containing an appended signature when the public key
>>>>>> is contained within the kernel keyring instead of the ima keyring. Why force a
>>>>>> person to add the same keys into the ima keyring for validation? Especially when
>>>>>> the kernel keyring is now used to verify appended signatures in the compressed
>>>>>> modules.
>>>>> Different use case scenarios have different requirements. Suppose for
>>>>> example that the group creating the kernel image is not the same as
>>>>> using it. The group using the kernel image could sign all files,
>>>>> including kernel modules (imasig), with their own private key. Only
>>>>> files that they signed would be permitted. Your proposal would break
>>>>> the current expectations, allowing kernel modules signed by someone
>>>>> else to be loaded.
>>>>>
>>>> All the end user needs to do is compress any module created by the group that built
>>>> the original kernel image to work around the scenario above. Then the appended
>>>> signature in the compressed module will be verified by the kernel keyring. Does
>>>> this mean there is a security problem that should be fixed, if this is a concern?
>>> Again, the issue isn't compressed/uncompressed kernel modules, but the
>>> syscall used to load the kernel module. IMA can prevent using the the
>>> init_module syscall. Refer to the ima_load_data() LOADING_MODULE
>>> case.
>> Within the ima_load_data() LOADING_MODULE case, to prevent IMA from using
>> the init_module syscall, is_module_sig_enforced() must return false. Currently
>> when is_module_sig_enforced() returns true, the kernel keyring is always used
>> for verification.
>>
>> What if I change this part of my patch from
>>
>> + if (rc && func == MODULE_CHECK)
>>
>> to
>>
>> + sig_enforce = is_module_sig_enforced();
>> + if (sig_enforce && rc && func == MODULE_CHECK)
>>
>> Now when the init_module syscall is available, finit_module syscall will use
>> both the ima keyring and kernel keyring for verification. When the
>> init_module syscall is blocked from use, the finit_module syscall will only use
>> the ima keyring for validation. I believe this would satisfy both your use
>> case and mine.
>>
> There are two syscalls - init_module, finit_module - and two signature verification methods. The problem you are trying to address is the finit_module syscall, using both signature verification methods. Why enable both signature verification methods ?
I am enabling both in my patch since a person can turn around and use the other syscall by
simply compressing their module. Now their module is verified by a different keyring.
Other than completely disabling the init_module syscall, which we don’t do, there is nothing
preventing them from doing that. We have one kernel config per architecture. We build
and sign the modules with an appended signature.
I can not predict all the ways someone will use a kernel built from this single config.
I do believe if someone has IMA working with module verification and appended signatures,
some are not going to understand why their module that was compressed and loading
(via syscall init_module) suddenly fails to load (via syscall finit_module) once they
uncompress it.
^ permalink raw reply
* Re: [PATCH v26 10/22] x86/sgx: Linux Enclave Driver
From: Jethro Beekman @ 2020-02-13 13:59 UTC (permalink / raw)
To: Jarkko Sakkinen, linux-kernel, x86, linux-sgx
Cc: akpm, dave.hansen, sean.j.christopherson, nhorman, npmccallum,
haitao.huang, andriy.shevchenko, tglx, kai.svahn, bp, josh, luto,
kai.huang, rientjes, cedric.xing, puiterwijk,
linux-security-module, Suresh Siddha, Haitao Huang
In-Reply-To: <20200209212609.7928-11-jarkko.sakkinen@linux.intel.com>
On 2020-02-09 22:25, Jarkko Sakkinen wrote:
> Intel Software Guard eXtensions (SGX) is a set of CPU instructions that
> can be used by applications to set aside private regions of code and
> data. The code outside the SGX hosted software entity is disallowed to
> access the memory inside the enclave enforced by the CPU. We call these
> entities as enclaves.
>
> This commit implements a driver that provides an ioctl API to construct
> and run enclaves. Enclaves are constructed from pages residing in
> reserved physical memory areas. The contents of these pages can only be
> accessed when they are mapped as part of an enclave, by a hardware
> thread running inside the enclave.
>
> The starting state of an enclave consists of a fixed measured set of
> pages that are copied to the EPC during the construction process by
> using ENCLS leaf functions and Software Enclave Control Structure (SECS)
> that defines the enclave properties.
>
> Enclave are constructed by using ENCLS leaf functions ECREATE, EADD and
> EINIT. ECREATE initializes SECS, EADD copies pages from system memory to
> the EPC and EINIT check a given signed measurement and moves the enclave
> into a state ready for execution.
>
> An initialized enclave can only be accessed through special Thread Control
> Structure (TCS) pages by using ENCLU (ring-3 only) leaf EENTER. This leaf
> function converts a thread into enclave mode and continues the execution in
> the offset defined by the TCS provided to EENTER. An enclave is exited
> through syscall, exception, interrupts or by explicitly calling another
> ENCLU leaf EEXIT.
>
> The permissions, which enclave page is added will set the limit for maximum
> permissions that can be set for mmap() and mprotect(). This will
> effectively allow to build different security schemes between producers and
> consumers of enclaves. Later on we can increase granularity with LSM hooks
> for page addition (i.e. for producers) and mapping of the enclave (i.e. for
> consumers)
>
> Cc: linux-security-module@vger.kernel.org
> Cc: Nathaniel McCallum <npmccallum@redhat.com>
> Co-developed-by: Sean Christopherson <sean.j.christopherson@intel.com>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> Co-developed-by: Suresh Siddha <suresh.b.siddha@intel.com>
> Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com>
> Tested-by: Haitao Huang <haitao.huang@linux.intel.com>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
> .../userspace-api/ioctl/ioctl-number.rst | 1 +
> arch/x86/include/uapi/asm/sgx.h | 66 ++
> arch/x86/kernel/cpu/sgx/Makefile | 3 +
> arch/x86/kernel/cpu/sgx/driver.c | 194 +++++
> arch/x86/kernel/cpu/sgx/driver.h | 30 +
> arch/x86/kernel/cpu/sgx/encl.c | 329 +++++++++
> arch/x86/kernel/cpu/sgx/encl.h | 87 +++
> arch/x86/kernel/cpu/sgx/ioctl.c | 697 ++++++++++++++++++
> arch/x86/kernel/cpu/sgx/main.c | 12 +-
> arch/x86/kernel/cpu/sgx/reclaim.c | 1 +
> 10 files changed, 1419 insertions(+), 1 deletion(-)
> create mode 100644 arch/x86/include/uapi/asm/sgx.h
> create mode 100644 arch/x86/kernel/cpu/sgx/driver.c
> create mode 100644 arch/x86/kernel/cpu/sgx/driver.h
> create mode 100644 arch/x86/kernel/cpu/sgx/encl.c
> create mode 100644 arch/x86/kernel/cpu/sgx/encl.h
> create mode 100644 arch/x86/kernel/cpu/sgx/ioctl.c
>
> diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
> index 2e91370dc159..1c54dd2704db 100644
> --- a/Documentation/userspace-api/ioctl/ioctl-number.rst
> +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
> @@ -321,6 +321,7 @@ Code Seq# Include File Comments
> <mailto:tlewis@mindspring.com>
> 0xA3 90-9F linux/dtlk.h
> 0xA4 00-1F uapi/linux/tee.h Generic TEE subsystem
> +0xA4 00-1F uapi/asm/sgx.h Intel SGX subsystem (a legit conflict as TEE and SGX do not co-exist)
> 0xAA 00-3F linux/uapi/linux/userfaultfd.h
> 0xAB 00-1F linux/nbd.h
> 0xAC 00-1F linux/raw.h
> diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
> new file mode 100644
> index 000000000000..5edb08ab8fd0
> --- /dev/null
> +++ b/arch/x86/include/uapi/asm/sgx.h
> @@ -0,0 +1,66 @@
> +/* SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) WITH Linux-syscall-note */
> +/*
> + * Copyright(c) 2016-19 Intel Corporation.
> + */
> +#ifndef _UAPI_ASM_X86_SGX_H
> +#define _UAPI_ASM_X86_SGX_H
> +
> +#include <linux/types.h>
> +#include <linux/ioctl.h>
> +
> +/**
> + * enum sgx_epage_flags - page control flags
> + * %SGX_PAGE_MEASURE: Measure the page contents with a sequence of
> + * ENCLS[EEXTEND] operations.
> + */
> +enum sgx_page_flags {
> + SGX_PAGE_MEASURE = 0x01,
> +};
> +
> +#define SGX_MAGIC 0xA4
> +
> +#define SGX_IOC_ENCLAVE_CREATE \
> + _IOW(SGX_MAGIC, 0x00, struct sgx_enclave_create)
> +#define SGX_IOC_ENCLAVE_ADD_PAGES \
> + _IOWR(SGX_MAGIC, 0x01, struct sgx_enclave_add_pages)
> +#define SGX_IOC_ENCLAVE_INIT \
> + _IOW(SGX_MAGIC, 0x02, struct sgx_enclave_init)
> +
> +/**
> + * struct sgx_enclave_create - parameter structure for the
> + * %SGX_IOC_ENCLAVE_CREATE ioctl
> + * @src: address for the SECS page data
> + */
> +struct sgx_enclave_create {
> + __u64 src;
> +};
> +
> +/**
> + * struct sgx_enclave_add_pages - parameter structure for the
> + * %SGX_IOC_ENCLAVE_ADD_PAGE ioctl
> + * @src: start address for the page data
> + * @offset: starting page offset
> + * @length: length of the data (multiple of the page size)
> + * @secinfo: address for the SECINFO data
> + * @flags: page control flags
> + * @count: number of bytes added (multiple of the page size)
> + */
> +struct sgx_enclave_add_pages {
> + __u64 src;
> + __u64 offset;
> + __u64 length;
> + __u64 secinfo;
> + __u64 flags;
> + __u64 count;
> +};
Compared to the last time I looked at the patch set, this API removes the ability to measure individual pages chunks. That is not acceptable.
On 2019-10-11 16:37, Sean Christopherson wrote:
> Hiding the 256-byte granualarity from userspace is a good idea as it's not
> intrinsically tied to the SGX architecture and exists only because of
> latency requirements.
What do you mean by "it's not intrinsically tied to the SGX architecture"? This is a fundamental part of the SGX instruction set. This is the instruction definition from the SDM: "EEXTEND—Extend Uninitialized Enclave Measurement by 256 Bytes".
The exact sequence of EADD/EEXTEND calls is part of the enclave hash. The OS mustn't arbitrarily restrict how an enclave may be loaded. If the enclave loader were to follows OS-specific restrictions, that would result in effectively different enclaves. Because of these interoperability concerns, 256-byte granularity *must* be exposed through the UAPI.
Besides only partially measuring a page, there are some other fringe cases that are technically possible, although I haven't seen any toolchains that do that. These include not interleaving EADD and EEXTEND, not using logical ordering for the EEXTENDs, and call EEXTEND multiple times on the same chunk. Maximum interoperability would require supporting any EADD/EEXTEND sequence.
Maybe we should just add an EEXTEND@offset ioctl? This would give fine-grained control when needed (one could set flags=0 in the add pages ioctl and interleave with EEXTEND as needed). If you're ok adding an EEXTEND ioctl I don't think this issue needs to block landing the driver in its current form, in which case:
Tested-by: Jethro Beekman <jethro@fortanix.com>
Sorry for being super late with this, I know you asked me for feedback about this specific point in October. However, I did previously mention several times that being able to measure individual 256-byte chunks is necessary.
--
Jethro Beekman | Fortanix
^ permalink raw reply
* Re: [PATCH v26 13/22] x86/sgx: Add provisioning
From: Jethro Beekman @ 2020-02-13 10:49 UTC (permalink / raw)
To: Jarkko Sakkinen, linux-kernel, x86, linux-sgx
Cc: akpm, dave.hansen, sean.j.christopherson, nhorman, npmccallum,
haitao.huang, andriy.shevchenko, tglx, kai.svahn, bp, josh, luto,
kai.huang, rientjes, cedric.xing, puiterwijk,
linux-security-module
In-Reply-To: <20200209212609.7928-14-jarkko.sakkinen@linux.intel.com>
This patch and 22/22 contain the following email header:
Content-Type: text/plain; charset=a
git am doesn't like this.
--
Jethro Beekman | Fortanix
On 2020-02-09 22:26, Jarkko Sakkinen wrote:
> In order to provide a mechanism for devilering provisoning rights:
>
> 1. Add a new device file /dev/sgx/provision that works as a token for
> allowing an enclave to have the provisioning privileges.
> 2. Add a new ioctl called SGX_IOC_ENCLAVE_SET_ATTRIBUTE that accepts the
> following data structure:
>
> struct sgx_enclave_set_attribute {
> __u64 addr;
> __u64 attribute_fd;
> };
>
> A daemon could sit on top of /dev/sgx/provision and send a file
> descriptor of this file to a process that needs to be able to provision
> enclaves.
>
> The way this API is used is straight-forward. Lets assume that dev_fd is
> a handle to /dev/sgx/enclave and prov_fd is a handle to
> /dev/sgx/provision. You would allow SGX_IOC_ENCLAVE_CREATE to
> initialize an enclave with the PROVISIONKEY attribute by
>
> params.addr = <enclave address>;
> params.token_fd = prov_fd;
>
> ioctl(dev_fd, SGX_IOC_ENCLAVE_SET_ATTRIBUTE, ¶ms);
>
> Cc: linux-security-module@vger.kernel.org
> Suggested-by: Andy Lutomirski <luto@kernel.org>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
> arch/x86/include/uapi/asm/sgx.h | 11 ++++++++
> arch/x86/kernel/cpu/sgx/driver.c | 14 ++++++++++
> arch/x86/kernel/cpu/sgx/driver.h | 2 ++
> arch/x86/kernel/cpu/sgx/ioctl.c | 47 ++++++++++++++++++++++++++++++++
> 4 files changed, 74 insertions(+)
>
> diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
> index 5edb08ab8fd0..57d0d30c79b3 100644
> --- a/arch/x86/include/uapi/asm/sgx.h
> +++ b/arch/x86/include/uapi/asm/sgx.h
> @@ -25,6 +25,8 @@ enum sgx_page_flags {
> _IOWR(SGX_MAGIC, 0x01, struct sgx_enclave_add_pages)
> #define SGX_IOC_ENCLAVE_INIT \
> _IOW(SGX_MAGIC, 0x02, struct sgx_enclave_init)
> +#define SGX_IOC_ENCLAVE_SET_ATTRIBUTE \
> + _IOW(SGX_MAGIC, 0x03, struct sgx_enclave_set_attribute)
>
> /**
> * struct sgx_enclave_create - parameter structure for the
> @@ -63,4 +65,13 @@ struct sgx_enclave_init {
> __u64 sigstruct;
> };
>
> +/**
> + * struct sgx_enclave_set_attribute - parameter structure for the
> + * %SGX_IOC_ENCLAVE_SET_ATTRIBUTE ioctl
> + * @attribute_fd: file handle of the attribute file in the securityfs
> + */
> +struct sgx_enclave_set_attribute {
> + __u64 attribute_fd;
> +};
> +
> #endif /* _UAPI_ASM_X86_SGX_H */
> diff --git a/arch/x86/kernel/cpu/sgx/driver.c b/arch/x86/kernel/cpu/sgx/driver.c
> index b4aa7b9f8376..d90114cec1c3 100644
> --- a/arch/x86/kernel/cpu/sgx/driver.c
> +++ b/arch/x86/kernel/cpu/sgx/driver.c
> @@ -150,6 +150,13 @@ static struct miscdevice sgx_dev_enclave = {
> .fops = &sgx_encl_fops,
> };
>
> +static struct miscdevice sgx_dev_provision = {
> + .minor = MISC_DYNAMIC_MINOR,
> + .name = "provision",
> + .nodename = "sgx/provision",
> + .fops = &sgx_provision_fops,
> +};
> +
> int __init sgx_drv_init(void)
> {
> unsigned int eax, ebx, ecx, edx;
> @@ -190,5 +197,12 @@ int __init sgx_drv_init(void)
> return ret;
> }
>
> + ret = misc_register(&sgx_dev_provision);
> + if (ret) {
> + pr_err("Creating /dev/sgx/provision failed with %d.\n", ret);
> + misc_deregister(&sgx_dev_enclave);
> + return ret;
> + }
> +
> return 0;
> }
> diff --git a/arch/x86/kernel/cpu/sgx/driver.h b/arch/x86/kernel/cpu/sgx/driver.h
> index e4063923115b..72747d01c046 100644
> --- a/arch/x86/kernel/cpu/sgx/driver.h
> +++ b/arch/x86/kernel/cpu/sgx/driver.h
> @@ -23,6 +23,8 @@ extern u64 sgx_attributes_reserved_mask;
> extern u64 sgx_xfrm_reserved_mask;
> extern u32 sgx_xsave_size_tbl[64];
>
> +extern const struct file_operations sgx_provision_fops;
> +
> long sgx_ioctl(struct file *filep, unsigned int cmd, unsigned long arg);
>
> int sgx_drv_init(void);
> diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
> index 83513cdfd1c0..262001df3ae4 100644
> --- a/arch/x86/kernel/cpu/sgx/ioctl.c
> +++ b/arch/x86/kernel/cpu/sgx/ioctl.c
> @@ -663,6 +663,50 @@ static long sgx_ioc_enclave_init(struct sgx_encl *encl, void __user *arg)
> return ret;
> }
>
> +/**
> + * sgx_ioc_enclave_set_attribute - handler for %SGX_IOC_ENCLAVE_SET_ATTRIBUTE
> + * @filep: open file to /dev/sgx
> + * @arg: userspace pointer to a struct sgx_enclave_set_attribute instance
> + *
> + * Mark the enclave as being allowed to access a restricted attribute bit.
> + * The requested attribute is specified via the attribute_fd field in the
> + * provided struct sgx_enclave_set_attribute. The attribute_fd must be a
> + * handle to an SGX attribute file, e.g. “/dev/sgx/provision".
> + *
> + * Failure to explicitly request access to a restricted attribute will cause
> + * sgx_ioc_enclave_init() to fail. Currently, the only restricted attribute
> + * is access to the PROVISION_KEY.
> + *
> + * Note, access to the EINITTOKEN_KEY is disallowed entirely.
> + *
> + * Return: 0 on success, -errno otherwise
> + */
> +static long sgx_ioc_enclave_set_attribute(struct sgx_encl *encl,
> + void __user *arg)
> +{
> + struct sgx_enclave_set_attribute params;
> + struct file *attribute_file;
> + int ret;
> +
> + if (copy_from_user(¶ms, arg, sizeof(params)))
> + return -EFAULT;
> +
> + attribute_file = fget(params.attribute_fd);
> + if (!attribute_file)
> + return -EINVAL;
> +
> + if (attribute_file->f_op != &sgx_provision_fops) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + encl->allowed_attributes |= SGX_ATTR_PROVISIONKEY;
> + ret = 0;
> +
> +out:
> + fput(attribute_file);
> + return ret;
> +}
>
> long sgx_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
> {
> @@ -686,6 +730,9 @@ long sgx_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
> case SGX_IOC_ENCLAVE_INIT:
> ret = sgx_ioc_enclave_init(encl, (void __user *)arg);
> break;
> + case SGX_IOC_ENCLAVE_SET_ATTRIBUTE:
> + ret = sgx_ioc_enclave_set_attribute(encl, (void __user *)arg);
> + break;
> default:
> ret = -ENOIOCTLCMD;
> break;
>
^ permalink raw reply
* Re: [PATCH v5 01/10] capabilities: introduce CAP_PERFMON to kernel and user space
From: Alexey Budankov @ 2020-02-13 9:05 UTC (permalink / raw)
To: Stephen Smalley
Cc: Alexei Starovoitov, Peter Zijlstra, Arnaldo Carvalho de Melo,
Ingo Molnar, jani.nikula@linux.intel.com,
joonas.lahtinen@linux.intel.com, rodrigo.vivi@intel.com,
benh@kernel.crashing.org, Paul Mackerras, Michael Ellerman,
james.bottomley@hansenpartnership.com, Serge Hallyn, James Morris,
Will Deacon, Mark Rutland, Robert Richter, Alexei Starovoitov,
Jiri Olsa, Andi Kleen, Stephane Eranian, Igor Lubashev,
Alexander Shishkin, Namhyung Kim, Song Liu, Lionel Landwerlin,
Thomas Gleixner, linux-kernel,
linux-security-module@vger.kernel.org, selinux@vger.kernel.org,
intel-gfx@lists.freedesktop.org, linux-parisc@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org, linux-arm-kernel,
linux-perf-users@vger.kernel.org, oprofile-list, Andy Lutomirski
In-Reply-To: <46751eb9-deca-53cc-95fb-1602cfdf62a2@tycho.nsa.gov>
On 12.02.2020 20:09, Stephen Smalley wrote:
> On 2/12/20 11:56 AM, Alexey Budankov wrote:
>>
>>
>> On 12.02.2020 18:45, Stephen Smalley wrote:
>>> On 2/12/20 10:21 AM, Stephen Smalley wrote:
>>>> On 2/12/20 8:53 AM, Alexey Budankov wrote:
>>>>> On 12.02.2020 16:32, Stephen Smalley wrote:
>>>>>> On 2/12/20 3:53 AM, Alexey Budankov wrote:
>>>>>>> Hi Stephen,
>>>>>>>
>>>>>>> On 22.01.2020 17:07, Stephen Smalley wrote:
>>>>>>>> On 1/22/20 5:45 AM, Alexey Budankov wrote:
>>>>>>>>>
>>>>>>>>> On 21.01.2020 21:27, Alexey Budankov wrote:
>>>>>>>>>>
>>>>>>>>>> On 21.01.2020 20:55, Alexei Starovoitov wrote:
>>>>>>>>>>> On Tue, Jan 21, 2020 at 9:31 AM Alexey Budankov
>>>>>>>>>>> <alexey.budankov@linux.intel.com> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> On 21.01.2020 17:43, Stephen Smalley wrote:
>>>>>>>>>>>>> On 1/20/20 6:23 AM, Alexey Budankov wrote:
>>>>>>>>>>>>>>
>>>>>>> <SNIP>
>>>>>>>>>>>>>> Introduce CAP_PERFMON capability designed to secure system performance
>>>>>>>>>>>>>
>>>>>>>>>>>>> Why _noaudit()? Normally only used when a permission failure is non-fatal to the operation. Otherwise, we want the audit message.
>>>>>>>>>
>>>>>>>>> So far so good, I suggest using the simplest version for v6:
>>>>>>>>>
>>>>>>>>> static inline bool perfmon_capable(void)
>>>>>>>>> {
>>>>>>>>> return capable(CAP_PERFMON) || capable(CAP_SYS_ADMIN);
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> It keeps the implementation simple and readable. The implementation is more
>>>>>>>>> performant in the sense of calling the API - one capable() call for CAP_PERFMON
>>>>>>>>> privileged process.
>>>>>>>>>
>>>>>>>>> Yes, it bloats audit log for CAP_SYS_ADMIN privileged and unprivileged processes,
>>>>>>>>> but this bloating also advertises and leverages using more secure CAP_PERFMON
>>>>>>>>> based approach to use perf_event_open system call.
>>>>>>>>
>>>>>>>> I can live with that. We just need to document that when you see both a CAP_PERFMON and a CAP_SYS_ADMIN audit message for a process, try only allowing CAP_PERFMON first and see if that resolves the issue. We have a similar issue with CAP_DAC_READ_SEARCH versus CAP_DAC_OVERRIDE.
>>>>>>>
>>>>>>> I am trying to reproduce this double logging with CAP_PERFMON.
>>>>>>> I am using the refpolicy version with enabled perf_event tclass [1], in permissive mode.
>>>>>>> When running perf stat -a I am observing this AVC audit messages:
>>>>>>>
>>>>>>> type=AVC msg=audit(1581496695.666:8691): avc: denied { open } for pid=2779 comm="perf" scontext=user_u:user_r:user_systemd_t tcontext=user_u:user_r:user_systemd_t tclass=perf_event permissive=1
>>>>>>> type=AVC msg=audit(1581496695.666:8691): avc: denied { kernel } for pid=2779 comm="perf" scontext=user_u:user_r:user_systemd_t tcontext=user_u:user_r:user_systemd_t tclass=perf_event permissive=1
>>>>>>> type=AVC msg=audit(1581496695.666:8691): avc: denied { cpu } for pid=2779 comm="perf" scontext=user_u:user_r:user_systemd_t tcontext=user_u:user_r:user_systemd_t tclass=perf_event permissive=1
>>>>>>> type=AVC msg=audit(1581496695.666:8692): avc: denied { write } for pid=2779 comm="perf" scontext=user_u:user_r:user_systemd_t tcontext=user_u:user_r:user_systemd_t tclass=perf_event permissive=1
>>>>>>>
>>>>>>> However there is no capability related messages around. I suppose my refpolicy should
>>>>>>> be modified somehow to observe capability related AVCs.
>>>>>>>
>>>>>>> Could you please comment or clarify on how to enable caps related AVCs in order
>>>>>>> to test the concerned logging.
>>>>>>
>>>>>> The new perfmon permission has to be defined in your policy; you'll have a message in dmesg about "Permission perfmon in class capability2 not defined in policy.". You can either add it to the common cap2 definition in refpolicy/policy/flask/access_vectors and rebuild your policy or extract your base module as CIL, add it there, and insert the updated module.
>>>>>
>>>>> Yes, I already have it like this:
>>>>> common cap2
>>>>> {
>>>>> <------>mac_override<--># unused by SELinux
>>>>> <------>mac_admin
>>>>> <------>syslog
>>>>> <------>wake_alarm
>>>>> <------>block_suspend
>>>>> <------>audit_read
>>>>> <------>perfmon
>>>>> }
>>>>>
>>>>> dmesg stopped reporting perfmon as not defined but audit.log still doesn't report CAP_PERFMON denials.
>>>>> BTW, audit even doesn't report CAP_SYS_ADMIN denials, however perfmon_capable() does check for it.
>>>>
>>>> Some denials may be silenced by dontaudit rules; semodule -DB will strip those and semodule -B will restore them. Other possibility is that the process doesn't have CAP_PERFMON in its effective set and therefore never reaches SELinux at all; denied first by the capability module.
>>>
>>> Also, the fact that your denials are showing up in user_systemd_t suggests that something is off in your policy or userspace/distro; I assume that is a domain type for the systemd --user instance, but your shell and commands shouldn't be running in that domain (user_t would be more appropriate for that).
>>
>> It is user_t for local terminal session:
>> ps -Z
>> LABEL PID TTY TIME CMD
>> user_u:user_r:user_t 11317 pts/9 00:00:00 bash
>> user_u:user_r:user_t 11796 pts/9 00:00:00 ps
>>
>> For local terminal root session:
>> ps -Z
>> LABEL PID TTY TIME CMD
>> user_u:user_r:user_su_t 2926 pts/3 00:00:00 bash
>> user_u:user_r:user_su_t 10995 pts/3 00:00:00 ps
>>
>> For remote ssh session:
>> ps -Z
>> LABEL PID TTY TIME CMD
>> user_u:user_r:user_t 7540 pts/8 00:00:00 ps
>> user_u:user_r:user_systemd_t 8875 pts/8 00:00:00 bash
>
> That's a bug in either your policy or your userspace/distro integration. In any event, unless user_systemd_t is allowed all capability2 permissions by your policy, you should see the denials if CAP_PERFMON is set in the effective capability set of the process.
>
That all seems to be true. After instrumentation, rebuilding and rebooting, in CAP_PERFMON case:
$ getcap perf
perf = cap_sys_ptrace,cap_syslog,cap_perfmon+ep
$ perf stat -a
type=AVC msg=audit(1581580399.165:784): avc: denied { open } for pid=8859 comm="perf" scontext=user_u:user_r:user_t tcontext=user_u:user_r:user_t tclass=perf_event permissive=1
type=AVC msg=audit(1581580399.165:785): avc: denied { perfmon } for pid=8859 comm="perf" capability=38 scontext=user_u:user_r:user_t tcontext=user_u:user_r:user_t tclass=capability2 permissive=1
type=AVC msg=audit(1581580399.165:786): avc: denied { kernel } for pid=8859 comm="perf" scontext=user_u:user_r:user_t tcontext=user_u:user_r:user_t tclass=perf_event permissive=1
type=AVC msg=audit(1581580399.165:787): avc: denied { cpu } for pid=8859 comm="perf" scontext=user_u:user_r:user_t tcontext=user_u:user_r:user_t tclass=perf_event permissive=1
type=AVC msg=audit(1581580399.165:788): avc: denied { write } for pid=8859 comm="perf" scontext=user_u:user_r:user_t tcontext=user_u:user_r:user_t tclass=perf_event permissive=1
type=AVC msg=audit(1581580408.078:791): avc: denied { read } for pid=8859 comm="perf" scontext=user_u:user_r:user_t tcontext=user_u:user_r:user_t tclass=perf_event permissive=1
dmesg:
[ 137.877713] security_capable(0000000071f7ee6e, 000000009dd7a5fc, CAP_PERFMON, 0) = ?
[ 137.877774] cread_has_capability(CAP_PERFMON) = 0
[ 137.877775] prior avc_audit(CAP_PERFMON)
[ 137.877779] security_capable(0000000071f7ee6e, 000000009dd7a5fc, CAP_PERFMON, 0) = 0
[ 137.877784] security_capable(0000000071f7ee6e, 000000009dd7a5fc, CAP_PERFMON, 0) = ?
[ 137.877785] cread_has_capability(CAP_PERFMON) = 0
[ 137.877786] security_capable(0000000071f7ee6e, 000000009dd7a5fc, CAP_PERFMON, 0) = 0
[ 137.877794] security_capable(0000000071f7ee6e, 000000009dd7a5fc, CAP_PERFMON, 0) = ?
[ 137.877795] cread_has_capability(CAP_PERFMON) = 0
[ 137.877796] security_capable(0000000071f7ee6e, 000000009dd7a5fc, CAP_PERFMON, 0) = 0
...
in CAP_SYS_ADMIN case:
$ getcap perf
perf = cap_sys_ptrace,cap_sys_admin,cap_syslog+ep
$ perf stat -a
type=AVC msg=audit(1581580747.928:835): avc: denied { open } for pid=8927 comm="perf" scontext=user_u:user_r:user_t tcontext=user_u:user_r:user_t tclass=perf_event permissive=1
type=AVC msg=audit(1581580747.928:836): avc: denied { cpu } for pid=8927 comm="perf" scontext=user_u:user_r:user_t tcontext=user_u:user_r:user_t tclass=perf_event permissive=1
type=AVC msg=audit(1581580747.928:837): avc: denied { kernel } for pid=8927 comm="perf" scontext=user_u:user_r:user_t tcontext=user_u:user_r:user_t tclass=perf_event permissive=1
type=AVC msg=audit(1581580747.928:838): avc: denied { read } for pid=8927 comm="perf" scontext=user_u:user_r:user_t tcontext=user_u:user_r:user_t tclass=perf_event permissive=1
type=AVC msg=audit(1581580747.928:839): avc: denied { write } for pid=8927 comm="perf" scontext=user_u:user_r:user_t tcontext=user_u:user_r:user_t tclass=perf_event permissive=1
...
$ perf record -- ls
...
type=AVC msg=audit(1581580747.930:843): avc: denied { sys_ptrace } for pid=8927 comm="perf" capability=19 scontext=user_u:user_r:user_t tcontext=user_u:user_r:user_t tclass=capability permissive=1
...
dmesg:
[ 276.714266] security_capable(000000006b09ad8a, 000000009dd7a5fc, CAP_PERFMON, 0) = ?
[ 276.714268] security_capable(000000006b09ad8a, 000000009dd7a5fc, CAP_PERFMON, 0) = -1
[ 276.714269] security_capable(000000006b09ad8a, 000000009dd7a5fc, CAP_SYS_ADMIN, 0) = ?
[ 276.714270] cread_has_capability(CAP_SYS_ADMIN) = 0
[ 276.714270] security_capable(000000006b09ad8a, 000000009dd7a5fc, CAP_SYS_ADMIN, 0) = 0
[ 276.714287] security_capable(000000006b09ad8a, 000000009dd7a5fc, CAP_PERFMON, 0) = ?
[ 276.714287] security_capable(000000006b09ad8a, 000000009dd7a5fc, CAP_PERFMON, 0) = -1
[ 276.714288] security_capable(000000006b09ad8a, 000000009dd7a5fc, CAP_SYS_ADMIN, 0) = ?
[ 276.714288] cread_has_capability(CAP_SYS_ADMIN) = 0
[ 276.714289] security_capable(000000006b09ad8a, 000000009dd7a5fc, CAP_SYS_ADMIN, 0) = 0
[ 276.714294] security_capable(000000006b09ad8a, 000000009dd7a5fc, CAP_PERFMON, 0) = ?
[ 276.714295] security_capable(000000006b09ad8a, 000000009dd7a5fc, CAP_PERFMON, 0) = -1
[ 276.714295] security_capable(000000006b09ad8a, 000000009dd7a5fc, CAP_SYS_ADMIN, 0) = ?
[ 276.714296] cread_has_capability(CAP_SYS_ADMIN) = 0
[ 276.714296] security_capable(000000006b09ad8a, 000000009dd7a5fc, CAP_SYS_ADMIN, 0) = 0
...
in unprivileged case:
$ getcap perf
perf =
$ perf stat -a; perf record -a
...
dmesg:
[ 947.275611] security_capable(00000000d3a75377, 000000009dd7a5fc, CAP_PERFMON, 0) = ?
[ 947.275613] security_capable(00000000d3a75377, 000000009dd7a5fc, CAP_PERFMON, 0) = -1
[ 947.275614] security_capable(00000000d3a75377, 000000009dd7a5fc, CAP_SYS_ADMIN, 0) = ?
[ 947.275615] security_capable(00000000d3a75377, 000000009dd7a5fc, CAP_SYS_ADMIN, 0) = -1
[ 947.275636] security_capable(00000000d3a75377, 000000009dd7a5fc, CAP_PERFMON, 0) = ?
[ 947.275637] security_capable(00000000d3a75377, 000000009dd7a5fc, CAP_PERFMON, 0) = -1
[ 947.275638] security_capable(00000000d3a75377, 000000009dd7a5fc, CAP_SYS_ADMIN, 0) = ?
[ 947.275638] security_capable(00000000d3a75377, 000000009dd7a5fc, CAP_SYS_ADMIN, 0) = -1
...
So it looks like CAP_PERFMON and CAP_SYS_ADMIN are not ever logged by AVC simultaneously,
in the current LSM and perfmon_capable() implementations.
If perfmon is granted:
perfmon is not logged by capabilities, perfmon is logged by AVC,
no check for sys_admin by perfmon_capable().
If perfmon is not granted but sys_admin is granted:
perfmon is not logged by capabilities, AVC logging is not called for perfmon,
sys_admin is not logged by capabilities, sys_admin is not logged by AVC, for some intended reason?
No caps are granted:
AVC logging is not called either for perfmon or for sys_admin.
BTW, is there a way to may be drop some AV cache so denials would appear in audit in the next AV access?
Well, I guess you have initially mentioned some case similar to this (note that ids are not the same but pids= are):
type=AVC msg=audit(1581580399.165:784): avc: denied { open } for pid=8859 comm="perf" scontext=user_u:user_r:user_t tcontext=user_u:user_r:user_t tclass=perf_event permissive=1
type=AVC msg=audit(1581580399.165:785): avc: denied { perfmon } for pid=8859 comm="perf" capability=38 scontext=user_u:user_r:user_t tcontext=user_u:user_r:user_t tclass=capability2 permissive=1
type=AVC msg=audit( . : ): avc: denied { sys_admin } for pid=8859 comm="perf" capability=21 scontext=user_u:user_r:user_t tcontext=user_u:user_r:user_t tclass=capability2 permissive=1
type=AVC msg=audit(1581580399.165:786): avc: denied { kernel } for pid=8859 comm="perf" scontext=user_u:user_r:user_t tcontext=user_u:user_r:user_t tclass=perf_event permissive=1
type=AVC msg=audit(1581580399.165:787): avc: denied { cpu } for pid=8859 comm="perf" scontext=user_u:user_r:user_t tcontext=user_u:user_r:user_t tclass=perf_event permissive=1
type=AVC msg=audit(1581580399.165:788): avc: denied { write } for pid=8859 comm="perf" scontext=user_u:user_r:user_t tcontext=user_u:user_r:user_t tclass=perf_event permissive=1
type=AVC msg=audit(1581580408.078:791): avc: denied { read } for pid=8859 comm="perf" scontext=user_u:user_r:user_t tcontext=user_u:user_r:user_t tclass=perf_event permissive=1
So the message could be like this:
"If audit logs for a process using perf_events related syscalls i.e. perf_event_open(), read(), write(),
ioctl(), mmap() contain denials both for CAP_PERFMON and CAP_SYS_ADMIN capabilities then providing the
process with CAP_PERFMON capability singly is the secure preferred approach to resolve access denials
to performance monitoring and observability operations."
~Alexey
^ permalink raw reply
* Re: [PATCH v2] ima: export the measurement list when needed
From: Janne Karhunen @ 2020-02-13 6:41 UTC (permalink / raw)
To: Mimi Zohar
Cc: david.safford, linux-integrity, linux-security-module,
Ken Goldman, Wiseman, Monty (GE Global Research, US),
Amir Goldstein, linux-fsdevel
In-Reply-To: <1581555796.8515.130.camel@linux.ibm.com>
On Thu, Feb 13, 2020 at 3:03 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
> > This is a pretty important new feature.
> > A lot of people can't use IMA because of the memory issue.
> > Also, I really think we need to let administrators choose the tradeoffs
> > of keeping the list in memory, on a local file, or only on the
> > attestation server, as best fits their use cases.
>
> Dave, I understand that some use cases require the ability of
> truncating the measurement list. We're discussing how to truncate the
> measurement list. For example, in addition to the existing securityfs
> binary_runtime_measurements file, we could define a new securityfs
> file indicating the number of records to delete.
I don't have strong opinions either way, just let me know how to adapt
the patch and we will get it done asap. I'd prefer a solution where
the kernel can initiate the flush, but if not then not.
Thanks everyone for all the help.
--
Janne
^ permalink raw reply
* Re: [PATCH v8 07/11] proc: flush task dcache entries from all procfs instances
From: Al Viro @ 2020-02-13 5:55 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Linus Torvalds, LKML, Kernel Hardening, Linux API, Linux FS Devel,
Linux Security Module, Akinobu Mita, Alexey Dobriyan,
Andrew Morton, Andy Lutomirski, Daniel Micay, Djalal Harouni,
Dmitry V . Levin, Greg Kroah-Hartman, Ingo Molnar,
J . Bruce Fields, Jeff Layton, Jonathan Corbet, Kees Cook,
Oleg Nesterov, Solar Designer
In-Reply-To: <87pnejf6fz.fsf@x220.int.ebiederm.org>
On Wed, Feb 12, 2020 at 10:37:52PM -0600, Eric W. Biederman wrote:
> I think I have an alternate idea that could work. Add some extra code
> into proc_task_readdir, that would look for dentries that no longer
> point to tasks and d_invalidate them. With the same logic probably
> being called from a few more places as well like proc_pid_readdir,
> proc_task_lookup, and proc_pid_lookup.
>
> We could even optimize it and have a process died flag we set in the
> superblock.
>
> That would would batch up the freeing work until the next time someone
> reads from proc in a way that would create more dentries. So it would
> prevent dentries from reaped zombies from growing without bound.
>
> Hmm. Given the existence of proc_fill_cache it would really be a good
> idea if readdir and lookup performed some of the freeing work as well.
> As on readdir we always populate the dcache for all of the directory
> entries.
First of all, that won't do a damn thing when nobody is accessing
given superblock. What's more, readdir in root of that procfs instance
is not enough - you need it in task/ of group leader.
What I don't understand is the insistence on getting those dentries
via dcache lookups. _IF_ we are willing to live with cacheline
contention (on ->d_lock of root dentry, if nothing else), why not
do the following:
* put all dentries of such directories ([0-9]* and [0-9]*/task/*)
into a list anchored in task_struct; have non-counting reference to
task_struct stored in them (might simplify part of get_proc_task() users,
BTW - avoids pid-to-task_struct lookups if we have a dentry and not just
the inode; many callers do)
* have ->d_release() remove from it (protecting per-task_struct lock
nested outside of all ->d_lock)
* on exit:
lock the (per-task_struct) list
while list is non-empty
pick the first dentry
remove from the list
sb = dentry->d_sb
try to bump sb->s_active (if non-zero, that is).
if failed
continue // move on to the next one - nothing to do here
grab ->d_lock
res = handle_it(dentry, &temp_list)
drop ->d_lock
unlock the list
if (!list_empty(&temp_list))
shrink_dentry_list(&temp_list)
if (res)
d_invalidate(dentry)
dput(dentry)
deactivate_super(sb)
lock the list
unlock the list
handle_it(dentry, temp_list) // ->d_lock held; that one should be in dcache.c
if ->d_count is negative // unlikely
return 0;
if ->d_count is positive,
increment ->d_count
return 1;
// OK, it's still alive, but ->d_count is 0
__d_drop // equivalent of d_invalidate in this case
if not on a shrink list // otherwise it's not our headache
if on lru list
d_lru_del
d_shrink_add dentry to temp_list
return 0;
And yeah, that'll dirty ->s_active for each procfs superblock that
has dentry for our process present in dcache. On exit()...
^ permalink raw reply
* Re: [PATCH v8 07/11] proc: flush task dcache entries from all procfs instances
From: Eric W. Biederman @ 2020-02-13 4:37 UTC (permalink / raw)
To: Linus Torvalds
Cc: Al Viro, LKML, Kernel Hardening, Linux API, Linux FS Devel,
Linux Security Module, Akinobu Mita, Alexey Dobriyan,
Andrew Morton, Andy Lutomirski, Daniel Micay, Djalal Harouni,
Dmitry V . Levin, Greg Kroah-Hartman, Ingo Molnar,
J . Bruce Fields, Jeff Layton, Jonathan Corbet, Kees Cook,
Oleg Nesterov, Solar Designer
In-Reply-To: <CAHk-=wgmn9Qds0VznyphouSZW6e42GWDT5H1dpZg8pyGDGN+=w@mail.gmail.com>
Linus Torvalds <torvalds@linux-foundation.org> writes:
> On Wed, Feb 12, 2020 at 1:48 PM Eric W. Biederman <ebiederm@xmission.com> wrote:
>>
>> The good news is proc_flush_task isn't exactly called from process exit.
>> proc_flush_task is called during zombie clean up. AKA release_task.
>
> Yeah, that at least avoids some of the nasty locking while dying debug problems.
>
> But the one I was more worried about was actually the lock contention
> issue with lots of processes. The lock is basically a single global
> lock in many situations - yes, it's technically per-ns, but in a lot
> of cases you really only have one namespace anyway.
>
> And we've had problems with global locks in this area before, notably
> the one you call out:
>
>> Further after proc_flush_task does it's thing the code goes
>> and does "write_lock_irq(&task_list_lock);"
>
> Yeah, so it's not introducing a new issue, but it is potentially
> making something we already know is bad even worse.
>
>> What would be downside of having a mutex for a list of proc superblocks?
>> A mutex that is taken for both reading and writing the list.
>
> That's what the original patch actually was, and I was hoping we could
> avoid that thing.
>
> An rwsem would be possibly better, since most cases by far are likely
> about reading.
>
> And yes, I'm very aware of the task_list_lock, but it's literally why
> I don't want to make a new one.
>
> I'm _hoping_ we can some day come up with something better than
> task_list_lock.
Yes. I understand that.
I occassionally play with ideas, and converted all of proc to rcu
to help with situation but I haven't come up with anything clearly
better.
All of this is why I was really hoping we could have a change in
strategy and see if we can make the shrinker be able to better prune
proc inodes.
I think I have an alternate idea that could work. Add some extra code
into proc_task_readdir, that would look for dentries that no longer
point to tasks and d_invalidate them. With the same logic probably
being called from a few more places as well like proc_pid_readdir,
proc_task_lookup, and proc_pid_lookup.
We could even optimize it and have a process died flag we set in the
superblock.
That would would batch up the freeing work until the next time someone
reads from proc in a way that would create more dentries. So it would
prevent dentries from reaped zombies from growing without bound.
Hmm. Given the existence of proc_fill_cache it would really be a good
idea if readdir and lookup performed some of the freeing work as well.
As on readdir we always populate the dcache for all of the directory
entries.
I am booked solid for the next little while but if no one beats me to it
I will try and code something like that up where at least readdir
looks for and invalidates stale dentries.
Eric
^ permalink raw reply
* Re: [PATCH v2] ima: export the measurement list when needed
From: Mimi Zohar @ 2020-02-13 1:03 UTC (permalink / raw)
To: david.safford, Janne Karhunen, linux-integrity,
linux-security-module
Cc: Ken Goldman, monty.wiseman, Amir Goldstein, linux-fsdevel
In-Reply-To: <6b787049b965c8056d0e27360e2eaa8fa2f38b35.camel@gmail.com>
On Wed, 2020-02-12 at 16:08 -0500, david.safford@gmail.com wrote:
> On Tue, 2020-02-11 at 18:10 -0500, Mimi Zohar wrote:
> > On Tue, 2020-02-11 at 11:10 -0500, david.safford@gmail.com wrote:
>
> > > <snip>
> > >
> > This new feature will require setting up some infrastructure for
> > storing the partial measurement list(s) in order to validate a TPM
> > quote. Userspace already can save partial measurement list(s) without
> > any kernel changes. The entire measurement list does not need to be
> > read each time. lseek can read past the last record previously read.
> > The only new aspect is truncating the in kernel measurement list in
> > order to free kernel memory.
>
> This is a pretty important new feature.
> A lot of people can't use IMA because of the memory issue.
> Also, I really think we need to let administrators choose the tradeoffs
> of keeping the list in memory, on a local file, or only on the
> attestation server, as best fits their use cases.
Dave, I understand that some use cases require the ability of
truncating the measurement list. We're discussing how to truncate the
measurement list. For example, in addition to the existing securityfs
binary_runtime_measurements file, we could define a new securityfs
file indicating the number of records to delete.
> >
> > < snip>
> >
> > Until there is proof that the measurement list can be exported to a
> > file before kexec, instead of carrying the measurement list across
> > kexec, and a TPM quote can be validated after the kexec, there isn't a
> > compelling reason for the kernel needing to truncate the measurement
> > list.
>
> If this approach doesn't work with all the kexec use cases, then it is
> useless, and the ball is in my court to prove that it does. Fortunately
> I have to test that anyway for the coming TLV support.
>
> Working on it...
Testing could be done independently of the TLV support. To verify
that you aren't loosing any measurements, boot with a measurement
policy like "ima_policy=tcb" on the boot command line.
thanks,
Mimi
^ permalink raw reply
* Re: [PATCH v8 07/11] proc: flush task dcache entries from all procfs instances
From: Linus Torvalds @ 2020-02-13 0:48 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Al Viro, LKML, Kernel Hardening, Linux API, Linux FS Devel,
Linux Security Module, Akinobu Mita, Alexey Dobriyan,
Andrew Morton, Andy Lutomirski, Daniel Micay, Djalal Harouni,
Dmitry V . Levin, Greg Kroah-Hartman, Ingo Molnar,
J . Bruce Fields, Jeff Layton, Jonathan Corbet, Kees Cook,
Oleg Nesterov, Solar Designer
In-Reply-To: <87lfp7h422.fsf@x220.int.ebiederm.org>
On Wed, Feb 12, 2020 at 1:48 PM Eric W. Biederman <ebiederm@xmission.com> wrote:
>
> The good news is proc_flush_task isn't exactly called from process exit.
> proc_flush_task is called during zombie clean up. AKA release_task.
Yeah, that at least avoids some of the nasty locking while dying debug problems.
But the one I was more worried about was actually the lock contention
issue with lots of processes. The lock is basically a single global
lock in many situations - yes, it's technically per-ns, but in a lot
of cases you really only have one namespace anyway.
And we've had problems with global locks in this area before, notably
the one you call out:
> Further after proc_flush_task does it's thing the code goes
> and does "write_lock_irq(&task_list_lock);"
Yeah, so it's not introducing a new issue, but it is potentially
making something we already know is bad even worse.
> What would be downside of having a mutex for a list of proc superblocks?
> A mutex that is taken for both reading and writing the list.
That's what the original patch actually was, and I was hoping we could
avoid that thing.
An rwsem would be possibly better, since most cases by far are likely
about reading.
And yes, I'm very aware of the task_list_lock, but it's literally why
I don't want to make a new one.
I'm _hoping_ we can some day come up with something better than task_list_lock.
Linus
^ 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