Re: [PATCH] dcache: trivial comment fix

[J. Bruce Fields]

On Tue, Sep 11, 2007 at 07:33:43PM +0200, Neil Brown wrote:
> On Monday September 10, bfields@xxxxxxxxxxxx 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/