* Greg Kurz (gr...@kaod.org) wrote:
> On Sun, 14 Mar 2021 19:36:04 -0400
> Vivek Goyal <vgo...@redhat.com> wrote:
>
> > On Fri, Mar 12, 2021 at 03:10:03PM +0100, Greg Kurz wrote:
> > > POSIX.1-2017 clearly stipulates that empty filenames aren't
> > > allowed ([1] and [2]). Since virtiofsd is supposed to mirror
> > > the host file system hierarchy and the host can be assumed to
> > > be linux, we don't really expect clients to pass requests with
> > > an empty path in it. If they do so anyway, this would eventually
> > > cause an error when trying to create/lookup the actual inode
> > > on the underlying POSIX filesystem. But this could still confuse
> > > some code that wouldn't be ready to cope with this.
> > >
> > > Filter out empty names coming from the client at the top level,
> > > so that the rest doesn't have to care about it. This is done
> > > everywhere we already call is_safe_path_component(), but
> > > in a separate helper since the usual error for empty path
> > > names is ENOENT instead of EINVAL.
> > >
> > > [1]
> > > https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_170
> > > [2]
> > > https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap04.html#tag_04_13
> > >
> > > Signed-off-by: Greg Kurz <gr...@kaod.org>
> >
> > Hi Greg,
> >
> > Minor nit, if you happen to respin this patch, it probably should come
> > before the first patch in series. Once we make it clear that file server
> > is not expecting empty path in these top level functions, then it is
> > easy to clear AT_EMPTY_PATH in function these paths are calling as
> > appropriate.
> >
>
> The patch order is chronological : I just spotted the AT_EMPTY_PATH
> oddity before coming up with the bigger hammer of patch 3. But you're
> right, it probably makes more sense to do the other way around.
>
> > What about lo_create(). Should we put a check in there as well.
> >
>
> Good catch ! I'll post a v2 then ;)
I'm just brewing a pull now, since soft freeze is tomorrow.
I'll take 3,1,2 - please follow up with a separate lo_create one - we
can add that later.
Dave
> > We are passed xattr names in lo_getxattr()/lo_removexattr()/lo_setxattr().
> > In general, should we put an empty in_name check there as well and
> > probably simply return -EINVAL.
> >
>
> An empty xattr name doesn't likely make sense either, even if this
> isn't written down anywhere, not in an explicit manner at least.
>
> The kernel checks this in setxattr() and errors out with -ERANGE
> in this case.
>
> error = strncpy_from_user(kname, name, sizeof(kname));
> if (error == 0 || error == sizeof(kname))
> error = -ERANGE;
> if (error < 0)
> return error;
>
> Same goes for the other *xattr() syscalls, i.e. nothing nasty can ever
> happen with an empty xattr name since this is always considered as an
> error by the kernel. Not sure this would bring much to also check this
> in QEMU. This is a bit different from the empty path name case because
> an empty path name is valid for syscalls that support AT_EMPTY_PATH,
> and we just want to make sure these are never exercised with names
> from the client.
>
> Cheers,
>
> --
> Greg
>
> > Thanks
> > Vivek
> >
> > > ---
> > > tools/virtiofsd/passthrough_ll.c | 35 ++++++++++++++++++++++++++++++++
> > > 1 file changed, 35 insertions(+)
> > >
> > > diff --git a/tools/virtiofsd/passthrough_ll.c
> > > b/tools/virtiofsd/passthrough_ll.c
> > > index f63016d35626..bff9dc2cd26d 100644
> > > --- a/tools/virtiofsd/passthrough_ll.c
> > > +++ b/tools/virtiofsd/passthrough_ll.c
> > > @@ -237,6 +237,11 @@ static bool is_safe_path_component(const char *path)
> > > return !is_dot_or_dotdot(path);
> > > }
> > >
> > > +static bool is_empty(const char *name)
> > > +{
> > > + return name[0] == '\0';
> > > +}
> > > +
> > > static struct lo_data *lo_data(fuse_req_t req)
> > > {
> > > return (struct lo_data *)fuse_req_userdata(req);
> > > @@ -1083,6 +1088,11 @@ static void lo_lookup(fuse_req_t req, fuse_ino_t
> > > parent, const char *name)
> > > fuse_log(FUSE_LOG_DEBUG, "lo_lookup(parent=%" PRIu64 ", name=%s)\n",
> > > parent,
> > > name);
> > >
> > > + if (is_empty(name)) {
> > > + fuse_reply_err(req, ENOENT);
> > > + return;
> > > + }
> > > +
> > > /*
> > > * Don't use is_safe_path_component(), allow "." and ".." for NFS
> > > export
> > > * support.
> > > @@ -1174,6 +1184,11 @@ static void lo_mknod_symlink(fuse_req_t req,
> > > fuse_ino_t parent,
> > > struct fuse_entry_param e;
> > > struct lo_cred old = {};
> > >
> > > + if (is_empty(name)) {
> > > + fuse_reply_err(req, ENOENT);
> > > + return;
> > > + }
> > > +
> > > if (!is_safe_path_component(name)) {
> > > fuse_reply_err(req, EINVAL);
> > > return;
> > > @@ -1246,6 +1261,11 @@ static void lo_link(fuse_req_t req, fuse_ino_t
> > > ino, fuse_ino_t parent,
> > > char procname[64];
> > > int saverr;
> > >
> > > + if (is_empty(name)) {
> > > + fuse_reply_err(req, ENOENT);
> > > + return;
> > > + }
> > > +
> > > if (!is_safe_path_component(name)) {
> > > fuse_reply_err(req, EINVAL);
> > > return;
> > > @@ -1323,6 +1343,11 @@ static void lo_rmdir(fuse_req_t req, fuse_ino_t
> > > parent, const char *name)
> > > struct lo_inode *inode;
> > > struct lo_data *lo = lo_data(req);
> > >
> > > + if (is_empty(name)) {
> > > + fuse_reply_err(req, ENOENT);
> > > + return;
> > > + }
> > > +
> > > if (!is_safe_path_component(name)) {
> > > fuse_reply_err(req, EINVAL);
> > > return;
> > > @@ -1352,6 +1377,11 @@ static void lo_rename(fuse_req_t req, fuse_ino_t
> > > parent, const char *name,
> > > struct lo_inode *newinode = NULL;
> > > struct lo_data *lo = lo_data(req);
> > >
> > > + if (is_empty(name) || is_empty(newname)) {
> > > + fuse_reply_err(req, ENOENT);
> > > + return;
> > > + }
> > > +
> > > if (!is_safe_path_component(name) ||
> > > !is_safe_path_component(newname)) {
> > > fuse_reply_err(req, EINVAL);
> > > return;
> > > @@ -1405,6 +1435,11 @@ static void lo_unlink(fuse_req_t req, fuse_ino_t
> > > parent, const char *name)
> > > struct lo_inode *inode;
> > > struct lo_data *lo = lo_data(req);
> > >
> > > + if (is_empty(name)) {
> > > + fuse_reply_err(req, ENOENT);
> > > + return;
> > > + }
> > > +
> > > if (!is_safe_path_component(name)) {
> > > fuse_reply_err(req, EINVAL);
> > > return;
> > > --
> > > 2.26.2
> > >
> > > _______________________________________________
> > > Virtio-fs mailing list
> > > virtio...@redhat.com
> > > https://listman.redhat.com/mailman/listinfo/virtio-fs
> >
>
> _______________________________________________
> Virtio-fs mailing list
> virtio...@redhat.com
> https://listman.redhat.com/mailman/listinfo/virtio-fs
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
