[PATCH 0/3] virtiofsd: Deal with empty filenames

[PATCH 0/3] virtiofsd: Deal with empty filenames

[Greg Kurz]

There's no such thing as an empty file name in POSIX-compliant file systems.
The current code base doesn't ensure the client doesn't send requests with
such empty names. I've audited the code and only found one place where
the behavior is somewhat altered in lookup_name() :

    res = do_statx(lo, dir->fd, name, &attr,
                   AT_EMPTY_PATH | AT_SYMLINK_NOFOLLOW, &mnt_id);

lookup_name() is used by lo_rmdir(), lo_rename() and lo_unlink() which
all share the same behavior of doing some action on a file or directory
under a given parent directory. But if an empty name reaches the code
above, do_statx() returns the attributes of the parent directory itself
and lookup_name() might return the inode of the parent directory. This
could potentially cause security concerns in the callers.

Fortunately, it doesn't as of today. If the parent directory is the root
inode, lookup_name() returns NULL because lo_find() fails to find an
inode with a matching .st_dev. Otherwise, lookup_name() does return the
parent inode but the empty name then gets passed to either unlinkat(),
renameat() or renameat2(), all of which fail with ENOENT in this case.

Drop AT_EMPTY_PATH from the above code anyway to make it clear empty
names aren't expected by the existing callers. If the need for it
arises in the future, it can be added back but stay safe for now.

The FUSE protocol doesn't have a notion of AT_EMPTY_PATH actually. The
server should hence never see empty names in client requests. Detect
this early and systematically fail the request with ENOENT in this
case.

No regression is observed with the POSIX-oriented pjdfstest file system
test suite (https://github.com/pjd/pjdfstest).

Greg Kurz (3):
  virtiofsd: Don't allow empty paths in lookup_name()
  virtiofsd: Convert some functions to return bool
  virtiofsd: Don't allow empty filenames

 tools/virtiofsd/passthrough_ll.c | 44 ++++++++++++++++++++++++++++----
 1 file changed, 39 insertions(+), 5 deletions(-)

-- 
2.26.2

[Virtio-fs] [PATCH 0/3] virtiofsd: Deal with empty filenames

[Greg Kurz]

There's no such thing as an empty file name in POSIX-compliant file systems.
The current code base doesn't ensure the client doesn't send requests with
such empty names. I've audited the code and only found one place where
the behavior is somewhat altered in lookup_name() :

    res = do_statx(lo, dir->fd, name, &attr,
                   AT_EMPTY_PATH | AT_SYMLINK_NOFOLLOW, &mnt_id);

lookup_name() is used by lo_rmdir(), lo_rename() and lo_unlink() which
all share the same behavior of doing some action on a file or directory
under a given parent directory. But if an empty name reaches the code
above, do_statx() returns the attributes of the parent directory itself
and lookup_name() might return the inode of the parent directory. This
could potentially cause security concerns in the callers.

Fortunately, it doesn't as of today. If the parent directory is the root
inode, lookup_name() returns NULL because lo_find() fails to find an
inode with a matching .st_dev. Otherwise, lookup_name() does return the
parent inode but the empty name then gets passed to either unlinkat(),
renameat() or renameat2(), all of which fail with ENOENT in this case.

Drop AT_EMPTY_PATH from the above code anyway to make it clear empty
names aren't expected by the existing callers. If the need for it
arises in the future, it can be added back but stay safe for now.

The FUSE protocol doesn't have a notion of AT_EMPTY_PATH actually. The
server should hence never see empty names in client requests. Detect
this early and systematically fail the request with ENOENT in this
case.

No regression is observed with the POSIX-oriented pjdfstest file system
test suite (https://github.com/pjd/pjdfstest).

Greg Kurz (3):
  virtiofsd: Don't allow empty paths in lookup_name()
  virtiofsd: Convert some functions to return bool
  virtiofsd: Don't allow empty filenames

 tools/virtiofsd/passthrough_ll.c | 44 ++++++++++++++++++++++++++++----
 1 file changed, 39 insertions(+), 5 deletions(-)

-- 
2.26.2

[PATCH 1/3] virtiofsd: Don't allow empty paths in lookup_name()

[Greg Kurz]

When passed an empty filename, lookup_name() returns the inode of
the parent directory, unless the parent is the root in which case
the st_dev doesn't match and lo_find() returns NULL. This is
because lookup_name() passes AT_EMPTY_PATH down to fstatat() or
statx().

This behavior doesn't quite make sense because users of lookup_name()
then pass the name to unlinkat(), renameat() or renameat2(), all of
which will always fail on empty names.

Drop AT_EMPTY_PATH from the flags in lookup_name() so that it has
the consistent behavior of "returning an existing child inode or
NULL" for all directories.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 tools/virtiofsd/passthrough_ll.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index fc7e1b1e8e2b..27a6c636dcaf 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -1308,8 +1308,7 @@ static struct lo_inode *lookup_name(fuse_req_t req, fuse_ino_t parent,
         return NULL;
     }
 
-    res = do_statx(lo, dir->fd, name, &attr,
-                   AT_EMPTY_PATH | AT_SYMLINK_NOFOLLOW, &mnt_id);
+    res = do_statx(lo, dir->fd, name, &attr, AT_SYMLINK_NOFOLLOW, &mnt_id);
     lo_inode_put(lo, &dir);
     if (res == -1) {
         return NULL;
-- 
2.26.2

[Virtio-fs] [PATCH 1/3] virtiofsd: Don't allow empty paths in lookup_name()

[Greg Kurz]

When passed an empty filename, lookup_name() returns the inode of
the parent directory, unless the parent is the root in which case
the st_dev doesn't match and lo_find() returns NULL. This is
because lookup_name() passes AT_EMPTY_PATH down to fstatat() or
statx().

This behavior doesn't quite make sense because users of lookup_name()
then pass the name to unlinkat(), renameat() or renameat2(), all of
which will always fail on empty names.

Drop AT_EMPTY_PATH from the flags in lookup_name() so that it has
the consistent behavior of "returning an existing child inode or
NULL" for all directories.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 tools/virtiofsd/passthrough_ll.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index fc7e1b1e8e2b..27a6c636dcaf 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -1308,8 +1308,7 @@ static struct lo_inode *lookup_name(fuse_req_t req, fuse_ino_t parent,
         return NULL;
     }
 
-    res = do_statx(lo, dir->fd, name, &attr,
-                   AT_EMPTY_PATH | AT_SYMLINK_NOFOLLOW, &mnt_id);
+    res = do_statx(lo, dir->fd, name, &attr, AT_SYMLINK_NOFOLLOW, &mnt_id);
     lo_inode_put(lo, &dir);
     if (res == -1) {
         return NULL;
-- 
2.26.2

[PATCH 2/3] virtiofsd: Convert some functions to return bool

[Greg Kurz]

Both currently only return 0 or 1.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 tools/virtiofsd/passthrough_ll.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 27a6c636dcaf..f63016d35626 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -221,17 +221,17 @@ static struct lo_inode *lo_find(struct lo_data *lo, struct stat *st,
 static int xattr_map_client(const struct lo_data *lo, const char *client_name,
                             char **out_name);
 
-static int is_dot_or_dotdot(const char *name)
+static bool is_dot_or_dotdot(const char *name)
 {
     return name[0] == '.' &&
            (name[1] == '\0' || (name[1] == '.' && name[2] == '\0'));
 }
 
 /* Is `path` a single path component that is not "." or ".."? */
-static int is_safe_path_component(const char *path)
+static bool is_safe_path_component(const char *path)
 {
     if (strchr(path, '/')) {
-        return 0;
+        return false;
     }
 
     return !is_dot_or_dotdot(path);
-- 
2.26.2

[Virtio-fs] [PATCH 2/3] virtiofsd: Convert some functions to return bool

[Greg Kurz]

Both currently only return 0 or 1.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 tools/virtiofsd/passthrough_ll.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 27a6c636dcaf..f63016d35626 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -221,17 +221,17 @@ static struct lo_inode *lo_find(struct lo_data *lo, struct stat *st,
 static int xattr_map_client(const struct lo_data *lo, const char *client_name,
                             char **out_name);
 
-static int is_dot_or_dotdot(const char *name)
+static bool is_dot_or_dotdot(const char *name)
 {
     return name[0] == '.' &&
            (name[1] == '\0' || (name[1] == '.' && name[2] == '\0'));
 }
 
 /* Is `path` a single path component that is not "." or ".."? */
-static int is_safe_path_component(const char *path)
+static bool is_safe_path_component(const char *path)
 {
     if (strchr(path, '/')) {
-        return 0;
+        return false;
     }
 
     return !is_dot_or_dotdot(path);
-- 
2.26.2

[PATCH 3/3] virtiofsd: Don't allow empty filenames

[Greg Kurz]

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 <groug@kaod.org>
---
 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] [PATCH 3/3] virtiofsd: Don't allow empty filenames

[Greg Kurz]

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 <groug@kaod.org>
---
 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

Re: [Virtio-fs] [PATCH 1/3] virtiofsd: Don't allow empty paths in lookup_name()

[Connor Kuehl]

On 3/12/21 8:10 AM, Greg Kurz wrote:
> When passed an empty filename, lookup_name() returns the inode of
> the parent directory, unless the parent is the root in which case
> the st_dev doesn't match and lo_find() returns NULL. This is
> because lookup_name() passes AT_EMPTY_PATH down to fstatat() or
> statx().
> 
> This behavior doesn't quite make sense because users of lookup_name()
> then pass the name to unlinkat(), renameat() or renameat2(), all of
> which will always fail on empty names.
> 
> Drop AT_EMPTY_PATH from the flags in lookup_name() so that it has
> the consistent behavior of "returning an existing child inode or
> NULL" for all directories.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>   tools/virtiofsd/passthrough_ll.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> index fc7e1b1e8e2b..27a6c636dcaf 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -1308,8 +1308,7 @@ static struct lo_inode *lookup_name(fuse_req_t req, fuse_ino_t parent,
>           return NULL;
>       }
>   
> -    res = do_statx(lo, dir->fd, name, &attr,
> -                   AT_EMPTY_PATH | AT_SYMLINK_NOFOLLOW, &mnt_id);
> +    res = do_statx(lo, dir->fd, name, &attr, AT_SYMLINK_NOFOLLOW, &mnt_id);
>       lo_inode_put(lo, &dir);
>       if (res == -1) {
>           return NULL;
> 

Should the statx() in lo_do_lookup() have this flag removed as well? I 
don't think its callers will pass in an empty name because:

   - One of your later patches prevents lo_mknod_symlink() from doing so
   - lo_create() will fail an earlier call against the host file system 
(open)
   - lo_do_readdir() shouldn't be reading empty names because that 
implies someone was able to pull off making an entry with an empty name

However, I don't know if there will one day be future callers to 
lo_do_lookup() that will depend on that flag.

If the answer to the above is no, then:

Reviewed-by: Connor Kuehl <ckuehl@redhat.com>

Re: [Virtio-fs] [PATCH 2/3] virtiofsd: Convert some functions to return bool

[Connor Kuehl]

On 3/12/21 8:10 AM, Greg Kurz wrote:
> Both currently only return 0 or 1.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>   tools/virtiofsd/passthrough_ll.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> index 27a6c636dcaf..f63016d35626 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -221,17 +221,17 @@ static struct lo_inode *lo_find(struct lo_data *lo, struct stat *st,
>   static int xattr_map_client(const struct lo_data *lo, const char *client_name,
>                               char **out_name);
>   
> -static int is_dot_or_dotdot(const char *name)
> +static bool is_dot_or_dotdot(const char *name)
>   {
>       return name[0] == '.' &&
>              (name[1] == '\0' || (name[1] == '.' && name[2] == '\0'));
>   }
>   
>   /* Is `path` a single path component that is not "." or ".."? */
> -static int is_safe_path_component(const char *path)
> +static bool is_safe_path_component(const char *path)
>   {
>       if (strchr(path, '/')) {
> -        return 0;
> +        return false;
>       }
>   
>       return !is_dot_or_dotdot(path);
> 

Reviewed-by: Connor Kuehl <ckuehl@redhat.com>

Re: [Virtio-fs] [PATCH 3/3] virtiofsd: Don't allow empty filenames

[Connor Kuehl]

On 3/12/21 8:10 AM, 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 <groug@kaod.org>

Reviewed-by: Connor Kuehl <ckuehl@redhat.com>

Re: [Virtio-fs] [PATCH 1/3] virtiofsd: Don't allow empty paths in lookup_name()

[Greg Kurz]

On Fri, 12 Mar 2021 09:13:36 -0600
Connor Kuehl <ckuehl@redhat.com> wrote:

> On 3/12/21 8:10 AM, Greg Kurz wrote:
> > When passed an empty filename, lookup_name() returns the inode of
> > the parent directory, unless the parent is the root in which case
> > the st_dev doesn't match and lo_find() returns NULL. This is
> > because lookup_name() passes AT_EMPTY_PATH down to fstatat() or
> > statx().
> > 
> > This behavior doesn't quite make sense because users of lookup_name()
> > then pass the name to unlinkat(), renameat() or renameat2(), all of
> > which will always fail on empty names.
> > 
> > Drop AT_EMPTY_PATH from the flags in lookup_name() so that it has
> > the consistent behavior of "returning an existing child inode or
> > NULL" for all directories.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >   tools/virtiofsd/passthrough_ll.c | 3 +--
> >   1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> > index fc7e1b1e8e2b..27a6c636dcaf 100644
> > --- a/tools/virtiofsd/passthrough_ll.c
> > +++ b/tools/virtiofsd/passthrough_ll.c
> > @@ -1308,8 +1308,7 @@ static struct lo_inode *lookup_name(fuse_req_t req, fuse_ino_t parent,
> >           return NULL;
> >       }
> >   
> > -    res = do_statx(lo, dir->fd, name, &attr,
> > -                   AT_EMPTY_PATH | AT_SYMLINK_NOFOLLOW, &mnt_id);
> > +    res = do_statx(lo, dir->fd, name, &attr, AT_SYMLINK_NOFOLLOW, &mnt_id);
> >       lo_inode_put(lo, &dir);
> >       if (res == -1) {
> >           return NULL;
> > 
> 
> Should the statx() in lo_do_lookup() have this flag removed as well? I 

This is different.

    newfd = openat(dir->fd, name, O_PATH | O_NOFOLLOW);
    if (newfd == -1) {
        goto out_err;
    }

    res = do_statx(lo, newfd, "", &e->attr, AT_EMPTY_PATH | AT_SYMLINK_NOFOLLOW,
                   &mnt_id);
    if (res == -1) {
        goto out_err;
    }

No client originated name is passed to do_statx() in this case. On the
contrary, AT_EMPTY_PATH is provided on purpose since the third argument
is "". The goal here is to get the attributes of the entry newfd points
to.

Same stands for other occurences of AT_EMPTY_PATH with fstatat() in
lo_getattr() and lo_link(), fchownat() in lo_setattr() or do_statx()
again in setup_root().

> don't think its callers will pass in an empty name because:
> 
>    - One of your later patches prevents lo_mknod_symlink() from doing so
>    - lo_create() will fail an earlier call against the host file system 
> (open)
>    - lo_do_readdir() shouldn't be reading empty names because that 
> implies someone was able to pull off making an entry with an empty name
> 
> However, I don't know if there will one day be future callers to 
> lo_do_lookup() that will depend on that flag.
> 
> If the answer to the above is no, then:
> 

It is :)

> Reviewed-by: Connor Kuehl <ckuehl@redhat.com>
> 

Thanks !

Re: [Virtio-fs] [PATCH 3/3] virtiofsd: Don't allow empty filenames

[Vivek Goyal]

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 <groug@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.

What about lo_create(). Should we put a check in there as well.

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.

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-fs@redhat.com
> https://listman.redhat.com/mailman/listinfo/virtio-fs

Re: [Virtio-fs] [PATCH 2/3] virtiofsd: Convert some functions to return bool

[Vivek Goyal]

On Fri, Mar 12, 2021 at 03:10:02PM +0100, Greg Kurz wrote:
> Both currently only return 0 or 1.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Looks good to me.

Reviewed-by: Vivek Goyal <vgoyal@redhat.com>

Vivek

> ---
>  tools/virtiofsd/passthrough_ll.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> index 27a6c636dcaf..f63016d35626 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -221,17 +221,17 @@ static struct lo_inode *lo_find(struct lo_data *lo, struct stat *st,
>  static int xattr_map_client(const struct lo_data *lo, const char *client_name,
>                              char **out_name);
>  
> -static int is_dot_or_dotdot(const char *name)
> +static bool is_dot_or_dotdot(const char *name)
>  {
>      return name[0] == '.' &&
>             (name[1] == '\0' || (name[1] == '.' && name[2] == '\0'));
>  }
>  
>  /* Is `path` a single path component that is not "." or ".."? */
> -static int is_safe_path_component(const char *path)
> +static bool is_safe_path_component(const char *path)
>  {
>      if (strchr(path, '/')) {
> -        return 0;
> +        return false;
>      }
>  
>      return !is_dot_or_dotdot(path);
> -- 
> 2.26.2
> 
> _______________________________________________
> Virtio-fs mailing list
> Virtio-fs@redhat.com
> https://listman.redhat.com/mailman/listinfo/virtio-fs

Re: [Virtio-fs] [PATCH 1/3] virtiofsd: Don't allow empty paths in lookup_name()

[Vivek Goyal]

On Fri, Mar 12, 2021 at 03:10:01PM +0100, Greg Kurz wrote:
> When passed an empty filename, lookup_name() returns the inode of
> the parent directory, unless the parent is the root in which case
> the st_dev doesn't match and lo_find() returns NULL. This is
> because lookup_name() passes AT_EMPTY_PATH down to fstatat() or
> statx().
> 
> This behavior doesn't quite make sense because users of lookup_name()
> then pass the name to unlinkat(), renameat() or renameat2(), all of
> which will always fail on empty names.
> 
> Drop AT_EMPTY_PATH from the flags in lookup_name() so that it has
> the consistent behavior of "returning an existing child inode or
> NULL" for all directories.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Looks good to me. Can't think in what cases fuse will need to pass in
empty path for lookup.

Reviewed-by: Vivek Goyal <vgoyal@redhat.com>

Vivek
> ---
>  tools/virtiofsd/passthrough_ll.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> index fc7e1b1e8e2b..27a6c636dcaf 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -1308,8 +1308,7 @@ static struct lo_inode *lookup_name(fuse_req_t req, fuse_ino_t parent,
>          return NULL;
>      }
>  
> -    res = do_statx(lo, dir->fd, name, &attr,
> -                   AT_EMPTY_PATH | AT_SYMLINK_NOFOLLOW, &mnt_id);
> +    res = do_statx(lo, dir->fd, name, &attr, AT_SYMLINK_NOFOLLOW, &mnt_id);
>      lo_inode_put(lo, &dir);
>      if (res == -1) {
>          return NULL;
> -- 
> 2.26.2
> 
> _______________________________________________
> Virtio-fs mailing list
> Virtio-fs@redhat.com
> https://listman.redhat.com/mailman/listinfo/virtio-fs

Re: [Virtio-fs] [PATCH 3/3] virtiofsd: Don't allow empty filenames

[Greg Kurz]

On Sun, 14 Mar 2021 19:36:04 -0400
Vivek Goyal <vgoyal@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 <groug@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 ;)

> 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-fs@redhat.com
> > https://listman.redhat.com/mailman/listinfo/virtio-fs
> 

Re: [Virtio-fs] [PATCH 3/3] virtiofsd: Don't allow empty filenames

[Dr. David Alan Gilbert]

* Greg Kurz (groug@kaod.org) wrote:
> On Sun, 14 Mar 2021 19:36:04 -0400
> Vivek Goyal <vgoyal@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 <groug@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-fs@redhat.com
> > > https://listman.redhat.com/mailman/listinfo/virtio-fs
> > 
> 
> _______________________________________________
> Virtio-fs mailing list
> Virtio-fs@redhat.com
> https://listman.redhat.com/mailman/listinfo/virtio-fs
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Virtio-fs] [PATCH 3/3] virtiofsd: Don't allow empty filenames

[Greg Kurz]

On Mon, 15 Mar 2021 15:18:48 +0000
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:

> * Greg Kurz (groug@kaod.org) wrote:
> > On Sun, 14 Mar 2021 19:36:04 -0400
> > Vivek Goyal <vgoyal@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 <groug@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.
> 

Sure, I'll do that.

Cheers,

--
Greg

Re: [Virtio-fs] [PATCH 3/3] virtiofsd: Don't allow empty filenames

[Vivek Goyal]

On Mon, Mar 15, 2021 at 11:06:30AM +0100, Greg Kurz wrote:
> On Sun, 14 Mar 2021 19:36:04 -0400
> Vivek Goyal <vgoyal@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 <groug@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 ;)
> 
> > 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.

Fair enough. Lets not worry about empty name for xattr calls. That's
probably for some other day.

Vivek

> 
> 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-fs@redhat.com
> > > https://listman.redhat.com/mailman/listinfo/virtio-fs
> > 
>