* [RFC] corner cases of open() on procfs symlinks
@ 2013-06-06 1:20 Al Viro
2013-06-06 1:38 ` Linus Torvalds
0 siblings, 1 reply; 4+ messages in thread
From: Al Viro @ 2013-06-06 1:20 UTC (permalink / raw)
To: Linus Torvalds; +Cc: linux-fsdevel, linux-kernel
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;
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [RFC] corner cases of open() on procfs symlinks
2013-06-06 1:20 [RFC] corner cases of open() on procfs symlinks Al Viro
@ 2013-06-06 1:38 ` Linus Torvalds
2013-06-06 2:29 ` Al Viro
0 siblings, 1 reply; 4+ messages in thread
From: Linus Torvalds @ 2013-06-06 1:38 UTC (permalink / raw)
To: Al Viro; +Cc: linux-fsdevel, Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC] corner cases of open() on procfs symlinks
2013-06-06 1:38 ` Linus Torvalds
@ 2013-06-06 2:29 ` Al Viro
2013-06-06 2:40 ` Linus Torvalds
0 siblings, 1 reply; 4+ messages in thread
From: Al Viro @ 2013-06-06 2:29 UTC (permalink / raw)
To: Linus Torvalds; +Cc: linux-fsdevel, Linux Kernel Mailing List
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC] corner cases of open() on procfs symlinks
2013-06-06 2:29 ` Al Viro
@ 2013-06-06 2:40 ` Linus Torvalds
0 siblings, 0 replies; 4+ messages in thread
From: Linus Torvalds @ 2013-06-06 2:40 UTC (permalink / raw)
To: Al Viro; +Cc: linux-fsdevel, Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2013-06-06 2:40 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-06-06 1:20 [RFC] corner cases of open() on procfs symlinks Al Viro
2013-06-06 1:38 ` Linus Torvalds
2013-06-06 2:29 ` Al Viro
2013-06-06 2:40 ` Linus Torvalds
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).