* [PATCH 0/3] virtiofsd: Deal with empty filenames
@ 2021-03-12 14:10 ` Greg Kurz
0 siblings, 0 replies; 19+ messages in thread
From: Greg Kurz @ 2021-03-12 14:10 UTC (permalink / raw)
To: qemu-devel
Cc: virtio-fs, Miklos Szeredi, Dr. David Alan Gilbert,
Stefan Hajnoczi, 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
^ permalink raw reply [flat|nested] 19+ messages in thread* [Virtio-fs] [PATCH 0/3] virtiofsd: Deal with empty filenames @ 2021-03-12 14:10 ` Greg Kurz 0 siblings, 0 replies; 19+ messages in thread From: Greg Kurz @ 2021-03-12 14:10 UTC (permalink / raw) To: qemu-devel; +Cc: virtio-fs 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 1/3] virtiofsd: Don't allow empty paths in lookup_name() 2021-03-12 14:10 ` [Virtio-fs] " Greg Kurz @ 2021-03-12 14:10 ` Greg Kurz -1 siblings, 0 replies; 19+ messages in thread From: Greg Kurz @ 2021-03-12 14:10 UTC (permalink / raw) To: qemu-devel Cc: virtio-fs, Miklos Szeredi, Dr. David Alan Gilbert, Stefan Hajnoczi, 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [Virtio-fs] [PATCH 1/3] virtiofsd: Don't allow empty paths in lookup_name() @ 2021-03-12 14:10 ` Greg Kurz 0 siblings, 0 replies; 19+ messages in thread From: Greg Kurz @ 2021-03-12 14:10 UTC (permalink / raw) To: qemu-devel; +Cc: virtio-fs 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [Virtio-fs] [PATCH 1/3] virtiofsd: Don't allow empty paths in lookup_name() 2021-03-12 14:10 ` [Virtio-fs] " Greg Kurz (?) @ 2021-03-12 15:13 ` Connor Kuehl 2021-03-12 15:49 ` Greg Kurz -1 siblings, 1 reply; 19+ messages in thread From: Connor Kuehl @ 2021-03-12 15:13 UTC (permalink / raw) To: Greg Kurz, qemu-devel; +Cc: virtio-fs 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> ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Virtio-fs] [PATCH 1/3] virtiofsd: Don't allow empty paths in lookup_name() 2021-03-12 15:13 ` Connor Kuehl @ 2021-03-12 15:49 ` Greg Kurz 0 siblings, 0 replies; 19+ messages in thread From: Greg Kurz @ 2021-03-12 15:49 UTC (permalink / raw) To: Connor Kuehl; +Cc: virtio-fs, qemu-devel 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 ! ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Virtio-fs] [PATCH 1/3] virtiofsd: Don't allow empty paths in lookup_name() 2021-03-12 14:10 ` [Virtio-fs] " Greg Kurz (?) (?) @ 2021-03-14 23:38 ` Vivek Goyal -1 siblings, 0 replies; 19+ messages in thread From: Vivek Goyal @ 2021-03-14 23:38 UTC (permalink / raw) To: Greg Kurz; +Cc: virtio-fs, qemu-devel 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 2/3] virtiofsd: Convert some functions to return bool 2021-03-12 14:10 ` [Virtio-fs] " Greg Kurz @ 2021-03-12 14:10 ` Greg Kurz -1 siblings, 0 replies; 19+ messages in thread From: Greg Kurz @ 2021-03-12 14:10 UTC (permalink / raw) To: qemu-devel Cc: virtio-fs, Miklos Szeredi, Dr. David Alan Gilbert, Stefan Hajnoczi, 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [Virtio-fs] [PATCH 2/3] virtiofsd: Convert some functions to return bool @ 2021-03-12 14:10 ` Greg Kurz 0 siblings, 0 replies; 19+ messages in thread From: Greg Kurz @ 2021-03-12 14:10 UTC (permalink / raw) To: qemu-devel; +Cc: virtio-fs 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [Virtio-fs] [PATCH 2/3] virtiofsd: Convert some functions to return bool 2021-03-12 14:10 ` [Virtio-fs] " Greg Kurz (?) @ 2021-03-12 15:13 ` Connor Kuehl -1 siblings, 0 replies; 19+ messages in thread From: Connor Kuehl @ 2021-03-12 15:13 UTC (permalink / raw) To: Greg Kurz, qemu-devel; +Cc: virtio-fs 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> ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Virtio-fs] [PATCH 2/3] virtiofsd: Convert some functions to return bool 2021-03-12 14:10 ` [Virtio-fs] " Greg Kurz (?) (?) @ 2021-03-14 23:36 ` Vivek Goyal -1 siblings, 0 replies; 19+ messages in thread From: Vivek Goyal @ 2021-03-14 23:36 UTC (permalink / raw) To: Greg Kurz; +Cc: virtio-fs, qemu-devel 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 3/3] virtiofsd: Don't allow empty filenames 2021-03-12 14:10 ` [Virtio-fs] " Greg Kurz @ 2021-03-12 14:10 ` Greg Kurz -1 siblings, 0 replies; 19+ messages in thread From: Greg Kurz @ 2021-03-12 14:10 UTC (permalink / raw) To: qemu-devel Cc: virtio-fs, Miklos Szeredi, Dr. David Alan Gilbert, Stefan Hajnoczi, 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [Virtio-fs] [PATCH 3/3] virtiofsd: Don't allow empty filenames @ 2021-03-12 14:10 ` Greg Kurz 0 siblings, 0 replies; 19+ messages in thread From: Greg Kurz @ 2021-03-12 14:10 UTC (permalink / raw) To: qemu-devel; +Cc: virtio-fs 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [Virtio-fs] [PATCH 3/3] virtiofsd: Don't allow empty filenames 2021-03-12 14:10 ` [Virtio-fs] " Greg Kurz (?) @ 2021-03-12 15:13 ` Connor Kuehl -1 siblings, 0 replies; 19+ messages in thread From: Connor Kuehl @ 2021-03-12 15:13 UTC (permalink / raw) To: Greg Kurz, qemu-devel; +Cc: virtio-fs 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> ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Virtio-fs] [PATCH 3/3] virtiofsd: Don't allow empty filenames 2021-03-12 14:10 ` [Virtio-fs] " Greg Kurz (?) (?) @ 2021-03-14 23:36 ` Vivek Goyal 2021-03-15 10:06 ` Greg Kurz -1 siblings, 1 reply; 19+ messages in thread From: Vivek Goyal @ 2021-03-14 23:36 UTC (permalink / raw) To: Greg Kurz; +Cc: virtio-fs, qemu-devel 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Virtio-fs] [PATCH 3/3] virtiofsd: Don't allow empty filenames 2021-03-14 23:36 ` Vivek Goyal @ 2021-03-15 10:06 ` Greg Kurz 2021-03-15 15:18 ` Dr. David Alan Gilbert 2021-03-15 17:55 ` Vivek Goyal 0 siblings, 2 replies; 19+ messages in thread From: Greg Kurz @ 2021-03-15 10:06 UTC (permalink / raw) To: Vivek Goyal; +Cc: virtio-fs, qemu-devel 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 > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Virtio-fs] [PATCH 3/3] virtiofsd: Don't allow empty filenames 2021-03-15 10:06 ` Greg Kurz @ 2021-03-15 15:18 ` Dr. David Alan Gilbert 2021-03-15 17:37 ` Greg Kurz 2021-03-15 17:55 ` Vivek Goyal 1 sibling, 1 reply; 19+ messages in thread From: Dr. David Alan Gilbert @ 2021-03-15 15:18 UTC (permalink / raw) To: Greg Kurz; +Cc: virtio-fs, qemu-devel, Vivek Goyal * 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Virtio-fs] [PATCH 3/3] virtiofsd: Don't allow empty filenames 2021-03-15 15:18 ` Dr. David Alan Gilbert @ 2021-03-15 17:37 ` Greg Kurz 0 siblings, 0 replies; 19+ messages in thread From: Greg Kurz @ 2021-03-15 17:37 UTC (permalink / raw) To: Dr. David Alan Gilbert; +Cc: virtio-fs, qemu-devel, Vivek Goyal 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Virtio-fs] [PATCH 3/3] virtiofsd: Don't allow empty filenames 2021-03-15 10:06 ` Greg Kurz 2021-03-15 15:18 ` Dr. David Alan Gilbert @ 2021-03-15 17:55 ` Vivek Goyal 1 sibling, 0 replies; 19+ messages in thread From: Vivek Goyal @ 2021-03-15 17:55 UTC (permalink / raw) To: Greg Kurz; +Cc: virtio-fs, qemu-devel 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 > > > ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2021-03-15 17:55 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-03-12 14:10 [PATCH 0/3] virtiofsd: Deal with empty filenames Greg Kurz 2021-03-12 14:10 ` [Virtio-fs] " Greg Kurz 2021-03-12 14:10 ` [PATCH 1/3] virtiofsd: Don't allow empty paths in lookup_name() Greg Kurz 2021-03-12 14:10 ` [Virtio-fs] " Greg Kurz 2021-03-12 15:13 ` Connor Kuehl 2021-03-12 15:49 ` Greg Kurz 2021-03-14 23:38 ` Vivek Goyal 2021-03-12 14:10 ` [PATCH 2/3] virtiofsd: Convert some functions to return bool Greg Kurz 2021-03-12 14:10 ` [Virtio-fs] " Greg Kurz 2021-03-12 15:13 ` Connor Kuehl 2021-03-14 23:36 ` Vivek Goyal 2021-03-12 14:10 ` [PATCH 3/3] virtiofsd: Don't allow empty filenames Greg Kurz 2021-03-12 14:10 ` [Virtio-fs] " Greg Kurz 2021-03-12 15:13 ` Connor Kuehl 2021-03-14 23:36 ` Vivek Goyal 2021-03-15 10:06 ` Greg Kurz 2021-03-15 15:18 ` Dr. David Alan Gilbert 2021-03-15 17:37 ` Greg Kurz 2021-03-15 17:55 ` Vivek Goyal
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.