lkml.org 
[lkml]   [2013]   [Aug]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    Subject[PATCH v2 2/3] exec: kill "int depth" in search_binary_handler()
    On 08/03, Kees Cook wrote:
    >
    > On Thu, Aug 1, 2013 at 12:05 PM, Oleg Nesterov <oleg@redhat.com> wrote:
    > > Nobody except search_binary_handler() should touch ->recursion_depth,
    > > "int depth" buys nothing but complicates the code, kill it.
    >
    > I'd like to see a comment added to binfmts.h's recursion_depth field
    > that reminds people that recursion_depth is for
    > search_binary_handler()'s use only, and a binfmt loader shouldn't
    > touch it.

    And this comment probably makes sense even without this change

    > Besides that, yeah, sensible clean up.

    OK, thanks, please see v2. The only change is the comment in .h

    ------------------------------------------------------------------------------
    Subject: [PATCH 1/1] exec: kill "int depth" in search_binary_handler()

    Nobody except search_binary_handler() should touch ->recursion_depth,
    "int depth" buys nothing but complicates the code, kill it.

    Probably we should also kill "fn" and the !NULL check, ->load_binary
    should be always defined. And it can not go away after read_unlock()
    or this code is buggy anyway.

    v2: add the comment about linux_binprm->recursion_depth

    Signed-off-by: Oleg Nesterov <oleg@redhat.com>
    ---
    fs/exec.c | 9 ++++-----
    include/linux/binfmts.h | 2 +-
    2 files changed, 5 insertions(+), 6 deletions(-)

    diff --git a/fs/exec.c b/fs/exec.c
    index a9ae4f2..f32079c 100644
    --- a/fs/exec.c
    +++ b/fs/exec.c
    @@ -1370,12 +1370,11 @@ EXPORT_SYMBOL(remove_arg_zero);
    */
    int search_binary_handler(struct linux_binprm *bprm)
    {
    - unsigned int depth = bprm->recursion_depth;
    - int try,retval;
    + int try, retval;
    struct linux_binfmt *fmt;

    /* This allows 4 levels of binfmt rewrites before failing hard. */
    - if (depth > 5)
    + if (bprm->recursion_depth > 5)
    return -ELOOP;

    retval = security_bprm_check(bprm);
    @@ -1396,9 +1395,9 @@ int search_binary_handler(struct linux_binprm *bprm)
    if (!try_module_get(fmt->module))
    continue;
    read_unlock(&binfmt_lock);
    - bprm->recursion_depth = depth + 1;
    + bprm->recursion_depth++;
    retval = fn(bprm);
    - bprm->recursion_depth = depth;
    + bprm->recursion_depth--;
    if (retval >= 0) {
    put_binfmt(fmt);
    allow_write_access(bprm->file);
    diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
    index 70cf138..e8112ae 100644
    --- a/include/linux/binfmts.h
    +++ b/include/linux/binfmts.h
    @@ -31,7 +31,7 @@ struct linux_binprm {
    #ifdef __alpha__
    unsigned int taso:1;
    #endif
    - unsigned int recursion_depth;
    + unsigned int recursion_depth; /* only for search_binary_handler() */
    struct file * file;
    struct cred *cred; /* new credentials */
    int unsafe; /* how unsafe this exec is (mask of LSM_UNSAFE_*) */
    --
    1.5.5.1



    \
     
     \ /
      Last update: 2013-08-03 21:21    [W:2.609 / U:0.776 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site