On Fri, Oct 9, 2009 at 8:13 PM, Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote: >> + 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!. access_process_vm() does not return an error code - just the number of bytes successfully transferred. If there's a fault, we just get 0 (or some intermediate value) back (as discussed on lkml). >> + 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. This code was originally there - it's just been lifted into an if :) and since access_process_vm will never return a negative errno, it's not a problem. Now, there might be a case for arguing that we should try harder to get an error code (-EIO if we don't read the number of bytes we asked for), but /proc/pid/cmdline never has before, and that would then make this a visible change for consumers of /proc/pid/cmdline. Can ps handle reading cmdline returning -EIO? > 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'> > } > > ? As discussed on the lkml thread, this opens up a nasty race: Process A: calls PR_SET_PROCTITLE_AREA Process B: read cmdline: Process B: spin_lock Process B: read mm->arg_* Process B: unlock Process A: mm->arg_* = ... Process A: free(old_cmdline_area) Process A: secret_password = malloc(...) Process A: strcpy(secret_password, stuff); Process B: access_process_vm If secret_password == arg_start, then process B just read secret information from process A's address space. Since process A has no idea when or even if process B will complete its read, it has no way of protecting itself from this, save from never reusing any memory which has ever been assigned to a proctitle area. The solution is to use the seqlock to detect this, and prevent the secret information from ever making it back to process B's userspace. Note that it's not enough to just recheck arg_start, as process A may reassign the proctitle area back to its original position after having it somewhere else for a while. -- To unsubscribe from this list: send the line "unsubscribe linux-api" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html