lkml.org 
[lkml]   [2012]   [Jul]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] proc: do not allow negative offsets on /proc/<pid>/environ
    Hi Djalal,

    On 07/23, Djalal Harouni wrote:
    >
    > Hi Oleg,
    >
    > On Sun, Jul 22, 2012 at 10:00:49PM +0200, Oleg Nesterov wrote:
    > >
    > > Probablt the patch makes sense, but I can't understand the changelog...
    > >
    > > > Allowing negative offsets on /proc/<pid>/environ can turn it to act like
    > > > /proc/<pid>/mem. A negative offset will pass the
    > > > fs/read_write.c:lseek_execute() and the environ_read() checks and will
    > > > point to another VMA.
    > >
    > > which VMA?
    > It depends on the offset. Please see below.
    >
    > > environ_read() can only read the memory from [env_start, env_end], and
    > > it should check *ppos anyway to ensure it doesn't read something else.
    > Yes I agree, but currently that's not the case, there are no checks on *ppos.
    ^^^^^^^^^^^^^^^^^^^
    There is, unless I missed something, just it is buggy, no?

    > So if you pass a negative offset you will be able to read from an arbitrary
    > address.
    >
    > [...snip...]
    >
    > inside environ_read() there is only a one check:
    >
    > int this_len = mm->env_end - (mm->env_start + src);
    >
    > if (this_len <= 0)
    > break;
    >
    >
    > Here 'src' is 'src = *ppos' the negative offset converted to unsigned long
    > and (mm->env_start + src) can overflow and point to another VMA.
    >
    > int this_len = mm->env_end - (mm->env_start + src)
    >
    > 'this_len' will be positive and we pass that check.

    OK, thanks, but doesn't this mean that this check should be fixed
    to avoid the overflow, no matter what *ppos is?

    With or without FMODE_UNSIGNED_OFFSET change. And perhaps it is
    possible to trigger the overflow even with the positive *ppos,
    because:

    > I also don't like the truncation of the result to 'int this_len'

    Yes.

    > BTW should I resend the patch with a better changelog entry ?

    Up to you, but I think this makes sense ;)

    > I'll also add another patch to check the offsets inside environ_read().

    Yes, agreed, but please see above.

    Please correct me, but afaics this patch should come 1st and fix the bug.
    FMODE_UNSIGNED_OFFSET change can be considered as a cleanup after that.

    What do you think?

    Oleg.



    \
     
     \ /
      Last update: 2012-07-23 18:21    [W:0.025 / U:120.888 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site