Message-ID: <CAGXu5jJ-NApCpANjfz+bAEfwZJ8xizkM-jDHVhOPCzhxV-aqdA@mail.gmail.com>
Date:	Mon, 7 Dec 2015 14:04:41 -0800
From:	Kees Cook <keescook@...omium.org>
To:	Rasmus Villemoes <linux@...musvillemoes.dk>
Cc:	LKML <linux-kernel@...r.kernel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Julia Lawall <julia.lawall@...6.fr>
Subject: Re: snprintf, overlapping destination and source

On Sat, Dec 5, 2015 at 12:38 PM, Rasmus Villemoes
<linux@...musvillemoes.dk> wrote:
> I did a search for code doing
>
>   s[n]printf(buf, "...", ..., buf, ...)
>
> and found a few instances. They all do it with the format string
> beginning with "%s" and buf being passed as the corresponding parameter
> (obviously to append to the existing string). That works (AFAICT), both
> with the current printf implementation and with the string()
> modification which is now in -mm. It would obviously go horribly wrong
> if anything, even non-specifiers, precede the "%s" in the format
> string.

I think if we remove the ability, we get much uglier code that is
trying to do a strcat-like snprintf. Is there a clean replacement for
this design pattern? While it is technically considered an "undefined"
behavior, it's not true in practice, since it is defined: it's been
working fine. :)

> The question is, do we want to officially support this particular case of
> overlapping src and dst? Or should we close our eyes and hope it will
> continue to work [1] and that it won't cause a caffeine-deprived hacker
> to accidentally think one could also prepend to a buffer by doing
> sprintf(buf, "...%s", ..., buf)? I'm actually surprised gcc doesn't warn
> about this.
>
> [1] Not that I can immediately think of a sane way to implement snprintf
> where it won't work, but you never know...

(As an aside, yes, it's possible: glibc broke this when they tried to
harden sprintf by initializing the destination with \0 before ever
starting to process the format strings.)

If the replacement isn't ugly/complex/error-prone, we should fix it
and find a way to detect the issue. Otherwise, we should leave it and
add it to the printf test cases so we'll notice if it ever regresses.

-Kees

> My coccinelle-fu isn't sufficient to find cases where one of the buf
> instances is a more complicated expressions involving buf as a
> subexpression, as in
>
>   s[n]printf(buf, "...", ..., buf + 4, ...)
>
> or
>
>   s[n]printf(&buf[len], "...", ..., buf, ...)
>
> which would presumably always be wrong. Julia?
>
> Rasmus
>
> The cases I've found are
>
> ./drivers/gpu/drm/amd/amdkfd/kfd_topology.c:613:53-54: s[n]printf, overlapping source and destination buffers
> ./drivers/gpu/drm/amd/amdkfd/kfd_topology.c:618:16-17: s[n]printf, overlapping source and destination buffers
> ./drivers/gpu/drm/amd/amdkfd/kfd_topology.c:488:58-59: s[n]printf, overlapping source and destination buffers
> ./drivers/input/joystick/analog.c:445:59-60: s[n]printf, overlapping source and destination buffers
> ./drivers/leds/led-class-flash.c:215:32-33: s[n]printf, overlapping source and destination buffers
> ./drivers/media/pci/zoran/videocodec.c:120:39-40: s[n]printf, overlapping source and destination buffers
> ./drivers/media/rc/ati_remote.c:875:47-48: s[n]printf, overlapping source and destination buffers
> ./drivers/net/wireless/ti/wlcore/boot.c:125:24-25: s[n]printf, overlapping source and destination buffers
> ./drivers/net/wireless/ti/wlcore/boot.c:128:37-38: s[n]printf, overlapping source and destination buffers
> ./drivers/usb/atm/usbatm.c:1341:46-47: s[n]printf, overlapping source and destination buffers



-- 
Kees Cook
Chrome OS & Brillo Security
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists