Al Viro wrote:
On Wed, Sep 14, 2005 at 07:31:12PM -0500, Sripathi Kodi wrote:
I can move this code from proc_root_link() to proc_check_root(), but it
will still not be completely limited to ->permission() path. I can create a
separate ->permission() for proc_task_inode_operations, and have this
additional code there. If I do that, I think I will have to duplicate much
of proc_check_root(). Or else, I will have to split proc_check_root() into
two functions to prevent code duplication. Please let me know if any of
these makes sense, and I will send another patch.
The last variant would be preferable if we go in that direction...
Al,
I have done the following changes in this new patch:
1) Changed proc_permission to call new function proc_task_check_root instead
of proc_check_root. Other paths that call proc_check_root are not affected.
2) proc_task_check_root calls a new function proc_task_root_link to get fs.
This is same as proc_root_link, but additionally tries to get fs from one of
the other threads of the group when the main thread doesn't have it.
3) Moved the code that checks chroot boundary from proc_root_link to a new
function proc_check_chroot. Both proc_check_root and proc_task_check_root
call this function. This avoids duplication of this code.
With these changes, only ->permission path will encounter the new kludge code.
proc_root_link and proc_task_root_link still have some duplicated code. I
could have split these functions further to avoid duplication completely,
but that would move incrementing and decrementing fs->lock to two different
functions, which I think will be confusing.
The other way of implementing this that I could think of was to have a flag
to indicate that the call is from ->permission path and pass it all along.
This will avoid having to change many existing functions, but it will defeat
the purpose of limiting this kludge code to ->permission path.
Please let me know how it is looking now.
Further, about actual permission checks that we are doing, can we say: "A
process should be able to see /proc/<pid>/task/* of another process only if
they both belong to same uid or reader is root"? But any such change will
change the behavior of commands like 'ps', right?
Thanks,
Sripathi.
Signed-off-by: Sripathi Kodi <[email protected]>
--- linux-2.6.13.1-orig/fs/proc/base.c 2005-09-09 21:42:58.000000000 -0500
+++ linux-2.6.13.1/fs/proc/base.c 2005-09-16 00:57:31.000000000 -0500
@@ -291,6 +291,52 @@ static int proc_root_link(struct inode *
return result;
}
+
+/* Same as proc_root_link, but this addionally tries to get fs from other
+ * threads in the group */
+static int proc_task_root_link(struct inode *inode, struct dentry **dentry,
struct vfsmount **mnt)
+{
+ struct fs_struct *fs;
+ int result = -ENOENT;
+ struct task_struct *leader = proc_task(inode);
+
+ task_lock(leader);
+ fs = leader->fs;
+ if (fs) {
+ atomic_inc(&fs->count);
+ task_unlock(leader);
+ } else {
+ /* Try to get fs from other threads */
+ task_unlock(leader);
+ struct task_struct *task = leader;
+ read_lock(&tasklist_lock);
+ if (pid_alive(task)) {
+ while ((task = next_thread(task)) != leader) {
+ task_lock(task);
+ fs = task->fs;
+ if (fs) {
+ atomic_inc(&fs->count);
+ task_unlock(task);
+ break;
+ }
+ task_unlock(task);
+ }
+ }
+ read_unlock(&tasklist_lock);
+ }
+
+ if (fs) {
+ read_lock(&fs->lock);
+ *mnt = mntget(fs->rootmnt);
+ *dentry = dget(fs->root);
+ read_unlock(&fs->lock);
+ result = 0;
+ put_fs_struct(fs);
+ }
+ return result;
+}
+
+
#define MAY_PTRACE(task) \
(task == current || \
(task->parent == current && \
@@ -449,14 +495,14 @@ static int proc_oom_score(struct task_st
/* permission checks */
-static int proc_check_root(struct inode *inode)
+/* If the process being read is separated by chroot from the reading process,
+ * don't let the reader access the threads.
+ */
+static int proc_check_chroot(struct dentry *root, struct vfsmount *vfsmnt)
{
- struct dentry *de, *base, *root;
- struct vfsmount *our_vfsmnt, *vfsmnt, *mnt;
+ struct dentry *de, *base;
+ struct vfsmount *our_vfsmnt, *mnt;
int res = 0;
-
- if (proc_root_link(inode, &root, &vfsmnt)) /* Ewww... */
- return -ENOENT;
read_lock(¤t->fs->lock);
our_vfsmnt = mntget(current->fs->rootmnt);
base = dget(current->fs->root);
@@ -489,11 +535,33 @@ out:
goto exit;
}
+static int proc_check_root(struct inode *inode)
+{
+ struct dentry *root;
+ struct vfsmount *vfsmnt;
+
+ if (proc_root_link(inode, &root, &vfsmnt)) /* Ewww... */
+ return -ENOENT;
+
+ return proc_check_chroot(root, vfsmnt);
+}
+
+static int proc_task_check_root(struct inode *inode)
+{
+ struct dentry *root;
+ struct vfsmount *vfsmnt;
+
+ if (proc_task_root_link(inode, &root, &vfsmnt)) /* Ewww... */
+ return -ENOENT;
+
+ return proc_check_chroot(root, vfsmnt);
+}
+
static int proc_permission(struct inode *inode, int mask, struct nameidata
*nd)
{
if (generic_permission(inode, mask, NULL) != 0)
return -EACCES;
- return proc_check_root(inode);
+ return proc_task_check_root(inode);
}
extern struct seq_operations proc_pid_maps_op;
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
| |
 |