[RFC] corner cases of open() on procfs symlinks

[RFC] corner cases of open() on procfs symlinks

[Al Viro]

	I'm not sure whether to treat that as a bug or as a weird misfeature
enshrined in userland ABI:
	open("/tmp", O_CREAT, 0) => -EISDIR	// LAST_NORM case
	open("/", O_CREAT, 0) => -EISDIR	// LAST_ROOT
	open(".", O_CREAT, 0) => -EISDIR	// LAST_DOT
	open("..", O_CREAT, 0) => -EISDIR	// LAST_DOTDOT
	open("/proc/self/cwd", O_CREAT, 0) => success	// LAST_BIND
	open("/proc/self/cwd/", O_CREAT, 0) => -EISDIR	// trailing slashes
At the very least, it's inconsistent.  OTOH, it's exposed to userland.
OTTH, SuS says that O_CREAT without O_RDWR or O_WRONLY is undefined, and
either of those two would suffice for -EISDIR in all cases (may_open() takes
care of that).

Another unpleasantness:
	open("/proc/self/exe", LOOKUP_DIRECTORY, 0) -> success
That one is clearly a bug.  Moreover, getting rid of both of those
bugs actually simplifies the mess in do_last().  I would obviously
like to do that - do_last() is far too convoluted as it is; the only
question is whether we can change the first weirdness...  Comments?

FWIW, the simplification of do_last() would look like that:

diff --git a/fs/namei.c b/fs/namei.c
index 85e40d1..617599c 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2690,28 +2690,10 @@ static int do_last(struct nameidata *nd, struct path *path,
 	nd->flags &= ~LOOKUP_PARENT;
 	nd->flags |= op->intent;
 
-	switch (nd->last_type) {
-	case LAST_DOTDOT:
-	case LAST_DOT:
+	if (nd->last_type != LAST_NORM) {
 		error = handle_dots(nd, nd->last_type);
 		if (error)
 			return error;
-		/* fallthrough */
-	case LAST_ROOT:
-		error = complete_walk(nd);
-		if (error)
-			return error;
-		audit_inode(name, nd->path.dentry, 0);
-		if (open_flag & O_CREAT) {
-			error = -EISDIR;
-			goto out;
-		}
-		goto finish_open;
-	case LAST_BIND:
-		error = complete_walk(nd);
-		if (error)
-			return error;
-		audit_inode(name, dir, 0);
 		goto finish_open;
 	}
 
@@ -2841,6 +2823,7 @@ finish_lookup:
 	}
 	nd->inode = inode;
 	/* Why this, you ask?  _Now_ we might have grown LOOKUP_JUMPED... */
+finish_open:
 	error = complete_walk(nd);
 	if (error) {
 		path_put(&save_parent);
@@ -2853,7 +2836,6 @@ finish_lookup:
 	if ((nd->flags & LOOKUP_DIRECTORY) && !nd->inode->i_op->lookup)
 		goto out;
 	audit_inode(name, nd->path.dentry, 0);
-finish_open:
 	if (!S_ISREG(nd->inode->i_mode))
 		will_truncate = false;
 

Re: [RFC] corner cases of open() on procfs symlinks

[Linus Torvalds]

On Thu, Jun 6, 2013 at 10:20 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>         I'm not sure whether to treat that as a bug or as a weird misfeature
> enshrined in userland ABI:
>         open("/tmp", O_CREAT, 0) => -EISDIR     // LAST_NORM case
>         open("/", O_CREAT, 0) => -EISDIR        // LAST_ROOT
>         open(".", O_CREAT, 0) => -EISDIR        // LAST_DOT
>         open("..", O_CREAT, 0) => -EISDIR       // LAST_DOTDOT
>         open("/proc/self/cwd", O_CREAT, 0) => success   // LAST_BIND
>         open("/proc/self/cwd/", O_CREAT, 0) => -EISDIR  // trailing slashes

Ok, that looks buggy. O_CREAT should definitely return EISDIR for
/proc/self/cwd too, since it's a directory. I don't think the
O_RDWR/O_WRONLY thing should matter.

>        I would obviously
> like to do that - do_last() is far too convoluted as it is; the only
> question is whether we can change the first weirdness...  Comments?

Exactly which cases does that change? I have no objections if it's
only the "LAST_BIND" case that now starts returning EISDIR. Is there
anything else it affects?

That said, obviously if something breaks, we'd have to revert it, and
as a cleanup rather than some serious bug (ie this doesn't cause
crashes or security issues), I suspect this should wait until 3.11
regardless. No?

           Linus

Re: [RFC] corner cases of open() on procfs symlinks

[Al Viro]

On Thu, Jun 06, 2013 at 10:38:31AM +0900, Linus Torvalds wrote:
> On Thu, Jun 6, 2013 at 10:20 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> >         I'm not sure whether to treat that as a bug or as a weird misfeature
> > enshrined in userland ABI:
> >         open("/tmp", O_CREAT, 0) => -EISDIR     // LAST_NORM case
> >         open("/", O_CREAT, 0) => -EISDIR        // LAST_ROOT
> >         open(".", O_CREAT, 0) => -EISDIR        // LAST_DOT
> >         open("..", O_CREAT, 0) => -EISDIR       // LAST_DOTDOT
> >         open("/proc/self/cwd", O_CREAT, 0) => success   // LAST_BIND
> >         open("/proc/self/cwd/", O_CREAT, 0) => -EISDIR  // trailing slashes
> 
> Ok, that looks buggy. O_CREAT should definitely return EISDIR for
> /proc/self/cwd too, since it's a directory. I don't think the
> O_RDWR/O_WRONLY thing should matter.
> 
> >        I would obviously
> > like to do that - do_last() is far too convoluted as it is; the only
> > question is whether we can change the first weirdness...  Comments?
> 
> Exactly which cases does that change? I have no objections if it's
> only the "LAST_BIND" case that now starts returning EISDIR. Is there
> anything else it affects?

LAST_BIND gets to go through the EISDIR and ENOTDIR checks that way, which
fixes these two bugs.

LAST_DOT/LAST_DOTDOT/LAST_ROOT end up checking whether we are at the
directory or not; sure, we know that we are, so these tests are
redundant, but I really don't think it's worth optimizing for.  We are
not generating any data misses and arguably we reduce instruction cache
footprint a bit, not that it would be noticable with the I$ horror
do_last() still is...

What really happens in that switch is that do_last() tries to be too smart
and ends up skipping a few things too many.

> That said, obviously if something breaks, we'd have to revert it, and
> as a cleanup rather than some serious bug (ie this doesn't cause
> crashes or security issues), I suspect this should wait until 3.11
> regardless. No?

Probably...  procfs symlinks neutering O_DIRECTORY might, in theory, be usable
to cook something nasty, but I don't see any obvious ways to exploit that.
FWIW, resulting kernel seems to survive the minimal beating, but obviously
more is needed.

Re: [RFC] corner cases of open() on procfs symlinks

[Linus Torvalds]

On Thu, Jun 6, 2013 at 11:29 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> Probably...  procfs symlinks neutering O_DIRECTORY might, in theory, be usable
> to cook something nasty, but I don't see any obvious ways to exploit that.
> FWIW, resulting kernel seems to survive the minimal beating, but obviously
> more is needed.

Let's plan on merging that patch in the 3.11 merge window, and perhaps
mark it for stable?

                 Linus