linux-kernel - Re: [PATCH 6/6] procfs: Kill the bkl in ioctl

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: <201004121934.18307.arnd@arndb.de>
Date:	Mon, 12 Apr 2010 19:34:17 +0200
From:	Arnd Bergmann <arnd@...db.de>
To:	Christoph Hellwig <hch@...radead.org>
Cc:	Frederic Weisbecker <fweisbec@...il.com>,
	Stefan Richter <stefanr@...6.in-berlin.de>,
	Alexey Dobriyan <adobriyan@...il.com>,
	LKML <linux-kernel@...r.kernel.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Andrew Morton <akpm@...ux-foundation.org>,
	John Kacur <jkacur@...hat.com>,
	KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>,
	Al Viro <viro@...iv.linux.org.uk>, Ingo Molnar <mingo@...e.hu>
Subject: Re: [PATCH 6/6] procfs: Kill the bkl in ioctl

On Sunday 11 April 2010, Christoph Hellwig wrote:
> On Sat, Apr 10, 2010 at 05:28:16PM +0200, Frederic Weisbecker wrote:
> > So you mean we should attribute explicit default_llseek to the evil
> > places instead of explicit generic_file_llseek in the safe ones?
> > That's not a bad idea as it would result in much less changes.
> > 
> > The problem happens the day you switch to generic_file_llseek() as the
> > new default llseek(), how do you prove that all remaining fops
> > that don't implement .llseek don't use the bkl? There will be
> > hundreds of them and saying "we've looked all of them and they don't
> > need it" will be a scary justification.
> > 
> > On the opposite, attributing explicit generic_file_llseek or
> > non_seekable_open on the safe places and default_llseek on
> > the dozens of others doubtful places is easier to get a
> > safe conclusion.
> > 
> > But yeah we should try, at least attributing explicit
> > default_llseek won't harm, quite the opposite.
> 
> Note that an lssek that actually does something is the wrong default,
> even if we have it that way currently.  If the default is changed it
> should be changed to give the semantics that nonseekable_open()
> gives us.  Given that you guys are so motivated to do something in
> this area it might be a good idea to do this in a few simple steps:
> 
>  - make sure every file operation either has a ->llseek instead or
>    calls nonseekable_open from ->open

I still think it would be better to always set llseek if we do that,
even if nonseekable_open is already there. I can come up with scripts
that check that case, but checking that the open function always
calls nonseekable_open when it returns success is beyond my grep
skills ;-)

>  - remove nonseekable_open and all calls to it
>  - switch all users of no_llseek to not set a ->llsek after auditing
>    that there's no corner case where we want to allow pread/pwrite
>    but not lseek, which is rather unlikely

This parts seems fine.

>  - walk through the instances now using default_llseek and chose
>    a better implementation for this particular instance.  Often
>    this will be just removing the the lssek method as not allowing
>    seeks is the right thing to do for character drivers, even if it
>    is a behaviour change from the current version which usually
>    is the result of sloppy coding.

This part is really hard. While in many cases, the driver maintainer
might know what user space is potentially opening some character
device, it's really hard to tell for outsiders whether the behaviour
should be no_llseek (then the default) or noop_llseek to work around
broken user space.

I think the rule set for the conversion needs to be one that can
be done purely based on the code. How about this:

For each file operation {
	if (uses f_pos) {
		if (same module uses BKL)
			-> default_llseek
		else
			-> generic_file_llseek
	} else {
		if (driver maintained)
			-> no_llseek (with maintainer ACK)
		else
			-> noop_llseek
	}
}

Once that is done, we can turn the default into nonseekable
behavior and start removing instances of explicit no_llseek
and nonseekable_open.

Should we also rename default_llseek to deprecated_llseek in the
process, to go along with the approach for ioctl?

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