On Wed, 1 Jun 2005 02:15:48 -0600
Andreas Dilger <adilger@xxxxxxxxxxxxx> wrote:
> On May 31, 2005 16:02 -0700, Mike Waychison wrote:
> > We've hit the situation a few times where a corrupt symlink could easily
> > oops the kernel. The problem was tracked down to an older e2fsutils
> > that didn't do much sanity checking on symlinks during a fsck. This
> > patch uses strnlen when reading in the symlink and ensures that it
> > doesn't exceed PATH_MAX.
> >
> > Would you accept this kind of 'hardening'?
> >
> > Signed-off-by: Mike Waychison <mikew@xxxxxxxxxx>
>
> Andrew Morton suggested a very similar fix 3 years ago to this list:
>
> Subject: Re: ext3 -> crash -> fsck -> readlink -> oops
>
> I thought it made it into the kernel then, but I guess not.
This fix has drifted across my google desk - I thought we'd fixed it, but
still not. Third time lucky, perhaps.
> > --- 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))
>
A few things...
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?
-
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
