lkml.org 
[lkml]   [2009]   [Oct]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [resend][PATCH] Added PR_SET_PROCTITLE_AREA option for prctl()
    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.
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2009-10-10 04:29    [W:0.028 / U:0.144 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site