linux-kernel - Re: [PATCH 2/2] vfs: fix d_path() for unreachable paths

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:	Mon, 21 Sep 2009 17:03:48 +0200
From:	Miklos Szeredi <miklos@...redi.hu>
To:	Al Viro <viro@...IV.linux.org.uk>
CC:	miklos@...redi.hu, akpm@...ux-foundation.org,
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
	Valdis.Kletnieks@...edu, agruen@...e.de, hch@....de,
	hugh.dickins@...cali.co.uk, matthew@....cx
Subject: Re: [PATCH 2/2] vfs: fix d_path() for unreachable paths

On Mon, 21 Sep 2009, Al Viro wrote:
> On Mon, Sep 21, 2009 at 02:51:42PM +0200, Miklos Szeredi wrote:
> > Hugh Dickins reported that an old version of gnome-vfs-daemon crashes
> > because it finds an entry in /proc/mounts where the mountpoint is
> > unreachable.  So revert /proc/mounts to the old behavior (or rather a
> > less crazy version of the old behavior).
> > 
> > Also revert the effect on /proc/${PID}/maps for memory maps set up
> > with shmem_file_setup() or hugetlb_file_setup().  These functions set
> > up unlinked files under a kernel-private vfsmount.  Since this
> > vfsmount is unreachable from userspace, these maps will be reported
> > with the "(unreachable)" prefix, which is undesirable, because it
> > changes the kernel ABI and might break applications for no good
> > reason.
> 
> Ho-hum...
> 	a) d_op you've got there look like a candidate for libfs, if
> we go for that at all
> 	b) I *really* don't like changing the behaviour of d_path() instead
> of doing new behaviour in a new function and deciding where to use it on
> case-by-case basis
> 	c) having just grepped for d_path()... oh, man
> * blackfin cplbinfo: utter crap; it's used to decide which procfs file
> is being opened - by dumping full pathname into a (on-stack) buffer
> and then parsing it.  Stupid *and* broken.
> * blackfin traps.c:decode_address(): for one thing, pathnames has been
> known to be longer than 256 bytes.  For another... locking in that loop
> over processes and VMAs of each looks very suspicios, while we are at it.
> * pohmelfs_construct_path_string(): just what will happen if we are called
> from process chrooted into that bugger?  d_path() is badly abused there.
> * ext4_file_open(): d_path() per se is probably OK, but initializing
> path.mnt/path.dentry isn't.  mount --move racing with that thing => loads
> of fun.
> * sysfs_open_file(): racy in an obvious way, but probably won't do anything
> worse than garbage in oopsen.
> 
> I'm very uncomfortable with the silent change of behaviour, especially since
> existing callers seem to be split between "part of user-visible ABI" and
> "generally bogus, waiting for a gnat to fart".  At the very least it needs
> a serious audit of callers.


Fair enough, I should have done a review of internal callers...  Will
do that now.

The big question is, however, if we accept the unknown risk of
changing the user-visible ABI from "broken, but path at least *looks*
normal" to "paths don't necessarily begin with a slash anymore".

Hmm?

Thanks,
Miklos

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