lkml.org 
[lkml]   [2012]   [Aug]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    Subject[NAK] Re: [PATCHv3 1/4] fs: Move core dump functionality into its own file
    Hi Alex,

    Overall, this seems like a fine idea. However, I think the code move
    has gone very badly. Comments are mismatched, there are typos added,
    formatting changed, case changed, trailing white space added, and
    punctuation dropped.

    NAKed-by: Kees Cook <keescook@chromium.org>

    On Sun, Aug 05, 2012 at 04:18:38AM -0700, Alex Kelly wrote:
    > +/* The maximal length of core_pattern is also specified in sysctl.c */
    > +static int zap_process(struct task_struct *start, int exit_code)

    These two lines have nothing to do with each other. This comment belongs
    with "char core_pattern[CORENAME_MAX_SIZE] = "core";", IIUC.

    Why is zap_process not with the zap_threads helpers?

    > +void do_coredump(long signr, int exit_code, struct pt_regs *regs)
    > +[...]
    > + * so we dump it as root in mode 2, and onlt into a controlled

    "onlt"? The removed code didn't have this typo. Was the moved code
    not cut/pasted? That got my attention. In fact, I went and compared
    all the code that was moved, and this was not the only change -- what
    happened here?

    --- /tmp/del.code 2012-08-05 11:32:36.602881193 -0700
    +++ /tmp/add.code 2012-08-05 11:33:06.615270285 -0700
    @@ -327,7 +328,7 @@
    core_waiters = zap_threads(tsk, mm, core_state, exit_code);
    up_write(&mm->mmap_sem);

    - if (core_waiters > 0) {
    + if (core_waiters > 0){
    struct core_thread *ptr;

    wait_for_completion(&core_state->startup);
    @@ -466,8 +467,8 @@
    /*
    * We cannot trust fsuid as being the "true" uid of the process
    * nor do we know its entire history. We only know it was tainted
    - * so we dump it as root in mode 2, and only into a controlled
    - * environment (pipe handler or fully qualified path).
    + * so we dump it as root in mode 2, and onlt into a controlled
    + * environment (pipe handler or fully qualified path)
    */
    if (__get_dumpable(cprm.mm_flags) == SUID_DUMPABLE_SAFE) {
    /* Setuid core dump mode */
    @@ -505,11 +506,11 @@
    *
    * Normally core limits are irrelevant to pipes, since
    * we're not writing to the file system, but we use
    - * cprm.limit of 1 here as a speacial value, this is a
    + * cprm.limit of 1 here as a speacial value, this is a
    * consistent way to catch recursive crashes.
    * We can still crash if the core_pattern binary sets
    * RLIM_CORE = !1, but it runs as root, and can do
    - * lots of stupid things.
    + * lots of stupid things
    *
    * Note that we use task_tgid_vnr here to grab the pid
    * of the process group leader. That way we get the
    @@ -556,7 +557,7 @@

    if (need_nonrelative && cn.corename[0] != '/') {
    printk(KERN_WARNING "Pid %d(%s) can only dump core "\
    - "to fully qualified path!\n",
    + "To fully qualified path!\n",
    task_tgid_vnr(current), current->comm);
    printk(KERN_WARNING "Skipping core dump\n");
    goto fail_unlock;

    -Kees

    --
    Kees Cook @outflux.net


    \
     
     \ /
      Last update: 2012-08-05 21:21    [W:0.026 / U:123.084 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site