From: Kees Cook <keescook@chromium.org>
To: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Cc: LKML <linux-kernel@vger.kernel.org>,
Andrew Morton <akpm@linux-foundation.org>,
Julia Lawall <julia.lawall@lip6.fr>
Subject: Re: snprintf, overlapping destination and source
Date: Mon, 7 Dec 2015 14:04:41 -0800 [thread overview]
Message-ID: <CAGXu5jJ-NApCpANjfz+bAEfwZJ8xizkM-jDHVhOPCzhxV-aqdA@mail.gmail.com> (raw)
In-Reply-To: <87d1ukr5pf.fsf@rasmusvillemoes.dk>
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
next prev parent reply other threads:[~2015-12-07 22:04 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-12-05 20:38 snprintf, overlapping destination and source Rasmus Villemoes
2015-12-05 20:42 ` Julia Lawall
2015-12-07 22:04 ` Kees Cook [this message]
2015-12-16 13:14 ` Rasmus Villemoes
2016-03-08 20:40 ` [RFC 0/7] eliminate snprintf with overlapping src and dst Rasmus Villemoes
2016-03-08 20:40 ` [RFC 1/7] drm/amdkfd: avoid fragile and inefficient snprintf use Rasmus Villemoes
2016-03-14 14:33 ` Oded Gabbay
2016-03-14 19:18 ` Rasmus Villemoes
2016-03-08 20:40 ` [RFC 2/7] Input: joystick - avoid fragile " Rasmus Villemoes
2016-03-09 6:49 ` Andy Shevchenko
2016-03-08 20:40 ` [RFC 3/7] leds: avoid fragile sprintf use Rasmus Villemoes
2016-03-08 20:40 ` [RFC 4/7] drivers/media/pci/zoran: avoid fragile snprintf use Rasmus Villemoes
2016-03-08 20:40 ` [RFC 5/7] wlcore: " Rasmus Villemoes
2016-03-09 11:49 ` Kalle Valo
2016-03-08 20:40 ` [RFC 6/7] [media] ati_remote: " Rasmus Villemoes
2016-03-08 20:40 ` [RFC 7/7] USB: usbatm: avoid fragile and inefficient " Rasmus Villemoes
2016-03-08 21:01 ` Joe Perches
2016-03-10 23:56 ` Greg Kroah-Hartman
2016-03-09 13:08 ` Sergei Shtylyov
2016-03-08 23:07 ` [RFC 0/7] eliminate snprintf with overlapping src and dst Kees Cook
2016-03-08 23:13 ` Kees Cook
2016-03-09 6:51 ` Andy Shevchenko
2016-03-09 20:49 ` Andrew Morton
2016-03-09 22:19 ` Rasmus Villemoes
2016-03-10 13:59 ` One Thousand Gnomes
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAGXu5jJ-NApCpANjfz+bAEfwZJ8xizkM-jDHVhOPCzhxV-aqdA@mail.gmail.com \
--to=keescook@chromium.org \
--cc=akpm@linux-foundation.org \
--cc=julia.lawall@lip6.fr \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@rasmusvillemoes.dk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).