lkml.org 
[lkml]   [2006]   [Aug]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH] binfmt_elf.c : the BAD_ADDR macro again
    Date
    From
    On Sunday, 20-Aug-2006 at 18:23 +0200, Willy Tarreau wrote:

    > On Sun, Aug 20, 2006 at 07:51:22PM +0400, Solar Designer wrote:
    > > On Sun, Aug 20, 2006 at 11:15:15AM +0200, Willy Tarreau wrote:
    > > > The proper fix would then be :
    > > [...]
    > > > -#define BAD_ADDR(x) ((unsigned long)(x) > TASK_SIZE)
    > > > +#define BAD_ADDR(x) ((unsigned long)(x) >= TASK_SIZE)
    > > [...]
    > > > - if (k > TASK_SIZE || eppnt->p_filesz > eppnt->p_memsz ||
    > > > + if (BAD_ADDR(k) || eppnt->p_filesz > eppnt->p_memsz ||
    > > [...]
    > > > - if (k > TASK_SIZE || elf_ppnt->p_filesz > elf_ppnt->p_memsz ||
    > > > + if (BAD_ADDR(k) || elf_ppnt->p_filesz > elf_ppnt->p_memsz ||
    > >
    > > Looks OK to me.

    These are all correct.



    > > > And even then, I'm not happy with this test :
    > > >
    > > > TASK_SIZE - elf_ppnt->p_memsz < k
    > > >
    > > > because it means that we only raise the error when
    > > >
    > > > k + elf_ppnt->p_memsz > TASK_SIZE
    > > >
    > > > I really think that we want to check this instead :
    > > >
    > > > k + elf_ppnt->p_memsz >= TASK_SIZE
    > > >
    > > > Otherwise we leave a window where this is undetected :
    > > >
    > > > load_addr + eppnt->p_vaddr == TASK_SIZE - eppnt->p_memsz

    The reason I did not propose changing these is because these are
    end-point checks (as opposed to starting address checks). I think
    that the following "equals" condition is conceptually valid:

    (starting-address + region-size == TASK_SIZE)



    > > > This will later lead to last_bss getting readjusted to TASK_SIZE, which I
    > > > don't think is expected at all :
    > > >
    > > > k = load_addr + eppnt->p_memsz + eppnt->p_vaddr;
    > > > if (k > last_bss)
    > > > last_bss = k;

    This is an interesting case, but I think the error checking works okay.

    After the ELF phdr loop, the resulting "last_bss" is used as follows:

    /* Map the last of the bss segment */
    if (last_bss > elf_bss) {
    down_write(&current->mm->mmap_sem);
    error = do_brk(elf_bss, last_bss - elf_bss);
    up_write(&current->mm->mmap_sem);
    if (BAD_ADDR(error))
    goto out_close;
    }

    The variable "last_bss" is used to compute the size argument in the
    call to do_brk(). If the section extends beyond TASK_SIZE, then do_brk()
    will return -EINVAL. If the do_brk() call succeeds but "elf_bss" is itself
    exactly at TASK_SIZE, then the BAD_ADDR() call above will catch it.



    > [...] But before this, I'd like to get comments from
    > the people who discussed the subject recently.

    Thus, I think that both 2.4.33 and 2.6.<latest> are okay without any
    further changes.



    Cheers. -ernie
    -
    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: 2006-08-21 22:39    [W:0.028 / U:61.628 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site