Re: [resend][PATCH] Added PR_SET_PROCTITLE_AREA option for prctl()

[Bryan Donlan <bdonlan@gmail.com>]

On Fri, Oct 9, 2009 at 8:13 PM, Andrew Morton <akpm@linux-foundation.org> 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.