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 for Android: free password hash cracker in your pocket

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

Message-ID: <s5hpqvg1m7v.wl%tiwai@suse.de>
Date:	Mon, 11 Oct 2010 22:57:40 +0200
From:	Takashi Iwai <tiwai@...e.de>
To:	Dan Carpenter <error27@...il.com>
Cc:	Mark Brown <broonie@...nsource.wolfsonmicro.com>,
	Liam Girdwood <lrg@...mlogic.co.uk>,
	Jaroslav Kysela <perex@...ex.cz>,
	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

At Mon, 11 Oct 2010 21:45:02 +0200,
Dan Carpenter wrote:
> 
> On Mon, Oct 11, 2010 at 07:51:48PM +0100, Mark Brown wrote:
> > 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?
> 
> That's odd.  I'm sorry, I can't explain why you wouldn't see a stack
> trace...  The code is straight forward:
> 
>         /* Reject out-of-range values early.  Large positive sizes are
>            used for unknown buffer sizes. */
>         if (WARN_ON_ONCE((int) size < 0))
>                 return 0;
> 
> It would still give you truncated output but after the NULL terminator
> there would be information leaked from the kernel.  If the reader
> program had allocated a large enough buffer to handle the extra
> information it wouldn't cause a problem.

Well, actually we should fix either:

- check the return of snprintf() at each time properly,

	list_for_each_entry(dai, &dai_list, list) {
		int len = snprintf(buf + ret, PAGE_SIZE - ret, "%s\n", dai->name);
		if (len < 0)
			return len;
		ret += len;
	}

- or just assume it's never negative (as is on Linux kernel code)

In either case, a negative check after for loop is superfluous.

And, when no negative return value is assured (or filtered out like
above), there can't be overflow, too.  snprintf() fills the string
at most the size including NUL-char.  OTOH, it returns the size that
doesn't include NUL-char.


Takashi
--
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/