lkml.org 
[lkml]   [2009]   [Nov]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH] Allow threads to rename siblings via /proc/pid/tasks/tid/comm
    Date
    (cc to linux-security-module and James)

    > On Mon, 16 Nov 2009 13:11:07 -0800
    > john stultz <johnstul@us.ibm.com> wrote:
    >
    > > Setting a thread's comm to be something unique is a very useful ability
    > > and is helpful for debugging complicated threaded applications. However
    > > currently the only way to set a thread name is for the thread to name
    > > itself via the PR_SET_NAME prctl.
    > >
    > > However, there may be situations where it would be advantageous for a
    > > thread dispatcher to be naming the threads its managing, rather then
    > > having the threads self-describe themselves. This sort of behavior is
    > > available on other systems via the pthread_setname_np() interface.
    > >
    > > This patch exports a task's comm via proc/pid/comm and
    > > proc/pid/task/tid/comm interfaces, and allows thread siblings to write
    > > to these values.
    > >
    >
    > Would be nice to document the new userspace interface.
    > Documentation/filesystems/proc.txt, perhaps.
    >
    > >
    > > diff --git a/fs/exec.c b/fs/exec.c
    > > index d49be6b..90003f8 100644
    > > --- a/fs/exec.c
    > > +++ b/fs/exec.c
    > > @@ -926,6 +926,15 @@ char *get_task_comm(char *buf, struct task_struct *tsk)
    > > void set_task_comm(struct task_struct *tsk, char *buf)
    > > {
    > > task_lock(tsk);
    > > +
    > > + /*
    > > + * Threads may access current->comm without holding
    > > + * the task lock, so write the string carefully.
    > > + * Readers without a lock may see incomplete new
    > > + * names but are safe from non-terminating string reads.
    > > + */
    > > + memset(tsk->comm, 0, TASK_COMM_LEN);
    > > + wmb();
    >
    > OK.

    Hmm, I don't like mix TASK_COMM_LEN and sizeof(tsk->comm).
    John, Is there any reason?

    >
    > > strlcpy(tsk->comm, buf, sizeof(tsk->comm));
    > > task_unlock(tsk);
    > > perf_event_comm(tsk);
    > > diff --git a/fs/proc/base.c b/fs/proc/base.c
    > > index 837469a..7f59af1 100644
    > > --- a/fs/proc/base.c
    > > +++ b/fs/proc/base.c
    > > @@ -1265,6 +1265,78 @@ static const struct file_operations proc_pid_sched_operations = {
    > >
    > > #endif
    > >
    > > +
    > > +
    > > +static ssize_t
    > > +comm_write(struct file *file, const char __user *buf,
    > > + size_t count, loff_t *offset)
    > > +{
    > > + struct inode *inode = file->f_path.dentry->d_inode;
    > > + struct task_struct *p;
    > > + char buffer[TASK_COMM_LEN];
    > > +
    > > + memset(buffer, 0, sizeof(buffer));
    > > + if (count > sizeof(buffer) - 1)
    > > + count = sizeof(buffer) - 1;
    >
    > Is this the best policy? If userspace tries to apply a too-long name
    > to a thread, the kernel will silently truncate (ie: corrupt) it? I'd
    > have thought that returning an error would be more robust?
    >
    > > + if (copy_from_user(buffer, buf, count))
    > > + return -EFAULT;
    > > +
    > > + p = get_proc_task(inode);
    > > + if (!p)
    > > + return -ESRCH;
    > > +
    > > + if (same_thread_group(current, p))
    > > + set_task_comm(p, buffer);
    > > + else
    > > + count = -EINVAL;
    > > +
    > > + put_task_struct(p);
    > > +
    > > + return count;
    > > +}
    >
    > Is same_thread_group() sufficient? Are any security/permission-related
    > checks appropriate here, for example?
    >
    > The restriction to a separate thread group seems a bit arbitrary,
    > really. There's no reason I can see why we cannot permit unrelated
    > (but suitably authorised) processes to do this.

    At least, currently /proc/pid/cmdline read the process stack. A stack
    can be rewritten without any security check by the same group thread.
    I guess we can't make consist check of task name change.

    Plus, now we don't have any LSM hook of task name change nor security
    capability. I guess all security module don't need task name.

    I hope security folks correct me if I misunderstood.


    > This patch makes task->comm inconsistent with /prod/pid/cmdline. What
    > are the implications of that for userspace? None, I guess, given that
    > this can already be done.

    ditto.
    Process stack isn't guarded now. we can't make reasonable protection.

    - kosaki

    >
    > > +
    > > +static int comm_show(struct seq_file *m, void *v)
    > > +{
    > > + struct inode *inode = m->private;
    > > + struct task_struct *p;
    > > +
    > > + p = get_proc_task(inode);
    > > + if (!p)
    > > + return -ESRCH;
    > > +
    > > + task_lock(p);
    > > + seq_printf(m, "%s\n", p->comm);
    > > + task_unlock(p);
    > > +
    > > + put_task_struct(p);
    > > +
    > > + return 0;
    > > +}
    > > +
    > > +static int comm_open(struct inode *inode, struct file *filp)
    > > +{
    > > + int ret;
    > > +
    > > + ret = single_open(filp, comm_show, NULL);
    > > + if (!ret) {
    > > + struct seq_file *m = filp->private_data;
    > > +
    > > + m->private = inode;
    > > + }
    > > + return ret;
    > > +}
    > > +
    > > +
    >
    > The patch has a seemingly-random inexplicable mixture of \n and \n\n.
    >
    > > +static const struct file_operations proc_pid_set_comm_operations = {
    > > + .open = comm_open,
    > > + .read = seq_read,
    > > + .write = comm_write,
    > > + .llseek = seq_lseek,
    > > + .release = single_release,
    > > +};
    > > +
    > > +
    > > /*
    > > * We added or removed a vma mapping the executable. The vmas are only mapped
    > > * during exec and are not mapped with the mmap system call.
    > > @@ -2504,6 +2576,7 @@ static const struct pid_entry tgid_base_stuff[] = {
    > > #ifdef CONFIG_SCHED_DEBUG
    > > REG("sched", S_IRUGO|S_IWUSR, proc_pid_sched_operations),
    > > #endif
    > > + REG("comm", S_IRUGO|S_IWUSR, proc_pid_set_comm_operations),
    > > #ifdef CONFIG_HAVE_ARCH_TRACEHOOK
    > > INF("syscall", S_IRUSR, proc_pid_syscall),
    > > #endif
    > > @@ -2839,6 +2912,7 @@ static const struct pid_entry tid_base_stuff[] = {
    > > #ifdef CONFIG_SCHED_DEBUG
    > > REG("sched", S_IRUGO|S_IWUSR, proc_pid_sched_operations),
    > > #endif
    > > + REG("comm", S_IRUGO|S_IWUSR, proc_pid_set_comm_operations),
    > > #ifdef CONFIG_HAVE_ARCH_TRACEHOOK
    > > INF("syscall", S_IRUSR, proc_pid_syscall),
    > > #endif
    >





    \
     
     \ /
      Last update: 2009-11-19 02:07    [W:6.035 / U:0.712 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site