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

[Stephen Brennan]

Stephen Smalley <stephen.smalley.work@xxxxxxxxx> writes:

> On Fri, Dec 18, 2020 at 7:06 PM Stephen Brennan
> <stephen.s.brennan@xxxxxxxxxx> 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 <stephen.s.brennan@xxxxxxxxxx>
>
> 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)