Re: [patch 050/160] prctl: add PR_SET_PROCTITLE_AREA option for prctl()

[Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

On 03/07, Bryan Donlan wrote:
>
> On Sun, Mar 7, 2010 at 14:28, Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > On 03/06, Linus Torvalds wrote:
> >>
> >> On Fri, 5 Mar 2010, akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org wrote:
> >> >
> >> > 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.
> >>
> >> This looks overly complicated. Why do you change the whole locking rules,
> >> instead of protecting _only_ the "arg_start/arg_end" case?
> >>
> >> The thing is, there's no reason to hold the mmap_sem over the whole thing,
> >> and I don't think this is important enough to be a valid reason for
> >> exposing a whole new "locked" access variant, when a simple "protect just
> >> the arg_start/end" would handle it.
> >
> > It was me who suggested to re-use mm->mmap_sem instead of adding the new
> > lock, but looking at this patch again I do not understand the reason this
> > lock should be held throughout in proc_pid_cmdline(). If there is no such
> > a reason, then the new access_process_vm_locked (cough, also suggested by
> > me ;) is not needed.
>
> Consider:
>
> Process A is reading /proc/pidB/cmdline. As it enters
> proc_pid_cmdline, the compiler stashes mm->arg_end and mm->arg_start
> in a register or something, and then the process is preempted.
> Process B now changes the location of its cmdline buffer. After
> changing it, it free()s the buffer. Then it malloc()s a buffer for
> sensitive data. Unfortunately, this happens to be at the same address.

Indeed, I see, thanks.

Oleg.