linux-kernel - Re: [PATCH] vsprintf: drop comment claiming %n is ignored

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]

Date:	Sat, 14 Sep 2013 04:48:02 +0100
From:	Al Viro <viro@...IV.linux.org.uk>
To:	Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
Cc:	keescook@...omium.org, joe@...ches.com, linux@...izon.com,
	akpm@...ux-foundation.org, dan.carpenter@...cle.com,
	davem@...emloft.net, eldad@...refinery.com, jbeulich@...e.com,
	jkosina@...e.cz, linux-kernel@...r.kernel.org,
	rdunlap@...radead.org
Subject: Re: [PATCH] vsprintf: drop comment claiming %n is ignored

On Sat, Sep 14, 2013 at 04:05:21AM +0100, Al Viro wrote:
> Again, the normal return value of ->show() is 0 and that includes the case of
> overflow.  THE ONLY reason to check for overflow early is when subsequent
> output of ->show() takes long to generate and we want to skip that and
> have seq_read() do realloc-and-call-show-again immediately.  And in that
> case the right fix is often to get saner iterator and stop shoving everything
> into a single ->show() call...

Actually, let's take a look at the suckers.

* a bunch in arch/arm/plat-pxa/dma.c:dbg_show_requester_chan(): all buggy;
shouldn't care about pos at all and return 0.   Additionally,
dbg_show_descriptors() is buggy _and_ dumb - max_show shouldn't exist at all
and it shouldn't be using single_open(); it's iterating over a linked list,
for fsck sake...

* cris fasttimer: apparently valid "skip pointless work if we'd overflown"
uses; not sure if it's really needed here.

* cris show_cpuinfo(): buggy, should return 0 regardless.

* microblaze show_cpuinfo(): pointless sum is calculated and never used.

* openrisc show_cpuinfo(): buggy, should return 0 regardless.

* s390 pci_perf_show(): ditto.

* mtrr_seq_show(): pointless sum is calculated and never used.

* print_wakeup_source_stats(): buggy, should return 0 regardless.

* i8k_proc_show(): ditto.

* 3 ->show() instances in impi: ditto.

* i2o_report_query_status(): ditto.

* drivers/mfd/ab8500-debugfs.c:ab8500_registers_print(): legimitate use,
a side effect of lousy iterator, unfortunately complicated by the same
thing used for printk as well.

* same file, in ab8500_print_modem_registers() - return value of seq_printf()
is stored in local variable and immediately overwritten.

* same file, a metric arseload of return seq_printf(....): buggy, should
return 0.

* doc3g: ditto.

* drivers/parisc callers: pointless sums calculated and never used.

* drivers/regulator/dbx500-prcmu.c: somebody got religious about reporting
!!!scary!!! overflows.  Even though debugging printks are completely pointless
there.

* drivers/rtc: return value passed to caller and discarded there.  If it ever
stops being discarded, we'll get a bug.

drivers/s390/cio/blacklist.c: buggy, should return 0.

lustre: a bunch "should return 0" cases, AFAICS all of them are of that kind.

rtl8192*: a couple of such cases.

* drivers/usb/gadget/goku_udc.c: apparently valid uses, this time with what
looks like a damn good reason to skip extra work - it's reading from IO
ports and that can be slow.

* drivers/usb/gadget/pxa27x_udc.c: pointless sums discarded.

* debugfs_print_regs32(): should return 0.  The only caller ignores its
return value, fortunately, otherwise we'd have bugs.

* fs/dlm: valid, but very likely to be begging for better iterators.

* fs/proc children_seq_show(): buggy, should return 0.

* sysvipc_proc_show() and stuff it's calling (i.e. all show_... in
ipc/*): buggy, should return 0.

Enough for now...

Overall: I suspect that Joe might be right.  The very few callers that
use the return value and use it correctly can bloody well call
seq_overflow(), preferably with a detailed comment about the reasons
for doing so.  Anything that really wants the length of output (if we
have such places at all) can use %n or see Figure 1.  I haven't
crawled through lib/*, net/* and sound/* yet, but that's how the things
look so far.
--
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/