Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1284639 > unrolled thread

snprintf, overlapping destination and source

Back to article view | Back to linux.kernel

Contents

  snprintf, overlapping destination and source Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-12-05 21:40 +0100
    Re: snprintf, overlapping destination and source Julia Lawall <julia.lawall@lip6.fr> - 2015-12-05 21:50 +0100
    Re: snprintf, overlapping destination and source Kees Cook <keescook@chromium.org> - 2015-12-07 23:10 +0100

#1284639 — snprintf, overlapping destination and source

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.

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

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

[toc] | [next] | [standalone]

#1284642

On Sat, 5 Dec 2015, Rasmus Villemoes 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.
>
> 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...
>
> 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?

If you just want an argument expression that contains buf somewhere, you
can write <+...buf...+>.

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

[toc] | [prev] | [next] | [standalone]

#1285999

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web