| Message ID | 20180731161025.189534-1-jannh@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] fs: don't let getdents return bogus names | expand |
On Tue, Jul 31, 2018 at 06:10:27PM +0200, Jann Horn wrote: > +/* > + * Most filesystems don't filter out bogus directory entry names, and userspace > + * can get very confused by such names. Behave as if a low-level IO error had > + * happened while reading directory entries. > + */ > +bool bogus_dirent_name(int *errp, const char *name, int namlen, > + const char *caller) > +{ > + if (namlen == 0) { > + pr_err_once("%s: filesystem returned bogus empty name\n", > + caller); > + *errp = -EUCLEAN; > + return true; > + } > + if (memchr(name, '/', namlen)) { > + pr_err_once("%s: filesystem returned bogus name '%*pEhp' (contains slash)\n", > + caller, namlen, name); > + *errp = -EUCLEAN; > + return true; > + } > + return false; > +} > + if (bogus_dirent_name(&buf->result, name, namlen, __func__)) > + return -EUCLEAN; These calling conventions st^Ware rather suboptimal. First of all, * none of ->actor() callbacks will ever get called directly. * there are only 4 callers. 3 of them (all in fs.h) are of the form return ....->actor(...) == 0; The fourth is return orig_ctx->actor(orig_ctx, name, namelen, offset, ino, d_type); in ovl_fill_real(), which itself is an ->actor() callback. So all these "return -E..." in the instances are completely pointless; we should just turn filldir_t into pointer-to-function-returning-bool and get rid of that boilerplate, rather than adding more to it. Furthermore, who the hell cares which callback has stepped into it? "The first time it happened from getdents(2) in a 32bit process and that's all you'll ever get out of me" seems to be less than helpful... And frankly, I would prefer buf->result = check_dirent_name(name, namelen); if (unlikely(buf->result)) return false; making that thing return -EUCLEAN or 0. Quite possibly - inlining it as well...
On Tue, Jul 31, 2018 at 6:51 PM Al Viro <viro@zeniv.linux.org.uk> wrote: > > On Tue, Jul 31, 2018 at 06:10:27PM +0200, Jann Horn wrote: > > +/* > > + * Most filesystems don't filter out bogus directory entry names, and userspace > > + * can get very confused by such names. Behave as if a low-level IO error had > > + * happened while reading directory entries. > > + */ > > +bool bogus_dirent_name(int *errp, const char *name, int namlen, > > + const char *caller) > > +{ > > + if (namlen == 0) { > > + pr_err_once("%s: filesystem returned bogus empty name\n", > > + caller); > > + *errp = -EUCLEAN; > > + return true; > > + } > > + if (memchr(name, '/', namlen)) { > > + pr_err_once("%s: filesystem returned bogus name '%*pEhp' (contains slash)\n", > > + caller, namlen, name); > > + *errp = -EUCLEAN; > > + return true; > > + } > > + return false; > > +} > > > + if (bogus_dirent_name(&buf->result, name, namlen, __func__)) > > + return -EUCLEAN; > > These calling conventions st^Ware rather suboptimal. First of all, > * none of ->actor() callbacks will ever get called directly. > * there are only 4 callers. 3 of them (all in fs.h) are > of the form return ....->actor(...) == 0; The fourth is > return orig_ctx->actor(orig_ctx, name, namelen, offset, ino, d_type); > in ovl_fill_real(), which itself is an ->actor() callback. > > So all these "return -E..." in the instances are completely pointless; > we should just turn filldir_t into pointer-to-function-returning-bool > and get rid of that boilerplate, rather than adding more to it. Do you want me to try to write a patch that does that change? > Furthermore, who the hell cares which callback has stepped into it? > "The first time it happened from getdents(2) in a 32bit process and > that's all you'll ever get out of me" seems to be less than helpful... Yeah, true. Should I just remove the method name from the pr_err_once? Or should I also use one of the _ratelimited things and print multiple messages? > And frankly, I would prefer > buf->result = check_dirent_name(name, namelen); > if (unlikely(buf->result)) > return false; > making that thing return -EUCLEAN or 0. Quite possibly - inlining it > as well... I guess that would conform to normal kernel coding standards a bit better. Thanks for the quick feedback!
diff --git a/arch/alpha/kernel/osf_sys.c b/arch/alpha/kernel/osf_sys.c index c210a25dd6da..e04e51a2320d 100644 --- a/arch/alpha/kernel/osf_sys.c +++ b/arch/alpha/kernel/osf_sys.c @@ -40,6 +40,7 @@ #include <linux/vfs.h> #include <linux/rcupdate.h> #include <linux/slab.h> +#include <linux/fs.h> #include <asm/fpu.h> #include <asm/io.h> @@ -117,6 +118,8 @@ osf_filldir(struct dir_context *ctx, const char *name, int namlen, unsigned int reclen = ALIGN(NAME_OFFSET + namlen + 1, sizeof(u32)); unsigned int d_ino; + if (bogus_dirent_name(&buf->error, name, namlen, __func__)) + return -EUCLEAN; buf->error = -EINVAL; /* only used if we fail */ if (reclen > buf->count) return -EINVAL; diff --git a/fs/readdir.c b/fs/readdir.c index d97f548e6323..fa0ac1e33230 100644 --- a/fs/readdir.c +++ b/fs/readdir.c @@ -64,6 +64,29 @@ int iterate_dir(struct file *file, struct dir_context *ctx) } EXPORT_SYMBOL(iterate_dir); +/* + * Most filesystems don't filter out bogus directory entry names, and userspace + * can get very confused by such names. Behave as if a low-level IO error had + * happened while reading directory entries. + */ +bool bogus_dirent_name(int *errp, const char *name, int namlen, + const char *caller) +{ + if (namlen == 0) { + pr_err_once("%s: filesystem returned bogus empty name\n", + caller); + *errp = -EUCLEAN; + return true; + } + if (memchr(name, '/', namlen)) { + pr_err_once("%s: filesystem returned bogus name '%*pEhp' (contains slash)\n", + caller, namlen, name); + *errp = -EUCLEAN; + return true; + } + return false; +} + /* * Traditional linux readdir() handling.. * @@ -98,6 +121,8 @@ static int fillonedir(struct dir_context *ctx, const char *name, int namlen, if (buf->result) return -EINVAL; + if (bogus_dirent_name(&buf->result, name, namlen, __func__)) + return -EUCLEAN; d_ino = ino; if (sizeof(d_ino) < sizeof(ino) && d_ino != ino) { buf->result = -EOVERFLOW; @@ -173,6 +198,8 @@ static int filldir(struct dir_context *ctx, const char *name, int namlen, int reclen = ALIGN(offsetof(struct linux_dirent, d_name) + namlen + 2, sizeof(long)); + if (bogus_dirent_name(&buf->error, name, namlen, __func__)) + return -EUCLEAN; buf->error = -EINVAL; /* only used if we fail.. */ if (reclen > buf->count) return -EINVAL; @@ -259,6 +286,8 @@ static int filldir64(struct dir_context *ctx, const char *name, int namlen, int reclen = ALIGN(offsetof(struct linux_dirent64, d_name) + namlen + 1, sizeof(u64)); + if (bogus_dirent_name(&buf->error, name, namlen, __func__)) + return -EUCLEAN; buf->error = -EINVAL; /* only used if we fail.. */ if (reclen > buf->count) return -EINVAL; @@ -358,6 +387,8 @@ static int compat_fillonedir(struct dir_context *ctx, const char *name, if (buf->result) return -EINVAL; + if (bogus_dirent_name(&buf->result, name, namlen, __func__)) + return -EUCLEAN; d_ino = ino; if (sizeof(d_ino) < sizeof(ino) && d_ino != ino) { buf->result = -EOVERFLOW; @@ -427,6 +458,8 @@ static int compat_filldir(struct dir_context *ctx, const char *name, int namlen, int reclen = ALIGN(offsetof(struct compat_linux_dirent, d_name) + namlen + 2, sizeof(compat_long_t)); + if (bogus_dirent_name(&buf->error, name, namlen, __func__)) + return -EUCLEAN; buf->error = -EINVAL; /* only used if we fail.. */ if (reclen > buf->count) return -EINVAL; diff --git a/include/linux/fs.h b/include/linux/fs.h index 805bf22898cf..62ad476563f0 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -1680,6 +1680,9 @@ struct dir_context { loff_t pos; }; +bool bogus_dirent_name(int *errp, const char *name, int namlen, + const char *caller); + struct block_device_operations; /* These macros are for out of kernel modules to test that
When you e.g. run `find` on a directory for which getdents returns "filenames" that contain slashes, `find` passes those "filenames" back to the kernel, which then interprets them as paths. That could conceivably cause userspace to do something bad when accessing something like an untrusted USB stick, but I'm not aware of any specific example. Instead of returning bogus filenames to userspace, return -EUCLEAN. Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") Cc: stable@vger.kernel.org Signed-off-by: Jann Horn <jannh@google.com> --- changed in v2: - move bogus_dirent_name() out of the #ifdef where it doesn't belong (kbuild test robot) @Al: Given what Dave and Pavel said, are you okay with this? arch/alpha/kernel/osf_sys.c | 3 +++ fs/readdir.c | 33 +++++++++++++++++++++++++++++++++ include/linux/fs.h | 3 +++ 3 files changed, 39 insertions(+)