linux-kernel - Re: [patch] ASoC: soc: snprintf() doesn't return negative

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20101011185148.GB22355@opensource.wolfsonmicro.com>
Date:	Mon, 11 Oct 2010 19:51:48 +0100
From:	Mark Brown <broonie@...nsource.wolfsonmicro.com>
To:	Dan Carpenter <error27@...il.com>
Cc:	Liam Girdwood <lrg@...mlogic.co.uk>,
	Jaroslav Kysela <perex@...ex.cz>, Takashi Iwai <tiwai@...e.de>,
	Peter Ujfalusi <peter.ujfalusi@...ia.com>,
	Jassi Brar <jassi.brar@...sung.com>,
	alsa-devel@...a-project.org, kernel-janitors@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [patch] ASoC: soc: snprintf() doesn't return negative

On Mon, Oct 11, 2010 at 06:40:38PM +0200, Dan Carpenter wrote:
> On Mon, Oct 11, 2010 at 11:40:09AM +0100, Mark Brown wrote:

> > I'm not going to apply this. snprintf() returns a signed type, checking
> > that the return value is a reasonable thing to do here - at worst we're
> > wasting a few cycles in code that's nowhere near a hot path, at best
> > we're robust in the face of a decision to add error reporting to
> > snprintf() so it's hard to see this change as an improvement.

> It's not a matter of cycles.  My check probably takes the exact same
> number of cycles as your check.  The point is that checking for negative
> doesn't make any sort of sense.  If we did return an error code then we

Remember that we also have to be able to read the code; it's probably
safe to assume that not everyone has the quirks of every snprintf()
implementation committed to memory.  Except for the microoptimisation
all you're doing here is making the code harder to read.

> If "PAGE_SIZE - ret" is a negative number then it triggers a
> WARN_ON_ONCE() in vsnprintf().  It's not possible in this case however.
> Really the original code works fine in practice because the information
> we are printing out is less than PAGE_SIZE.

In actual fact quite a few devices have enough registers to be
truncated, meaning that it's not only possible but likely we'll exercise
the cases that deal with the end of buffer.  If snprintf() is returning
values larger than buffer size it was given we're likely to have an
issue but it seems that there's something missing in your analysis since
we're never seeing WARN_ON()s and are instead seeing the behaviour the
code is intended to give, which is to truncate the output when we run
out of space.

Could you re-check your analysis, please?
--
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/