[v3,2/2] proc: ensure security hook is called after exec

Commit Message

Stephen Brennan Dec. 19, 2020, 12:06 a.m. UTC

Smack needs its security_task_to_inode() hook to be called when a task
execs a new executable. Store the self_exec_id of the task and call the
hook via pid_update_inode() whenever the exec_id changes.

Signed-off-by: Stephen Brennan <[e-mail]>
---

As discussed on the v2 of the patch, this should allow Smack to receive a
security_task_to_inode() call only when the uid/gid changes, or when the task
execs a new binary. I have verified that this doesn't change the performance of
the patch set, and that we do fall out of RCU walk on tasks which have recently
exec'd.

 fs/proc/base.c     | 4 +++-
 fs/proc/internal.h | 5 ++++-
 2 files changed, 7 insertions(+), 2 deletions(-)

Comments

Stephen Smalley Jan. 4, 2021, 2:16 p.m. UTC | #1

On Fri, Dec 18, 2020 at 7:06 PM Stephen Brennan
<[e-mail]> wrote:
>
> Smack needs its security_task_to_inode() hook to be called when a task
> execs a new executable. Store the self_exec_id of the task and call the
> hook via pid_update_inode() whenever the exec_id changes.
>
> Signed-off-by: Stephen Brennan <[e-mail]>

Sorry to be late in responding, but the proc inode security structure
needs to be updated not only upon a context-changing exec but also
upon a setcon(3) aka write to /proc/self/attr/current just like the
uid/gid needs to be updated not only upon a setuid exec but also upon
a setuid(2).  I'm also unclear as to why you can't call
security_task_to_inode during RCU lookup; it doesn't block/sleep
AFAICT.
All it does is take a spinlock and update a few fields.

Stephen Smalley Jan. 4, 2021, 2:22 p.m. UTC | #2

On Mon, Jan 4, 2021 at 9:16 AM Stephen Smalley
<[e-mail]> wrote:
>
> On Fri, Dec 18, 2020 at 7:06 PM Stephen Brennan
> <[e-mail]> wrote:
> >
> > Smack needs its security_task_to_inode() hook to be called when a task
> > execs a new executable. Store the self_exec_id of the task and call the
> > hook via pid_update_inode() whenever the exec_id changes.
> >
> > Signed-off-by: Stephen Brennan <[e-mail]>
>
> Sorry to be late in responding, but the proc inode security structure
> needs to be updated not only upon a context-changing exec but also
> upon a setcon(3) aka write to /proc/self/attr/current just like the
> uid/gid needs to be updated not only upon a setuid exec but also upon
> a setuid(2).  I'm also unclear as to why you can't call
> security_task_to_inode during RCU lookup; it doesn't block/sleep
> AFAICT.
> All it does is take a spinlock and update a few fields.

You could also optimize this by comparing the sid similar to how the
uid/gid are compared and only updating it within the hook if it has
not yet been initialized or has changed since it was originally set.

Stephen Brennan Jan. 4, 2021, 7:51 p.m. UTC | #3

Stephen Smalley <[e-mail]> writes:

> On Fri, Dec 18, 2020 at 7:06 PM Stephen Brennan
> <[e-mail]> wrote:
>>
>> Smack needs its security_task_to_inode() hook to be called when a task
>> execs a new executable. Store the self_exec_id of the task and call the
>> hook via pid_update_inode() whenever the exec_id changes.
>>
>> Signed-off-by: Stephen Brennan <[e-mail]>
>
> Sorry to be late in responding, but the proc inode security structure
> needs to be updated not only upon a context-changing exec but also
> upon a setcon(3) aka write to /proc/self/attr/current just like the
> uid/gid needs to be updated not only upon a setuid exec but also upon
> a setuid(2).  I'm also unclear as to why you can't call
> security_task_to_inode during RCU lookup; it doesn't block/sleep
> AFAICT.
> All it does is take a spinlock and update a few fields.

The reason I assumed that we need to drop out of RCU mode to update the
inode and call the security hooks was simply because that is how the
code worked originally. I wanted to be conservative in my changes, by
only leaving RCU mode "when necessary", but this assumed that it was
necessary to leave RCU mode at all!

None of the data in a proc inode (at least, i_mode, i_uid, i_gid) seems
to be "RCU protected" in the sense that they could not be modified
during an RCU read critical section. If this were the case, then there
would have to be some sort of copying and a synchronize_rcu() used
somewhere.  So it seems that running pid_update_inode() (which does not
sleep and simply takes some spinlocks) should be safe during RCU mode.

My assumption had originally been that the security_pid_to_inode() calls
could be liable to sleep. But during this review we've seen that both
the selinux and smack security_pid_to_inode() implementations are also
"RCU safe" in that they do not sleep.

So rather than trying to guess when this security hook would like to be
called, it seems that it would be safe to take the easiest option: just
execute pid_revalidate() in RCU mode always, for instance with the
example changes below. Is there anything obviously wrong with this
approach that I'm missing?

diff --git a/fs/proc/base.c b/fs/proc/base.c
index ebea9501afb8..105581e51032 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -1830,19 +1830,18 @@ static int pid_revalidate(struct dentry *dentry, unsigned int flags)
 {
 	struct inode *inode;
 	struct task_struct *task;
+	int rv = 0;
 
-	if (flags & LOOKUP_RCU)
-		return -ECHILD;
-
-	inode = d_inode(dentry);
-	task = get_proc_task(inode);
+	rcu_read_lock();
+	inode = d_inode_rcu(dentry);
+	task = pid_task(proc_pid(inode), PIDTYPE_PID);
 
 	if (task) {
 		pid_update_inode(task, inode);
-		put_task_struct(task);
-		return 1;
+		rv = 1;
 	}
-	return 0;
+	rcu_read_unlock();
+	return rv;
 }
 
 static inline bool proc_inode_is_dead(struct inode *inode)

Patch

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 4b246e9bd5df..ad59e92e8433 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -1917,6 +1917,7 @@  struct inode *proc_pid_make_inode(struct super_block * sb,
 	}
 
 	task_dump_owner(task, 0, &inode->i_uid, &inode->i_gid);
+	ei->exec_id = task->self_exec_id;
 	security_task_to_inode(task, inode);
 
 out:
@@ -1965,6 +1966,7 @@  void pid_update_inode(struct task_struct *task, struct inode *inode)
 	task_dump_owner(task, inode->i_mode, &inode->i_uid, &inode->i_gid);
 
 	inode->i_mode &= ~(S_ISUID | S_ISGID);
+	PROC_I(inode)->exec_id = task->self_exec_id;
 	security_task_to_inode(task, inode);
 }
 
@@ -1979,7 +1981,7 @@  static bool pid_inode_needs_update(struct task_struct *task, struct inode *inode
 	task_dump_owner(task, inode->i_mode, &uid, &gid);
 	if (!uid_eq(uid, inode->i_uid) || !gid_eq(gid, inode->i_gid))
 		return true;
-	return false;
+	return task->self_exec_id != PROC_I(inode)->exec_id;
 }
 
 /*
diff --git a/fs/proc/internal.h b/fs/proc/internal.h
index f60b379dcdc7..1df9b039dfc3 100644
--- a/fs/proc/internal.h
+++ b/fs/proc/internal.h
@@ -92,7 +92,10 @@  union proc_op {
 
 struct proc_inode {
 	struct pid *pid;
-	unsigned int fd;
+	union {
+		unsigned int fd;
+		u32 exec_id;
+	};
 	union proc_op op;
 	struct proc_dir_entry *pde;
 	struct ctl_table_header *sysctl;