lkml.org 
[lkml]   [2009]   [Jan]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    Subjectdm_attr_{name,uuid}_show buffer overflow? (was: Re: linux-next: Tree for January 5)
    On Mon, 5 Jan 2009, Stephen Rothwell wrote:
    > Status of my local build tests will be at
    > http://kisskb.ellerman.id.au/linux-next . If maintainers want to give
    > advice about cross compilers/configs that work, we are always open to add
    > more builds.

    On m68k, you get http://kisskb.ellerman.id.au/kisskb/buildresult/65466/:

    | ERROR: "strlen" [drivers/md/dm-mod.ko] undefined!

    which triggered my attention.

    drivers/md/dm-sysfs.c:

    | static ssize_t dm_attr_name_show(struct mapped_device *md, char *buf)
    | {
    | if (dm_copy_name_and_uuid(md, buf, NULL))
    | return -EIO;
    |
    | strncat(buf, "\n", DM_NAME_LEN);
    | return strnlen(buf, DM_NAME_LEN);
    | }

    is turned into:

    | .type dm_attr_name_show, @function
    | dm_attr_name_show:
    | move.l %a3,-(%sp)
    | move.l 12(%sp),%a3 | buf, buf
    | clr.l -(%sp) |
    | move.l %a3,-(%sp) | buf,
    | move.l 16(%sp),-(%sp) | md,
    | jbsr dm_copy_name_and_uuid |
    | lea (12,%sp),%sp |,
    | tst.l %d0 |
    | jeq .L23 |
    | move.w #-5,%a0 |, D.21217
    | jbra .L25 |
    | .L23:
    | move.l %a3,-(%sp) | buf,
    | jbsr strlen |
    | addq.l #4,%sp |,
    | move.w #2560,(%a3,%d0.l) |,* buf
    | moveq #127,%d0 |, tmp43
    | not.b %d0 | tmp43
    | move.l %a3,%a0 | buf, sc.399
    | #APP
    |
    | 1: subq.l #1,%d0 | count
    | jcs 2f
    | tst.b (%a0)+ | sc.399
    | jne 1b
    | subq.l #1,%a0 | sc.399
    | 2:
    | #NO_APP
    | sub.l %a3,%a0 | buf, D.21217
    | .L25:
    | move.l %a0,%d0 | D.21217, <result>
    | move.l (%sp)+,%a3
    | rts
    | .size dm_attr_name_show, .-dm_attr_name_show

    So gcc (version 4.1.2 20061115 (prerelease) (Debian 4.1.1-21) in my case) is
    smart and turns the strncat into a strlen() (without obeying our override of
    strlen() in <{linux,asm}/string.h>) followed by an _unconditional_ store.

    And I think that's the real bug: `strncat(buf, "\n", DM_NAME_LEN);' copies at
    most DM_NAME_LEN characters from source "\n", which has the known size 2,
    which is always smaller than DM_NAME_LEN. Hence the check is optimized away,
    and the "\n" is _always_ appended!

    Probably the intention was to limit the string in _buf_ (not the source string
    "\n") to DM_NAME_LEN? If yes, this may cause a buffer overflow.

    The same bug is present in dm_attr_uuid_show().

    Fixing it will probably get rid of the undefined reference, too (I hope :-)

    Gr{oetje,eeting}s,

    Geert

    --
    Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

    In personal conversations with technical people, I call myself a hacker. But
    when I'm talking to journalists I just say "programmer" or something like that.
    -- Linus Torvalds


    \
     
     \ /
      Last update: 2009-01-05 22:43    [W:2.857 / U:0.176 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site