Messages in this thread | | | Date | Mon, 7 Dec 2015 14:04:41 -0800 | Subject | Re: snprintf, overlapping destination and source | From | Kees Cook <> |
| |
On Sat, Dec 5, 2015 at 12:38 PM, Rasmus Villemoes <linux@rasmusvillemoes.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
| |