On Fri, 9 Oct 2009 13:50:17 +0900 (JST) KOSAKI Motohiro <[email protected]> wrote:
> ok, all lkml reviewer ack this patch. > then, I've switched to linux-api review. > > Any comments? > > > > ================================================== > From: Timo Sirainen <[email protected]> > > Currently glibc2 doesn't have setproctitle(3), so several userland > daemons attempt to emulate it by doing some brutal stack modifications. > This works most of the time, but it has problems. For example: > > % ps -ef |grep avahi-daemon > avahi 1679 1 0 09:20 ? 00:00:00 avahi-daemon: running > [kosadesk.local] > > # cat /proc/1679/cmdline > avahi-daemon: running [kosadesk.local] > > This looks good, but the process has also overwritten its environment > area and made the environ file useless: > > # cat /proc/1679/environ > adesk.local] > > Another problem is that the process title length is limited by the size of > the environment. Security conscious people try to avoid potential information > leaks by clearing most of the environment before running a daemon: > > # env - MINIMUM_NEEDED_VAR=foo /path/to/daemon > > The resulting environment size may be too small to fit the wanted process > titles. > > This patch makes it possible for userspace to implement setproctitle() > cleanly. It adds a new PR_SET_PROCTITLE_AREA option for prctl(), which > updates task's mm_struct->arg_start and arg_end to the given area. > > test_setproctitle.c > ================================================ > #define ERR(str) (perror(str), exit(1)) > > void settitle(char* title){ > int err; > > err = prctl(34, title, strlen(title)+1); > if (err < 0) > ERR("prctl "); > } > > void main(void){ > long i; > char buf[1024]; > > for (i = 0; i < 10000000000LL; i++){ > sprintf(buf, "loooooooooooooooooooooooong string %d",i); > settitle(buf); > } > } gee that's a good changelog. > ... > > @@ -255,32 +255,47 @@ static int proc_pid_cmdline(struct task_struct *task, > char * buffer) > int res = 0; > unsigned int len; > struct mm_struct *mm = get_task_mm(task); > + unsigned seq; > + > if (!mm) > goto out; > + > + /* The process was not constructed yet? */ > if (!mm->arg_end) > - goto out_mm; /* Shh! No looking before we're done */ > + goto out_mm; > > - len = mm->arg_end - mm->arg_start; > - > - if (len > PAGE_SIZE) > - len = PAGE_SIZE; > - > - res = access_process_vm(task, mm->arg_start, buffer, len, 0); > - > - // If the nul at the end of args has been overwritten, then > - // assume application is using setproctitle(3). > - if (res > 0 && buffer[res-1] != '\0' && len < PAGE_SIZE) { > - len = strnlen(buffer, res); > - if (len < res) { > - res = len; > - } else { > - len = mm->env_end - mm->env_start; > - if (len > PAGE_SIZE - res) > - len = PAGE_SIZE - res; > - res += access_process_vm(task, mm->env_start, > buffer+res, len, 0); > + do { > + seq = read_seqbegin(&mm->arg_lock); > + > + len = mm->arg_end - mm->arg_start; > + if (len > PAGE_SIZE) > + len = PAGE_SIZE; > + > + res = access_process_vm(task, mm->arg_start, buffer, len, 0); > + > + if (mm->arg_end != mm->env_start) > + /* PR_SET_PROCTITLE_AREA used */ > res = strnlen(buffer, res); Hang on. If PR_SET_PROCTITLE_AREA installed a bad address then access_process_vm() will return -EFAULT and nothing was written to `buffer'? Also, concurrent PR_SET_PROCTITLE_AREA could cause arg_start and arg_end to have inconsistent/incoherent values. This might result in a fault but it also might result in a read of garbage userspace memory. I guess all of these issues could be written off as "userspace bugs", but our behaviour here isn't nice. We should at least return that -EFAULT!. > + else if (res > 0 && buffer[res-1] != '\0' && len < PAGE_SIZE) { > + /* > + * If the nul at the end of args has been overwritten, > + * then assume application is using sendmail's > + * SPT_REUSEARGV style argv override. > + */ > + len = strnlen(buffer, res); > + if (len < res) { > + res = len; > + } else { > + len = mm->env_end - mm->env_start; > + if (len > PAGE_SIZE - res) > + len = PAGE_SIZE - res; > + res += access_process_vm(task, mm->env_start, > + buffer+res, len, 0); > + res = strnlen(buffer, res); > + } And if access_process_vm() returns a -ve errno here, the code simply flies off into the weeds. > } > - } > + } while (read_seqretry(&mm->arg_lock, seq)); > + > out_mm: > mmput(mm); > > ... > > @@ -236,6 +237,7 @@ struct mm_struct { > unsigned long stack_vm, reserved_vm, def_flags, nr_ptes; > unsigned long start_code, end_code, start_data, end_data; > unsigned long start_brk, brk, start_stack; > + seqlock_t arg_lock; > unsigned long arg_start, arg_end, env_start, env_end; > > unsigned long saved_auxv[AT_VECTOR_SIZE]; /* for /proc/PID/auxv */ seqlocks are nice but I have to wonder if they made this patch unnecessarily complex. Couldn't we just do: PR_SET_PROCTITLE_AREA: spin_lock(mm->lock); mm->arg_start = addr; mm->arg_end = addr + len; spin_unlock(mm->lock); and proc_pid_cmdline() { unsigned long addr; unsigned long end; spin_lock(mm->lock); addr = mm->arg_start; end = mm->arg_end; spin_unlock(mm->lock); <use `addr' and `len'> } ? -- To unsubscribe from this list: send the line "unsubscribe linux-api" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
