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

[Geert Uytterhoeven <geert@linux-m68k.org>]

On Mon, 5 Jan 2009, Geert Uytterhoeven wrote:
> 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);
> | }

    [...]

> 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().

A quick `git grep strncat' shows a few more misusers of strncat() that may
cause buffer overflows and should be converted to strlcat():

| drivers/media/video/usbvideo/konicawc.c:	strncat(cam->input_physname, "/input0", sizeof(cam->input_physname));
| drivers/media/video/usbvideo/quickcam_messenger.c:	strncat(cam->input_physname, "/input0", sizeof(cam->input_physname));
| drivers/media/video/uvc/uvc_driver.c:		strncat(format->name, buffer[8] & (1 << 7) ? " 60Hz" : " 50Hz",
| usr/gen_init_cpio.c:                       strncat(expanded, getenv(env_var), PATH_MAX);
| usr/gen_init_cpio.c:                       strncat(expanded, end + 1, PATH_MAX);

Let's hope I didn't miss any. Good night! ;-)

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