* [PATCH 4/7] proc: Use d_invalidate in proc_prune_siblings_dcache
From: Eric W. Biederman @ 2020-02-20 20:49 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: <871rqpaswu.fsf_-_@x220.int.ebiederm.org>
The function d_prune_aliases has the problem that it will only prune
aliases thare are completely unused. It will not remove aliases for
the dcache or even think of removing mounts from the dcache. For that
behavior d_invalidate is needed.
To use d_invalidate replace d_prune_aliases with d_find_alias
followed by d_invalidate and dput. This is safe and complete
because no inode in proc has any hardlinks or aliases.
To make this behavior change clear rename
proc_prune_siblings_dache proc_invalidate_siblings_dcache,
and rename proc_sys_prune_dcache proc_sys_invalidate_dcache.
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
fs/proc/inode.c | 9 +++++++--
fs/proc/internal.h | 2 +-
fs/proc/proc_sysctl.c | 8 ++++----
3 files changed, 12 insertions(+), 7 deletions(-)
diff --git a/fs/proc/inode.c b/fs/proc/inode.c
index 38a7baa41aba..c4528c419876 100644
--- a/fs/proc/inode.c
+++ b/fs/proc/inode.c
@@ -103,12 +103,13 @@ void __init proc_init_kmemcache(void)
BUILD_BUG_ON(sizeof(struct proc_dir_entry) >= SIZEOF_PDE);
}
-void proc_prune_siblings_dcache(struct hlist_head *inodes, spinlock_t *lock)
+void proc_invalidate_siblings_dcache(struct hlist_head *inodes, spinlock_t *lock)
{
struct inode *inode;
struct proc_inode *ei;
struct hlist_node *node;
struct super_block *sb;
+ struct dentry *dentry;
for (;;) {
rcu_read_lock();
@@ -135,7 +136,11 @@ void proc_prune_siblings_dcache(struct hlist_head *inodes, spinlock_t *lock)
continue;
}
- d_prune_aliases(inode);
+ dentry = d_find_alias(inode);
+ if (dentry) {
+ d_invalidate(dentry);
+ dput(dentry);
+ }
iput(inode);
deactivate_super(sb);
}
diff --git a/fs/proc/internal.h b/fs/proc/internal.h
index ba9a991824a5..fd470172675f 100644
--- a/fs/proc/internal.h
+++ b/fs/proc/internal.h
@@ -210,7 +210,7 @@ extern const struct inode_operations proc_pid_link_inode_operations;
extern const struct super_operations proc_sops;
void proc_init_kmemcache(void);
-void proc_prune_siblings_dcache(struct hlist_head *inodes, spinlock_t *lock);
+void proc_invalidate_siblings_dcache(struct hlist_head *inodes, spinlock_t *lock);
void set_proc_pid_nlink(void);
extern struct inode *proc_get_inode(struct super_block *, struct proc_dir_entry *);
extern void proc_entry_rundown(struct proc_dir_entry *);
diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index 5da9d7f7ae34..b6f5d459b087 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -267,9 +267,9 @@ static void unuse_table(struct ctl_table_header *p)
complete(p->unregistering);
}
-static void proc_sys_prune_dcache(struct ctl_table_header *head)
+static void proc_sys_invalidate_dcache(struct ctl_table_header *head)
{
- proc_prune_siblings_dcache(&head->inodes, &sysctl_lock);
+ proc_invalidate_siblings_dcache(&head->inodes, &sysctl_lock);
}
/* called under sysctl_lock, will reacquire if has to wait */
@@ -291,10 +291,10 @@ static void start_unregistering(struct ctl_table_header *p)
spin_unlock(&sysctl_lock);
}
/*
- * Prune dentries for unregistered sysctls: namespaced sysctls
+ * Invalidate dentries for unregistered sysctls: namespaced sysctls
* can have duplicate names and contaminate dcache very badly.
*/
- proc_sys_prune_dcache(p);
+ proc_sys_invalidate_dcache(p);
/*
* do not remove from the list until nobody holds it; walking the
* list in do_sysctl() relies on that.
--
2.20.1
^ permalink raw reply related
* [PATCH 5/7] proc: Clear the pieces of proc_inode that proc_evict_inode cares about
From: Eric W. Biederman @ 2020-02-20 20:51 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: <871rqpaswu.fsf_-_@x220.int.ebiederm.org>
This just keeps everything tidier, and allows for using flags like
SLAB_TYPESAFE_BY_RCU where slabs are not always cleared before reuse.
I don't see reuse without reinitializing happening with the proc_inode
but I had a false alarm while reworking flushing of proc dentries and
indoes when a process dies that caused me to tidy this up.
The code is a little easier to follow and reason about this
way so I figured the changes might as well be kept.
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
fs/proc/inode.c | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/fs/proc/inode.c b/fs/proc/inode.c
index c4528c419876..3c9082cd257b 100644
--- a/fs/proc/inode.c
+++ b/fs/proc/inode.c
@@ -33,21 +33,27 @@ static void proc_evict_inode(struct inode *inode)
{
struct proc_dir_entry *de;
struct ctl_table_header *head;
+ struct proc_inode *ei = PROC_I(inode);
truncate_inode_pages_final(&inode->i_data);
clear_inode(inode);
/* Stop tracking associated processes */
- put_pid(PROC_I(inode)->pid);
+ if (ei->pid) {
+ put_pid(ei->pid);
+ ei->pid = NULL;
+ }
/* Let go of any associated proc directory entry */
- de = PDE(inode);
- if (de)
+ de = ei->pde;
+ if (de) {
pde_put(de);
+ ei->pde = NULL;
+ }
- head = PROC_I(inode)->sysctl;
+ head = ei->sysctl;
if (head) {
- RCU_INIT_POINTER(PROC_I(inode)->sysctl, NULL);
+ RCU_INIT_POINTER(ei->sysctl, NULL);
proc_sys_evict_inode(inode, head);
}
}
--
2.20.1
^ permalink raw reply related
* [PATCH 6/7] proc: Use a list of inodes to flush from proc
From: Eric W. Biederman @ 2020-02-20 20:52 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: <871rqpaswu.fsf_-_@x220.int.ebiederm.org>
Rework the flushing of proc to use a list of directory inodes that
need to be flushed.
The list is kept on struct pid not on struct task_struct, as there is
a fixed connection between proc inodes and pids but at least for the
case of de_thread the pid of a task_struct changes.
This removes the dependency on proc_mnt which allows for different
mounts of proc having different mount options even in the same pid
namespace and this allows for the removal of proc_mnt which will
trivially the first mount of proc to honor it's mount options.
This flushing remains an optimization. The functions
pid_delete_dentry and pid_revalidate ensure that ordinary dcache
management will not attempt to use dentries past the point their
respective task has died. When unused the shrinker will
eventually be able to remove these dentries.
There is a case in de_thread where proc_flush_pid can be
called early for a given pid. Which winds up being
safe (if suboptimal) as this is just an optiimization.
Only pid directories are put on the list as the other
per pid files are children of those directories and
d_invalidate on the directory will get them as well.
So that the pid can be used during flushing it's reference count is
taken in release_task and dropped in proc_flush_pid. Further the call
of proc_flush_pid is moved after the tasklist_lock is released in
release_task so that it is certain that the pid has already been
unhashed when flushing it taking place. This removes a small race
where a dentry could recreated.
As struct pid is supposed to be small and I need a per pid lock
I reuse the only lock that currently exists in struct pid the
the wait_pidfd.lock.
The net result is that this adds all of this functionality
with just a little extra list management overhead and
a single extra pointer in struct pid.
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
fs/proc/base.c | 111 +++++++++++++---------------------------
fs/proc/inode.c | 2 +-
fs/proc/internal.h | 1 +
include/linux/pid.h | 1 +
include/linux/proc_fs.h | 4 +-
kernel/exit.c | 4 +-
6 files changed, 44 insertions(+), 79 deletions(-)
diff --git a/fs/proc/base.c b/fs/proc/base.c
index c7c64272b0fa..e7efe9d6f3d6 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -1834,11 +1834,25 @@ void task_dump_owner(struct task_struct *task, umode_t mode,
*rgid = gid;
}
+void proc_pid_evict_inode(struct proc_inode *ei)
+{
+ struct pid *pid = ei->pid;
+
+ if (S_ISDIR(ei->vfs_inode.i_mode)) {
+ spin_lock(&pid->wait_pidfd.lock);
+ hlist_del_init_rcu(&ei->sibling_inodes);
+ spin_unlock(&pid->wait_pidfd.lock);
+ }
+
+ put_pid(pid);
+}
+
struct inode *proc_pid_make_inode(struct super_block * sb,
struct task_struct *task, umode_t mode)
{
struct inode * inode;
struct proc_inode *ei;
+ struct pid *pid;
/* We need a new inode */
@@ -1856,10 +1870,18 @@ struct inode *proc_pid_make_inode(struct super_block * sb,
/*
* grab the reference to task.
*/
- ei->pid = get_task_pid(task, PIDTYPE_PID);
- if (!ei->pid)
+ pid = get_task_pid(task, PIDTYPE_PID);
+ if (!pid)
goto out_unlock;
+ /* Let the pid remember us for quick removal */
+ ei->pid = pid;
+ if (S_ISDIR(mode)) {
+ spin_lock(&pid->wait_pidfd.lock);
+ hlist_add_head_rcu(&ei->sibling_inodes, &pid->inodes);
+ spin_unlock(&pid->wait_pidfd.lock);
+ }
+
task_dump_owner(task, 0, &inode->i_uid, &inode->i_gid);
security_task_to_inode(task, inode);
@@ -3230,90 +3252,29 @@ static const struct inode_operations proc_tgid_base_inode_operations = {
.permission = proc_pid_permission,
};
-static void proc_flush_task_mnt(struct vfsmount *mnt, pid_t pid, pid_t tgid)
-{
- struct dentry *dentry, *leader, *dir;
- char buf[10 + 1];
- struct qstr name;
-
- name.name = buf;
- name.len = snprintf(buf, sizeof(buf), "%u", pid);
- /* no ->d_hash() rejects on procfs */
- dentry = d_hash_and_lookup(mnt->mnt_root, &name);
- if (dentry) {
- d_invalidate(dentry);
- dput(dentry);
- }
-
- if (pid == tgid)
- return;
-
- name.name = buf;
- name.len = snprintf(buf, sizeof(buf), "%u", tgid);
- leader = d_hash_and_lookup(mnt->mnt_root, &name);
- if (!leader)
- goto out;
-
- name.name = "task";
- name.len = strlen(name.name);
- dir = d_hash_and_lookup(leader, &name);
- if (!dir)
- goto out_put_leader;
-
- name.name = buf;
- name.len = snprintf(buf, sizeof(buf), "%u", pid);
- dentry = d_hash_and_lookup(dir, &name);
- if (dentry) {
- d_invalidate(dentry);
- dput(dentry);
- }
-
- dput(dir);
-out_put_leader:
- dput(leader);
-out:
- return;
-}
-
/**
- * proc_flush_task - Remove dcache entries for @task from the /proc dcache.
- * @task: task that should be flushed.
+ * proc_flush_pid - Remove dcache entries for @pid from the /proc dcache.
+ * @pid: pid that should be flushed.
*
- * When flushing dentries from proc, one needs to flush them from global
- * proc (proc_mnt) and from all the namespaces' procs this task was seen
- * in. This call is supposed to do all of this job.
- *
- * Looks in the dcache for
- * /proc/@pid
- * /proc/@tgid/task/@pid
- * if either directory is present flushes it and all of it'ts children
- * from the dcache.
+ * This function walks a list of inodes (that belong to any proc
+ * filesystem) that are attached to the pid and flushes them from
+ * the dentry cache.
*
* It is safe and reasonable to cache /proc entries for a task until
* that task exits. After that they just clog up the dcache with
* useless entries, possibly causing useful dcache entries to be
- * flushed instead. This routine is proved to flush those useless
- * dcache entries at process exit time.
+ * flushed instead. This routine is provided to flush those useless
+ * dcache entries when a process is reaped.
*
* NOTE: This routine is just an optimization so it does not guarantee
- * that no dcache entries will exist at process exit time it
- * just makes it very unlikely that any will persist.
+ * that no dcache entries will exist after a process is reaped
+ * it just makes it very unlikely that any will persist.
*/
-void proc_flush_task(struct task_struct *task)
+void proc_flush_pid(struct pid *pid)
{
- int i;
- struct pid *pid, *tgid;
- struct upid *upid;
-
- pid = task_pid(task);
- tgid = task_tgid(task);
-
- for (i = 0; i <= pid->level; i++) {
- upid = &pid->numbers[i];
- proc_flush_task_mnt(upid->ns->proc_mnt, upid->nr,
- tgid->numbers[i].nr);
- }
+ proc_invalidate_siblings_dcache(&pid->inodes, &pid->wait_pidfd.lock);
+ put_pid(pid);
}
static struct dentry *proc_pid_instantiate(struct dentry * dentry,
diff --git a/fs/proc/inode.c b/fs/proc/inode.c
index 3c9082cd257b..979e867be3e5 100644
--- a/fs/proc/inode.c
+++ b/fs/proc/inode.c
@@ -40,7 +40,7 @@ static void proc_evict_inode(struct inode *inode)
/* Stop tracking associated processes */
if (ei->pid) {
- put_pid(ei->pid);
+ proc_pid_evict_inode(ei);
ei->pid = NULL;
}
diff --git a/fs/proc/internal.h b/fs/proc/internal.h
index fd470172675f..9e294f0290e5 100644
--- a/fs/proc/internal.h
+++ b/fs/proc/internal.h
@@ -158,6 +158,7 @@ extern int proc_pid_statm(struct seq_file *, struct pid_namespace *,
extern const struct dentry_operations pid_dentry_operations;
extern int pid_getattr(const struct path *, struct kstat *, u32, unsigned int);
extern int proc_setattr(struct dentry *, struct iattr *);
+extern void proc_pid_evict_inode(struct proc_inode *);
extern struct inode *proc_pid_make_inode(struct super_block *, struct task_struct *, umode_t);
extern void pid_update_inode(struct task_struct *, struct inode *);
extern int pid_delete_dentry(const struct dentry *);
diff --git a/include/linux/pid.h b/include/linux/pid.h
index 998ae7d24450..01a0d4e28506 100644
--- a/include/linux/pid.h
+++ b/include/linux/pid.h
@@ -62,6 +62,7 @@ struct pid
unsigned int level;
/* lists of tasks that use this pid */
struct hlist_head tasks[PIDTYPE_MAX];
+ struct hlist_head inodes;
/* wait queue for pidfd notifications */
wait_queue_head_t wait_pidfd;
struct rcu_head rcu;
diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
index 3dfa92633af3..40a7982b7285 100644
--- a/include/linux/proc_fs.h
+++ b/include/linux/proc_fs.h
@@ -32,7 +32,7 @@ struct proc_ops {
typedef int (*proc_write_t)(struct file *, char *, size_t);
extern void proc_root_init(void);
-extern void proc_flush_task(struct task_struct *);
+extern void proc_flush_pid(struct pid *);
extern struct proc_dir_entry *proc_symlink(const char *,
struct proc_dir_entry *, const char *);
@@ -105,7 +105,7 @@ static inline void proc_root_init(void)
{
}
-static inline void proc_flush_task(struct task_struct *task)
+static inline void proc_flush_pid(struct pid *pid)
{
}
diff --git a/kernel/exit.c b/kernel/exit.c
index 2833ffb0c211..502b4995b688 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -191,6 +191,7 @@ void put_task_struct_rcu_user(struct task_struct *task)
void release_task(struct task_struct *p)
{
struct task_struct *leader;
+ struct pid *thread_pid;
int zap_leader;
repeat:
/* don't need to get the RCU readlock here - the process is dead and
@@ -199,11 +200,11 @@ void release_task(struct task_struct *p)
atomic_dec(&__task_cred(p)->user->processes);
rcu_read_unlock();
- proc_flush_task(p);
cgroup_release(p);
write_lock_irq(&tasklist_lock);
ptrace_release_task(p);
+ thread_pid = get_pid(p->thread_pid);
__exit_signal(p);
/*
@@ -226,6 +227,7 @@ void release_task(struct task_struct *p)
}
write_unlock_irq(&tasklist_lock);
+ proc_flush_pid(thread_pid);
release_thread(p);
put_task_struct_rcu_user(p);
--
2.20.1
^ permalink raw reply related
* [PATCH 7/7] proc: Ensure we see the exit of each process tid exactly once
From: Eric W. Biederman @ 2020-02-20 20:52 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: <871rqpaswu.fsf_-_@x220.int.ebiederm.org>
When the thread group leader changes during exec and the old leaders
thread is reaped proc_flush_pid will flush the dentries for the entire
process because the leader still has it's original pid.
Fix this by exchanging the pids in an rcu safe manner,
and wrapping the code to do that up in a helper exchange_tids.
When I removed switch_exec_pids and introduced this behavior
in d73d65293e3e ("[PATCH] pidhash: kill switch_exec_pids") there
really was nothing that cared as flushing happened with
the cached dentry and de_thread flushed both of them on exec.
This lack of fully exchanging pids became a problem a few months later
when I introduced 48e6484d4902 ("[PATCH] proc: Rewrite the proc dentry
flush on exit optimization"). Which overlooked the de_thread case
was no longer swapping pids, and I was looking up proc dentries
by task->pid.
The current behavior isn't properly a bug as everything in proc will
continue to work correctly just a little bit less efficiently. Fix
this just so there are no little surprise corner cases waiting to bite
people.
Fixes: 48e6484d4902 ("[PATCH] proc: Rewrite the proc dentry flush on exit optimization").
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
fs/exec.c | 5 +----
include/linux/pid.h | 1 +
kernel/pid.c | 16 ++++++++++++++++
3 files changed, 18 insertions(+), 4 deletions(-)
diff --git a/fs/exec.c b/fs/exec.c
index db17be51b112..3f0bc293442e 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1148,11 +1148,8 @@ static int de_thread(struct task_struct *tsk)
/* Become a process group leader with the old leader's pid.
* The old leader becomes a thread of the this thread group.
- * Note: The old leader also uses this pid until release_task
- * is called. Odd but simple and correct.
*/
- tsk->pid = leader->pid;
- change_pid(tsk, PIDTYPE_PID, task_pid(leader));
+ exchange_tids(tsk, leader);
transfer_pid(leader, tsk, PIDTYPE_TGID);
transfer_pid(leader, tsk, PIDTYPE_PGID);
transfer_pid(leader, tsk, PIDTYPE_SID);
diff --git a/include/linux/pid.h b/include/linux/pid.h
index 01a0d4e28506..0f40b5f1c32c 100644
--- a/include/linux/pid.h
+++ b/include/linux/pid.h
@@ -101,6 +101,7 @@ extern void attach_pid(struct task_struct *task, enum pid_type);
extern void detach_pid(struct task_struct *task, enum pid_type);
extern void change_pid(struct task_struct *task, enum pid_type,
struct pid *pid);
+extern void exchange_tids(struct task_struct *task, struct task_struct *old);
extern void transfer_pid(struct task_struct *old, struct task_struct *new,
enum pid_type);
diff --git a/kernel/pid.c b/kernel/pid.c
index 0f4ecb57214c..0085b15478fb 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -359,6 +359,22 @@ void change_pid(struct task_struct *task, enum pid_type type,
attach_pid(task, type);
}
+void exchange_tids(struct task_struct *ntask, struct task_struct *otask)
+{
+ /* pid_links[PIDTYPE_PID].next is always NULL */
+ struct pid *npid = READ_ONCE(ntask->thread_pid);
+ struct pid *opid = READ_ONCE(otask->thread_pid);
+
+ rcu_assign_pointer(opid->tasks[PIDTYPE_PID].first, &ntask->pid_links[PIDTYPE_PID]);
+ rcu_assign_pointer(npid->tasks[PIDTYPE_PID].first, &otask->pid_links[PIDTYPE_PID]);
+ rcu_assign_pointer(ntask->thread_pid, opid);
+ rcu_assign_pointer(otask->thread_pid, npid);
+ WRITE_ONCE(ntask->pid_links[PIDTYPE_PID].pprev, &opid->tasks[PIDTYPE_PID].first);
+ WRITE_ONCE(otask->pid_links[PIDTYPE_PID].pprev, &npid->tasks[PIDTYPE_PID].first);
+ WRITE_ONCE(ntask->pid, pid_nr(opid));
+ WRITE_ONCE(otask->pid, pid_nr(npid));
+}
+
/* transfer_pid is an optimization of attach_pid(new), detach_pid(old) */
void transfer_pid(struct task_struct *old, struct task_struct *new,
enum pid_type type)
--
2.20.1
^ permalink raw reply related
* Re: [PATCH v26 10/22] x86/sgx: Linux Enclave Driver
From: Jarkko Sakkinen @ 2020-02-20 22:10 UTC (permalink / raw)
To: Jordan Hand
Cc: linux-kernel, x86, linux-sgx, 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: <15074c16-4832-456d-dd12-af8548e46d6d@linux.microsoft.com>
On Tue, Feb 18, 2020 at 07:26:31PM -0800, Jordan Hand wrote:
> if (!page || (~page->vm_max_prot_bits & vm_prot_bits))
> return -EACCESS
>
> This means that for any process where READ_IMPLIES_EXECUTE is set and
> page where (vma->vm_flags & VM_MAYEXEC) == true, mmap/mprotect calls to
> that request PROT_READ on a page that was not added with PROT_EXEC will
> fail.
Right. You would end up requesting RX from a R region.
And you are suggesting that we tweak it along the lines of to make RIE
processes work:
unsigned long max_prot_bits = page->vm_max_prot_bits;
if (!!(current->personality & READ_IMPLIES_EXEC) &&
vma->vm_flags & VM_MAY_EXEC)
max_prot_bits |= VM_EXEC;
/* ... */
if (!page || (~max_prot_bits & vm_prot_bits))
return -EACCESS
?
/Jarkko
^ permalink raw reply
* Re: [PATCH v26 10/22] x86/sgx: Linux Enclave Driver
From: Jarkko Sakkinen @ 2020-02-20 22:16 UTC (permalink / raw)
To: Sean Christopherson
Cc: Jordan Hand, 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: <20200220184842.GE3972@linux.intel.com>
On Thu, Feb 20, 2020 at 10:48:42AM -0800, Sean Christopherson wrote:
> My biggest concern for allowing PROT_EXEC if RIE is that it would result
> in #PF(SGX) (#GP on Skylake) due to an EPCM violation if the enclave
> actually tried to execute from such a page. This isn't a problem for the
> kernel as the fault will be reported cleanly through the vDSO (or get
> delivered as a SIGSEGV if the enclave isn't entered through the vDSO), but
> it's a bit weird for userspace as userspace will see the #PF(SGX) and
> likely assume the EPC was lost, e.g. silently restart the enclave instead
> of logging an error that the enclave is broken.
I think right way to fix the current implementation is to -EACCES mmap()
(and mprotect) when !!(current->personality & READ_IMPLIES_EXEC).
This way supporting RIE can be reconsidered later on without any
potential ABI bottlenecks.
/Jarkko
^ permalink raw reply
* Re: [PATCH 3/7] proc: Mov rcu_read_(lock|unlock) in proc_prune_siblings_dcache
From: Linus Torvalds @ 2020-02-20 22:33 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: <87h7zl9e7u.fsf_-_@x220.int.ebiederm.org>
On Thu, Feb 20, 2020 at 12:51 PM Eric W. Biederman
<ebiederm@xmission.com> wrote:
>
> Don't make it look like rcu_read_lock is held over the entire loop
> instead just take the rcu_read_lock over the part of the loop that
> matters. This makes the intent of the code a little clearer.
No, this is horrid.
Maybe it makes the intent clearer, but it also causes that "continue"
case to unlock and relock immediately.
And maybe that case never triggers, and that's ok. But then it needs a
big comment about it.
Linus
^ permalink raw reply
* Re: [PATCH 4/7] proc: Use d_invalidate in proc_prune_siblings_dcache
From: Linus Torvalds @ 2020-02-20 22:43 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: <87blpt9e6m.fsf_-_@x220.int.ebiederm.org>
On Thu, Feb 20, 2020 at 12:51 PM Eric W. Biederman
<ebiederm@xmission.com> wrote:
>
> To use d_invalidate replace d_prune_aliases with d_find_alias
> followed by d_invalidate and dput. This is safe and complete
> because no inode in proc has any hardlinks or aliases.
Are you sure you can't create them some way? This makes em go "what
if we had multiple dentries associated with that inode?" Then the code
would just invalidate the first one.
I guess we don't have export_operations or anything like that, but
this makes me worry...
Linus
^ permalink raw reply
* Re: [PATCH 4/7] proc: Use d_invalidate in proc_prune_siblings_dcache
From: Al Viro @ 2020-02-20 22:54 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: <87blpt9e6m.fsf_-_@x220.int.ebiederm.org>
On Thu, Feb 20, 2020 at 02:49:53PM -0600, Eric W. Biederman wrote:
>
> The function d_prune_aliases has the problem that it will only prune
> aliases thare are completely unused. It will not remove aliases for
> the dcache or even think of removing mounts from the dcache. For that
> behavior d_invalidate is needed.
>
> To use d_invalidate replace d_prune_aliases with d_find_alias
> followed by d_invalidate and dput. This is safe and complete
> because no inode in proc has any hardlinks or aliases.
s/no inode.*/it's a fucking directory inode./
^ permalink raw reply
* Re: [PATCH 0/7] proc: Dentry flushing without proc_mnt
From: Linus Torvalds @ 2020-02-20 23:02 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: <871rqpaswu.fsf_-_@x220.int.ebiederm.org>
On Thu, Feb 20, 2020 at 12:48 PM Eric W. Biederman
<ebiederm@xmission.com> wrote:
>
> Linus, does this approach look like something you can stand?
A couple of worries, although one of them seem to have already been
resolved by Al.
I think the real gatekeeper should be Al in general. But other than
the small comments I had, I think this might work just fine.
Al?
Linus
^ permalink raw reply
* Re: [PATCH 4/7] proc: Use d_invalidate in proc_prune_siblings_dcache
From: Al Viro @ 2020-02-20 23:03 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: <20200220225420.GR23230@ZenIV.linux.org.uk>
On Thu, Feb 20, 2020 at 10:54:20PM +0000, Al Viro wrote:
> On Thu, Feb 20, 2020 at 02:49:53PM -0600, Eric W. Biederman wrote:
> >
> > The function d_prune_aliases has the problem that it will only prune
> > aliases thare are completely unused. It will not remove aliases for
> > the dcache or even think of removing mounts from the dcache. For that
> > behavior d_invalidate is needed.
> >
> > To use d_invalidate replace d_prune_aliases with d_find_alias
> > followed by d_invalidate and dput. This is safe and complete
> > because no inode in proc has any hardlinks or aliases.
>
> s/no inode.*/it's a fucking directory inode./
Wait... You are using it for sysctls as well? Ho-hum... The thing is,
for sysctls you are likely to run into consequent entries with the
same superblock, making for a big pile of useless playing with
->s_active... And yes, that applied to mainline as well
^ permalink raw reply
* Re: [PATCH 4/7] proc: Use d_invalidate in proc_prune_siblings_dcache
From: Linus Torvalds @ 2020-02-20 23:00 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: <20200220225420.GR23230@ZenIV.linux.org.uk>
On Thu, Feb 20, 2020 at 2:54 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> s/no inode.*/it's a directory inode./
That actually makes my worry go away too. We don't allow aliases for
directory inodes, iirc.
So then it doesn't depend on some /proc implementation issue any more,
then it's fundamental that there's only one dentry.
Linus
^ permalink raw reply
* Re: [PATCH 0/7] proc: Dentry flushing without proc_mnt
From: Al Viro @ 2020-02-20 23:07 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-=whX7UmXgCKPPvjyQFqBiKw-Zsgj22_rH8epDPoWswAnLA@mail.gmail.com>
On Thu, Feb 20, 2020 at 03:02:22PM -0800, Linus Torvalds wrote:
> On Thu, Feb 20, 2020 at 12:48 PM Eric W. Biederman
> <ebiederm@xmission.com> wrote:
> >
> > Linus, does this approach look like something you can stand?
>
> A couple of worries, although one of them seem to have already been
> resolved by Al.
>
> I think the real gatekeeper should be Al in general. But other than
> the small comments I had, I think this might work just fine.
>
> Al?
I'll need to finish RTFS there; I have initially misread that patch,
actually - Eric _is_ using that thing both for those directories
and for sysctl inodes. And the prototype for that machinery (the
one he'd pulled from proc_sysctl.c) is playing with pinning superblocks
way too much; for per-pid directories that's not an issue, but
for sysctl table removal you are very likely to hit a bunch of
evictees on the same superblock...
^ permalink raw reply
* Re: [PATCH 0/7] proc: Dentry flushing without proc_mnt
From: Eric W. Biederman @ 2020-02-20 23:37 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: <20200220230758.GT23230@ZenIV.linux.org.uk>
Al Viro <viro@zeniv.linux.org.uk> writes:
> On Thu, Feb 20, 2020 at 03:02:22PM -0800, Linus Torvalds wrote:
>> On Thu, Feb 20, 2020 at 12:48 PM Eric W. Biederman
>> <ebiederm@xmission.com> wrote:
>> >
>> > Linus, does this approach look like something you can stand?
>>
>> A couple of worries, although one of them seem to have already been
>> resolved by Al.
>>
>> I think the real gatekeeper should be Al in general. But other than
>> the small comments I had, I think this might work just fine.
>>
>> Al?
>
> I'll need to finish RTFS there; I have initially misread that patch,
> actually - Eric _is_ using that thing both for those directories
> and for sysctl inodes. And the prototype for that machinery (the
> one he'd pulled from proc_sysctl.c) is playing with pinning superblocks
> way too much; for per-pid directories that's not an issue, but
> for sysctl table removal you are very likely to hit a bunch of
> evictees on the same superblock...
I saw that was possible. If the broad strokes look correct I don't have
a problem at all with optimizing for the case where many of the entries
are for inodes on the same superblock. I just had enough other details
on my mind I was afraid if I got a little more clever I would have
introduced a typo somewhere.
I wish I could limit the sysctl parts to just directories, but
unfortunately the sysctl tables don't always give a guarantee that a
directory is what will be removed. But sysctls do have one name per
inode invarant like fat. There is no way to express a sysctl
table that doesn't have that invariant.
As for d_find_alias/d_invalidate.
Just for completeness I wanted to write a loop:
while (dentry = d_find_alias(inode)) {
d_invalidate(dentry);
dput(dentry);
}
Unfortunately that breaks on directories, because for directories
d_find_alias turns into d_find_any_alias, and continues to return aliases
even when they are unhashed.
It might be nice to write a cousin of d_prune_aliases call
it d_invalidate_aliases that just does that loop the correct way
in dcache.c
Eric
^ permalink raw reply
* Re: [PATCH 4/7] proc: Use d_invalidate in proc_prune_siblings_dcache
From: Eric W. Biederman @ 2020-02-20 23:39 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: <20200220230309.GS23230@ZenIV.linux.org.uk>
Al Viro <viro@zeniv.linux.org.uk> writes:
> On Thu, Feb 20, 2020 at 10:54:20PM +0000, Al Viro wrote:
>> On Thu, Feb 20, 2020 at 02:49:53PM -0600, Eric W. Biederman wrote:
>> >
>> > The function d_prune_aliases has the problem that it will only prune
>> > aliases thare are completely unused. It will not remove aliases for
>> > the dcache or even think of removing mounts from the dcache. For that
>> > behavior d_invalidate is needed.
>> >
>> > To use d_invalidate replace d_prune_aliases with d_find_alias
>> > followed by d_invalidate and dput. This is safe and complete
>> > because no inode in proc has any hardlinks or aliases.
>>
>> s/no inode.*/it's a fucking directory inode./
>
> Wait... You are using it for sysctls as well? Ho-hum... The thing is,
> for sysctls you are likely to run into consequent entries with the
> same superblock, making for a big pile of useless playing with
> ->s_active... And yes, that applied to mainline as well
Which is why I worked to merge the two cases since they were so close.
Fewer things to fix and more eyeballs on the code.
Eric
^ permalink raw reply
* Re: [PATCH bpf-next v4 3/8] bpf: lsm: provide attachment points for BPF LSM programs
From: Casey Schaufler @ 2020-02-20 23:49 UTC (permalink / raw)
To: KP Singh, LKML, Linux Security Module list
In-Reply-To: <20200220175250.10795-4-kpsingh@chromium.org>
On 2/20/2020 9:52 AM, KP Singh wrote:
> From: KP Singh <kpsingh@google.com>
Sorry about the heavy list pruning - the original set
blows thunderbird up.
>
> The BPF LSM programs are implemented as fexit trampolines to avoid the
> overhead of retpolines. These programs cannot be attached to security_*
> wrappers as there are quite a few security_* functions that do more than
> just calling the LSM callbacks.
>
> This was discussed on the lists in:
>
> https://lore.kernel.org/bpf/20200123152440.28956-1-kpsingh@chromium.org/T/#m068becce588a0cdf01913f368a97aea4c62d8266
>
> Adding a NOP callback after all the static LSM callbacks are called has
> the following benefits:
>
> - The BPF programs run at the right stage of the security_* wrappers.
> - They run after all the static LSM hooks allowed the operation,
> therefore cannot allow an action that was already denied.
I still say that the special call-out to BPF is unnecessary.
I remain unconvinced by the arguments. You aren't doing anything
so special that the general mechanism won't work.
>
> There are some hooks which do not call call_int_hooks or
> call_void_hooks. It's not possible to call the bpf_lsm_* functions
> without checking if there is BPF LSM program attached to these hooks.
> This is added further in a subsequent patch. For now, these hooks are
> marked as NO_BPF (i.e. attachment of BPF programs is not possible).
>
> Signed-off-by: KP Singh <kpsingh@google.com>
> ---
> include/linux/bpf_lsm.h | 34 ++++++++++++++++++++++++++++++++++
> kernel/bpf/bpf_lsm.c | 16 ++++++++++++++++
> security/security.c | 3 +++
> 3 files changed, 53 insertions(+)
> create mode 100644 include/linux/bpf_lsm.h
>
> diff --git a/include/linux/bpf_lsm.h b/include/linux/bpf_lsm.h
> new file mode 100644
> index 000000000000..f867f72f6aa9
> --- /dev/null
> +++ b/include/linux/bpf_lsm.h
> @@ -0,0 +1,34 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +/*
> + * Copyright 2019 Google LLC.
> + */
> +
> +#ifndef _LINUX_BPF_LSM_H
> +#define _LINUX_BPF_LSM_H
> +
> +#include <linux/bpf.h>
> +
> +#ifdef CONFIG_BPF_LSM
> +
> +#define LSM_HOOK(RET, NAME, ...) RET bpf_lsm_##NAME(__VA_ARGS__);
> +#include <linux/lsm_hook_names.h>
> +#undef LSM_HOOK
> +
> +#define RUN_BPF_LSM_VOID_PROGS(FUNC, ...) bpf_lsm_##FUNC(__VA_ARGS__)
> +#define RUN_BPF_LSM_INT_PROGS(RC, FUNC, ...) ({ \
> + do { \
> + if (RC == 0) \
> + RC = bpf_lsm_##FUNC(__VA_ARGS__); \
> + } while (0); \
> + RC; \
> +})
> +
> +#else /* !CONFIG_BPF_LSM */
> +
> +#define RUN_BPF_LSM_INT_PROGS(RC, FUNC, ...) (RC)
> +#define RUN_BPF_LSM_VOID_PROGS(FUNC, ...)
> +
> +#endif /* CONFIG_BPF_LSM */
> +
> +#endif /* _LINUX_BPF_LSM_H */
> diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
> index affb6941622e..abc847c9b9a1 100644
> --- a/kernel/bpf/bpf_lsm.c
> +++ b/kernel/bpf/bpf_lsm.c
> @@ -7,6 +7,22 @@
> #include <linux/filter.h>
> #include <linux/bpf.h>
> #include <linux/btf.h>
> +#include <linux/bpf_lsm.h>
> +
> +/* For every LSM hook that allows attachment of BPF programs, declare a NOP
> + * function where a BPF program can be attached as an fexit trampoline.
> + */
> +#define LSM_HOOK(RET, NAME, ...) LSM_HOOK_##RET(NAME, __VA_ARGS__)
> +#define LSM_HOOK_int(NAME, ...) noinline int bpf_lsm_##NAME(__VA_ARGS__) \
> +{ \
> + return 0; \
> +}
> +
> +#define LSM_HOOK_void(NAME, ...) \
> + noinline void bpf_lsm_##NAME(__VA_ARGS__) {}
> +
> +#include <linux/lsm_hook_names.h>
> +#undef LSM_HOOK
>
> const struct bpf_prog_ops lsm_prog_ops = {
> };
> diff --git a/security/security.c b/security/security.c
> index 565bc9b67276..aa111392a700 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -28,6 +28,7 @@
> #include <linux/string.h>
> #include <linux/msg.h>
> #include <net/flow.h>
> +#include <linux/bpf_lsm.h>
>
> #define MAX_LSM_EVM_XATTR 2
>
> @@ -684,6 +685,7 @@ static void __init lsm_early_task(struct task_struct *task)
> \
> hlist_for_each_entry(P, &security_hook_heads.FUNC, list) \
> P->hook.FUNC(__VA_ARGS__); \
> + RUN_BPF_LSM_VOID_PROGS(FUNC, __VA_ARGS__); \
> } while (0)
>
> #define call_int_hook(FUNC, IRC, ...) ({ \
> @@ -696,6 +698,7 @@ static void __init lsm_early_task(struct task_struct *task)
> if (RC != 0) \
> break; \
> } \
> + RC = RUN_BPF_LSM_INT_PROGS(RC, FUNC, __VA_ARGS__); \
> } while (0); \
> RC; \
> })
^ permalink raw reply
* Re: [PATCH v26 10/22] x86/sgx: Linux Enclave Driver
From: Jordan Hand @ 2020-02-21 0:11 UTC (permalink / raw)
To: Jarkko Sakkinen, Sean Christopherson
Cc: 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: <20200220221607.GB26618@linux.intel.com>
On 2/20/20 2:16 PM, Jarkko Sakkinen wrote:
> On Thu, Feb 20, 2020 at 10:48:42AM -0800, Sean Christopherson wrote:
>> My biggest concern for allowing PROT_EXEC if RIE is that it would result
>> in #PF(SGX) (#GP on Skylake) due to an EPCM violation if the enclave
>> actually tried to execute from such a page. This isn't a problem for the
>> kernel as the fault will be reported cleanly through the vDSO (or get
>> delivered as a SIGSEGV if the enclave isn't entered through the vDSO), but
>> it's a bit weird for userspace as userspace will see the #PF(SGX) and
>> likely assume the EPC was lost, e.g. silently restart the enclave instead
>> of logging an error that the enclave is broken.
>
> I think right way to fix the current implementation is to -EACCES mmap()
> (and mprotect) when !!(current->personality & READ_IMPLIES_EXEC).
>
I agree. It still means userspace code with an executable stack can't
mmap/mprotect enclave pages and request PROT_READ but the check you've
proposed would more consistently enforce this which is easier to
understand from userspace perspective.
-Jordan
^ permalink raw reply
* Re: [PATCH v26 10/22] x86/sgx: Linux Enclave Driver
From: Andy Lutomirski @ 2020-02-21 0:32 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: Sean Christopherson, Jordan Hand, 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: <20200220221607.GB26618@linux.intel.com>
> On Feb 20, 2020, at 2:16 PM, Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
>
> On Thu, Feb 20, 2020 at 10:48:42AM -0800, Sean Christopherson wrote:
>> My biggest concern for allowing PROT_EXEC if RIE is that it would result
>> in #PF(SGX) (#GP on Skylake) due to an EPCM violation if the enclave
>> actually tried to execute from such a page. This isn't a problem for the
>> kernel as the fault will be reported cleanly through the vDSO (or get
>> delivered as a SIGSEGV if the enclave isn't entered through the vDSO), but
>> it's a bit weird for userspace as userspace will see the #PF(SGX) and
>> likely assume the EPC was lost, e.g. silently restart the enclave instead
>> of logging an error that the enclave is broken.
>
> I think right way to fix the current implementation is to -EACCES mmap()
> (and mprotect) when !!(current->personality & READ_IMPLIES_EXEC).
>
> This way supporting RIE can be reconsidered later on without any
> potential ABI bottlenecks.
>
Sounds good to me. I see no credible reason why anyone would use RIE and SGX.
> /Jarkko
^ permalink raw reply
* Re: [PATCH bpf-next v4 5/8] bpf: lsm: Implement attach, detach and execution
From: Alexei Starovoitov @ 2020-02-21 2:17 UTC (permalink / raw)
To: KP Singh
Cc: linux-kernel, bpf, linux-security-module, Daniel Borkmann,
James Morris, Kees Cook, Jann Horn, David S. Miller,
Greg Kroah-Hartman
In-Reply-To: <20200220175250.10795-6-kpsingh@chromium.org>
On Thu, Feb 20, 2020 at 06:52:47PM +0100, KP Singh wrote:
> +
> + /* This is the first program to be attached to the LSM hook, the hook
> + * needs to be enabled.
> + */
> + if (prog->type == BPF_PROG_TYPE_LSM && tr->progs_cnt[kind] == 1)
> + err = bpf_lsm_set_enabled(prog->aux->attach_func_name, true);
> out:
> mutex_unlock(&tr->mutex);
> return err;
> @@ -336,7 +348,11 @@ int bpf_trampoline_unlink_prog(struct bpf_prog *prog)
> }
> hlist_del(&prog->aux->tramp_hlist);
> tr->progs_cnt[kind]--;
> - err = bpf_trampoline_update(prog->aux->trampoline);
> + err = bpf_trampoline_update(prog);
> +
> + /* There are no more LSM programs, the hook should be disabled */
> + if (prog->type == BPF_PROG_TYPE_LSM && tr->progs_cnt[kind] == 0)
> + err = bpf_lsm_set_enabled(prog->aux->attach_func_name, false);
Overall looks good, but I don't think above logic works.
Consider lsm being attached, then fexit, then lsm detached, then fexit detached.
Both are kind==fexit and static_key stays enabled.
^ permalink raw reply
* Re: [PATCH bpf-next v4 3/8] bpf: lsm: provide attachment points for BPF LSM programs
From: Alexei Starovoitov @ 2020-02-21 2:25 UTC (permalink / raw)
To: KP Singh
Cc: linux-kernel, bpf, linux-security-module, Daniel Borkmann,
James Morris, Kees Cook, Jann Horn, David S. Miller,
Greg Kroah-Hartman
In-Reply-To: <20200220175250.10795-4-kpsingh@chromium.org>
On Thu, Feb 20, 2020 at 06:52:45PM +0100, KP Singh wrote:
> From: KP Singh <kpsingh@google.com>
>
> The BPF LSM programs are implemented as fexit trampolines to avoid the
> overhead of retpolines. These programs cannot be attached to security_*
> wrappers as there are quite a few security_* functions that do more than
> just calling the LSM callbacks.
>
> This was discussed on the lists in:
>
> https://lore.kernel.org/bpf/20200123152440.28956-1-kpsingh@chromium.org/T/#m068becce588a0cdf01913f368a97aea4c62d8266
>
> Adding a NOP callback after all the static LSM callbacks are called has
> the following benefits:
>
> - The BPF programs run at the right stage of the security_* wrappers.
> - They run after all the static LSM hooks allowed the operation,
> therefore cannot allow an action that was already denied.
>
> There are some hooks which do not call call_int_hooks or
> call_void_hooks. It's not possible to call the bpf_lsm_* functions
> without checking if there is BPF LSM program attached to these hooks.
> This is added further in a subsequent patch. For now, these hooks are
> marked as NO_BPF (i.e. attachment of BPF programs is not possible).
the commit log doesn't match the code.
> +
> +/* For every LSM hook that allows attachment of BPF programs, declare a NOP
> + * function where a BPF program can be attached as an fexit trampoline.
> + */
> +#define LSM_HOOK(RET, NAME, ...) LSM_HOOK_##RET(NAME, __VA_ARGS__)
> +#define LSM_HOOK_int(NAME, ...) noinline int bpf_lsm_##NAME(__VA_ARGS__) \
Did you check generated asm?
I think I saw cases when gcc ignored 'noinline' when function is defined in the
same file and still performed inlining while keeping the function body.
To be safe I think __weak is necessary. That will guarantee noinline.
And please reduce your cc next time. It's way too long.
^ permalink raw reply
* [PATCH -next] security: remove duplicated include from security.h
From: YueHaibing @ 2020-02-21 7:43 UTC (permalink / raw)
To: dhowells, jmorris
Cc: casey, sds, zohar, linux-kernel, linux-security-module,
YueHaibing
Remove duplicated include.
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
include/linux/security.h | 1 -
1 file changed, 1 deletion(-)
diff --git a/include/linux/security.h b/include/linux/security.h
index 910a1ef..fe2c566 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -30,7 +30,6 @@
#include <linux/err.h>
#include <linux/string.h>
#include <linux/mm.h>
-#include <linux/fs.h>
struct linux_binprm;
struct cred;
--
2.7.4
^ permalink raw reply related
* [PATCH net] ipv4: ensure rcu_read_lock() in cipso_v4_error()
From: Matteo Croce @ 2020-02-21 11:28 UTC (permalink / raw)
To: netdev, linux-security-module
Cc: linux-kernel, Paul Moore, David S. Miller, Alexey Kuznetsov,
Hideaki YOSHIFUJI, Jakub Kicinski, Guillaume Nault, Eric Dumazet
Similarly to commit c543cb4a5f07 ("ipv4: ensure rcu_read_lock() in
ipv4_link_failure()"), __ip_options_compile() must be called under rcu
protection.
Fixes: 3da1ed7ac398 ("net: avoid use IPCB in cipso_v4_error")
Suggested-by: Guillaume Nault <gnault@redhat.com>
Signed-off-by: Matteo Croce <mcroce@redhat.com>
---
net/ipv4/cipso_ipv4.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
index 376882215919..0bd10a1f477f 100644
--- a/net/ipv4/cipso_ipv4.c
+++ b/net/ipv4/cipso_ipv4.c
@@ -1724,6 +1724,7 @@ void cipso_v4_error(struct sk_buff *skb, int error, u32 gateway)
{
unsigned char optbuf[sizeof(struct ip_options) + 40];
struct ip_options *opt = (struct ip_options *)optbuf;
+ int res;
if (ip_hdr(skb)->protocol == IPPROTO_ICMP || error != -EACCES)
return;
@@ -1735,7 +1736,11 @@ void cipso_v4_error(struct sk_buff *skb, int error, u32 gateway)
memset(opt, 0, sizeof(struct ip_options));
opt->optlen = ip_hdr(skb)->ihl*4 - sizeof(struct iphdr);
- if (__ip_options_compile(dev_net(skb->dev), opt, skb, NULL))
+ rcu_read_lock();
+ res = __ip_options_compile(dev_net(skb->dev), opt, skb, NULL);
+ rcu_read_unlock();
+
+ if (res)
return;
if (gateway)
--
2.24.1
^ permalink raw reply related
* Re: [PATCH bpf-next v4 3/8] bpf: lsm: provide attachment points for BPF LSM programs
From: KP Singh @ 2020-02-21 11:44 UTC (permalink / raw)
To: Casey Schaufler; +Cc: KP Singh, LKML, Linux Security Module list, bpf
In-Reply-To: <0ef26943-9619-3736-4452-fec536a8d169@schaufler-ca.com>
On 20-Feb 15:49, Casey Schaufler wrote:
> On 2/20/2020 9:52 AM, KP Singh wrote:
> > From: KP Singh <kpsingh@google.com>
>
> Sorry about the heavy list pruning - the original set
> blows thunderbird up.
>
> >
> > The BPF LSM programs are implemented as fexit trampolines to avoid the
> > overhead of retpolines. These programs cannot be attached to security_*
> > wrappers as there are quite a few security_* functions that do more than
> > just calling the LSM callbacks.
> >
> > This was discussed on the lists in:
> >
> > https://lore.kernel.org/bpf/20200123152440.28956-1-kpsingh@chromium.org/T/#m068becce588a0cdf01913f368a97aea4c62d8266
> >
> > Adding a NOP callback after all the static LSM callbacks are called has
> > the following benefits:
> >
> > - The BPF programs run at the right stage of the security_* wrappers.
> > - They run after all the static LSM hooks allowed the operation,
> > therefore cannot allow an action that was already denied.
>
> I still say that the special call-out to BPF is unnecessary.
> I remain unconvinced by the arguments. You aren't doing anything
> so special that the general mechanism won't work.
The existing mechanism would work functionally, but the cost of an
indirect call for all the hooks, even those that are completely unused
is not really acceptable for KRSI’s use cases. It’s easy to avoid and
I do think that what we’re doing here (with hooks being defined at
runtime) has significant functional differences from existing LSMs.
- KP
>
> >
> > There are some hooks which do not call call_int_hooks or
> > call_void_hooks. It's not possible to call the bpf_lsm_* functions
> > without checking if there is BPF LSM program attached to these hooks.
> > This is added further in a subsequent patch. For now, these hooks are
> > marked as NO_BPF (i.e. attachment of BPF programs is not possible).
> >
[...]
^ permalink raw reply
* Re: [PATCH bpf-next v4 3/8] bpf: lsm: provide attachment points for BPF LSM programs
From: KP Singh @ 2020-02-21 11:47 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: KP Singh, linux-kernel, bpf, linux-security-module,
Daniel Borkmann, James Morris, Kees Cook, Jann Horn,
David S. Miller, Greg Kroah-Hartman
In-Reply-To: <20200221022537.wbmhdfkdbfvw2pww@ast-mbp>
On 20-Feb 18:25, Alexei Starovoitov wrote:
> On Thu, Feb 20, 2020 at 06:52:45PM +0100, KP Singh wrote:
> > From: KP Singh <kpsingh@google.com>
> >
> > The BPF LSM programs are implemented as fexit trampolines to avoid the
> > overhead of retpolines. These programs cannot be attached to security_*
> > wrappers as there are quite a few security_* functions that do more than
> > just calling the LSM callbacks.
> >
> > This was discussed on the lists in:
> >
> > https://lore.kernel.org/bpf/20200123152440.28956-1-kpsingh@chromium.org/T/#m068becce588a0cdf01913f368a97aea4c62d8266
> >
> > Adding a NOP callback after all the static LSM callbacks are called has
> > the following benefits:
> >
> > - The BPF programs run at the right stage of the security_* wrappers.
> > - They run after all the static LSM hooks allowed the operation,
> > therefore cannot allow an action that was already denied.
> >
> > There are some hooks which do not call call_int_hooks or
> > call_void_hooks. It's not possible to call the bpf_lsm_* functions
> > without checking if there is BPF LSM program attached to these hooks.
> > This is added further in a subsequent patch. For now, these hooks are
> > marked as NO_BPF (i.e. attachment of BPF programs is not possible).
>
> the commit log doesn't match the code.
Fixed. Thanks!
>
> > +
> > +/* For every LSM hook that allows attachment of BPF programs, declare a NOP
> > + * function where a BPF program can be attached as an fexit trampoline.
> > + */
> > +#define LSM_HOOK(RET, NAME, ...) LSM_HOOK_##RET(NAME, __VA_ARGS__)
> > +#define LSM_HOOK_int(NAME, ...) noinline int bpf_lsm_##NAME(__VA_ARGS__) \
>
> Did you check generated asm?
> I think I saw cases when gcc ignored 'noinline' when function is defined in the
> same file and still performed inlining while keeping the function body.
> To be safe I think __weak is necessary. That will guarantee noinline.
Sure, will change it to __weak.
>
> And please reduce your cc next time. It's way too long.
Will do.
- KP
^ permalink raw reply
* Re: [PATCH bpf-next v4 5/8] bpf: lsm: Implement attach, detach and execution
From: KP Singh @ 2020-02-21 12:02 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: KP Singh, open list, bpf, Linux Security Module list,
Daniel Borkmann, James Morris, Kees Cook, Jann Horn,
David S. Miller, Greg Kroah-Hartman
In-Reply-To: <20200221021755.3z7ifyyeh6seo3zs@ast-mbp>
On 20-Feb 18:17, Alexei Starovoitov wrote:
> On Thu, Feb 20, 2020 at 06:52:47PM +0100, KP Singh wrote:
> > +
> > + /* This is the first program to be attached to the LSM hook, the hook
> > + * needs to be enabled.
> > + */
> > + if (prog->type == BPF_PROG_TYPE_LSM && tr->progs_cnt[kind] == 1)
> > + err = bpf_lsm_set_enabled(prog->aux->attach_func_name, true);
> > out:
> > mutex_unlock(&tr->mutex);
> > return err;
> > @@ -336,7 +348,11 @@ int bpf_trampoline_unlink_prog(struct bpf_prog *prog)
> > }
> > hlist_del(&prog->aux->tramp_hlist);
> > tr->progs_cnt[kind]--;
> > - err = bpf_trampoline_update(prog->aux->trampoline);
> > + err = bpf_trampoline_update(prog);
> > +
> > + /* There are no more LSM programs, the hook should be disabled */
> > + if (prog->type == BPF_PROG_TYPE_LSM && tr->progs_cnt[kind] == 0)
> > + err = bpf_lsm_set_enabled(prog->aux->attach_func_name, false);
>
> Overall looks good, but I don't think above logic works.
> Consider lsm being attached, then fexit, then lsm detached, then fexit detached.
> Both are kind==fexit and static_key stays enabled.
You're right. I was weary of introducing a new kind (something like
BPF_TRAMP_LSM) since they are just fexit trampolines. For now, I
added nr_lsm_progs as a member in struct bpf_trampoline and refactored
the increment and decrement logic into inline helper functions e.g.
static inline void bpf_trampoline_dec_progs(struct bpf_prog *prog,
enum bpf_tramp_prog_type kind)
{
struct bpf_trampoline *tr = prog->aux->trampoline;
if (prog->type == BPF_PROG_TYPE_LSM)
tr->nr_lsm_progs--;
tr->progs_cnt[kind]--;
}
and doing the check as:
if (prog->type == BPF_PROG_TYPE_LSM && tr->nr_lsm_progs == 0)
err = bpf_lsm_set_enabled(prog->aux->attach_func_name, false);
This should work, If you're okay with it, I will update it in the next
revision of the patch-set.
- KP
^ 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