[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20070911180036.GK13948@fieldses.org>
Date: Tue, 11 Sep 2007 14:00:36 -0400
From: "J. Bruce Fields" <bfields@...ldses.org>
To: Neil Brown <neilb@...e.de>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] dcache: trivial comment fix
On Tue, Sep 11, 2007 at 07:33:43PM +0200, Neil Brown wrote:
> On Monday September 10, bfields@...ldses.org wrote:
> > On Mon, Sep 10, 2007 at 02:46:32PM -0400, J. Bruce Fields wrote:
> > > * This forceful removal will result in ugly /proc output if
> > > * somebody holds a file open that got deleted due to a rename.
> > > * We could be nicer about the deleted file, and let it show
> > > - * up under the name it got deleted rather than the name that
> > > - * deleted it.
> > > + * up under the name it had before it was deleted rather than
> > > + * under the original name of the file that was moved on top of it.
> >
> > By the way, on further examination of the code it doesn't actually do
> > what's described in the case where the target name is large and the
> > moved-from name is small. Instead, it reports random garbage (usually
> > part of a name left over from some other dentry?) as far as I can tell:
> >
> > from switch_names():
> >
> >
> > if (dname_external(target)) {
> > if (dname_external(dentry)) {
> > ...
> > } else {
> > /*
> > * dentry:internal, target:external. Steal target's
> > * storage and make target internal.
> > */
> > dentry->d_name.name = target->d_name.name;
> > target->d_name.name = target->d_iname;
> >
> > ... but target->d_iname could have anything in it, right?
>
> Right, but not relevant.
The effect of it is that the name reported in /proc/<pid>/fd/<fd> is
random garbage if you're holding the target file open. In quick tests,
I found that
touch abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyz
tail -f abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyz
touch foo
mv foo abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyz
readlink /proc/<pid>/fd/<fd>
prints the initial portion of some other random name (often, not always,
"foo").
In theory I think that could disclose a little uninitialized kernel
memory, couldn't it? I don't know if there's any practical way that
could be exploited.
> The name "switch_names" is somewhat misleading. It is really
> "copyname" or similar. From the comment at the top:
>
> * When switching names, the actual string doesn't strictly have to
> * be preserved in the target - because we're dropping the target
> * anyway. As such, we can just do a simple memcpy() to copy over
> * the new name before we switch.
>
> so the apparent name of 'target' after the 'swap' is not important.
>
> The purpose of the assignment
> target->d_name.name = target->d_iname;
> is to make "dname_external(target)" false, that making "target
> internal" as the comment says.
Right. But it looks like the contents of the buffer target->d_iname
also need to be initialized in this case--I suppose somebody just didn't
want to perform a memcpy they thought was pointless--so the name
reported in /proc is undefined.
--b.
-
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/
Powered by blists - more mailing lists