linux-kernel - Re: [PATCH] perf: Incorrect use of snprintf results in SEGV

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 7 Mar 2012 15:19:51 -0600
From:	Peter Seebach <peter.seebach@...driver.com>
To:	Ingo Molnar <mingo@...e.hu>
CC:	Arnaldo Carvalho de Melo <acme@...hat.com>,
	Anton Blanchard <anton@...ba.org>, <paulus@...ba.org>,
	<peterz@...radead.org>, <dsahern@...il.com>, <fweisbec@...il.com>,
	<yanmin_zhang@...ux.intel.com>, <emunson@...bm.net>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] perf: Incorrect use of snprintf results in SEGV

On Wed, 7 Mar 2012 21:37:25 +0100
Ingo Molnar <mingo@...e.hu> wrote:

> You are missing two important aspects:
> 
> 1) Dynamic reallocation on snprintf() failure is an utterly rare
>    thing - it is used in less than 1% of snprintf() invocations. 
>    (Yes, I just checked a couple of codebases.)

I would agree that it's very rare.  But then, using the return value at
all isn't especially common in my experience -- the only interesting
part, most of the time, is "we're sure this didn't overrun the buffer".

>    We *DONT* want to make APIs more fragile just to accomodate a
>    rare, esoteric usecase!

I would view snprintf as an API which already exists.  If it's the
wrong API, by all means, write a different one -- but I would suggest
not using the same name for it.  If a function is going to be called
snprintf, IMO it should have the semantics of snprintf.  If those are
the wrong semantics (and they may well be), then I would say use a
function which has the right semantics, and isn't named snprintf.

> 2) It's not even true that should some code want to
>    dynamically allocate the 'required' number of bytes is not
>    available. Some oddball side API could be added for that 1%:

> 	size_needed = snprintf_size(...);

That's where the "can write one in terms of the other" argument comes
into play.

If you have snprintf_needed(), it's easy to write snprintf_written()
in terms of it.  If you have only snprintf_written(), it is unreasonably
ugly and/or inefficient to write snprintf_needed() in terms of it.

> So this API could have been designed right but it was messed up 
> out of concern for an insane 1% case - FAIL.

Well, the thing is.  If there *exists* a reallocation case, those
semantics end up being needed.

I do agree that this is a source of errors in usage (and a quick audit
shows that I have at least one use which falls prey to this, as well as
several which check for it correctly).  In practice, I'd guess that
treating probably-negative sizes as an error would likely resolve
things, although that's also a semantics change -- it's just that it's
a semantics change which only affects single snprintf calls that were
expected to write to half the address space.

-s
-- 
Listen, get this.  Nobody with a good compiler needs to be justified.
--
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/