On Aug 11, 2006 15:48 -0700, Andrew Morton wrote:
> > > --- linux-2.6/fs/namei.c 2005-05-24 11:13:09.000000000 -0700
> > > +++ linux-2.6/fs/namei.c 2005-05-24 11:13:12.000000000 -0700
> > > @@ -1936,7 +1936,12 @@ int vfs_readlink(struct dentry *dentry,
> > > if (IS_ERR(link))
> > > goto out;
> > >
> > > - len = strlen(link);
> > > + len = strnlen(link, PATH_MAX);
> > > + if (len == PATH_MAX) {
> > > + len = -ENAMETOOLONG;
> > > + goto out;
> > > + }
> > > +
> > > if (len > (unsigned) buflen)
> > > len = buflen;
> > > if (copy_to_user(buffer, link, len))
> > > @@ -1953,6 +1958,11 @@ __vfs_follow_link(struct nameidata *nd,
> > > if (IS_ERR(link))
> > > goto fail;
> > >
> > > + if (strnlen(link, PATH_MAX) == PATH_MAX) {
> > > + link = ERR_PTR(-ENAMETOOLONG);
> > > + goto fail;
> > > + }
> > > +
> > > if (*link == '/') {
> > > path_release(nd);
> > > if (!walk_init_root(link, nd))
> >
>
> Should we instead be treating this as a filesystem driver bug, and fix up
> the filesystem(s) to not return overly-long pathname components?
>
> Running strnlen() against each component in __vfs_follow_link() looks
> expensive. Can it be avoided?
>
> Given that this is the VFS correcting for filesystem misbehaviour, it would
> seem to make sense to perform this correction at a low level, immediately
> after the filesytem driver has returned us the pathname component. An
> apparently-obvious way of doing this is to wrap ->follow_link. Can anyone
> think of a smarter way?
#define PATH_MAX 4096 /* # chars in a path name including nul */
One possibility is for the case of long symlinks to always set the last byte
of the page to NUL to ensure that the link is terminated. This appears
easily doable by having page_getlink() do the NUL termination after kmap()
but before returning, something like:
static char *page_getlink(struct dentry * dentry, struct page **ppage)
{
char *link;
page = read_cache_page(mapping, 0, (filler_t *)mapping->a_ops->readpage,
NULL);
if (IS_ERR(page))
goto sync_fail;
wait_on_page_locked(page);
if (!PageUptodate(page))
goto async_fail;
*ppage = page;
- return kmap(page);
+ link = kmap(page);
+ /* PATH_MAX is strictly <= PAGE_SIZE */
+ link[PATH_MAX - 1] = '\0';
+ return link;
Many of the other filesystems that don't use page_follow_link_light()
already do NUL termination themselves.
nfs_follow_link() is very similar, but not identical and needs the same fix.
ocfs2_page_getlink() is an exact duplicate of page_getlink() and the bug
duplication could be avoided if page_getlink() was exported. Otherwise it
needs the same fix.
I _think_ PATH_MAX is the right thing here (instead of PAGE_SIZE), since
the caller expects at most PATH_MAX in the returned link, and PAGE_SIZE
may be considerably larger. I don't think PATH_MAX will ever be larger
than PAGE_SIZE. We could also use min(PAGE_SIZE, PATH_MAX) and it would
be resolved at compile time, but it seems wishy-washy to me.
The other option is to actually get the link length out of the filesystem
itself, and avoid strlen(link) entirely, but that is a more complex change.
Cheers, Andreas
--
Andreas Dilger
Principal Software Engineer
Cluster File Systems, Inc.
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
