dm_attr_{name,uuid}_show buffer overflow? (was: Re: linux-next: Treefor January 5)

[Geert Uytterhoeven]

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@xxxxxxxxxxxxxx

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/