On Fri, 5 Dec 2008 14:48:10 +0000 "Duane Griffin" <duaneg@xxxxxxxxx> wrote: > Hi folks, > > I am looking at a report of an intermittent BUG caused by an > intentionally corrupted ext2 filesystem: > http://bugzilla.kernel.org/show_bug.cgi?id=11412 > > What I think is happening is generic_readlink gets the name via > i_ops->follow_link and passes it into vfs_readlink, without it > necessarily being validating anywhere. If the name is not > NULL-terminated the strlen call in vfs_readlink may run off past the end > of the page. I think this is potentially happening in > page_follow_link_light, as well as ext2_follow_link, so it isn't just > ext* that is affected. > > Does this sound correct, or have I missed something? > > Assuming this is a real problem, does anyone have a better solution than > scanning the name for a \0 (in ext2_follow_link and > page_follow_link_light) and returning -ENAMETOOLONG if we can't find > one? I.e. something like this: It would be nice to fix this in a single place, for all filesystems, for all time. But how to do that? > diff --git a/fs/ext2/symlink.c b/fs/ext2/symlink.c > index 4e2426e..9b01af2 100644 > --- a/fs/ext2/symlink.c > +++ b/fs/ext2/symlink.c > @@ -24,8 +24,14 @@ > static void *ext2_follow_link(struct dentry *dentry, struct nameidata *nd) > { > struct ext2_inode_info *ei = EXT2_I(dentry->d_inode); > - nd_set_link(nd, (char *)ei->i_data); > - return NULL; > + void *err = NULL; > + > + if (memchr(ei->i_data, 0, sizeof(ei->i_data)) == NULL) > + err = ERR_PTR(-ENAMETOOLONG); > + else > + nd_set_link(nd, (char *)ei->i_data); > + > + return err; > } Perhaps nd_set_link() is a suitable place? Change that function so that it is passed a third argument (max_len) and then check that within nd_set_link(). Change nd_set_link() to return a __must_check-marked errno, change callers to handle errors appropriately. Or something totally different ;) But along those lines? -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
