* [PATCH v4 0/3] virtiofsd: prevent opening of special files (CVE-2020-35517)
@ 2021-02-03 11:37 ` Stefan Hajnoczi
0 siblings, 0 replies; 48+ messages in thread
From: Stefan Hajnoczi @ 2021-02-03 11:37 UTC (permalink / raw)
To: qemu-devel
Cc: mszeredi, Daniel Berrange, slp, Dr. David Alan Gilbert,
Greg Kurz, virtio-fs, Alex Xu, Stefan Hajnoczi, P J P,
Laszlo Ersek, vgoyal
v3:
* Restructure lo_create() to handle externally-created files (we need
to allocate an inode for them) [Greg]
* Patch 1 & 2 refactor the code so that Patch 3 can implement the CVE fix
v3:
* Protect lo_create() [Greg]
v2:
* Add doc comment clarifying that symlinks are traversed client-side
[Daniel]
A well-behaved FUSE client does not attempt to open special files with
FUSE_OPEN because they are handled on the client side (e.g. device nodes
are handled by client-side device drivers).
The check to prevent virtiofsd from opening special files is missing in
a few cases, most notably FUSE_OPEN. A malicious client can cause
virtiofsd to open a device node, potentially allowing the guest to
escape. This can be exploited by a modified guest device driver. It is
not exploitable from guest userspace since the guest kernel will handle
special files inside the guest instead of sending FUSE requests.
This patch series fixes this issue by introducing the lo_inode_open() function
to check the file type before opening it. This is a short-term solution because
it does not prevent a compromised virtiofsd process from opening device nodes
on the host.
This issue was diagnosed on public IRC and is therefore already known
and not embargoed.
Reported-by: Alex Xu <alex@alxu.ca>
Fixes: CVE-2020-35517
Stefan Hajnoczi (3):
virtiofsd: extract lo_do_open() from lo_open()
virtiofsd: optionally return inode pointer from lo_do_lookup()
virtiofsd: prevent opening of special files (CVE-2020-35517)
tools/virtiofsd/passthrough_ll.c | 221 ++++++++++++++++++++-----------
1 file changed, 145 insertions(+), 76 deletions(-)
--
2.29.2
^ permalink raw reply [flat|nested] 48+ messages in thread
* [Virtio-fs] [PATCH v4 0/3] virtiofsd: prevent opening of special files (CVE-2020-35517)
@ 2021-02-03 11:37 ` Stefan Hajnoczi
0 siblings, 0 replies; 48+ messages in thread
From: Stefan Hajnoczi @ 2021-02-03 11:37 UTC (permalink / raw)
To: qemu-devel
Cc: Daniel Berrange, virtio-fs, Alex Xu, P J P, Laszlo Ersek, vgoyal
v3:
* Restructure lo_create() to handle externally-created files (we need
to allocate an inode for them) [Greg]
* Patch 1 & 2 refactor the code so that Patch 3 can implement the CVE fix
v3:
* Protect lo_create() [Greg]
v2:
* Add doc comment clarifying that symlinks are traversed client-side
[Daniel]
A well-behaved FUSE client does not attempt to open special files with
FUSE_OPEN because they are handled on the client side (e.g. device nodes
are handled by client-side device drivers).
The check to prevent virtiofsd from opening special files is missing in
a few cases, most notably FUSE_OPEN. A malicious client can cause
virtiofsd to open a device node, potentially allowing the guest to
escape. This can be exploited by a modified guest device driver. It is
not exploitable from guest userspace since the guest kernel will handle
special files inside the guest instead of sending FUSE requests.
This patch series fixes this issue by introducing the lo_inode_open() function
to check the file type before opening it. This is a short-term solution because
it does not prevent a compromised virtiofsd process from opening device nodes
on the host.
This issue was diagnosed on public IRC and is therefore already known
and not embargoed.
Reported-by: Alex Xu <alex@alxu.ca>
Fixes: CVE-2020-35517
Stefan Hajnoczi (3):
virtiofsd: extract lo_do_open() from lo_open()
virtiofsd: optionally return inode pointer from lo_do_lookup()
virtiofsd: prevent opening of special files (CVE-2020-35517)
tools/virtiofsd/passthrough_ll.c | 221 ++++++++++++++++++++-----------
1 file changed, 145 insertions(+), 76 deletions(-)
--
2.29.2
^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH v4 1/3] virtiofsd: extract lo_do_open() from lo_open()
2021-02-03 11:37 ` [Virtio-fs] " Stefan Hajnoczi
@ 2021-02-03 11:37 ` Stefan Hajnoczi
-1 siblings, 0 replies; 48+ messages in thread
From: Stefan Hajnoczi @ 2021-02-03 11:37 UTC (permalink / raw)
To: qemu-devel
Cc: mszeredi, Daniel Berrange, slp, Dr. David Alan Gilbert,
Greg Kurz, virtio-fs, Stefan Hajnoczi, P J P, Laszlo Ersek,
vgoyal
Both lo_open() and lo_create() have similar code to open a file. Extract
a common lo_do_open() function from lo_open() that will be used by
lo_create() in a later commit.
Since lo_do_open() does not otherwise need fuse_req_t req, convert
lo_add_fd_mapping() to use struct lo_data *lo instead.
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
tools/virtiofsd/passthrough_ll.c | 73 ++++++++++++++++++++------------
1 file changed, 46 insertions(+), 27 deletions(-)
diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 5fb36d9407..e63cbd3fb7 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -459,17 +459,17 @@ static void lo_map_remove(struct lo_map *map, size_t key)
}
/* Assumes lo->mutex is held */
-static ssize_t lo_add_fd_mapping(fuse_req_t req, int fd)
+static ssize_t lo_add_fd_mapping(struct lo_data *lo, int fd)
{
struct lo_map_elem *elem;
- elem = lo_map_alloc_elem(&lo_data(req)->fd_map);
+ elem = lo_map_alloc_elem(&lo->fd_map);
if (!elem) {
return -1;
}
elem->fd = fd;
- return elem - lo_data(req)->fd_map.elems;
+ return elem - lo->fd_map.elems;
}
/* Assumes lo->mutex is held */
@@ -1651,6 +1651,38 @@ static void update_open_flags(int writeback, int allow_direct_io,
}
}
+static int lo_do_open(struct lo_data *lo, struct lo_inode *inode,
+ struct fuse_file_info *fi)
+{
+ char buf[64];
+ ssize_t fh;
+ int fd;
+
+ update_open_flags(lo->writeback, lo->allow_direct_io, fi);
+
+ sprintf(buf, "%i", inode->fd);
+ fd = openat(lo->proc_self_fd, buf, fi->flags & ~O_NOFOLLOW);
+ if (fd == -1) {
+ return -errno;
+ }
+
+ pthread_mutex_lock(&lo->mutex);
+ fh = lo_add_fd_mapping(lo, fd);
+ pthread_mutex_unlock(&lo->mutex);
+ if (fh == -1) {
+ close(fd);
+ return ENOMEM;
+ }
+
+ fi->fh = fh;
+ if (lo->cache == CACHE_NONE) {
+ fi->direct_io = 1;
+ } else if (lo->cache == CACHE_ALWAYS) {
+ fi->keep_cache = 1;
+ }
+ return 0;
+}
+
static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
mode_t mode, struct fuse_file_info *fi)
{
@@ -1691,7 +1723,7 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
ssize_t fh;
pthread_mutex_lock(&lo->mutex);
- fh = lo_add_fd_mapping(req, fd);
+ fh = lo_add_fd_mapping(lo, fd);
pthread_mutex_unlock(&lo->mutex);
if (fh == -1) {
close(fd);
@@ -1892,38 +1924,25 @@ static void lo_fsyncdir(fuse_req_t req, fuse_ino_t ino, int datasync,
static void lo_open(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
{
- int fd;
- ssize_t fh;
- char buf[64];
struct lo_data *lo = lo_data(req);
+ struct lo_inode *inode = lo_inode(req, ino);
+ int err;
fuse_log(FUSE_LOG_DEBUG, "lo_open(ino=%" PRIu64 ", flags=%d)\n", ino,
fi->flags);
- update_open_flags(lo->writeback, lo->allow_direct_io, fi);
-
- sprintf(buf, "%i", lo_fd(req, ino));
- fd = openat(lo->proc_self_fd, buf, fi->flags & ~O_NOFOLLOW);
- if (fd == -1) {
- return (void)fuse_reply_err(req, errno);
- }
-
- pthread_mutex_lock(&lo->mutex);
- fh = lo_add_fd_mapping(req, fd);
- pthread_mutex_unlock(&lo->mutex);
- if (fh == -1) {
- close(fd);
- fuse_reply_err(req, ENOMEM);
+ if (!inode) {
+ fuse_reply_err(req, EBADF);
return;
}
- fi->fh = fh;
- if (lo->cache == CACHE_NONE) {
- fi->direct_io = 1;
- } else if (lo->cache == CACHE_ALWAYS) {
- fi->keep_cache = 1;
+ err = lo_do_open(lo, inode, fi);
+ lo_inode_put(lo, &inode);
+ if (err) {
+ fuse_reply_err(req, err);
+ } else {
+ fuse_reply_open(req, fi);
}
- fuse_reply_open(req, fi);
}
static void lo_release(fuse_req_t req, fuse_ino_t ino,
--
2.29.2
^ permalink raw reply related [flat|nested] 48+ messages in thread
* [Virtio-fs] [PATCH v4 1/3] virtiofsd: extract lo_do_open() from lo_open()
@ 2021-02-03 11:37 ` Stefan Hajnoczi
0 siblings, 0 replies; 48+ messages in thread
From: Stefan Hajnoczi @ 2021-02-03 11:37 UTC (permalink / raw)
To: qemu-devel; +Cc: Daniel Berrange, virtio-fs, P J P, Laszlo Ersek, vgoyal
Both lo_open() and lo_create() have similar code to open a file. Extract
a common lo_do_open() function from lo_open() that will be used by
lo_create() in a later commit.
Since lo_do_open() does not otherwise need fuse_req_t req, convert
lo_add_fd_mapping() to use struct lo_data *lo instead.
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
tools/virtiofsd/passthrough_ll.c | 73 ++++++++++++++++++++------------
1 file changed, 46 insertions(+), 27 deletions(-)
diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 5fb36d9407..e63cbd3fb7 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -459,17 +459,17 @@ static void lo_map_remove(struct lo_map *map, size_t key)
}
/* Assumes lo->mutex is held */
-static ssize_t lo_add_fd_mapping(fuse_req_t req, int fd)
+static ssize_t lo_add_fd_mapping(struct lo_data *lo, int fd)
{
struct lo_map_elem *elem;
- elem = lo_map_alloc_elem(&lo_data(req)->fd_map);
+ elem = lo_map_alloc_elem(&lo->fd_map);
if (!elem) {
return -1;
}
elem->fd = fd;
- return elem - lo_data(req)->fd_map.elems;
+ return elem - lo->fd_map.elems;
}
/* Assumes lo->mutex is held */
@@ -1651,6 +1651,38 @@ static void update_open_flags(int writeback, int allow_direct_io,
}
}
+static int lo_do_open(struct lo_data *lo, struct lo_inode *inode,
+ struct fuse_file_info *fi)
+{
+ char buf[64];
+ ssize_t fh;
+ int fd;
+
+ update_open_flags(lo->writeback, lo->allow_direct_io, fi);
+
+ sprintf(buf, "%i", inode->fd);
+ fd = openat(lo->proc_self_fd, buf, fi->flags & ~O_NOFOLLOW);
+ if (fd == -1) {
+ return -errno;
+ }
+
+ pthread_mutex_lock(&lo->mutex);
+ fh = lo_add_fd_mapping(lo, fd);
+ pthread_mutex_unlock(&lo->mutex);
+ if (fh == -1) {
+ close(fd);
+ return ENOMEM;
+ }
+
+ fi->fh = fh;
+ if (lo->cache == CACHE_NONE) {
+ fi->direct_io = 1;
+ } else if (lo->cache == CACHE_ALWAYS) {
+ fi->keep_cache = 1;
+ }
+ return 0;
+}
+
static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
mode_t mode, struct fuse_file_info *fi)
{
@@ -1691,7 +1723,7 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
ssize_t fh;
pthread_mutex_lock(&lo->mutex);
- fh = lo_add_fd_mapping(req, fd);
+ fh = lo_add_fd_mapping(lo, fd);
pthread_mutex_unlock(&lo->mutex);
if (fh == -1) {
close(fd);
@@ -1892,38 +1924,25 @@ static void lo_fsyncdir(fuse_req_t req, fuse_ino_t ino, int datasync,
static void lo_open(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
{
- int fd;
- ssize_t fh;
- char buf[64];
struct lo_data *lo = lo_data(req);
+ struct lo_inode *inode = lo_inode(req, ino);
+ int err;
fuse_log(FUSE_LOG_DEBUG, "lo_open(ino=%" PRIu64 ", flags=%d)\n", ino,
fi->flags);
- update_open_flags(lo->writeback, lo->allow_direct_io, fi);
-
- sprintf(buf, "%i", lo_fd(req, ino));
- fd = openat(lo->proc_self_fd, buf, fi->flags & ~O_NOFOLLOW);
- if (fd == -1) {
- return (void)fuse_reply_err(req, errno);
- }
-
- pthread_mutex_lock(&lo->mutex);
- fh = lo_add_fd_mapping(req, fd);
- pthread_mutex_unlock(&lo->mutex);
- if (fh == -1) {
- close(fd);
- fuse_reply_err(req, ENOMEM);
+ if (!inode) {
+ fuse_reply_err(req, EBADF);
return;
}
- fi->fh = fh;
- if (lo->cache == CACHE_NONE) {
- fi->direct_io = 1;
- } else if (lo->cache == CACHE_ALWAYS) {
- fi->keep_cache = 1;
+ err = lo_do_open(lo, inode, fi);
+ lo_inode_put(lo, &inode);
+ if (err) {
+ fuse_reply_err(req, err);
+ } else {
+ fuse_reply_open(req, fi);
}
- fuse_reply_open(req, fi);
}
static void lo_release(fuse_req_t req, fuse_ino_t ino,
--
2.29.2
^ permalink raw reply related [flat|nested] 48+ messages in thread
* [PATCH v4 2/3] virtiofsd: optionally return inode pointer from lo_do_lookup()
2021-02-03 11:37 ` [Virtio-fs] " Stefan Hajnoczi
@ 2021-02-03 11:37 ` Stefan Hajnoczi
-1 siblings, 0 replies; 48+ messages in thread
From: Stefan Hajnoczi @ 2021-02-03 11:37 UTC (permalink / raw)
To: qemu-devel
Cc: mszeredi, Daniel Berrange, slp, Dr. David Alan Gilbert,
Greg Kurz, virtio-fs, Stefan Hajnoczi, P J P, Laszlo Ersek,
vgoyal
lo_do_lookup() finds an existing inode or allocates a new one. It
increments nlookup so that the inode stays alive until the client
releases it.
Existing callers don't need the struct lo_inode so the function doesn't
return it. Extend the function to optionally return the inode. The next
commit will need it.
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
tools/virtiofsd/passthrough_ll.c | 29 +++++++++++++++++++++--------
1 file changed, 21 insertions(+), 8 deletions(-)
diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index e63cbd3fb7..c87a1f3d72 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -831,11 +831,13 @@ static int do_statx(struct lo_data *lo, int dirfd, const char *pathname,
}
/*
- * Increments nlookup and caller must release refcount using
- * lo_inode_put(&parent).
+ * Increments nlookup on the inode on success. unref_inode_lolocked() must be
+ * called eventually to decrement nlookup again. If inodep is non-NULL, the
+ * inode pointer is stored and the caller must call lo_inode_put().
*/
static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
- struct fuse_entry_param *e)
+ struct fuse_entry_param *e,
+ struct lo_inode **inodep)
{
int newfd;
int res;
@@ -845,6 +847,10 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
struct lo_inode *inode = NULL;
struct lo_inode *dir = lo_inode(req, parent);
+ if (inodep) {
+ *inodep = NULL;
+ }
+
/*
* name_to_handle_at() and open_by_handle_at() can reach here with fuse
* mount point in guest, but we don't have its inode info in the
@@ -913,7 +919,14 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
pthread_mutex_unlock(&lo->mutex);
}
e->ino = inode->fuse_ino;
- lo_inode_put(lo, &inode);
+
+ /* Transfer ownership of inode pointer to caller or drop it */
+ if (inodep) {
+ *inodep = inode;
+ } else {
+ lo_inode_put(lo, &inode);
+ }
+
lo_inode_put(lo, &dir);
fuse_log(FUSE_LOG_DEBUG, " %lli/%s -> %lli\n", (unsigned long long)parent,
@@ -948,7 +961,7 @@ static void lo_lookup(fuse_req_t req, fuse_ino_t parent, const char *name)
return;
}
- err = lo_do_lookup(req, parent, name, &e);
+ err = lo_do_lookup(req, parent, name, &e, NULL);
if (err) {
fuse_reply_err(req, err);
} else {
@@ -1056,7 +1069,7 @@ static void lo_mknod_symlink(fuse_req_t req, fuse_ino_t parent,
goto out;
}
- saverr = lo_do_lookup(req, parent, name, &e);
+ saverr = lo_do_lookup(req, parent, name, &e, NULL);
if (saverr) {
goto out;
}
@@ -1534,7 +1547,7 @@ static void lo_do_readdir(fuse_req_t req, fuse_ino_t ino, size_t size,
if (plus) {
if (!is_dot_or_dotdot(name)) {
- err = lo_do_lookup(req, ino, name, &e);
+ err = lo_do_lookup(req, ino, name, &e, NULL);
if (err) {
goto error;
}
@@ -1732,7 +1745,7 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
}
fi->fh = fh;
- err = lo_do_lookup(req, parent, name, &e);
+ err = lo_do_lookup(req, parent, name, &e, NULL);
}
if (lo->cache == CACHE_NONE) {
fi->direct_io = 1;
--
2.29.2
^ permalink raw reply related [flat|nested] 48+ messages in thread
* [Virtio-fs] [PATCH v4 2/3] virtiofsd: optionally return inode pointer from lo_do_lookup()
@ 2021-02-03 11:37 ` Stefan Hajnoczi
0 siblings, 0 replies; 48+ messages in thread
From: Stefan Hajnoczi @ 2021-02-03 11:37 UTC (permalink / raw)
To: qemu-devel; +Cc: Daniel Berrange, virtio-fs, P J P, Laszlo Ersek, vgoyal
lo_do_lookup() finds an existing inode or allocates a new one. It
increments nlookup so that the inode stays alive until the client
releases it.
Existing callers don't need the struct lo_inode so the function doesn't
return it. Extend the function to optionally return the inode. The next
commit will need it.
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
tools/virtiofsd/passthrough_ll.c | 29 +++++++++++++++++++++--------
1 file changed, 21 insertions(+), 8 deletions(-)
diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index e63cbd3fb7..c87a1f3d72 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -831,11 +831,13 @@ static int do_statx(struct lo_data *lo, int dirfd, const char *pathname,
}
/*
- * Increments nlookup and caller must release refcount using
- * lo_inode_put(&parent).
+ * Increments nlookup on the inode on success. unref_inode_lolocked() must be
+ * called eventually to decrement nlookup again. If inodep is non-NULL, the
+ * inode pointer is stored and the caller must call lo_inode_put().
*/
static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
- struct fuse_entry_param *e)
+ struct fuse_entry_param *e,
+ struct lo_inode **inodep)
{
int newfd;
int res;
@@ -845,6 +847,10 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
struct lo_inode *inode = NULL;
struct lo_inode *dir = lo_inode(req, parent);
+ if (inodep) {
+ *inodep = NULL;
+ }
+
/*
* name_to_handle_at() and open_by_handle_at() can reach here with fuse
* mount point in guest, but we don't have its inode info in the
@@ -913,7 +919,14 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
pthread_mutex_unlock(&lo->mutex);
}
e->ino = inode->fuse_ino;
- lo_inode_put(lo, &inode);
+
+ /* Transfer ownership of inode pointer to caller or drop it */
+ if (inodep) {
+ *inodep = inode;
+ } else {
+ lo_inode_put(lo, &inode);
+ }
+
lo_inode_put(lo, &dir);
fuse_log(FUSE_LOG_DEBUG, " %lli/%s -> %lli\n", (unsigned long long)parent,
@@ -948,7 +961,7 @@ static void lo_lookup(fuse_req_t req, fuse_ino_t parent, const char *name)
return;
}
- err = lo_do_lookup(req, parent, name, &e);
+ err = lo_do_lookup(req, parent, name, &e, NULL);
if (err) {
fuse_reply_err(req, err);
} else {
@@ -1056,7 +1069,7 @@ static void lo_mknod_symlink(fuse_req_t req, fuse_ino_t parent,
goto out;
}
- saverr = lo_do_lookup(req, parent, name, &e);
+ saverr = lo_do_lookup(req, parent, name, &e, NULL);
if (saverr) {
goto out;
}
@@ -1534,7 +1547,7 @@ static void lo_do_readdir(fuse_req_t req, fuse_ino_t ino, size_t size,
if (plus) {
if (!is_dot_or_dotdot(name)) {
- err = lo_do_lookup(req, ino, name, &e);
+ err = lo_do_lookup(req, ino, name, &e, NULL);
if (err) {
goto error;
}
@@ -1732,7 +1745,7 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
}
fi->fh = fh;
- err = lo_do_lookup(req, parent, name, &e);
+ err = lo_do_lookup(req, parent, name, &e, NULL);
}
if (lo->cache == CACHE_NONE) {
fi->direct_io = 1;
--
2.29.2
^ permalink raw reply related [flat|nested] 48+ messages in thread
* [PATCH v4 3/3] virtiofsd: prevent opening of special files (CVE-2020-35517)
2021-02-03 11:37 ` [Virtio-fs] " Stefan Hajnoczi
@ 2021-02-03 11:37 ` Stefan Hajnoczi
-1 siblings, 0 replies; 48+ messages in thread
From: Stefan Hajnoczi @ 2021-02-03 11:37 UTC (permalink / raw)
To: qemu-devel
Cc: mszeredi, Daniel Berrange, slp, Dr. David Alan Gilbert,
Greg Kurz, virtio-fs, Alex Xu, Stefan Hajnoczi, P J P,
Laszlo Ersek, vgoyal
A well-behaved FUSE client does not attempt to open special files with
FUSE_OPEN because they are handled on the client side (e.g. device nodes
are handled by client-side device drivers).
The check to prevent virtiofsd from opening special files is missing in
a few cases, most notably FUSE_OPEN. A malicious client can cause
virtiofsd to open a device node, potentially allowing the guest to
escape. This can be exploited by a modified guest device driver. It is
not exploitable from guest userspace since the guest kernel will handle
special files inside the guest instead of sending FUSE requests.
This patch fixes this issue by introducing the lo_inode_open() function
to check the file type before opening it. This is a short-term solution
because it does not prevent a compromised virtiofsd process from opening
device nodes on the host.
Restructure lo_create() to try O_CREAT | O_EXCL first. Note that O_CREAT
| O_EXCL does not follow symlinks, so O_NOFOLLOW masking is not
necessary here. If the file exists and the user did not specify O_EXCL,
open it via lo_do_open().
Reported-by: Alex Xu <alex@alxu.ca>
Fixes: CVE-2020-35517
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Vivek Goyal <vgoyal@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
v3:
* Restructure lo_create() to handle externally-created files (we need
to allocate an inode for them) [Greg]
v3:
* Protect lo_create() [Greg]
v2:
* Add doc comment clarifying that symlinks are traversed client-side
[Daniel]
This issue was diagnosed on public IRC and is therefore already known
and not embargoed.
A stronger fix, and the long-term solution, is for users to mount the
shared directory and any sub-mounts with nodev, as well as nosuid and
noexec. Unfortunately virtiofsd cannot do this automatically because
bind mounts added by the user after virtiofsd has launched would not be
detected. I suggest the following:
1. Modify libvirt and Kata Containers to explicitly set these mount
options.
2. Then modify virtiofsd to check that the shared directory has the
necessary options at startup. Refuse to start if the options are
missing so that the user is aware of the security requirements.
As a bonus this also increases the likelihood that other host processes
besides virtiofsd will be protected by nosuid/noexec/nodev so that a
malicious guest cannot drop these files in place and then arrange for a
host process to come across them.
Additionally, user namespaces have been discussed. They seem like a
worthwhile addition as an unprivileged or privilege-separated mode
although there are limitations with respect to security xattrs and the
actual uid/gid stored on the host file system not corresponding to the
guest uid/gid.
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
tools/virtiofsd/passthrough_ll.c | 139 +++++++++++++++++++------------
1 file changed, 88 insertions(+), 51 deletions(-)
diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index c87a1f3d72..b607ef0f7e 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -555,6 +555,38 @@ static int lo_fd(fuse_req_t req, fuse_ino_t ino)
return fd;
}
+/*
+ * Open a file descriptor for an inode. Returns -EBADF if the inode is not a
+ * regular file or a directory.
+ *
+ * Use this helper function instead of raw openat(2) to prevent security issues
+ * when a malicious client opens special files such as block device nodes.
+ * Symlink inodes are also rejected since symlinks must already have been
+ * traversed on the client side.
+ */
+static int lo_inode_open(struct lo_data *lo, struct lo_inode *inode,
+ int open_flags)
+{
+ g_autofree char *fd_str = g_strdup_printf("%d", inode->fd);
+ int fd;
+
+ if (!S_ISREG(inode->filetype) && !S_ISDIR(inode->filetype)) {
+ return -EBADF;
+ }
+
+ /*
+ * The file is a symlink so O_NOFOLLOW must be ignored. We checked earlier
+ * that the inode is not a special file but if an external process races
+ * with us then symlinks are traversed here. It is not possible to escape
+ * the shared directory since it is mounted as "/" though.
+ */
+ fd = openat(lo->proc_self_fd, fd_str, open_flags & ~O_NOFOLLOW);
+ if (fd < 0) {
+ return -errno;
+ }
+ return fd;
+}
+
static void lo_init(void *userdata, struct fuse_conn_info *conn)
{
struct lo_data *lo = (struct lo_data *)userdata;
@@ -684,8 +716,7 @@ static void lo_setattr(fuse_req_t req, fuse_ino_t ino, struct stat *attr,
if (fi) {
truncfd = fd;
} else {
- sprintf(procname, "%i", ifd);
- truncfd = openat(lo->proc_self_fd, procname, O_RDWR);
+ truncfd = lo_inode_open(lo, inode, O_RDWR);
if (truncfd < 0) {
goto out_err;
}
@@ -1664,19 +1695,24 @@ static void update_open_flags(int writeback, int allow_direct_io,
}
}
+/*
+ * Open a regular file, set up an fd mapping, and fill out the struct
+ * fuse_file_info for it. If existing_fd is not negative, use that fd instead
+ * opening a new one. Takes ownership of existing_fd.
+ */
static int lo_do_open(struct lo_data *lo, struct lo_inode *inode,
- struct fuse_file_info *fi)
+ int existing_fd, struct fuse_file_info *fi)
{
- char buf[64];
ssize_t fh;
- int fd;
+ int fd = existing_fd;
update_open_flags(lo->writeback, lo->allow_direct_io, fi);
- sprintf(buf, "%i", inode->fd);
- fd = openat(lo->proc_self_fd, buf, fi->flags & ~O_NOFOLLOW);
- if (fd == -1) {
- return -errno;
+ if (fd < 0) {
+ fd = lo_inode_open(lo, inode, fi->flags);
+ if (fd == -1) {
+ return -errno;
+ }
}
pthread_mutex_lock(&lo->mutex);
@@ -1699,9 +1735,10 @@ static int lo_do_open(struct lo_data *lo, struct lo_inode *inode,
static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
mode_t mode, struct fuse_file_info *fi)
{
- int fd;
+ int fd = -1;
struct lo_data *lo = lo_data(req);
struct lo_inode *parent_inode;
+ struct lo_inode *inode = NULL;
struct fuse_entry_param e;
int err;
struct lo_cred old = {};
@@ -1727,36 +1764,38 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
update_open_flags(lo->writeback, lo->allow_direct_io, fi);
- fd = openat(parent_inode->fd, name, (fi->flags | O_CREAT) & ~O_NOFOLLOW,
- mode);
+ /* Try to create a new file but don't open existing files */
+ fd = openat(parent_inode->fd, name, fi->flags | O_CREAT | O_EXCL, mode);
err = fd == -1 ? errno : 0;
+
lo_restore_cred(&old);
- if (!err) {
- ssize_t fh;
-
- pthread_mutex_lock(&lo->mutex);
- fh = lo_add_fd_mapping(lo, fd);
- pthread_mutex_unlock(&lo->mutex);
- if (fh == -1) {
- close(fd);
- err = ENOMEM;
- goto out;
- }
+ /* Ignore the error if file exists and O_EXCL was not given */
+ if (err && !(err == EEXIST && !(fi->flags & O_EXCL))) {
+ goto out;
+ }
- fi->fh = fh;
- err = lo_do_lookup(req, parent, name, &e, NULL);
+ err = lo_do_lookup(req, parent, name, &e, &inode);
+ if (err) {
+ goto out;
}
- if (lo->cache == CACHE_NONE) {
- fi->direct_io = 1;
- } else if (lo->cache == CACHE_ALWAYS) {
- fi->keep_cache = 1;
+
+ err = lo_do_open(lo, inode, fd, fi);
+ fd = -1; /* lo_do_open() takes ownership of fd */
+ if (err) {
+ /* Undo lo_do_lookup() nlookup ref */
+ unref_inode_lolocked(lo, inode, 1);
}
out:
+ lo_inode_put(lo, &inode);
lo_inode_put(lo, &parent_inode);
if (err) {
+ if (fd >= 0) {
+ close(fd);
+ }
+
fuse_reply_err(req, err);
} else {
fuse_reply_create(req, &e, fi);
@@ -1770,7 +1809,6 @@ static struct lo_inode_plock *lookup_create_plock_ctx(struct lo_data *lo,
pid_t pid, int *err)
{
struct lo_inode_plock *plock;
- char procname[64];
int fd;
plock =
@@ -1787,12 +1825,10 @@ static struct lo_inode_plock *lookup_create_plock_ctx(struct lo_data *lo,
}
/* Open another instance of file which can be used for ofd locks. */
- sprintf(procname, "%i", inode->fd);
-
/* TODO: What if file is not writable? */
- fd = openat(lo->proc_self_fd, procname, O_RDWR);
- if (fd == -1) {
- *err = errno;
+ fd = lo_inode_open(lo, inode, O_RDWR);
+ if (fd < 0) {
+ *err = -fd;
free(plock);
return NULL;
}
@@ -1949,7 +1985,7 @@ static void lo_open(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
return;
}
- err = lo_do_open(lo, inode, fi);
+ err = lo_do_open(lo, inode, -1, fi);
lo_inode_put(lo, &inode);
if (err) {
fuse_reply_err(req, err);
@@ -2014,39 +2050,40 @@ static void lo_flush(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
static void lo_fsync(fuse_req_t req, fuse_ino_t ino, int datasync,
struct fuse_file_info *fi)
{
+ struct lo_inode *inode = lo_inode(req, ino);
+ struct lo_data *lo = lo_data(req);
int res;
int fd;
- char *buf;
fuse_log(FUSE_LOG_DEBUG, "lo_fsync(ino=%" PRIu64 ", fi=0x%p)\n", ino,
(void *)fi);
+ if (!inode) {
+ fuse_reply_err(req, EBADF);
+ return;
+ }
+
if (!fi) {
- struct lo_data *lo = lo_data(req);
-
- res = asprintf(&buf, "%i", lo_fd(req, ino));
- if (res == -1) {
- return (void)fuse_reply_err(req, errno);
- }
-
- fd = openat(lo->proc_self_fd, buf, O_RDWR);
- free(buf);
- if (fd == -1) {
- return (void)fuse_reply_err(req, errno);
+ fd = lo_inode_open(lo, inode, O_RDWR);
+ if (fd < 0) {
+ res = -fd;
+ goto out;
}
} else {
fd = lo_fi_fd(req, fi);
}
if (datasync) {
- res = fdatasync(fd);
+ res = fdatasync(fd) == -1 ? errno : 0;
} else {
- res = fsync(fd);
+ res = fsync(fd) == -1 ? errno : 0;
}
if (!fi) {
close(fd);
}
- fuse_reply_err(req, res == -1 ? errno : 0);
+out:
+ lo_inode_put(lo, &inode);
+ fuse_reply_err(req, res);
}
static void lo_read(fuse_req_t req, fuse_ino_t ino, size_t size, off_t offset,
--
2.29.2
^ permalink raw reply related [flat|nested] 48+ messages in thread
* [Virtio-fs] [PATCH v4 3/3] virtiofsd: prevent opening of special files (CVE-2020-35517)
@ 2021-02-03 11:37 ` Stefan Hajnoczi
0 siblings, 0 replies; 48+ messages in thread
From: Stefan Hajnoczi @ 2021-02-03 11:37 UTC (permalink / raw)
To: qemu-devel
Cc: Daniel Berrange, virtio-fs, Alex Xu, P J P, Laszlo Ersek, vgoyal
A well-behaved FUSE client does not attempt to open special files with
FUSE_OPEN because they are handled on the client side (e.g. device nodes
are handled by client-side device drivers).
The check to prevent virtiofsd from opening special files is missing in
a few cases, most notably FUSE_OPEN. A malicious client can cause
virtiofsd to open a device node, potentially allowing the guest to
escape. This can be exploited by a modified guest device driver. It is
not exploitable from guest userspace since the guest kernel will handle
special files inside the guest instead of sending FUSE requests.
This patch fixes this issue by introducing the lo_inode_open() function
to check the file type before opening it. This is a short-term solution
because it does not prevent a compromised virtiofsd process from opening
device nodes on the host.
Restructure lo_create() to try O_CREAT | O_EXCL first. Note that O_CREAT
| O_EXCL does not follow symlinks, so O_NOFOLLOW masking is not
necessary here. If the file exists and the user did not specify O_EXCL,
open it via lo_do_open().
Reported-by: Alex Xu <alex@alxu.ca>
Fixes: CVE-2020-35517
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Vivek Goyal <vgoyal@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
v3:
* Restructure lo_create() to handle externally-created files (we need
to allocate an inode for them) [Greg]
v3:
* Protect lo_create() [Greg]
v2:
* Add doc comment clarifying that symlinks are traversed client-side
[Daniel]
This issue was diagnosed on public IRC and is therefore already known
and not embargoed.
A stronger fix, and the long-term solution, is for users to mount the
shared directory and any sub-mounts with nodev, as well as nosuid and
noexec. Unfortunately virtiofsd cannot do this automatically because
bind mounts added by the user after virtiofsd has launched would not be
detected. I suggest the following:
1. Modify libvirt and Kata Containers to explicitly set these mount
options.
2. Then modify virtiofsd to check that the shared directory has the
necessary options at startup. Refuse to start if the options are
missing so that the user is aware of the security requirements.
As a bonus this also increases the likelihood that other host processes
besides virtiofsd will be protected by nosuid/noexec/nodev so that a
malicious guest cannot drop these files in place and then arrange for a
host process to come across them.
Additionally, user namespaces have been discussed. They seem like a
worthwhile addition as an unprivileged or privilege-separated mode
although there are limitations with respect to security xattrs and the
actual uid/gid stored on the host file system not corresponding to the
guest uid/gid.
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
tools/virtiofsd/passthrough_ll.c | 139 +++++++++++++++++++------------
1 file changed, 88 insertions(+), 51 deletions(-)
diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index c87a1f3d72..b607ef0f7e 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -555,6 +555,38 @@ static int lo_fd(fuse_req_t req, fuse_ino_t ino)
return fd;
}
+/*
+ * Open a file descriptor for an inode. Returns -EBADF if the inode is not a
+ * regular file or a directory.
+ *
+ * Use this helper function instead of raw openat(2) to prevent security issues
+ * when a malicious client opens special files such as block device nodes.
+ * Symlink inodes are also rejected since symlinks must already have been
+ * traversed on the client side.
+ */
+static int lo_inode_open(struct lo_data *lo, struct lo_inode *inode,
+ int open_flags)
+{
+ g_autofree char *fd_str = g_strdup_printf("%d", inode->fd);
+ int fd;
+
+ if (!S_ISREG(inode->filetype) && !S_ISDIR(inode->filetype)) {
+ return -EBADF;
+ }
+
+ /*
+ * The file is a symlink so O_NOFOLLOW must be ignored. We checked earlier
+ * that the inode is not a special file but if an external process races
+ * with us then symlinks are traversed here. It is not possible to escape
+ * the shared directory since it is mounted as "/" though.
+ */
+ fd = openat(lo->proc_self_fd, fd_str, open_flags & ~O_NOFOLLOW);
+ if (fd < 0) {
+ return -errno;
+ }
+ return fd;
+}
+
static void lo_init(void *userdata, struct fuse_conn_info *conn)
{
struct lo_data *lo = (struct lo_data *)userdata;
@@ -684,8 +716,7 @@ static void lo_setattr(fuse_req_t req, fuse_ino_t ino, struct stat *attr,
if (fi) {
truncfd = fd;
} else {
- sprintf(procname, "%i", ifd);
- truncfd = openat(lo->proc_self_fd, procname, O_RDWR);
+ truncfd = lo_inode_open(lo, inode, O_RDWR);
if (truncfd < 0) {
goto out_err;
}
@@ -1664,19 +1695,24 @@ static void update_open_flags(int writeback, int allow_direct_io,
}
}
+/*
+ * Open a regular file, set up an fd mapping, and fill out the struct
+ * fuse_file_info for it. If existing_fd is not negative, use that fd instead
+ * opening a new one. Takes ownership of existing_fd.
+ */
static int lo_do_open(struct lo_data *lo, struct lo_inode *inode,
- struct fuse_file_info *fi)
+ int existing_fd, struct fuse_file_info *fi)
{
- char buf[64];
ssize_t fh;
- int fd;
+ int fd = existing_fd;
update_open_flags(lo->writeback, lo->allow_direct_io, fi);
- sprintf(buf, "%i", inode->fd);
- fd = openat(lo->proc_self_fd, buf, fi->flags & ~O_NOFOLLOW);
- if (fd == -1) {
- return -errno;
+ if (fd < 0) {
+ fd = lo_inode_open(lo, inode, fi->flags);
+ if (fd == -1) {
+ return -errno;
+ }
}
pthread_mutex_lock(&lo->mutex);
@@ -1699,9 +1735,10 @@ static int lo_do_open(struct lo_data *lo, struct lo_inode *inode,
static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
mode_t mode, struct fuse_file_info *fi)
{
- int fd;
+ int fd = -1;
struct lo_data *lo = lo_data(req);
struct lo_inode *parent_inode;
+ struct lo_inode *inode = NULL;
struct fuse_entry_param e;
int err;
struct lo_cred old = {};
@@ -1727,36 +1764,38 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
update_open_flags(lo->writeback, lo->allow_direct_io, fi);
- fd = openat(parent_inode->fd, name, (fi->flags | O_CREAT) & ~O_NOFOLLOW,
- mode);
+ /* Try to create a new file but don't open existing files */
+ fd = openat(parent_inode->fd, name, fi->flags | O_CREAT | O_EXCL, mode);
err = fd == -1 ? errno : 0;
+
lo_restore_cred(&old);
- if (!err) {
- ssize_t fh;
-
- pthread_mutex_lock(&lo->mutex);
- fh = lo_add_fd_mapping(lo, fd);
- pthread_mutex_unlock(&lo->mutex);
- if (fh == -1) {
- close(fd);
- err = ENOMEM;
- goto out;
- }
+ /* Ignore the error if file exists and O_EXCL was not given */
+ if (err && !(err == EEXIST && !(fi->flags & O_EXCL))) {
+ goto out;
+ }
- fi->fh = fh;
- err = lo_do_lookup(req, parent, name, &e, NULL);
+ err = lo_do_lookup(req, parent, name, &e, &inode);
+ if (err) {
+ goto out;
}
- if (lo->cache == CACHE_NONE) {
- fi->direct_io = 1;
- } else if (lo->cache == CACHE_ALWAYS) {
- fi->keep_cache = 1;
+
+ err = lo_do_open(lo, inode, fd, fi);
+ fd = -1; /* lo_do_open() takes ownership of fd */
+ if (err) {
+ /* Undo lo_do_lookup() nlookup ref */
+ unref_inode_lolocked(lo, inode, 1);
}
out:
+ lo_inode_put(lo, &inode);
lo_inode_put(lo, &parent_inode);
if (err) {
+ if (fd >= 0) {
+ close(fd);
+ }
+
fuse_reply_err(req, err);
} else {
fuse_reply_create(req, &e, fi);
@@ -1770,7 +1809,6 @@ static struct lo_inode_plock *lookup_create_plock_ctx(struct lo_data *lo,
pid_t pid, int *err)
{
struct lo_inode_plock *plock;
- char procname[64];
int fd;
plock =
@@ -1787,12 +1825,10 @@ static struct lo_inode_plock *lookup_create_plock_ctx(struct lo_data *lo,
}
/* Open another instance of file which can be used for ofd locks. */
- sprintf(procname, "%i", inode->fd);
-
/* TODO: What if file is not writable? */
- fd = openat(lo->proc_self_fd, procname, O_RDWR);
- if (fd == -1) {
- *err = errno;
+ fd = lo_inode_open(lo, inode, O_RDWR);
+ if (fd < 0) {
+ *err = -fd;
free(plock);
return NULL;
}
@@ -1949,7 +1985,7 @@ static void lo_open(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
return;
}
- err = lo_do_open(lo, inode, fi);
+ err = lo_do_open(lo, inode, -1, fi);
lo_inode_put(lo, &inode);
if (err) {
fuse_reply_err(req, err);
@@ -2014,39 +2050,40 @@ static void lo_flush(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
static void lo_fsync(fuse_req_t req, fuse_ino_t ino, int datasync,
struct fuse_file_info *fi)
{
+ struct lo_inode *inode = lo_inode(req, ino);
+ struct lo_data *lo = lo_data(req);
int res;
int fd;
- char *buf;
fuse_log(FUSE_LOG_DEBUG, "lo_fsync(ino=%" PRIu64 ", fi=0x%p)\n", ino,
(void *)fi);
+ if (!inode) {
+ fuse_reply_err(req, EBADF);
+ return;
+ }
+
if (!fi) {
- struct lo_data *lo = lo_data(req);
-
- res = asprintf(&buf, "%i", lo_fd(req, ino));
- if (res == -1) {
- return (void)fuse_reply_err(req, errno);
- }
-
- fd = openat(lo->proc_self_fd, buf, O_RDWR);
- free(buf);
- if (fd == -1) {
- return (void)fuse_reply_err(req, errno);
+ fd = lo_inode_open(lo, inode, O_RDWR);
+ if (fd < 0) {
+ res = -fd;
+ goto out;
}
} else {
fd = lo_fi_fd(req, fi);
}
if (datasync) {
- res = fdatasync(fd);
+ res = fdatasync(fd) == -1 ? errno : 0;
} else {
- res = fsync(fd);
+ res = fsync(fd) == -1 ? errno : 0;
}
if (!fi) {
close(fd);
}
- fuse_reply_err(req, res == -1 ? errno : 0);
+out:
+ lo_inode_put(lo, &inode);
+ fuse_reply_err(req, res);
}
static void lo_read(fuse_req_t req, fuse_ino_t ino, size_t size, off_t offset,
--
2.29.2
^ permalink raw reply related [flat|nested] 48+ messages in thread
* Re: [PATCH v4 0/3] virtiofsd: prevent opening of special files (CVE-2020-35517)
2021-02-03 11:37 ` [Virtio-fs] " Stefan Hajnoczi
@ 2021-02-03 11:46 ` no-reply
-1 siblings, 0 replies; 48+ messages in thread
From: no-reply @ 2021-02-03 11:46 UTC (permalink / raw)
To: stefanha
Cc: mszeredi, berrange, slp, groug, qemu-devel, virtio-fs, alex,
vgoyal, stefanha, ppandit, lersek, dgilbert
Patchew URL: https://patchew.org/QEMU/20210203113719.83633-1-stefanha@redhat.com/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 20210203113719.83633-1-stefanha@redhat.com
Subject: [PATCH v4 0/3] virtiofsd: prevent opening of special files (CVE-2020-35517)
=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
9e44f0e virtiofsd: prevent opening of special files (CVE-2020-35517)
e3ddfae virtiofsd: optionally return inode pointer from lo_do_lookup()
a6c73fd virtiofsd: extract lo_do_open() from lo_open()
=== OUTPUT BEGIN ===
1/3 Checking commit a6c73fd0a630 (virtiofsd: extract lo_do_open() from lo_open())
ERROR: return of an errno should typically be -ve (return -ENOMEM)
#70: FILE: tools/virtiofsd/passthrough_ll.c:1674:
+ return ENOMEM;
total: 1 errors, 0 warnings, 114 lines checked
Patch 1/3 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
2/3 Checking commit e3ddfaebb90a (virtiofsd: optionally return inode pointer from lo_do_lookup())
3/3 Checking commit 9e44f0e0be3a (virtiofsd: prevent opening of special files (CVE-2020-35517))
=== OUTPUT END ===
Test command exited with code: 1
The full log is available at
http://patchew.org/logs/20210203113719.83633-1-stefanha@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [Virtio-fs] [PATCH v4 0/3] virtiofsd: prevent opening of special files (CVE-2020-35517)
@ 2021-02-03 11:46 ` no-reply
0 siblings, 0 replies; 48+ messages in thread
From: no-reply @ 2021-02-03 11:46 UTC (permalink / raw)
To: stefanha; +Cc: berrange, qemu-devel, virtio-fs, alex, vgoyal, ppandit, lersek
Patchew URL: https://patchew.org/QEMU/20210203113719.83633-1-stefanha@redhat.com/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 20210203113719.83633-1-stefanha@redhat.com
Subject: [PATCH v4 0/3] virtiofsd: prevent opening of special files (CVE-2020-35517)
=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
9e44f0e virtiofsd: prevent opening of special files (CVE-2020-35517)
e3ddfae virtiofsd: optionally return inode pointer from lo_do_lookup()
a6c73fd virtiofsd: extract lo_do_open() from lo_open()
=== OUTPUT BEGIN ===
1/3 Checking commit a6c73fd0a630 (virtiofsd: extract lo_do_open() from lo_open())
ERROR: return of an errno should typically be -ve (return -ENOMEM)
#70: FILE: tools/virtiofsd/passthrough_ll.c:1674:
+ return ENOMEM;
total: 1 errors, 0 warnings, 114 lines checked
Patch 1/3 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
2/3 Checking commit e3ddfaebb90a (virtiofsd: optionally return inode pointer from lo_do_lookup())
3/3 Checking commit 9e44f0e0be3a (virtiofsd: prevent opening of special files (CVE-2020-35517))
=== OUTPUT END ===
Test command exited with code: 1
The full log is available at
http://patchew.org/logs/20210203113719.83633-1-stefanha@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v4 2/3] virtiofsd: optionally return inode pointer from lo_do_lookup()
2021-02-03 11:37 ` [Virtio-fs] " Stefan Hajnoczi
@ 2021-02-03 14:20 ` Greg Kurz
-1 siblings, 0 replies; 48+ messages in thread
From: Greg Kurz @ 2021-02-03 14:20 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: mszeredi, Daniel Berrange, slp, qemu-devel,
Dr. David Alan Gilbert, virtio-fs, P J P, Laszlo Ersek, vgoyal
On Wed, 3 Feb 2021 11:37:18 +0000
Stefan Hajnoczi <stefanha@redhat.com> wrote:
> lo_do_lookup() finds an existing inode or allocates a new one. It
> increments nlookup so that the inode stays alive until the client
> releases it.
>
> Existing callers don't need the struct lo_inode so the function doesn't
> return it. Extend the function to optionally return the inode. The next
> commit will need it.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
> tools/virtiofsd/passthrough_ll.c | 29 +++++++++++++++++++++--------
> 1 file changed, 21 insertions(+), 8 deletions(-)
>
> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> index e63cbd3fb7..c87a1f3d72 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -831,11 +831,13 @@ static int do_statx(struct lo_data *lo, int dirfd, const char *pathname,
> }
>
> /*
> - * Increments nlookup and caller must release refcount using
> - * lo_inode_put(&parent).
> + * Increments nlookup on the inode on success. unref_inode_lolocked() must be
> + * called eventually to decrement nlookup again. If inodep is non-NULL, the
> + * inode pointer is stored and the caller must call lo_inode_put().
> */
> static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
> - struct fuse_entry_param *e)
> + struct fuse_entry_param *e,
> + struct lo_inode **inodep)
> {
> int newfd;
> int res;
> @@ -845,6 +847,10 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
> struct lo_inode *inode = NULL;
> struct lo_inode *dir = lo_inode(req, parent);
>
> + if (inodep) {
> + *inodep = NULL;
> + }
> +
Is this side-effect needed ? If lo_do_lookup() returns an error, it
rather seems that the caller shouldn't expect anything to be written
here, i.e. the content of *inodep still belongs to the caller and
whatever value it previously put in there (as patch 3/3 does) should
be preserved IMHO.
Apart from that LGTM.
> /*
> * name_to_handle_at() and open_by_handle_at() can reach here with fuse
> * mount point in guest, but we don't have its inode info in the
> @@ -913,7 +919,14 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
> pthread_mutex_unlock(&lo->mutex);
> }
> e->ino = inode->fuse_ino;
> - lo_inode_put(lo, &inode);
> +
> + /* Transfer ownership of inode pointer to caller or drop it */
> + if (inodep) {
> + *inodep = inode;
> + } else {
> + lo_inode_put(lo, &inode);
> + }
> +
> lo_inode_put(lo, &dir);
>
> fuse_log(FUSE_LOG_DEBUG, " %lli/%s -> %lli\n", (unsigned long long)parent,
> @@ -948,7 +961,7 @@ static void lo_lookup(fuse_req_t req, fuse_ino_t parent, const char *name)
> return;
> }
>
> - err = lo_do_lookup(req, parent, name, &e);
> + err = lo_do_lookup(req, parent, name, &e, NULL);
> if (err) {
> fuse_reply_err(req, err);
> } else {
> @@ -1056,7 +1069,7 @@ static void lo_mknod_symlink(fuse_req_t req, fuse_ino_t parent,
> goto out;
> }
>
> - saverr = lo_do_lookup(req, parent, name, &e);
> + saverr = lo_do_lookup(req, parent, name, &e, NULL);
> if (saverr) {
> goto out;
> }
> @@ -1534,7 +1547,7 @@ static void lo_do_readdir(fuse_req_t req, fuse_ino_t ino, size_t size,
>
> if (plus) {
> if (!is_dot_or_dotdot(name)) {
> - err = lo_do_lookup(req, ino, name, &e);
> + err = lo_do_lookup(req, ino, name, &e, NULL);
> if (err) {
> goto error;
> }
> @@ -1732,7 +1745,7 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
> }
>
> fi->fh = fh;
> - err = lo_do_lookup(req, parent, name, &e);
> + err = lo_do_lookup(req, parent, name, &e, NULL);
> }
> if (lo->cache == CACHE_NONE) {
> fi->direct_io = 1;
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [Virtio-fs] [PATCH v4 2/3] virtiofsd: optionally return inode pointer from lo_do_lookup()
@ 2021-02-03 14:20 ` Greg Kurz
0 siblings, 0 replies; 48+ messages in thread
From: Greg Kurz @ 2021-02-03 14:20 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Daniel Berrange, qemu-devel, virtio-fs, P J P, Laszlo Ersek, vgoyal
On Wed, 3 Feb 2021 11:37:18 +0000
Stefan Hajnoczi <stefanha@redhat.com> wrote:
> lo_do_lookup() finds an existing inode or allocates a new one. It
> increments nlookup so that the inode stays alive until the client
> releases it.
>
> Existing callers don't need the struct lo_inode so the function doesn't
> return it. Extend the function to optionally return the inode. The next
> commit will need it.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
> tools/virtiofsd/passthrough_ll.c | 29 +++++++++++++++++++++--------
> 1 file changed, 21 insertions(+), 8 deletions(-)
>
> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> index e63cbd3fb7..c87a1f3d72 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -831,11 +831,13 @@ static int do_statx(struct lo_data *lo, int dirfd, const char *pathname,
> }
>
> /*
> - * Increments nlookup and caller must release refcount using
> - * lo_inode_put(&parent).
> + * Increments nlookup on the inode on success. unref_inode_lolocked() must be
> + * called eventually to decrement nlookup again. If inodep is non-NULL, the
> + * inode pointer is stored and the caller must call lo_inode_put().
> */
> static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
> - struct fuse_entry_param *e)
> + struct fuse_entry_param *e,
> + struct lo_inode **inodep)
> {
> int newfd;
> int res;
> @@ -845,6 +847,10 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
> struct lo_inode *inode = NULL;
> struct lo_inode *dir = lo_inode(req, parent);
>
> + if (inodep) {
> + *inodep = NULL;
> + }
> +
Is this side-effect needed ? If lo_do_lookup() returns an error, it
rather seems that the caller shouldn't expect anything to be written
here, i.e. the content of *inodep still belongs to the caller and
whatever value it previously put in there (as patch 3/3 does) should
be preserved IMHO.
Apart from that LGTM.
> /*
> * name_to_handle_at() and open_by_handle_at() can reach here with fuse
> * mount point in guest, but we don't have its inode info in the
> @@ -913,7 +919,14 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
> pthread_mutex_unlock(&lo->mutex);
> }
> e->ino = inode->fuse_ino;
> - lo_inode_put(lo, &inode);
> +
> + /* Transfer ownership of inode pointer to caller or drop it */
> + if (inodep) {
> + *inodep = inode;
> + } else {
> + lo_inode_put(lo, &inode);
> + }
> +
> lo_inode_put(lo, &dir);
>
> fuse_log(FUSE_LOG_DEBUG, " %lli/%s -> %lli\n", (unsigned long long)parent,
> @@ -948,7 +961,7 @@ static void lo_lookup(fuse_req_t req, fuse_ino_t parent, const char *name)
> return;
> }
>
> - err = lo_do_lookup(req, parent, name, &e);
> + err = lo_do_lookup(req, parent, name, &e, NULL);
> if (err) {
> fuse_reply_err(req, err);
> } else {
> @@ -1056,7 +1069,7 @@ static void lo_mknod_symlink(fuse_req_t req, fuse_ino_t parent,
> goto out;
> }
>
> - saverr = lo_do_lookup(req, parent, name, &e);
> + saverr = lo_do_lookup(req, parent, name, &e, NULL);
> if (saverr) {
> goto out;
> }
> @@ -1534,7 +1547,7 @@ static void lo_do_readdir(fuse_req_t req, fuse_ino_t ino, size_t size,
>
> if (plus) {
> if (!is_dot_or_dotdot(name)) {
> - err = lo_do_lookup(req, ino, name, &e);
> + err = lo_do_lookup(req, ino, name, &e, NULL);
> if (err) {
> goto error;
> }
> @@ -1732,7 +1745,7 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
> }
>
> fi->fh = fh;
> - err = lo_do_lookup(req, parent, name, &e);
> + err = lo_do_lookup(req, parent, name, &e, NULL);
> }
> if (lo->cache == CACHE_NONE) {
> fi->direct_io = 1;
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v4 1/3] virtiofsd: extract lo_do_open() from lo_open()
2021-02-03 11:37 ` [Virtio-fs] " Stefan Hajnoczi
@ 2021-02-03 14:20 ` Greg Kurz
-1 siblings, 0 replies; 48+ messages in thread
From: Greg Kurz @ 2021-02-03 14:20 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: mszeredi, Daniel Berrange, slp, qemu-devel,
Dr. David Alan Gilbert, virtio-fs, P J P, Laszlo Ersek, vgoyal
On Wed, 3 Feb 2021 11:37:17 +0000
Stefan Hajnoczi <stefanha@redhat.com> wrote:
> Both lo_open() and lo_create() have similar code to open a file. Extract
> a common lo_do_open() function from lo_open() that will be used by
> lo_create() in a later commit.
>
> Since lo_do_open() does not otherwise need fuse_req_t req, convert
> lo_add_fd_mapping() to use struct lo_data *lo instead.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
With the s/ENOMEM/-ENOMEM/ change in lo_do_open() suggested by patchew,
Reviewed-by: Greg Kurz <groug@kaod.org>
> tools/virtiofsd/passthrough_ll.c | 73 ++++++++++++++++++++------------
> 1 file changed, 46 insertions(+), 27 deletions(-)
>
> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> index 5fb36d9407..e63cbd3fb7 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -459,17 +459,17 @@ static void lo_map_remove(struct lo_map *map, size_t key)
> }
>
> /* Assumes lo->mutex is held */
> -static ssize_t lo_add_fd_mapping(fuse_req_t req, int fd)
> +static ssize_t lo_add_fd_mapping(struct lo_data *lo, int fd)
> {
> struct lo_map_elem *elem;
>
> - elem = lo_map_alloc_elem(&lo_data(req)->fd_map);
> + elem = lo_map_alloc_elem(&lo->fd_map);
> if (!elem) {
> return -1;
> }
>
> elem->fd = fd;
> - return elem - lo_data(req)->fd_map.elems;
> + return elem - lo->fd_map.elems;
> }
>
> /* Assumes lo->mutex is held */
> @@ -1651,6 +1651,38 @@ static void update_open_flags(int writeback, int allow_direct_io,
> }
> }
>
> +static int lo_do_open(struct lo_data *lo, struct lo_inode *inode,
> + struct fuse_file_info *fi)
> +{
> + char buf[64];
> + ssize_t fh;
> + int fd;
> +
> + update_open_flags(lo->writeback, lo->allow_direct_io, fi);
> +
> + sprintf(buf, "%i", inode->fd);
> + fd = openat(lo->proc_self_fd, buf, fi->flags & ~O_NOFOLLOW);
> + if (fd == -1) {
> + return -errno;
> + }
> +
> + pthread_mutex_lock(&lo->mutex);
> + fh = lo_add_fd_mapping(lo, fd);
> + pthread_mutex_unlock(&lo->mutex);
> + if (fh == -1) {
> + close(fd);
> + return ENOMEM;
> + }
> +
> + fi->fh = fh;
> + if (lo->cache == CACHE_NONE) {
> + fi->direct_io = 1;
> + } else if (lo->cache == CACHE_ALWAYS) {
> + fi->keep_cache = 1;
> + }
> + return 0;
> +}
> +
> static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
> mode_t mode, struct fuse_file_info *fi)
> {
> @@ -1691,7 +1723,7 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
> ssize_t fh;
>
> pthread_mutex_lock(&lo->mutex);
> - fh = lo_add_fd_mapping(req, fd);
> + fh = lo_add_fd_mapping(lo, fd);
> pthread_mutex_unlock(&lo->mutex);
> if (fh == -1) {
> close(fd);
> @@ -1892,38 +1924,25 @@ static void lo_fsyncdir(fuse_req_t req, fuse_ino_t ino, int datasync,
>
> static void lo_open(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
> {
> - int fd;
> - ssize_t fh;
> - char buf[64];
> struct lo_data *lo = lo_data(req);
> + struct lo_inode *inode = lo_inode(req, ino);
> + int err;
>
> fuse_log(FUSE_LOG_DEBUG, "lo_open(ino=%" PRIu64 ", flags=%d)\n", ino,
> fi->flags);
>
> - update_open_flags(lo->writeback, lo->allow_direct_io, fi);
> -
> - sprintf(buf, "%i", lo_fd(req, ino));
> - fd = openat(lo->proc_self_fd, buf, fi->flags & ~O_NOFOLLOW);
> - if (fd == -1) {
> - return (void)fuse_reply_err(req, errno);
> - }
> -
> - pthread_mutex_lock(&lo->mutex);
> - fh = lo_add_fd_mapping(req, fd);
> - pthread_mutex_unlock(&lo->mutex);
> - if (fh == -1) {
> - close(fd);
> - fuse_reply_err(req, ENOMEM);
> + if (!inode) {
> + fuse_reply_err(req, EBADF);
> return;
> }
>
> - fi->fh = fh;
> - if (lo->cache == CACHE_NONE) {
> - fi->direct_io = 1;
> - } else if (lo->cache == CACHE_ALWAYS) {
> - fi->keep_cache = 1;
> + err = lo_do_open(lo, inode, fi);
> + lo_inode_put(lo, &inode);
> + if (err) {
> + fuse_reply_err(req, err);
> + } else {
> + fuse_reply_open(req, fi);
> }
> - fuse_reply_open(req, fi);
> }
>
> static void lo_release(fuse_req_t req, fuse_ino_t ino,
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [Virtio-fs] [PATCH v4 1/3] virtiofsd: extract lo_do_open() from lo_open()
@ 2021-02-03 14:20 ` Greg Kurz
0 siblings, 0 replies; 48+ messages in thread
From: Greg Kurz @ 2021-02-03 14:20 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Daniel Berrange, qemu-devel, virtio-fs, P J P, Laszlo Ersek, vgoyal
On Wed, 3 Feb 2021 11:37:17 +0000
Stefan Hajnoczi <stefanha@redhat.com> wrote:
> Both lo_open() and lo_create() have similar code to open a file. Extract
> a common lo_do_open() function from lo_open() that will be used by
> lo_create() in a later commit.
>
> Since lo_do_open() does not otherwise need fuse_req_t req, convert
> lo_add_fd_mapping() to use struct lo_data *lo instead.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
With the s/ENOMEM/-ENOMEM/ change in lo_do_open() suggested by patchew,
Reviewed-by: Greg Kurz <groug@kaod.org>
> tools/virtiofsd/passthrough_ll.c | 73 ++++++++++++++++++++------------
> 1 file changed, 46 insertions(+), 27 deletions(-)
>
> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> index 5fb36d9407..e63cbd3fb7 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -459,17 +459,17 @@ static void lo_map_remove(struct lo_map *map, size_t key)
> }
>
> /* Assumes lo->mutex is held */
> -static ssize_t lo_add_fd_mapping(fuse_req_t req, int fd)
> +static ssize_t lo_add_fd_mapping(struct lo_data *lo, int fd)
> {
> struct lo_map_elem *elem;
>
> - elem = lo_map_alloc_elem(&lo_data(req)->fd_map);
> + elem = lo_map_alloc_elem(&lo->fd_map);
> if (!elem) {
> return -1;
> }
>
> elem->fd = fd;
> - return elem - lo_data(req)->fd_map.elems;
> + return elem - lo->fd_map.elems;
> }
>
> /* Assumes lo->mutex is held */
> @@ -1651,6 +1651,38 @@ static void update_open_flags(int writeback, int allow_direct_io,
> }
> }
>
> +static int lo_do_open(struct lo_data *lo, struct lo_inode *inode,
> + struct fuse_file_info *fi)
> +{
> + char buf[64];
> + ssize_t fh;
> + int fd;
> +
> + update_open_flags(lo->writeback, lo->allow_direct_io, fi);
> +
> + sprintf(buf, "%i", inode->fd);
> + fd = openat(lo->proc_self_fd, buf, fi->flags & ~O_NOFOLLOW);
> + if (fd == -1) {
> + return -errno;
> + }
> +
> + pthread_mutex_lock(&lo->mutex);
> + fh = lo_add_fd_mapping(lo, fd);
> + pthread_mutex_unlock(&lo->mutex);
> + if (fh == -1) {
> + close(fd);
> + return ENOMEM;
> + }
> +
> + fi->fh = fh;
> + if (lo->cache == CACHE_NONE) {
> + fi->direct_io = 1;
> + } else if (lo->cache == CACHE_ALWAYS) {
> + fi->keep_cache = 1;
> + }
> + return 0;
> +}
> +
> static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
> mode_t mode, struct fuse_file_info *fi)
> {
> @@ -1691,7 +1723,7 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
> ssize_t fh;
>
> pthread_mutex_lock(&lo->mutex);
> - fh = lo_add_fd_mapping(req, fd);
> + fh = lo_add_fd_mapping(lo, fd);
> pthread_mutex_unlock(&lo->mutex);
> if (fh == -1) {
> close(fd);
> @@ -1892,38 +1924,25 @@ static void lo_fsyncdir(fuse_req_t req, fuse_ino_t ino, int datasync,
>
> static void lo_open(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
> {
> - int fd;
> - ssize_t fh;
> - char buf[64];
> struct lo_data *lo = lo_data(req);
> + struct lo_inode *inode = lo_inode(req, ino);
> + int err;
>
> fuse_log(FUSE_LOG_DEBUG, "lo_open(ino=%" PRIu64 ", flags=%d)\n", ino,
> fi->flags);
>
> - update_open_flags(lo->writeback, lo->allow_direct_io, fi);
> -
> - sprintf(buf, "%i", lo_fd(req, ino));
> - fd = openat(lo->proc_self_fd, buf, fi->flags & ~O_NOFOLLOW);
> - if (fd == -1) {
> - return (void)fuse_reply_err(req, errno);
> - }
> -
> - pthread_mutex_lock(&lo->mutex);
> - fh = lo_add_fd_mapping(req, fd);
> - pthread_mutex_unlock(&lo->mutex);
> - if (fh == -1) {
> - close(fd);
> - fuse_reply_err(req, ENOMEM);
> + if (!inode) {
> + fuse_reply_err(req, EBADF);
> return;
> }
>
> - fi->fh = fh;
> - if (lo->cache == CACHE_NONE) {
> - fi->direct_io = 1;
> - } else if (lo->cache == CACHE_ALWAYS) {
> - fi->keep_cache = 1;
> + err = lo_do_open(lo, inode, fi);
> + lo_inode_put(lo, &inode);
> + if (err) {
> + fuse_reply_err(req, err);
> + } else {
> + fuse_reply_open(req, fi);
> }
> - fuse_reply_open(req, fi);
> }
>
> static void lo_release(fuse_req_t req, fuse_ino_t ino,
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v4 1/3] virtiofsd: extract lo_do_open() from lo_open()
2021-02-03 14:20 ` [Virtio-fs] " Greg Kurz
@ 2021-02-03 14:47 ` Dr. David Alan Gilbert
-1 siblings, 0 replies; 48+ messages in thread
From: Dr. David Alan Gilbert @ 2021-02-03 14:47 UTC (permalink / raw)
To: Greg Kurz
Cc: mszeredi, Daniel Berrange, slp, qemu-devel, P J P, virtio-fs,
Stefan Hajnoczi, Laszlo Ersek, vgoyal
* Greg Kurz (groug@kaod.org) wrote:
> On Wed, 3 Feb 2021 11:37:17 +0000
> Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> > Both lo_open() and lo_create() have similar code to open a file. Extract
> > a common lo_do_open() function from lo_open() that will be used by
> > lo_create() in a later commit.
> >
> > Since lo_do_open() does not otherwise need fuse_req_t req, convert
> > lo_add_fd_mapping() to use struct lo_data *lo instead.
> >
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
>
> With the s/ENOMEM/-ENOMEM/ change in lo_do_open() suggested by patchew,
Isn't it actually the return -errno that's different from the original?
Dave
> Reviewed-by: Greg Kurz <groug@kaod.org>
>
> > tools/virtiofsd/passthrough_ll.c | 73 ++++++++++++++++++++------------
> > 1 file changed, 46 insertions(+), 27 deletions(-)
> >
> > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> > index 5fb36d9407..e63cbd3fb7 100644
> > --- a/tools/virtiofsd/passthrough_ll.c
> > +++ b/tools/virtiofsd/passthrough_ll.c
> > @@ -459,17 +459,17 @@ static void lo_map_remove(struct lo_map *map, size_t key)
> > }
> >
> > /* Assumes lo->mutex is held */
> > -static ssize_t lo_add_fd_mapping(fuse_req_t req, int fd)
> > +static ssize_t lo_add_fd_mapping(struct lo_data *lo, int fd)
> > {
> > struct lo_map_elem *elem;
> >
> > - elem = lo_map_alloc_elem(&lo_data(req)->fd_map);
> > + elem = lo_map_alloc_elem(&lo->fd_map);
> > if (!elem) {
> > return -1;
> > }
> >
> > elem->fd = fd;
> > - return elem - lo_data(req)->fd_map.elems;
> > + return elem - lo->fd_map.elems;
> > }
> >
> > /* Assumes lo->mutex is held */
> > @@ -1651,6 +1651,38 @@ static void update_open_flags(int writeback, int allow_direct_io,
> > }
> > }
> >
> > +static int lo_do_open(struct lo_data *lo, struct lo_inode *inode,
> > + struct fuse_file_info *fi)
> > +{
> > + char buf[64];
> > + ssize_t fh;
> > + int fd;
> > +
> > + update_open_flags(lo->writeback, lo->allow_direct_io, fi);
> > +
> > + sprintf(buf, "%i", inode->fd);
> > + fd = openat(lo->proc_self_fd, buf, fi->flags & ~O_NOFOLLOW);
> > + if (fd == -1) {
> > + return -errno;
> > + }
> > +
> > + pthread_mutex_lock(&lo->mutex);
> > + fh = lo_add_fd_mapping(lo, fd);
> > + pthread_mutex_unlock(&lo->mutex);
> > + if (fh == -1) {
> > + close(fd);
> > + return ENOMEM;
> > + }
> > +
> > + fi->fh = fh;
> > + if (lo->cache == CACHE_NONE) {
> > + fi->direct_io = 1;
> > + } else if (lo->cache == CACHE_ALWAYS) {
> > + fi->keep_cache = 1;
> > + }
> > + return 0;
> > +}
> > +
> > static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
> > mode_t mode, struct fuse_file_info *fi)
> > {
> > @@ -1691,7 +1723,7 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
> > ssize_t fh;
> >
> > pthread_mutex_lock(&lo->mutex);
> > - fh = lo_add_fd_mapping(req, fd);
> > + fh = lo_add_fd_mapping(lo, fd);
> > pthread_mutex_unlock(&lo->mutex);
> > if (fh == -1) {
> > close(fd);
> > @@ -1892,38 +1924,25 @@ static void lo_fsyncdir(fuse_req_t req, fuse_ino_t ino, int datasync,
> >
> > static void lo_open(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
> > {
> > - int fd;
> > - ssize_t fh;
> > - char buf[64];
> > struct lo_data *lo = lo_data(req);
> > + struct lo_inode *inode = lo_inode(req, ino);
> > + int err;
> >
> > fuse_log(FUSE_LOG_DEBUG, "lo_open(ino=%" PRIu64 ", flags=%d)\n", ino,
> > fi->flags);
> >
> > - update_open_flags(lo->writeback, lo->allow_direct_io, fi);
> > -
> > - sprintf(buf, "%i", lo_fd(req, ino));
> > - fd = openat(lo->proc_self_fd, buf, fi->flags & ~O_NOFOLLOW);
> > - if (fd == -1) {
> > - return (void)fuse_reply_err(req, errno);
> > - }
> > -
> > - pthread_mutex_lock(&lo->mutex);
> > - fh = lo_add_fd_mapping(req, fd);
> > - pthread_mutex_unlock(&lo->mutex);
> > - if (fh == -1) {
> > - close(fd);
> > - fuse_reply_err(req, ENOMEM);
> > + if (!inode) {
> > + fuse_reply_err(req, EBADF);
> > return;
> > }
> >
> > - fi->fh = fh;
> > - if (lo->cache == CACHE_NONE) {
> > - fi->direct_io = 1;
> > - } else if (lo->cache == CACHE_ALWAYS) {
> > - fi->keep_cache = 1;
> > + err = lo_do_open(lo, inode, fi);
> > + lo_inode_put(lo, &inode);
> > + if (err) {
> > + fuse_reply_err(req, err);
> > + } else {
> > + fuse_reply_open(req, fi);
> > }
> > - fuse_reply_open(req, fi);
> > }
> >
> > static void lo_release(fuse_req_t req, fuse_ino_t ino,
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [Virtio-fs] [PATCH v4 1/3] virtiofsd: extract lo_do_open() from lo_open()
@ 2021-02-03 14:47 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 48+ messages in thread
From: Dr. David Alan Gilbert @ 2021-02-03 14:47 UTC (permalink / raw)
To: Greg Kurz
Cc: Daniel Berrange, qemu-devel, P J P, virtio-fs, Laszlo Ersek, vgoyal
* Greg Kurz (groug@kaod.org) wrote:
> On Wed, 3 Feb 2021 11:37:17 +0000
> Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> > Both lo_open() and lo_create() have similar code to open a file. Extract
> > a common lo_do_open() function from lo_open() that will be used by
> > lo_create() in a later commit.
> >
> > Since lo_do_open() does not otherwise need fuse_req_t req, convert
> > lo_add_fd_mapping() to use struct lo_data *lo instead.
> >
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
>
> With the s/ENOMEM/-ENOMEM/ change in lo_do_open() suggested by patchew,
Isn't it actually the return -errno that's different from the original?
Dave
> Reviewed-by: Greg Kurz <groug@kaod.org>
>
> > tools/virtiofsd/passthrough_ll.c | 73 ++++++++++++++++++++------------
> > 1 file changed, 46 insertions(+), 27 deletions(-)
> >
> > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> > index 5fb36d9407..e63cbd3fb7 100644
> > --- a/tools/virtiofsd/passthrough_ll.c
> > +++ b/tools/virtiofsd/passthrough_ll.c
> > @@ -459,17 +459,17 @@ static void lo_map_remove(struct lo_map *map, size_t key)
> > }
> >
> > /* Assumes lo->mutex is held */
> > -static ssize_t lo_add_fd_mapping(fuse_req_t req, int fd)
> > +static ssize_t lo_add_fd_mapping(struct lo_data *lo, int fd)
> > {
> > struct lo_map_elem *elem;
> >
> > - elem = lo_map_alloc_elem(&lo_data(req)->fd_map);
> > + elem = lo_map_alloc_elem(&lo->fd_map);
> > if (!elem) {
> > return -1;
> > }
> >
> > elem->fd = fd;
> > - return elem - lo_data(req)->fd_map.elems;
> > + return elem - lo->fd_map.elems;
> > }
> >
> > /* Assumes lo->mutex is held */
> > @@ -1651,6 +1651,38 @@ static void update_open_flags(int writeback, int allow_direct_io,
> > }
> > }
> >
> > +static int lo_do_open(struct lo_data *lo, struct lo_inode *inode,
> > + struct fuse_file_info *fi)
> > +{
> > + char buf[64];
> > + ssize_t fh;
> > + int fd;
> > +
> > + update_open_flags(lo->writeback, lo->allow_direct_io, fi);
> > +
> > + sprintf(buf, "%i", inode->fd);
> > + fd = openat(lo->proc_self_fd, buf, fi->flags & ~O_NOFOLLOW);
> > + if (fd == -1) {
> > + return -errno;
> > + }
> > +
> > + pthread_mutex_lock(&lo->mutex);
> > + fh = lo_add_fd_mapping(lo, fd);
> > + pthread_mutex_unlock(&lo->mutex);
> > + if (fh == -1) {
> > + close(fd);
> > + return ENOMEM;
> > + }
> > +
> > + fi->fh = fh;
> > + if (lo->cache == CACHE_NONE) {
> > + fi->direct_io = 1;
> > + } else if (lo->cache == CACHE_ALWAYS) {
> > + fi->keep_cache = 1;
> > + }
> > + return 0;
> > +}
> > +
> > static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
> > mode_t mode, struct fuse_file_info *fi)
> > {
> > @@ -1691,7 +1723,7 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
> > ssize_t fh;
> >
> > pthread_mutex_lock(&lo->mutex);
> > - fh = lo_add_fd_mapping(req, fd);
> > + fh = lo_add_fd_mapping(lo, fd);
> > pthread_mutex_unlock(&lo->mutex);
> > if (fh == -1) {
> > close(fd);
> > @@ -1892,38 +1924,25 @@ static void lo_fsyncdir(fuse_req_t req, fuse_ino_t ino, int datasync,
> >
> > static void lo_open(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
> > {
> > - int fd;
> > - ssize_t fh;
> > - char buf[64];
> > struct lo_data *lo = lo_data(req);
> > + struct lo_inode *inode = lo_inode(req, ino);
> > + int err;
> >
> > fuse_log(FUSE_LOG_DEBUG, "lo_open(ino=%" PRIu64 ", flags=%d)\n", ino,
> > fi->flags);
> >
> > - update_open_flags(lo->writeback, lo->allow_direct_io, fi);
> > -
> > - sprintf(buf, "%i", lo_fd(req, ino));
> > - fd = openat(lo->proc_self_fd, buf, fi->flags & ~O_NOFOLLOW);
> > - if (fd == -1) {
> > - return (void)fuse_reply_err(req, errno);
> > - }
> > -
> > - pthread_mutex_lock(&lo->mutex);
> > - fh = lo_add_fd_mapping(req, fd);
> > - pthread_mutex_unlock(&lo->mutex);
> > - if (fh == -1) {
> > - close(fd);
> > - fuse_reply_err(req, ENOMEM);
> > + if (!inode) {
> > + fuse_reply_err(req, EBADF);
> > return;
> > }
> >
> > - fi->fh = fh;
> > - if (lo->cache == CACHE_NONE) {
> > - fi->direct_io = 1;
> > - } else if (lo->cache == CACHE_ALWAYS) {
> > - fi->keep_cache = 1;
> > + err = lo_do_open(lo, inode, fi);
> > + lo_inode_put(lo, &inode);
> > + if (err) {
> > + fuse_reply_err(req, err);
> > + } else {
> > + fuse_reply_open(req, fi);
> > }
> > - fuse_reply_open(req, fi);
> > }
> >
> > static void lo_release(fuse_req_t req, fuse_ino_t ino,
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v4 3/3] virtiofsd: prevent opening of special files (CVE-2020-35517)
2021-02-03 11:37 ` [Virtio-fs] " Stefan Hajnoczi
@ 2021-02-03 15:28 ` Vivek Goyal
-1 siblings, 0 replies; 48+ messages in thread
From: Vivek Goyal @ 2021-02-03 15:28 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: mszeredi, Daniel Berrange, slp, Greg Kurz, qemu-devel, virtio-fs,
Alex Xu, P J P, Laszlo Ersek, Dr. David Alan Gilbert
On Wed, Feb 03, 2021 at 11:37:19AM +0000, Stefan Hajnoczi wrote:
[..]
> @@ -1727,36 +1764,38 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
>
> update_open_flags(lo->writeback, lo->allow_direct_io, fi);
>
> - fd = openat(parent_inode->fd, name, (fi->flags | O_CREAT) & ~O_NOFOLLOW,
> - mode);
> + /* Try to create a new file but don't open existing files */
> + fd = openat(parent_inode->fd, name, fi->flags | O_CREAT | O_EXCL, mode);
> err = fd == -1 ? errno : 0;
> +
> lo_restore_cred(&old);
>
> - if (!err) {
> - ssize_t fh;
> -
> - pthread_mutex_lock(&lo->mutex);
> - fh = lo_add_fd_mapping(lo, fd);
> - pthread_mutex_unlock(&lo->mutex);
> - if (fh == -1) {
> - close(fd);
> - err = ENOMEM;
> - goto out;
> - }
> + /* Ignore the error if file exists and O_EXCL was not given */
> + if (err && !(err == EEXIST && !(fi->flags & O_EXCL))) {
Can this check be simplified to.
if (err && (err == EEXIST && (fi->flags & O_EXCL)) {
goto out;
}
> + goto out;
> + }
Vivek
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [Virtio-fs] [PATCH v4 3/3] virtiofsd: prevent opening of special files (CVE-2020-35517)
@ 2021-02-03 15:28 ` Vivek Goyal
0 siblings, 0 replies; 48+ messages in thread
From: Vivek Goyal @ 2021-02-03 15:28 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Daniel Berrange, qemu-devel, virtio-fs, Alex Xu, P J P, Laszlo Ersek
On Wed, Feb 03, 2021 at 11:37:19AM +0000, Stefan Hajnoczi wrote:
[..]
> @@ -1727,36 +1764,38 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
>
> update_open_flags(lo->writeback, lo->allow_direct_io, fi);
>
> - fd = openat(parent_inode->fd, name, (fi->flags | O_CREAT) & ~O_NOFOLLOW,
> - mode);
> + /* Try to create a new file but don't open existing files */
> + fd = openat(parent_inode->fd, name, fi->flags | O_CREAT | O_EXCL, mode);
> err = fd == -1 ? errno : 0;
> +
> lo_restore_cred(&old);
>
> - if (!err) {
> - ssize_t fh;
> -
> - pthread_mutex_lock(&lo->mutex);
> - fh = lo_add_fd_mapping(lo, fd);
> - pthread_mutex_unlock(&lo->mutex);
> - if (fh == -1) {
> - close(fd);
> - err = ENOMEM;
> - goto out;
> - }
> + /* Ignore the error if file exists and O_EXCL was not given */
> + if (err && !(err == EEXIST && !(fi->flags & O_EXCL))) {
Can this check be simplified to.
if (err && (err == EEXIST && (fi->flags & O_EXCL)) {
goto out;
}
> + goto out;
> + }
Vivek
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v4 1/3] virtiofsd: extract lo_do_open() from lo_open()
2021-02-03 14:47 ` [Virtio-fs] " Dr. David Alan Gilbert
@ 2021-02-03 15:45 ` Greg Kurz
-1 siblings, 0 replies; 48+ messages in thread
From: Greg Kurz @ 2021-02-03 15:45 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: mszeredi, Daniel Berrange, slp, qemu-devel, P J P, virtio-fs,
Stefan Hajnoczi, Laszlo Ersek, vgoyal
On Wed, 3 Feb 2021 14:47:30 +0000
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> * Greg Kurz (groug@kaod.org) wrote:
> > On Wed, 3 Feb 2021 11:37:17 +0000
> > Stefan Hajnoczi <stefanha@redhat.com> wrote:
> >
> > > Both lo_open() and lo_create() have similar code to open a file. Extract
> > > a common lo_do_open() function from lo_open() that will be used by
> > > lo_create() in a later commit.
> > >
> > > Since lo_do_open() does not otherwise need fuse_req_t req, convert
> > > lo_add_fd_mapping() to use struct lo_data *lo instead.
> > >
> > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > ---
> >
> > With the s/ENOMEM/-ENOMEM/ change in lo_do_open() suggested by patchew,
>
> Isn't it actually the return -errno that's different from the original?
>
Yes and this is expected. The original sits in the top level handler
lo_open() which sends the reply back to the client and doesn't have
a return value, while lo_do_open() has a return value which should be
0 or negative.
> Dave
>
> > Reviewed-by: Greg Kurz <groug@kaod.org>
> >
> > > tools/virtiofsd/passthrough_ll.c | 73 ++++++++++++++++++++------------
> > > 1 file changed, 46 insertions(+), 27 deletions(-)
> > >
> > > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> > > index 5fb36d9407..e63cbd3fb7 100644
> > > --- a/tools/virtiofsd/passthrough_ll.c
> > > +++ b/tools/virtiofsd/passthrough_ll.c
> > > @@ -459,17 +459,17 @@ static void lo_map_remove(struct lo_map *map, size_t key)
> > > }
> > >
> > > /* Assumes lo->mutex is held */
> > > -static ssize_t lo_add_fd_mapping(fuse_req_t req, int fd)
> > > +static ssize_t lo_add_fd_mapping(struct lo_data *lo, int fd)
> > > {
> > > struct lo_map_elem *elem;
> > >
> > > - elem = lo_map_alloc_elem(&lo_data(req)->fd_map);
> > > + elem = lo_map_alloc_elem(&lo->fd_map);
> > > if (!elem) {
> > > return -1;
> > > }
> > >
> > > elem->fd = fd;
> > > - return elem - lo_data(req)->fd_map.elems;
> > > + return elem - lo->fd_map.elems;
> > > }
> > >
> > > /* Assumes lo->mutex is held */
> > > @@ -1651,6 +1651,38 @@ static void update_open_flags(int writeback, int allow_direct_io,
> > > }
> > > }
> > >
> > > +static int lo_do_open(struct lo_data *lo, struct lo_inode *inode,
> > > + struct fuse_file_info *fi)
> > > +{
> > > + char buf[64];
> > > + ssize_t fh;
> > > + int fd;
> > > +
> > > + update_open_flags(lo->writeback, lo->allow_direct_io, fi);
> > > +
> > > + sprintf(buf, "%i", inode->fd);
> > > + fd = openat(lo->proc_self_fd, buf, fi->flags & ~O_NOFOLLOW);
> > > + if (fd == -1) {
> > > + return -errno;
> > > + }
> > > +
> > > + pthread_mutex_lock(&lo->mutex);
> > > + fh = lo_add_fd_mapping(lo, fd);
> > > + pthread_mutex_unlock(&lo->mutex);
> > > + if (fh == -1) {
> > > + close(fd);
> > > + return ENOMEM;
> > > + }
> > > +
> > > + fi->fh = fh;
> > > + if (lo->cache == CACHE_NONE) {
> > > + fi->direct_io = 1;
> > > + } else if (lo->cache == CACHE_ALWAYS) {
> > > + fi->keep_cache = 1;
> > > + }
> > > + return 0;
> > > +}
> > > +
> > > static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
> > > mode_t mode, struct fuse_file_info *fi)
> > > {
> > > @@ -1691,7 +1723,7 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
> > > ssize_t fh;
> > >
> > > pthread_mutex_lock(&lo->mutex);
> > > - fh = lo_add_fd_mapping(req, fd);
> > > + fh = lo_add_fd_mapping(lo, fd);
> > > pthread_mutex_unlock(&lo->mutex);
> > > if (fh == -1) {
> > > close(fd);
> > > @@ -1892,38 +1924,25 @@ static void lo_fsyncdir(fuse_req_t req, fuse_ino_t ino, int datasync,
> > >
> > > static void lo_open(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
> > > {
> > > - int fd;
> > > - ssize_t fh;
> > > - char buf[64];
> > > struct lo_data *lo = lo_data(req);
> > > + struct lo_inode *inode = lo_inode(req, ino);
> > > + int err;
> > >
> > > fuse_log(FUSE_LOG_DEBUG, "lo_open(ino=%" PRIu64 ", flags=%d)\n", ino,
> > > fi->flags);
> > >
> > > - update_open_flags(lo->writeback, lo->allow_direct_io, fi);
> > > -
> > > - sprintf(buf, "%i", lo_fd(req, ino));
> > > - fd = openat(lo->proc_self_fd, buf, fi->flags & ~O_NOFOLLOW);
> > > - if (fd == -1) {
> > > - return (void)fuse_reply_err(req, errno);
> > > - }
> > > -
> > > - pthread_mutex_lock(&lo->mutex);
> > > - fh = lo_add_fd_mapping(req, fd);
> > > - pthread_mutex_unlock(&lo->mutex);
> > > - if (fh == -1) {
> > > - close(fd);
> > > - fuse_reply_err(req, ENOMEM);
> > > + if (!inode) {
> > > + fuse_reply_err(req, EBADF);
> > > return;
> > > }
> > >
> > > - fi->fh = fh;
> > > - if (lo->cache == CACHE_NONE) {
> > > - fi->direct_io = 1;
> > > - } else if (lo->cache == CACHE_ALWAYS) {
> > > - fi->keep_cache = 1;
> > > + err = lo_do_open(lo, inode, fi);
> > > + lo_inode_put(lo, &inode);
> > > + if (err) {
> > > + fuse_reply_err(req, err);
> > > + } else {
> > > + fuse_reply_open(req, fi);
> > > }
> > > - fuse_reply_open(req, fi);
> > > }
> > >
> > > static void lo_release(fuse_req_t req, fuse_ino_t ino,
> >
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [Virtio-fs] [PATCH v4 1/3] virtiofsd: extract lo_do_open() from lo_open()
@ 2021-02-03 15:45 ` Greg Kurz
0 siblings, 0 replies; 48+ messages in thread
From: Greg Kurz @ 2021-02-03 15:45 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: Daniel Berrange, qemu-devel, P J P, virtio-fs, Laszlo Ersek, vgoyal
On Wed, 3 Feb 2021 14:47:30 +0000
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> * Greg Kurz (groug@kaod.org) wrote:
> > On Wed, 3 Feb 2021 11:37:17 +0000
> > Stefan Hajnoczi <stefanha@redhat.com> wrote:
> >
> > > Both lo_open() and lo_create() have similar code to open a file. Extract
> > > a common lo_do_open() function from lo_open() that will be used by
> > > lo_create() in a later commit.
> > >
> > > Since lo_do_open() does not otherwise need fuse_req_t req, convert
> > > lo_add_fd_mapping() to use struct lo_data *lo instead.
> > >
> > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > ---
> >
> > With the s/ENOMEM/-ENOMEM/ change in lo_do_open() suggested by patchew,
>
> Isn't it actually the return -errno that's different from the original?
>
Yes and this is expected. The original sits in the top level handler
lo_open() which sends the reply back to the client and doesn't have
a return value, while lo_do_open() has a return value which should be
0 or negative.
> Dave
>
> > Reviewed-by: Greg Kurz <groug@kaod.org>
> >
> > > tools/virtiofsd/passthrough_ll.c | 73 ++++++++++++++++++++------------
> > > 1 file changed, 46 insertions(+), 27 deletions(-)
> > >
> > > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> > > index 5fb36d9407..e63cbd3fb7 100644
> > > --- a/tools/virtiofsd/passthrough_ll.c
> > > +++ b/tools/virtiofsd/passthrough_ll.c
> > > @@ -459,17 +459,17 @@ static void lo_map_remove(struct lo_map *map, size_t key)
> > > }
> > >
> > > /* Assumes lo->mutex is held */
> > > -static ssize_t lo_add_fd_mapping(fuse_req_t req, int fd)
> > > +static ssize_t lo_add_fd_mapping(struct lo_data *lo, int fd)
> > > {
> > > struct lo_map_elem *elem;
> > >
> > > - elem = lo_map_alloc_elem(&lo_data(req)->fd_map);
> > > + elem = lo_map_alloc_elem(&lo->fd_map);
> > > if (!elem) {
> > > return -1;
> > > }
> > >
> > > elem->fd = fd;
> > > - return elem - lo_data(req)->fd_map.elems;
> > > + return elem - lo->fd_map.elems;
> > > }
> > >
> > > /* Assumes lo->mutex is held */
> > > @@ -1651,6 +1651,38 @@ static void update_open_flags(int writeback, int allow_direct_io,
> > > }
> > > }
> > >
> > > +static int lo_do_open(struct lo_data *lo, struct lo_inode *inode,
> > > + struct fuse_file_info *fi)
> > > +{
> > > + char buf[64];
> > > + ssize_t fh;
> > > + int fd;
> > > +
> > > + update_open_flags(lo->writeback, lo->allow_direct_io, fi);
> > > +
> > > + sprintf(buf, "%i", inode->fd);
> > > + fd = openat(lo->proc_self_fd, buf, fi->flags & ~O_NOFOLLOW);
> > > + if (fd == -1) {
> > > + return -errno;
> > > + }
> > > +
> > > + pthread_mutex_lock(&lo->mutex);
> > > + fh = lo_add_fd_mapping(lo, fd);
> > > + pthread_mutex_unlock(&lo->mutex);
> > > + if (fh == -1) {
> > > + close(fd);
> > > + return ENOMEM;
> > > + }
> > > +
> > > + fi->fh = fh;
> > > + if (lo->cache == CACHE_NONE) {
> > > + fi->direct_io = 1;
> > > + } else if (lo->cache == CACHE_ALWAYS) {
> > > + fi->keep_cache = 1;
> > > + }
> > > + return 0;
> > > +}
> > > +
> > > static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
> > > mode_t mode, struct fuse_file_info *fi)
> > > {
> > > @@ -1691,7 +1723,7 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
> > > ssize_t fh;
> > >
> > > pthread_mutex_lock(&lo->mutex);
> > > - fh = lo_add_fd_mapping(req, fd);
> > > + fh = lo_add_fd_mapping(lo, fd);
> > > pthread_mutex_unlock(&lo->mutex);
> > > if (fh == -1) {
> > > close(fd);
> > > @@ -1892,38 +1924,25 @@ static void lo_fsyncdir(fuse_req_t req, fuse_ino_t ino, int datasync,
> > >
> > > static void lo_open(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
> > > {
> > > - int fd;
> > > - ssize_t fh;
> > > - char buf[64];
> > > struct lo_data *lo = lo_data(req);
> > > + struct lo_inode *inode = lo_inode(req, ino);
> > > + int err;
> > >
> > > fuse_log(FUSE_LOG_DEBUG, "lo_open(ino=%" PRIu64 ", flags=%d)\n", ino,
> > > fi->flags);
> > >
> > > - update_open_flags(lo->writeback, lo->allow_direct_io, fi);
> > > -
> > > - sprintf(buf, "%i", lo_fd(req, ino));
> > > - fd = openat(lo->proc_self_fd, buf, fi->flags & ~O_NOFOLLOW);
> > > - if (fd == -1) {
> > > - return (void)fuse_reply_err(req, errno);
> > > - }
> > > -
> > > - pthread_mutex_lock(&lo->mutex);
> > > - fh = lo_add_fd_mapping(req, fd);
> > > - pthread_mutex_unlock(&lo->mutex);
> > > - if (fh == -1) {
> > > - close(fd);
> > > - fuse_reply_err(req, ENOMEM);
> > > + if (!inode) {
> > > + fuse_reply_err(req, EBADF);
> > > return;
> > > }
> > >
> > > - fi->fh = fh;
> > > - if (lo->cache == CACHE_NONE) {
> > > - fi->direct_io = 1;
> > > - } else if (lo->cache == CACHE_ALWAYS) {
> > > - fi->keep_cache = 1;
> > > + err = lo_do_open(lo, inode, fi);
> > > + lo_inode_put(lo, &inode);
> > > + if (err) {
> > > + fuse_reply_err(req, err);
> > > + } else {
> > > + fuse_reply_open(req, fi);
> > > }
> > > - fuse_reply_open(req, fi);
> > > }
> > >
> > > static void lo_release(fuse_req_t req, fuse_ino_t ino,
> >
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v4 3/3] virtiofsd: prevent opening of special files (CVE-2020-35517)
2021-02-03 11:37 ` [Virtio-fs] " Stefan Hajnoczi
@ 2021-02-03 15:57 ` Greg Kurz
-1 siblings, 0 replies; 48+ messages in thread
From: Greg Kurz @ 2021-02-03 15:57 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: mszeredi, Daniel Berrange, slp, qemu-devel,
Dr. David Alan Gilbert, virtio-fs, Alex Xu, P J P, Laszlo Ersek,
vgoyal
On Wed, 3 Feb 2021 11:37:19 +0000
Stefan Hajnoczi <stefanha@redhat.com> wrote:
> A well-behaved FUSE client does not attempt to open special files with
> FUSE_OPEN because they are handled on the client side (e.g. device nodes
> are handled by client-side device drivers).
>
> The check to prevent virtiofsd from opening special files is missing in
> a few cases, most notably FUSE_OPEN. A malicious client can cause
> virtiofsd to open a device node, potentially allowing the guest to
> escape. This can be exploited by a modified guest device driver. It is
> not exploitable from guest userspace since the guest kernel will handle
> special files inside the guest instead of sending FUSE requests.
>
> This patch fixes this issue by introducing the lo_inode_open() function
> to check the file type before opening it. This is a short-term solution
> because it does not prevent a compromised virtiofsd process from opening
> device nodes on the host.
>
> Restructure lo_create() to try O_CREAT | O_EXCL first. Note that O_CREAT
> | O_EXCL does not follow symlinks, so O_NOFOLLOW masking is not
> necessary here. If the file exists and the user did not specify O_EXCL,
> open it via lo_do_open().
>
> Reported-by: Alex Xu <alex@alxu.ca>
> Fixes: CVE-2020-35517
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Reviewed-by: Vivek Goyal <vgoyal@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
> v3:
> * Restructure lo_create() to handle externally-created files (we need
> to allocate an inode for them) [Greg]
> v3:
> * Protect lo_create() [Greg]
> v2:
> * Add doc comment clarifying that symlinks are traversed client-side
> [Daniel]
>
> This issue was diagnosed on public IRC and is therefore already known
> and not embargoed.
>
> A stronger fix, and the long-term solution, is for users to mount the
> shared directory and any sub-mounts with nodev, as well as nosuid and
> noexec. Unfortunately virtiofsd cannot do this automatically because
> bind mounts added by the user after virtiofsd has launched would not be
> detected. I suggest the following:
>
> 1. Modify libvirt and Kata Containers to explicitly set these mount
> options.
> 2. Then modify virtiofsd to check that the shared directory has the
> necessary options at startup. Refuse to start if the options are
> missing so that the user is aware of the security requirements.
>
> As a bonus this also increases the likelihood that other host processes
> besides virtiofsd will be protected by nosuid/noexec/nodev so that a
> malicious guest cannot drop these files in place and then arrange for a
> host process to come across them.
>
> Additionally, user namespaces have been discussed. They seem like a
> worthwhile addition as an unprivileged or privilege-separated mode
> although there are limitations with respect to security xattrs and the
> actual uid/gid stored on the host file system not corresponding to the
> guest uid/gid.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
> tools/virtiofsd/passthrough_ll.c | 139 +++++++++++++++++++------------
> 1 file changed, 88 insertions(+), 51 deletions(-)
>
> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> index c87a1f3d72..b607ef0f7e 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -555,6 +555,38 @@ static int lo_fd(fuse_req_t req, fuse_ino_t ino)
> return fd;
> }
>
> +/*
> + * Open a file descriptor for an inode. Returns -EBADF if the inode is not a
> + * regular file or a directory.
> + *
> + * Use this helper function instead of raw openat(2) to prevent security issues
> + * when a malicious client opens special files such as block device nodes.
> + * Symlink inodes are also rejected since symlinks must already have been
> + * traversed on the client side.
> + */
> +static int lo_inode_open(struct lo_data *lo, struct lo_inode *inode,
> + int open_flags)
> +{
> + g_autofree char *fd_str = g_strdup_printf("%d", inode->fd);
> + int fd;
> +
> + if (!S_ISREG(inode->filetype) && !S_ISDIR(inode->filetype)) {
> + return -EBADF;
> + }
> +
> + /*
> + * The file is a symlink so O_NOFOLLOW must be ignored. We checked earlier
> + * that the inode is not a special file but if an external process races
> + * with us then symlinks are traversed here. It is not possible to escape
> + * the shared directory since it is mounted as "/" though.
> + */
> + fd = openat(lo->proc_self_fd, fd_str, open_flags & ~O_NOFOLLOW);
> + if (fd < 0) {
> + return -errno;
> + }
> + return fd;
> +}
> +
> static void lo_init(void *userdata, struct fuse_conn_info *conn)
> {
> struct lo_data *lo = (struct lo_data *)userdata;
> @@ -684,8 +716,7 @@ static void lo_setattr(fuse_req_t req, fuse_ino_t ino, struct stat *attr,
> if (fi) {
> truncfd = fd;
> } else {
> - sprintf(procname, "%i", ifd);
> - truncfd = openat(lo->proc_self_fd, procname, O_RDWR);
> + truncfd = lo_inode_open(lo, inode, O_RDWR);
> if (truncfd < 0) {
> goto out_err;
> }
> @@ -1664,19 +1695,24 @@ static void update_open_flags(int writeback, int allow_direct_io,
> }
> }
>
> +/*
> + * Open a regular file, set up an fd mapping, and fill out the struct
> + * fuse_file_info for it. If existing_fd is not negative, use that fd instead
> + * opening a new one. Takes ownership of existing_fd.
> + */
> static int lo_do_open(struct lo_data *lo, struct lo_inode *inode,
> - struct fuse_file_info *fi)
> + int existing_fd, struct fuse_file_info *fi)
> {
> - char buf[64];
> ssize_t fh;
> - int fd;
> + int fd = existing_fd;
>
> update_open_flags(lo->writeback, lo->allow_direct_io, fi);
>
> - sprintf(buf, "%i", inode->fd);
> - fd = openat(lo->proc_self_fd, buf, fi->flags & ~O_NOFOLLOW);
> - if (fd == -1) {
> - return -errno;
> + if (fd < 0) {
> + fd = lo_inode_open(lo, inode, fi->flags);
> + if (fd == -1) {
> + return -errno;
> + }
lo_inode_open() returns a negative errno already so
this should be converted to:
if (fd < 0) {
return fd;
}
Apart from that LGTM.
> }
>
> pthread_mutex_lock(&lo->mutex);
> @@ -1699,9 +1735,10 @@ static int lo_do_open(struct lo_data *lo, struct lo_inode *inode,
> static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
> mode_t mode, struct fuse_file_info *fi)
> {
> - int fd;
> + int fd = -1;
> struct lo_data *lo = lo_data(req);
> struct lo_inode *parent_inode;
> + struct lo_inode *inode = NULL;
> struct fuse_entry_param e;
> int err;
> struct lo_cred old = {};
> @@ -1727,36 +1764,38 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
>
> update_open_flags(lo->writeback, lo->allow_direct_io, fi);
>
> - fd = openat(parent_inode->fd, name, (fi->flags | O_CREAT) & ~O_NOFOLLOW,
> - mode);
> + /* Try to create a new file but don't open existing files */
> + fd = openat(parent_inode->fd, name, fi->flags | O_CREAT | O_EXCL, mode);
> err = fd == -1 ? errno : 0;
> +
> lo_restore_cred(&old);
>
> - if (!err) {
> - ssize_t fh;
> -
> - pthread_mutex_lock(&lo->mutex);
> - fh = lo_add_fd_mapping(lo, fd);
> - pthread_mutex_unlock(&lo->mutex);
> - if (fh == -1) {
> - close(fd);
> - err = ENOMEM;
> - goto out;
> - }
> + /* Ignore the error if file exists and O_EXCL was not given */
> + if (err && !(err == EEXIST && !(fi->flags & O_EXCL))) {
> + goto out;
> + }
>
> - fi->fh = fh;
> - err = lo_do_lookup(req, parent, name, &e, NULL);
> + err = lo_do_lookup(req, parent, name, &e, &inode);
> + if (err) {
> + goto out;
> }
> - if (lo->cache == CACHE_NONE) {
> - fi->direct_io = 1;
> - } else if (lo->cache == CACHE_ALWAYS) {
> - fi->keep_cache = 1;
> +
> + err = lo_do_open(lo, inode, fd, fi);
> + fd = -1; /* lo_do_open() takes ownership of fd */
> + if (err) {
> + /* Undo lo_do_lookup() nlookup ref */
> + unref_inode_lolocked(lo, inode, 1);
> }
>
> out:
> + lo_inode_put(lo, &inode);
> lo_inode_put(lo, &parent_inode);
>
> if (err) {
> + if (fd >= 0) {
> + close(fd);
> + }
> +
> fuse_reply_err(req, err);
> } else {
> fuse_reply_create(req, &e, fi);
> @@ -1770,7 +1809,6 @@ static struct lo_inode_plock *lookup_create_plock_ctx(struct lo_data *lo,
> pid_t pid, int *err)
> {
> struct lo_inode_plock *plock;
> - char procname[64];
> int fd;
>
> plock =
> @@ -1787,12 +1825,10 @@ static struct lo_inode_plock *lookup_create_plock_ctx(struct lo_data *lo,
> }
>
> /* Open another instance of file which can be used for ofd locks. */
> - sprintf(procname, "%i", inode->fd);
> -
> /* TODO: What if file is not writable? */
> - fd = openat(lo->proc_self_fd, procname, O_RDWR);
> - if (fd == -1) {
> - *err = errno;
> + fd = lo_inode_open(lo, inode, O_RDWR);
> + if (fd < 0) {
> + *err = -fd;
> free(plock);
> return NULL;
> }
> @@ -1949,7 +1985,7 @@ static void lo_open(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
> return;
> }
>
> - err = lo_do_open(lo, inode, fi);
> + err = lo_do_open(lo, inode, -1, fi);
> lo_inode_put(lo, &inode);
> if (err) {
> fuse_reply_err(req, err);
> @@ -2014,39 +2050,40 @@ static void lo_flush(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
> static void lo_fsync(fuse_req_t req, fuse_ino_t ino, int datasync,
> struct fuse_file_info *fi)
> {
> + struct lo_inode *inode = lo_inode(req, ino);
> + struct lo_data *lo = lo_data(req);
> int res;
> int fd;
> - char *buf;
>
> fuse_log(FUSE_LOG_DEBUG, "lo_fsync(ino=%" PRIu64 ", fi=0x%p)\n", ino,
> (void *)fi);
>
> + if (!inode) {
> + fuse_reply_err(req, EBADF);
> + return;
> + }
> +
> if (!fi) {
> - struct lo_data *lo = lo_data(req);
> -
> - res = asprintf(&buf, "%i", lo_fd(req, ino));
> - if (res == -1) {
> - return (void)fuse_reply_err(req, errno);
> - }
> -
> - fd = openat(lo->proc_self_fd, buf, O_RDWR);
> - free(buf);
> - if (fd == -1) {
> - return (void)fuse_reply_err(req, errno);
> + fd = lo_inode_open(lo, inode, O_RDWR);
> + if (fd < 0) {
> + res = -fd;
> + goto out;
> }
> } else {
> fd = lo_fi_fd(req, fi);
> }
>
> if (datasync) {
> - res = fdatasync(fd);
> + res = fdatasync(fd) == -1 ? errno : 0;
> } else {
> - res = fsync(fd);
> + res = fsync(fd) == -1 ? errno : 0;
> }
> if (!fi) {
> close(fd);
> }
> - fuse_reply_err(req, res == -1 ? errno : 0);
> +out:
> + lo_inode_put(lo, &inode);
> + fuse_reply_err(req, res);
> }
>
> static void lo_read(fuse_req_t req, fuse_ino_t ino, size_t size, off_t offset,
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [Virtio-fs] [PATCH v4 3/3] virtiofsd: prevent opening of special files (CVE-2020-35517)
@ 2021-02-03 15:57 ` Greg Kurz
0 siblings, 0 replies; 48+ messages in thread
From: Greg Kurz @ 2021-02-03 15:57 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Daniel Berrange, qemu-devel, virtio-fs, Alex Xu, P J P,
Laszlo Ersek, vgoyal
On Wed, 3 Feb 2021 11:37:19 +0000
Stefan Hajnoczi <stefanha@redhat.com> wrote:
> A well-behaved FUSE client does not attempt to open special files with
> FUSE_OPEN because they are handled on the client side (e.g. device nodes
> are handled by client-side device drivers).
>
> The check to prevent virtiofsd from opening special files is missing in
> a few cases, most notably FUSE_OPEN. A malicious client can cause
> virtiofsd to open a device node, potentially allowing the guest to
> escape. This can be exploited by a modified guest device driver. It is
> not exploitable from guest userspace since the guest kernel will handle
> special files inside the guest instead of sending FUSE requests.
>
> This patch fixes this issue by introducing the lo_inode_open() function
> to check the file type before opening it. This is a short-term solution
> because it does not prevent a compromised virtiofsd process from opening
> device nodes on the host.
>
> Restructure lo_create() to try O_CREAT | O_EXCL first. Note that O_CREAT
> | O_EXCL does not follow symlinks, so O_NOFOLLOW masking is not
> necessary here. If the file exists and the user did not specify O_EXCL,
> open it via lo_do_open().
>
> Reported-by: Alex Xu <alex@alxu.ca>
> Fixes: CVE-2020-35517
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Reviewed-by: Vivek Goyal <vgoyal@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
> v3:
> * Restructure lo_create() to handle externally-created files (we need
> to allocate an inode for them) [Greg]
> v3:
> * Protect lo_create() [Greg]
> v2:
> * Add doc comment clarifying that symlinks are traversed client-side
> [Daniel]
>
> This issue was diagnosed on public IRC and is therefore already known
> and not embargoed.
>
> A stronger fix, and the long-term solution, is for users to mount the
> shared directory and any sub-mounts with nodev, as well as nosuid and
> noexec. Unfortunately virtiofsd cannot do this automatically because
> bind mounts added by the user after virtiofsd has launched would not be
> detected. I suggest the following:
>
> 1. Modify libvirt and Kata Containers to explicitly set these mount
> options.
> 2. Then modify virtiofsd to check that the shared directory has the
> necessary options at startup. Refuse to start if the options are
> missing so that the user is aware of the security requirements.
>
> As a bonus this also increases the likelihood that other host processes
> besides virtiofsd will be protected by nosuid/noexec/nodev so that a
> malicious guest cannot drop these files in place and then arrange for a
> host process to come across them.
>
> Additionally, user namespaces have been discussed. They seem like a
> worthwhile addition as an unprivileged or privilege-separated mode
> although there are limitations with respect to security xattrs and the
> actual uid/gid stored on the host file system not corresponding to the
> guest uid/gid.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
> tools/virtiofsd/passthrough_ll.c | 139 +++++++++++++++++++------------
> 1 file changed, 88 insertions(+), 51 deletions(-)
>
> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> index c87a1f3d72..b607ef0f7e 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -555,6 +555,38 @@ static int lo_fd(fuse_req_t req, fuse_ino_t ino)
> return fd;
> }
>
> +/*
> + * Open a file descriptor for an inode. Returns -EBADF if the inode is not a
> + * regular file or a directory.
> + *
> + * Use this helper function instead of raw openat(2) to prevent security issues
> + * when a malicious client opens special files such as block device nodes.
> + * Symlink inodes are also rejected since symlinks must already have been
> + * traversed on the client side.
> + */
> +static int lo_inode_open(struct lo_data *lo, struct lo_inode *inode,
> + int open_flags)
> +{
> + g_autofree char *fd_str = g_strdup_printf("%d", inode->fd);
> + int fd;
> +
> + if (!S_ISREG(inode->filetype) && !S_ISDIR(inode->filetype)) {
> + return -EBADF;
> + }
> +
> + /*
> + * The file is a symlink so O_NOFOLLOW must be ignored. We checked earlier
> + * that the inode is not a special file but if an external process races
> + * with us then symlinks are traversed here. It is not possible to escape
> + * the shared directory since it is mounted as "/" though.
> + */
> + fd = openat(lo->proc_self_fd, fd_str, open_flags & ~O_NOFOLLOW);
> + if (fd < 0) {
> + return -errno;
> + }
> + return fd;
> +}
> +
> static void lo_init(void *userdata, struct fuse_conn_info *conn)
> {
> struct lo_data *lo = (struct lo_data *)userdata;
> @@ -684,8 +716,7 @@ static void lo_setattr(fuse_req_t req, fuse_ino_t ino, struct stat *attr,
> if (fi) {
> truncfd = fd;
> } else {
> - sprintf(procname, "%i", ifd);
> - truncfd = openat(lo->proc_self_fd, procname, O_RDWR);
> + truncfd = lo_inode_open(lo, inode, O_RDWR);
> if (truncfd < 0) {
> goto out_err;
> }
> @@ -1664,19 +1695,24 @@ static void update_open_flags(int writeback, int allow_direct_io,
> }
> }
>
> +/*
> + * Open a regular file, set up an fd mapping, and fill out the struct
> + * fuse_file_info for it. If existing_fd is not negative, use that fd instead
> + * opening a new one. Takes ownership of existing_fd.
> + */
> static int lo_do_open(struct lo_data *lo, struct lo_inode *inode,
> - struct fuse_file_info *fi)
> + int existing_fd, struct fuse_file_info *fi)
> {
> - char buf[64];
> ssize_t fh;
> - int fd;
> + int fd = existing_fd;
>
> update_open_flags(lo->writeback, lo->allow_direct_io, fi);
>
> - sprintf(buf, "%i", inode->fd);
> - fd = openat(lo->proc_self_fd, buf, fi->flags & ~O_NOFOLLOW);
> - if (fd == -1) {
> - return -errno;
> + if (fd < 0) {
> + fd = lo_inode_open(lo, inode, fi->flags);
> + if (fd == -1) {
> + return -errno;
> + }
lo_inode_open() returns a negative errno already so
this should be converted to:
if (fd < 0) {
return fd;
}
Apart from that LGTM.
> }
>
> pthread_mutex_lock(&lo->mutex);
> @@ -1699,9 +1735,10 @@ static int lo_do_open(struct lo_data *lo, struct lo_inode *inode,
> static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
> mode_t mode, struct fuse_file_info *fi)
> {
> - int fd;
> + int fd = -1;
> struct lo_data *lo = lo_data(req);
> struct lo_inode *parent_inode;
> + struct lo_inode *inode = NULL;
> struct fuse_entry_param e;
> int err;
> struct lo_cred old = {};
> @@ -1727,36 +1764,38 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
>
> update_open_flags(lo->writeback, lo->allow_direct_io, fi);
>
> - fd = openat(parent_inode->fd, name, (fi->flags | O_CREAT) & ~O_NOFOLLOW,
> - mode);
> + /* Try to create a new file but don't open existing files */
> + fd = openat(parent_inode->fd, name, fi->flags | O_CREAT | O_EXCL, mode);
> err = fd == -1 ? errno : 0;
> +
> lo_restore_cred(&old);
>
> - if (!err) {
> - ssize_t fh;
> -
> - pthread_mutex_lock(&lo->mutex);
> - fh = lo_add_fd_mapping(lo, fd);
> - pthread_mutex_unlock(&lo->mutex);
> - if (fh == -1) {
> - close(fd);
> - err = ENOMEM;
> - goto out;
> - }
> + /* Ignore the error if file exists and O_EXCL was not given */
> + if (err && !(err == EEXIST && !(fi->flags & O_EXCL))) {
> + goto out;
> + }
>
> - fi->fh = fh;
> - err = lo_do_lookup(req, parent, name, &e, NULL);
> + err = lo_do_lookup(req, parent, name, &e, &inode);
> + if (err) {
> + goto out;
> }
> - if (lo->cache == CACHE_NONE) {
> - fi->direct_io = 1;
> - } else if (lo->cache == CACHE_ALWAYS) {
> - fi->keep_cache = 1;
> +
> + err = lo_do_open(lo, inode, fd, fi);
> + fd = -1; /* lo_do_open() takes ownership of fd */
> + if (err) {
> + /* Undo lo_do_lookup() nlookup ref */
> + unref_inode_lolocked(lo, inode, 1);
> }
>
> out:
> + lo_inode_put(lo, &inode);
> lo_inode_put(lo, &parent_inode);
>
> if (err) {
> + if (fd >= 0) {
> + close(fd);
> + }
> +
> fuse_reply_err(req, err);
> } else {
> fuse_reply_create(req, &e, fi);
> @@ -1770,7 +1809,6 @@ static struct lo_inode_plock *lookup_create_plock_ctx(struct lo_data *lo,
> pid_t pid, int *err)
> {
> struct lo_inode_plock *plock;
> - char procname[64];
> int fd;
>
> plock =
> @@ -1787,12 +1825,10 @@ static struct lo_inode_plock *lookup_create_plock_ctx(struct lo_data *lo,
> }
>
> /* Open another instance of file which can be used for ofd locks. */
> - sprintf(procname, "%i", inode->fd);
> -
> /* TODO: What if file is not writable? */
> - fd = openat(lo->proc_self_fd, procname, O_RDWR);
> - if (fd == -1) {
> - *err = errno;
> + fd = lo_inode_open(lo, inode, O_RDWR);
> + if (fd < 0) {
> + *err = -fd;
> free(plock);
> return NULL;
> }
> @@ -1949,7 +1985,7 @@ static void lo_open(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
> return;
> }
>
> - err = lo_do_open(lo, inode, fi);
> + err = lo_do_open(lo, inode, -1, fi);
> lo_inode_put(lo, &inode);
> if (err) {
> fuse_reply_err(req, err);
> @@ -2014,39 +2050,40 @@ static void lo_flush(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
> static void lo_fsync(fuse_req_t req, fuse_ino_t ino, int datasync,
> struct fuse_file_info *fi)
> {
> + struct lo_inode *inode = lo_inode(req, ino);
> + struct lo_data *lo = lo_data(req);
> int res;
> int fd;
> - char *buf;
>
> fuse_log(FUSE_LOG_DEBUG, "lo_fsync(ino=%" PRIu64 ", fi=0x%p)\n", ino,
> (void *)fi);
>
> + if (!inode) {
> + fuse_reply_err(req, EBADF);
> + return;
> + }
> +
> if (!fi) {
> - struct lo_data *lo = lo_data(req);
> -
> - res = asprintf(&buf, "%i", lo_fd(req, ino));
> - if (res == -1) {
> - return (void)fuse_reply_err(req, errno);
> - }
> -
> - fd = openat(lo->proc_self_fd, buf, O_RDWR);
> - free(buf);
> - if (fd == -1) {
> - return (void)fuse_reply_err(req, errno);
> + fd = lo_inode_open(lo, inode, O_RDWR);
> + if (fd < 0) {
> + res = -fd;
> + goto out;
> }
> } else {
> fd = lo_fi_fd(req, fi);
> }
>
> if (datasync) {
> - res = fdatasync(fd);
> + res = fdatasync(fd) == -1 ? errno : 0;
> } else {
> - res = fsync(fd);
> + res = fsync(fd) == -1 ? errno : 0;
> }
> if (!fi) {
> close(fd);
> }
> - fuse_reply_err(req, res == -1 ? errno : 0);
> +out:
> + lo_inode_put(lo, &inode);
> + fuse_reply_err(req, res);
> }
>
> static void lo_read(fuse_req_t req, fuse_ino_t ino, size_t size, off_t offset,
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v4 3/3] virtiofsd: prevent opening of special files (CVE-2020-35517)
2021-02-03 15:28 ` [Virtio-fs] " Vivek Goyal
@ 2021-02-03 16:02 ` Greg Kurz
-1 siblings, 0 replies; 48+ messages in thread
From: Greg Kurz @ 2021-02-03 16:02 UTC (permalink / raw)
To: Vivek Goyal
Cc: mszeredi, Daniel Berrange, slp, qemu-devel,
Dr. David Alan Gilbert, virtio-fs, Alex Xu, Stefan Hajnoczi,
P J P, Laszlo Ersek
On Wed, 3 Feb 2021 10:28:50 -0500
Vivek Goyal <vgoyal@redhat.com> wrote:
> On Wed, Feb 03, 2021 at 11:37:19AM +0000, Stefan Hajnoczi wrote:
>
> [..]
> > @@ -1727,36 +1764,38 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
> >
> > update_open_flags(lo->writeback, lo->allow_direct_io, fi);
> >
> > - fd = openat(parent_inode->fd, name, (fi->flags | O_CREAT) & ~O_NOFOLLOW,
> > - mode);
> > + /* Try to create a new file but don't open existing files */
> > + fd = openat(parent_inode->fd, name, fi->flags | O_CREAT | O_EXCL, mode);
> > err = fd == -1 ? errno : 0;
> > +
> > lo_restore_cred(&old);
> >
> > - if (!err) {
> > - ssize_t fh;
> > -
> > - pthread_mutex_lock(&lo->mutex);
> > - fh = lo_add_fd_mapping(lo, fd);
> > - pthread_mutex_unlock(&lo->mutex);
> > - if (fh == -1) {
> > - close(fd);
> > - err = ENOMEM;
> > - goto out;
> > - }
> > + /* Ignore the error if file exists and O_EXCL was not given */
> > + if (err && !(err == EEXIST && !(fi->flags & O_EXCL))) {
>
> Can this check be simplified to.
> if (err && (err == EEXIST && (fi->flags & O_EXCL)) {
I guess you meant :
if (err && (err != EEXIST || fi->flags & O_EXCL) {
> goto out;
> }
> > + goto out;
> > + }
>
>
> Vivek
>
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [Virtio-fs] [PATCH v4 3/3] virtiofsd: prevent opening of special files (CVE-2020-35517)
@ 2021-02-03 16:02 ` Greg Kurz
0 siblings, 0 replies; 48+ messages in thread
From: Greg Kurz @ 2021-02-03 16:02 UTC (permalink / raw)
To: Vivek Goyal
Cc: Daniel Berrange, qemu-devel, virtio-fs, Alex Xu, P J P, Laszlo Ersek
On Wed, 3 Feb 2021 10:28:50 -0500
Vivek Goyal <vgoyal@redhat.com> wrote:
> On Wed, Feb 03, 2021 at 11:37:19AM +0000, Stefan Hajnoczi wrote:
>
> [..]
> > @@ -1727,36 +1764,38 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
> >
> > update_open_flags(lo->writeback, lo->allow_direct_io, fi);
> >
> > - fd = openat(parent_inode->fd, name, (fi->flags | O_CREAT) & ~O_NOFOLLOW,
> > - mode);
> > + /* Try to create a new file but don't open existing files */
> > + fd = openat(parent_inode->fd, name, fi->flags | O_CREAT | O_EXCL, mode);
> > err = fd == -1 ? errno : 0;
> > +
> > lo_restore_cred(&old);
> >
> > - if (!err) {
> > - ssize_t fh;
> > -
> > - pthread_mutex_lock(&lo->mutex);
> > - fh = lo_add_fd_mapping(lo, fd);
> > - pthread_mutex_unlock(&lo->mutex);
> > - if (fh == -1) {
> > - close(fd);
> > - err = ENOMEM;
> > - goto out;
> > - }
> > + /* Ignore the error if file exists and O_EXCL was not given */
> > + if (err && !(err == EEXIST && !(fi->flags & O_EXCL))) {
>
> Can this check be simplified to.
> if (err && (err == EEXIST && (fi->flags & O_EXCL)) {
I guess you meant :
if (err && (err != EEXIST || fi->flags & O_EXCL) {
> goto out;
> }
> > + goto out;
> > + }
>
>
> Vivek
>
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v4 3/3] virtiofsd: prevent opening of special files (CVE-2020-35517)
2021-02-03 16:02 ` [Virtio-fs] " Greg Kurz
@ 2021-02-03 16:08 ` Vivek Goyal
-1 siblings, 0 replies; 48+ messages in thread
From: Vivek Goyal @ 2021-02-03 16:08 UTC (permalink / raw)
To: Greg Kurz
Cc: mszeredi, Daniel Berrange, slp, qemu-devel,
Dr. David Alan Gilbert, virtio-fs, Alex Xu, Stefan Hajnoczi,
P J P, Laszlo Ersek
On Wed, Feb 03, 2021 at 05:02:37PM +0100, Greg Kurz wrote:
> On Wed, 3 Feb 2021 10:28:50 -0500
> Vivek Goyal <vgoyal@redhat.com> wrote:
>
> > On Wed, Feb 03, 2021 at 11:37:19AM +0000, Stefan Hajnoczi wrote:
> >
> > [..]
> > > @@ -1727,36 +1764,38 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
> > >
> > > update_open_flags(lo->writeback, lo->allow_direct_io, fi);
> > >
> > > - fd = openat(parent_inode->fd, name, (fi->flags | O_CREAT) & ~O_NOFOLLOW,
> > > - mode);
> > > + /* Try to create a new file but don't open existing files */
> > > + fd = openat(parent_inode->fd, name, fi->flags | O_CREAT | O_EXCL, mode);
> > > err = fd == -1 ? errno : 0;
> > > +
> > > lo_restore_cred(&old);
> > >
> > > - if (!err) {
> > > - ssize_t fh;
> > > -
> > > - pthread_mutex_lock(&lo->mutex);
> > > - fh = lo_add_fd_mapping(lo, fd);
> > > - pthread_mutex_unlock(&lo->mutex);
> > > - if (fh == -1) {
> > > - close(fd);
> > > - err = ENOMEM;
> > > - goto out;
> > > - }
> > > + /* Ignore the error if file exists and O_EXCL was not given */
> > > + if (err && !(err == EEXIST && !(fi->flags & O_EXCL))) {
> >
> > Can this check be simplified to.
> > if (err && (err == EEXIST && (fi->flags & O_EXCL)) {
>
> I guess you meant :
>
> if (err && (err != EEXIST || fi->flags & O_EXCL) {
This sounds correct. I forgot to take into account that if error is
not -EEXIST, we still want to bail out irrespective of O_EXCL.
Vivek
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [Virtio-fs] [PATCH v4 3/3] virtiofsd: prevent opening of special files (CVE-2020-35517)
@ 2021-02-03 16:08 ` Vivek Goyal
0 siblings, 0 replies; 48+ messages in thread
From: Vivek Goyal @ 2021-02-03 16:08 UTC (permalink / raw)
To: Greg Kurz
Cc: Daniel Berrange, qemu-devel, virtio-fs, Alex Xu, P J P, Laszlo Ersek
On Wed, Feb 03, 2021 at 05:02:37PM +0100, Greg Kurz wrote:
> On Wed, 3 Feb 2021 10:28:50 -0500
> Vivek Goyal <vgoyal@redhat.com> wrote:
>
> > On Wed, Feb 03, 2021 at 11:37:19AM +0000, Stefan Hajnoczi wrote:
> >
> > [..]
> > > @@ -1727,36 +1764,38 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
> > >
> > > update_open_flags(lo->writeback, lo->allow_direct_io, fi);
> > >
> > > - fd = openat(parent_inode->fd, name, (fi->flags | O_CREAT) & ~O_NOFOLLOW,
> > > - mode);
> > > + /* Try to create a new file but don't open existing files */
> > > + fd = openat(parent_inode->fd, name, fi->flags | O_CREAT | O_EXCL, mode);
> > > err = fd == -1 ? errno : 0;
> > > +
> > > lo_restore_cred(&old);
> > >
> > > - if (!err) {
> > > - ssize_t fh;
> > > -
> > > - pthread_mutex_lock(&lo->mutex);
> > > - fh = lo_add_fd_mapping(lo, fd);
> > > - pthread_mutex_unlock(&lo->mutex);
> > > - if (fh == -1) {
> > > - close(fd);
> > > - err = ENOMEM;
> > > - goto out;
> > > - }
> > > + /* Ignore the error if file exists and O_EXCL was not given */
> > > + if (err && !(err == EEXIST && !(fi->flags & O_EXCL))) {
> >
> > Can this check be simplified to.
> > if (err && (err == EEXIST && (fi->flags & O_EXCL)) {
>
> I guess you meant :
>
> if (err && (err != EEXIST || fi->flags & O_EXCL) {
This sounds correct. I forgot to take into account that if error is
not -EEXIST, we still want to bail out irrespective of O_EXCL.
Vivek
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v4 1/3] virtiofsd: extract lo_do_open() from lo_open()
2021-02-03 14:47 ` [Virtio-fs] " Dr. David Alan Gilbert
@ 2021-02-03 16:57 ` Stefan Hajnoczi
-1 siblings, 0 replies; 48+ messages in thread
From: Stefan Hajnoczi @ 2021-02-03 16:57 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: mszeredi, Daniel Berrange, slp, Greg Kurz, qemu-devel, virtio-fs,
P J P, Laszlo Ersek, vgoyal
[-- Attachment #1: Type: text/plain, Size: 907 bytes --]
On Wed, Feb 03, 2021 at 02:47:30PM +0000, Dr. David Alan Gilbert wrote:
> * Greg Kurz (groug@kaod.org) wrote:
> > On Wed, 3 Feb 2021 11:37:17 +0000
> > Stefan Hajnoczi <stefanha@redhat.com> wrote:
> >
> > > Both lo_open() and lo_create() have similar code to open a file. Extract
> > > a common lo_do_open() function from lo_open() that will be used by
> > > lo_create() in a later commit.
> > >
> > > Since lo_do_open() does not otherwise need fuse_req_t req, convert
> > > lo_add_fd_mapping() to use struct lo_data *lo instead.
> > >
> > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > ---
> >
> > With the s/ENOMEM/-ENOMEM/ change in lo_do_open() suggested by patchew,
>
> Isn't it actually the return -errno that's different from the original?
Yes. It should be return errno since lo_open() expected err = 0
(success) or a positive errno.
Will fix.
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [Virtio-fs] [PATCH v4 1/3] virtiofsd: extract lo_do_open() from lo_open()
@ 2021-02-03 16:57 ` Stefan Hajnoczi
0 siblings, 0 replies; 48+ messages in thread
From: Stefan Hajnoczi @ 2021-02-03 16:57 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: Daniel Berrange, qemu-devel, virtio-fs, P J P, Laszlo Ersek, vgoyal
[-- Attachment #1: Type: text/plain, Size: 907 bytes --]
On Wed, Feb 03, 2021 at 02:47:30PM +0000, Dr. David Alan Gilbert wrote:
> * Greg Kurz (groug@kaod.org) wrote:
> > On Wed, 3 Feb 2021 11:37:17 +0000
> > Stefan Hajnoczi <stefanha@redhat.com> wrote:
> >
> > > Both lo_open() and lo_create() have similar code to open a file. Extract
> > > a common lo_do_open() function from lo_open() that will be used by
> > > lo_create() in a later commit.
> > >
> > > Since lo_do_open() does not otherwise need fuse_req_t req, convert
> > > lo_add_fd_mapping() to use struct lo_data *lo instead.
> > >
> > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > ---
> >
> > With the s/ENOMEM/-ENOMEM/ change in lo_do_open() suggested by patchew,
>
> Isn't it actually the return -errno that's different from the original?
Yes. It should be return errno since lo_open() expected err = 0
(success) or a positive errno.
Will fix.
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v4 2/3] virtiofsd: optionally return inode pointer from lo_do_lookup()
2021-02-03 14:20 ` [Virtio-fs] " Greg Kurz
@ 2021-02-03 17:00 ` Stefan Hajnoczi
-1 siblings, 0 replies; 48+ messages in thread
From: Stefan Hajnoczi @ 2021-02-03 17:00 UTC (permalink / raw)
To: Greg Kurz
Cc: mszeredi, Daniel Berrange, slp, qemu-devel,
Dr. David Alan Gilbert, virtio-fs, P J P, Laszlo Ersek, vgoyal
[-- Attachment #1: Type: text/plain, Size: 2577 bytes --]
On Wed, Feb 03, 2021 at 03:20:14PM +0100, Greg Kurz wrote:
> On Wed, 3 Feb 2021 11:37:18 +0000
> Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> > lo_do_lookup() finds an existing inode or allocates a new one. It
> > increments nlookup so that the inode stays alive until the client
> > releases it.
> >
> > Existing callers don't need the struct lo_inode so the function doesn't
> > return it. Extend the function to optionally return the inode. The next
> > commit will need it.
> >
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> > tools/virtiofsd/passthrough_ll.c | 29 +++++++++++++++++++++--------
> > 1 file changed, 21 insertions(+), 8 deletions(-)
> >
> > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> > index e63cbd3fb7..c87a1f3d72 100644
> > --- a/tools/virtiofsd/passthrough_ll.c
> > +++ b/tools/virtiofsd/passthrough_ll.c
> > @@ -831,11 +831,13 @@ static int do_statx(struct lo_data *lo, int dirfd, const char *pathname,
> > }
> >
> > /*
> > - * Increments nlookup and caller must release refcount using
> > - * lo_inode_put(&parent).
> > + * Increments nlookup on the inode on success. unref_inode_lolocked() must be
> > + * called eventually to decrement nlookup again. If inodep is non-NULL, the
> > + * inode pointer is stored and the caller must call lo_inode_put().
> > */
> > static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
> > - struct fuse_entry_param *e)
> > + struct fuse_entry_param *e,
> > + struct lo_inode **inodep)
> > {
> > int newfd;
> > int res;
> > @@ -845,6 +847,10 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
> > struct lo_inode *inode = NULL;
> > struct lo_inode *dir = lo_inode(req, parent);
> >
> > + if (inodep) {
> > + *inodep = NULL;
> > + }
> > +
>
> Is this side-effect needed ? If lo_do_lookup() returns an error, it
> rather seems that the caller shouldn't expect anything to be written
> here, i.e. the content of *inodep still belongs to the caller and
> whatever value it previously put in there (as patch 3/3 does) should
> be preserved IMHO.
>
> Apart from that LGTM.
I like this approach because it prevents accessing uninitialized memory
in the caller:
struct lo_inode *inode;
if (lo_do_lookup(..., &inodep) != 0) {
goto err;
}
...
err:
lo_inode_put(&inode); <-- uninitialized in the error case!
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [Virtio-fs] [PATCH v4 2/3] virtiofsd: optionally return inode pointer from lo_do_lookup()
@ 2021-02-03 17:00 ` Stefan Hajnoczi
0 siblings, 0 replies; 48+ messages in thread
From: Stefan Hajnoczi @ 2021-02-03 17:00 UTC (permalink / raw)
To: Greg Kurz
Cc: Daniel Berrange, qemu-devel, virtio-fs, P J P, Laszlo Ersek, vgoyal
[-- Attachment #1: Type: text/plain, Size: 2577 bytes --]
On Wed, Feb 03, 2021 at 03:20:14PM +0100, Greg Kurz wrote:
> On Wed, 3 Feb 2021 11:37:18 +0000
> Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> > lo_do_lookup() finds an existing inode or allocates a new one. It
> > increments nlookup so that the inode stays alive until the client
> > releases it.
> >
> > Existing callers don't need the struct lo_inode so the function doesn't
> > return it. Extend the function to optionally return the inode. The next
> > commit will need it.
> >
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> > tools/virtiofsd/passthrough_ll.c | 29 +++++++++++++++++++++--------
> > 1 file changed, 21 insertions(+), 8 deletions(-)
> >
> > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> > index e63cbd3fb7..c87a1f3d72 100644
> > --- a/tools/virtiofsd/passthrough_ll.c
> > +++ b/tools/virtiofsd/passthrough_ll.c
> > @@ -831,11 +831,13 @@ static int do_statx(struct lo_data *lo, int dirfd, const char *pathname,
> > }
> >
> > /*
> > - * Increments nlookup and caller must release refcount using
> > - * lo_inode_put(&parent).
> > + * Increments nlookup on the inode on success. unref_inode_lolocked() must be
> > + * called eventually to decrement nlookup again. If inodep is non-NULL, the
> > + * inode pointer is stored and the caller must call lo_inode_put().
> > */
> > static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
> > - struct fuse_entry_param *e)
> > + struct fuse_entry_param *e,
> > + struct lo_inode **inodep)
> > {
> > int newfd;
> > int res;
> > @@ -845,6 +847,10 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
> > struct lo_inode *inode = NULL;
> > struct lo_inode *dir = lo_inode(req, parent);
> >
> > + if (inodep) {
> > + *inodep = NULL;
> > + }
> > +
>
> Is this side-effect needed ? If lo_do_lookup() returns an error, it
> rather seems that the caller shouldn't expect anything to be written
> here, i.e. the content of *inodep still belongs to the caller and
> whatever value it previously put in there (as patch 3/3 does) should
> be preserved IMHO.
>
> Apart from that LGTM.
I like this approach because it prevents accessing uninitialized memory
in the caller:
struct lo_inode *inode;
if (lo_do_lookup(..., &inodep) != 0) {
goto err;
}
...
err:
lo_inode_put(&inode); <-- uninitialized in the error case!
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v4 3/3] virtiofsd: prevent opening of special files (CVE-2020-35517)
2021-02-03 16:08 ` [Virtio-fs] " Vivek Goyal
@ 2021-02-03 17:05 ` Stefan Hajnoczi
-1 siblings, 0 replies; 48+ messages in thread
From: Stefan Hajnoczi @ 2021-02-03 17:05 UTC (permalink / raw)
To: Vivek Goyal
Cc: mszeredi, Daniel Berrange, slp, qemu-devel, Greg Kurz,
Dr. David Alan Gilbert, virtio-fs, Alex Xu, P J P, Laszlo Ersek
[-- Attachment #1: Type: text/plain, Size: 1956 bytes --]
On Wed, Feb 03, 2021 at 11:08:58AM -0500, Vivek Goyal wrote:
> On Wed, Feb 03, 2021 at 05:02:37PM +0100, Greg Kurz wrote:
> > On Wed, 3 Feb 2021 10:28:50 -0500
> > Vivek Goyal <vgoyal@redhat.com> wrote:
> >
> > > On Wed, Feb 03, 2021 at 11:37:19AM +0000, Stefan Hajnoczi wrote:
> > >
> > > [..]
> > > > @@ -1727,36 +1764,38 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
> > > >
> > > > update_open_flags(lo->writeback, lo->allow_direct_io, fi);
> > > >
> > > > - fd = openat(parent_inode->fd, name, (fi->flags | O_CREAT) & ~O_NOFOLLOW,
> > > > - mode);
> > > > + /* Try to create a new file but don't open existing files */
> > > > + fd = openat(parent_inode->fd, name, fi->flags | O_CREAT | O_EXCL, mode);
> > > > err = fd == -1 ? errno : 0;
> > > > +
> > > > lo_restore_cred(&old);
> > > >
> > > > - if (!err) {
> > > > - ssize_t fh;
> > > > -
> > > > - pthread_mutex_lock(&lo->mutex);
> > > > - fh = lo_add_fd_mapping(lo, fd);
> > > > - pthread_mutex_unlock(&lo->mutex);
> > > > - if (fh == -1) {
> > > > - close(fd);
> > > > - err = ENOMEM;
> > > > - goto out;
> > > > - }
> > > > + /* Ignore the error if file exists and O_EXCL was not given */
> > > > + if (err && !(err == EEXIST && !(fi->flags & O_EXCL))) {
> > >
> > > Can this check be simplified to.
> > > if (err && (err == EEXIST && (fi->flags & O_EXCL)) {
> >
> > I guess you meant :
> >
> > if (err && (err != EEXIST || fi->flags & O_EXCL) {
>
> This sounds correct. I forgot to take into account that if error is
> not -EEXIST, we still want to bail out irrespective of O_EXCL.
I thought about De Morgan's law too but found the OR expression is not
easier to read than the AND expression :(. If you prefer it written this
way I can change it though.
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [Virtio-fs] [PATCH v4 3/3] virtiofsd: prevent opening of special files (CVE-2020-35517)
@ 2021-02-03 17:05 ` Stefan Hajnoczi
0 siblings, 0 replies; 48+ messages in thread
From: Stefan Hajnoczi @ 2021-02-03 17:05 UTC (permalink / raw)
To: Vivek Goyal
Cc: Daniel Berrange, qemu-devel, virtio-fs, Alex Xu, P J P, Laszlo Ersek
[-- Attachment #1: Type: text/plain, Size: 1956 bytes --]
On Wed, Feb 03, 2021 at 11:08:58AM -0500, Vivek Goyal wrote:
> On Wed, Feb 03, 2021 at 05:02:37PM +0100, Greg Kurz wrote:
> > On Wed, 3 Feb 2021 10:28:50 -0500
> > Vivek Goyal <vgoyal@redhat.com> wrote:
> >
> > > On Wed, Feb 03, 2021 at 11:37:19AM +0000, Stefan Hajnoczi wrote:
> > >
> > > [..]
> > > > @@ -1727,36 +1764,38 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
> > > >
> > > > update_open_flags(lo->writeback, lo->allow_direct_io, fi);
> > > >
> > > > - fd = openat(parent_inode->fd, name, (fi->flags | O_CREAT) & ~O_NOFOLLOW,
> > > > - mode);
> > > > + /* Try to create a new file but don't open existing files */
> > > > + fd = openat(parent_inode->fd, name, fi->flags | O_CREAT | O_EXCL, mode);
> > > > err = fd == -1 ? errno : 0;
> > > > +
> > > > lo_restore_cred(&old);
> > > >
> > > > - if (!err) {
> > > > - ssize_t fh;
> > > > -
> > > > - pthread_mutex_lock(&lo->mutex);
> > > > - fh = lo_add_fd_mapping(lo, fd);
> > > > - pthread_mutex_unlock(&lo->mutex);
> > > > - if (fh == -1) {
> > > > - close(fd);
> > > > - err = ENOMEM;
> > > > - goto out;
> > > > - }
> > > > + /* Ignore the error if file exists and O_EXCL was not given */
> > > > + if (err && !(err == EEXIST && !(fi->flags & O_EXCL))) {
> > >
> > > Can this check be simplified to.
> > > if (err && (err == EEXIST && (fi->flags & O_EXCL)) {
> >
> > I guess you meant :
> >
> > if (err && (err != EEXIST || fi->flags & O_EXCL) {
>
> This sounds correct. I forgot to take into account that if error is
> not -EEXIST, we still want to bail out irrespective of O_EXCL.
I thought about De Morgan's law too but found the OR expression is not
easier to read than the AND expression :(. If you prefer it written this
way I can change it though.
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v4 3/3] virtiofsd: prevent opening of special files (CVE-2020-35517)
2021-02-03 15:57 ` [Virtio-fs] " Greg Kurz
@ 2021-02-03 17:06 ` Stefan Hajnoczi
-1 siblings, 0 replies; 48+ messages in thread
From: Stefan Hajnoczi @ 2021-02-03 17:06 UTC (permalink / raw)
To: Greg Kurz
Cc: mszeredi, Daniel Berrange, slp, qemu-devel,
Dr. David Alan Gilbert, virtio-fs, Alex Xu, P J P, Laszlo Ersek,
vgoyal
[-- Attachment #1: Type: text/plain, Size: 1077 bytes --]
On Wed, Feb 03, 2021 at 04:57:59PM +0100, Greg Kurz wrote:
> On Wed, 3 Feb 2021 11:37:19 +0000
> Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > static int lo_do_open(struct lo_data *lo, struct lo_inode *inode,
> > - struct fuse_file_info *fi)
> > + int existing_fd, struct fuse_file_info *fi)
> > {
> > - char buf[64];
> > ssize_t fh;
> > - int fd;
> > + int fd = existing_fd;
> >
> > update_open_flags(lo->writeback, lo->allow_direct_io, fi);
> >
> > - sprintf(buf, "%i", inode->fd);
> > - fd = openat(lo->proc_self_fd, buf, fi->flags & ~O_NOFOLLOW);
> > - if (fd == -1) {
> > - return -errno;
> > + if (fd < 0) {
> > + fd = lo_inode_open(lo, inode, fi->flags);
> > + if (fd == -1) {
> > + return -errno;
> > + }
>
> lo_inode_open() returns a negative errno already so
> this should be converted to:
>
> if (fd < 0) {
> return fd;
> }
>
> Apart from that LGTM.
Thanks, will fix.
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [Virtio-fs] [PATCH v4 3/3] virtiofsd: prevent opening of special files (CVE-2020-35517)
@ 2021-02-03 17:06 ` Stefan Hajnoczi
0 siblings, 0 replies; 48+ messages in thread
From: Stefan Hajnoczi @ 2021-02-03 17:06 UTC (permalink / raw)
To: Greg Kurz
Cc: Daniel Berrange, qemu-devel, virtio-fs, Alex Xu, P J P,
Laszlo Ersek, vgoyal
[-- Attachment #1: Type: text/plain, Size: 1077 bytes --]
On Wed, Feb 03, 2021 at 04:57:59PM +0100, Greg Kurz wrote:
> On Wed, 3 Feb 2021 11:37:19 +0000
> Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > static int lo_do_open(struct lo_data *lo, struct lo_inode *inode,
> > - struct fuse_file_info *fi)
> > + int existing_fd, struct fuse_file_info *fi)
> > {
> > - char buf[64];
> > ssize_t fh;
> > - int fd;
> > + int fd = existing_fd;
> >
> > update_open_flags(lo->writeback, lo->allow_direct_io, fi);
> >
> > - sprintf(buf, "%i", inode->fd);
> > - fd = openat(lo->proc_self_fd, buf, fi->flags & ~O_NOFOLLOW);
> > - if (fd == -1) {
> > - return -errno;
> > + if (fd < 0) {
> > + fd = lo_inode_open(lo, inode, fi->flags);
> > + if (fd == -1) {
> > + return -errno;
> > + }
>
> lo_inode_open() returns a negative errno already so
> this should be converted to:
>
> if (fd < 0) {
> return fd;
> }
>
> Apart from that LGTM.
Thanks, will fix.
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v4 1/3] virtiofsd: extract lo_do_open() from lo_open()
2021-02-03 15:45 ` [Virtio-fs] " Greg Kurz
@ 2021-02-03 17:47 ` Greg Kurz
-1 siblings, 0 replies; 48+ messages in thread
From: Greg Kurz @ 2021-02-03 17:47 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: mszeredi, Daniel Berrange, slp, qemu-devel, P J P, virtio-fs,
Stefan Hajnoczi, Laszlo Ersek, vgoyal
On Wed, 3 Feb 2021 16:45:10 +0100
Greg Kurz <groug@kaod.org> wrote:
> On Wed, 3 Feb 2021 14:47:30 +0000
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
>
> > * Greg Kurz (groug@kaod.org) wrote:
> > > On Wed, 3 Feb 2021 11:37:17 +0000
> > > Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > >
> > > > Both lo_open() and lo_create() have similar code to open a file. Extract
> > > > a common lo_do_open() function from lo_open() that will be used by
> > > > lo_create() in a later commit.
> > > >
> > > > Since lo_do_open() does not otherwise need fuse_req_t req, convert
> > > > lo_add_fd_mapping() to use struct lo_data *lo instead.
> > > >
> > > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > > ---
> > >
> > > With the s/ENOMEM/-ENOMEM/ change in lo_do_open() suggested by patchew,
> >
> > Isn't it actually the return -errno that's different from the original?
> >
>
> Yes and this is expected. The original sits in the top level handler
> lo_open() which sends the reply back to the client and doesn't have
> a return value, while lo_do_open() has a return value which should be
> 0 or negative.
>
I got it backwards of course... sorry for the noise :)
Cheers,
--
Greg
> > Dave
> >
> > > Reviewed-by: Greg Kurz <groug@kaod.org>
> > >
> > > > tools/virtiofsd/passthrough_ll.c | 73 ++++++++++++++++++++------------
> > > > 1 file changed, 46 insertions(+), 27 deletions(-)
> > > >
> > > > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> > > > index 5fb36d9407..e63cbd3fb7 100644
> > > > --- a/tools/virtiofsd/passthrough_ll.c
> > > > +++ b/tools/virtiofsd/passthrough_ll.c
> > > > @@ -459,17 +459,17 @@ static void lo_map_remove(struct lo_map *map, size_t key)
> > > > }
> > > >
> > > > /* Assumes lo->mutex is held */
> > > > -static ssize_t lo_add_fd_mapping(fuse_req_t req, int fd)
> > > > +static ssize_t lo_add_fd_mapping(struct lo_data *lo, int fd)
> > > > {
> > > > struct lo_map_elem *elem;
> > > >
> > > > - elem = lo_map_alloc_elem(&lo_data(req)->fd_map);
> > > > + elem = lo_map_alloc_elem(&lo->fd_map);
> > > > if (!elem) {
> > > > return -1;
> > > > }
> > > >
> > > > elem->fd = fd;
> > > > - return elem - lo_data(req)->fd_map.elems;
> > > > + return elem - lo->fd_map.elems;
> > > > }
> > > >
> > > > /* Assumes lo->mutex is held */
> > > > @@ -1651,6 +1651,38 @@ static void update_open_flags(int writeback, int allow_direct_io,
> > > > }
> > > > }
> > > >
> > > > +static int lo_do_open(struct lo_data *lo, struct lo_inode *inode,
> > > > + struct fuse_file_info *fi)
> > > > +{
> > > > + char buf[64];
> > > > + ssize_t fh;
> > > > + int fd;
> > > > +
> > > > + update_open_flags(lo->writeback, lo->allow_direct_io, fi);
> > > > +
> > > > + sprintf(buf, "%i", inode->fd);
> > > > + fd = openat(lo->proc_self_fd, buf, fi->flags & ~O_NOFOLLOW);
> > > > + if (fd == -1) {
> > > > + return -errno;
> > > > + }
> > > > +
> > > > + pthread_mutex_lock(&lo->mutex);
> > > > + fh = lo_add_fd_mapping(lo, fd);
> > > > + pthread_mutex_unlock(&lo->mutex);
> > > > + if (fh == -1) {
> > > > + close(fd);
> > > > + return ENOMEM;
> > > > + }
> > > > +
> > > > + fi->fh = fh;
> > > > + if (lo->cache == CACHE_NONE) {
> > > > + fi->direct_io = 1;
> > > > + } else if (lo->cache == CACHE_ALWAYS) {
> > > > + fi->keep_cache = 1;
> > > > + }
> > > > + return 0;
> > > > +}
> > > > +
> > > > static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
> > > > mode_t mode, struct fuse_file_info *fi)
> > > > {
> > > > @@ -1691,7 +1723,7 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
> > > > ssize_t fh;
> > > >
> > > > pthread_mutex_lock(&lo->mutex);
> > > > - fh = lo_add_fd_mapping(req, fd);
> > > > + fh = lo_add_fd_mapping(lo, fd);
> > > > pthread_mutex_unlock(&lo->mutex);
> > > > if (fh == -1) {
> > > > close(fd);
> > > > @@ -1892,38 +1924,25 @@ static void lo_fsyncdir(fuse_req_t req, fuse_ino_t ino, int datasync,
> > > >
> > > > static void lo_open(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
> > > > {
> > > > - int fd;
> > > > - ssize_t fh;
> > > > - char buf[64];
> > > > struct lo_data *lo = lo_data(req);
> > > > + struct lo_inode *inode = lo_inode(req, ino);
> > > > + int err;
> > > >
> > > > fuse_log(FUSE_LOG_DEBUG, "lo_open(ino=%" PRIu64 ", flags=%d)\n", ino,
> > > > fi->flags);
> > > >
> > > > - update_open_flags(lo->writeback, lo->allow_direct_io, fi);
> > > > -
> > > > - sprintf(buf, "%i", lo_fd(req, ino));
> > > > - fd = openat(lo->proc_self_fd, buf, fi->flags & ~O_NOFOLLOW);
> > > > - if (fd == -1) {
> > > > - return (void)fuse_reply_err(req, errno);
> > > > - }
> > > > -
> > > > - pthread_mutex_lock(&lo->mutex);
> > > > - fh = lo_add_fd_mapping(req, fd);
> > > > - pthread_mutex_unlock(&lo->mutex);
> > > > - if (fh == -1) {
> > > > - close(fd);
> > > > - fuse_reply_err(req, ENOMEM);
> > > > + if (!inode) {
> > > > + fuse_reply_err(req, EBADF);
> > > > return;
> > > > }
> > > >
> > > > - fi->fh = fh;
> > > > - if (lo->cache == CACHE_NONE) {
> > > > - fi->direct_io = 1;
> > > > - } else if (lo->cache == CACHE_ALWAYS) {
> > > > - fi->keep_cache = 1;
> > > > + err = lo_do_open(lo, inode, fi);
> > > > + lo_inode_put(lo, &inode);
> > > > + if (err) {
> > > > + fuse_reply_err(req, err);
> > > > + } else {
> > > > + fuse_reply_open(req, fi);
> > > > }
> > > > - fuse_reply_open(req, fi);
> > > > }
> > > >
> > > > static void lo_release(fuse_req_t req, fuse_ino_t ino,
> > >
>
>
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [Virtio-fs] [PATCH v4 1/3] virtiofsd: extract lo_do_open() from lo_open()
@ 2021-02-03 17:47 ` Greg Kurz
0 siblings, 0 replies; 48+ messages in thread
From: Greg Kurz @ 2021-02-03 17:47 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: Daniel Berrange, qemu-devel, P J P, virtio-fs, Laszlo Ersek, vgoyal
On Wed, 3 Feb 2021 16:45:10 +0100
Greg Kurz <groug@kaod.org> wrote:
> On Wed, 3 Feb 2021 14:47:30 +0000
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
>
> > * Greg Kurz (groug@kaod.org) wrote:
> > > On Wed, 3 Feb 2021 11:37:17 +0000
> > > Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > >
> > > > Both lo_open() and lo_create() have similar code to open a file. Extract
> > > > a common lo_do_open() function from lo_open() that will be used by
> > > > lo_create() in a later commit.
> > > >
> > > > Since lo_do_open() does not otherwise need fuse_req_t req, convert
> > > > lo_add_fd_mapping() to use struct lo_data *lo instead.
> > > >
> > > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > > ---
> > >
> > > With the s/ENOMEM/-ENOMEM/ change in lo_do_open() suggested by patchew,
> >
> > Isn't it actually the return -errno that's different from the original?
> >
>
> Yes and this is expected. The original sits in the top level handler
> lo_open() which sends the reply back to the client and doesn't have
> a return value, while lo_do_open() has a return value which should be
> 0 or negative.
>
I got it backwards of course... sorry for the noise :)
Cheers,
--
Greg
> > Dave
> >
> > > Reviewed-by: Greg Kurz <groug@kaod.org>
> > >
> > > > tools/virtiofsd/passthrough_ll.c | 73 ++++++++++++++++++++------------
> > > > 1 file changed, 46 insertions(+), 27 deletions(-)
> > > >
> > > > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> > > > index 5fb36d9407..e63cbd3fb7 100644
> > > > --- a/tools/virtiofsd/passthrough_ll.c
> > > > +++ b/tools/virtiofsd/passthrough_ll.c
> > > > @@ -459,17 +459,17 @@ static void lo_map_remove(struct lo_map *map, size_t key)
> > > > }
> > > >
> > > > /* Assumes lo->mutex is held */
> > > > -static ssize_t lo_add_fd_mapping(fuse_req_t req, int fd)
> > > > +static ssize_t lo_add_fd_mapping(struct lo_data *lo, int fd)
> > > > {
> > > > struct lo_map_elem *elem;
> > > >
> > > > - elem = lo_map_alloc_elem(&lo_data(req)->fd_map);
> > > > + elem = lo_map_alloc_elem(&lo->fd_map);
> > > > if (!elem) {
> > > > return -1;
> > > > }
> > > >
> > > > elem->fd = fd;
> > > > - return elem - lo_data(req)->fd_map.elems;
> > > > + return elem - lo->fd_map.elems;
> > > > }
> > > >
> > > > /* Assumes lo->mutex is held */
> > > > @@ -1651,6 +1651,38 @@ static void update_open_flags(int writeback, int allow_direct_io,
> > > > }
> > > > }
> > > >
> > > > +static int lo_do_open(struct lo_data *lo, struct lo_inode *inode,
> > > > + struct fuse_file_info *fi)
> > > > +{
> > > > + char buf[64];
> > > > + ssize_t fh;
> > > > + int fd;
> > > > +
> > > > + update_open_flags(lo->writeback, lo->allow_direct_io, fi);
> > > > +
> > > > + sprintf(buf, "%i", inode->fd);
> > > > + fd = openat(lo->proc_self_fd, buf, fi->flags & ~O_NOFOLLOW);
> > > > + if (fd == -1) {
> > > > + return -errno;
> > > > + }
> > > > +
> > > > + pthread_mutex_lock(&lo->mutex);
> > > > + fh = lo_add_fd_mapping(lo, fd);
> > > > + pthread_mutex_unlock(&lo->mutex);
> > > > + if (fh == -1) {
> > > > + close(fd);
> > > > + return ENOMEM;
> > > > + }
> > > > +
> > > > + fi->fh = fh;
> > > > + if (lo->cache == CACHE_NONE) {
> > > > + fi->direct_io = 1;
> > > > + } else if (lo->cache == CACHE_ALWAYS) {
> > > > + fi->keep_cache = 1;
> > > > + }
> > > > + return 0;
> > > > +}
> > > > +
> > > > static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
> > > > mode_t mode, struct fuse_file_info *fi)
> > > > {
> > > > @@ -1691,7 +1723,7 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
> > > > ssize_t fh;
> > > >
> > > > pthread_mutex_lock(&lo->mutex);
> > > > - fh = lo_add_fd_mapping(req, fd);
> > > > + fh = lo_add_fd_mapping(lo, fd);
> > > > pthread_mutex_unlock(&lo->mutex);
> > > > if (fh == -1) {
> > > > close(fd);
> > > > @@ -1892,38 +1924,25 @@ static void lo_fsyncdir(fuse_req_t req, fuse_ino_t ino, int datasync,
> > > >
> > > > static void lo_open(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
> > > > {
> > > > - int fd;
> > > > - ssize_t fh;
> > > > - char buf[64];
> > > > struct lo_data *lo = lo_data(req);
> > > > + struct lo_inode *inode = lo_inode(req, ino);
> > > > + int err;
> > > >
> > > > fuse_log(FUSE_LOG_DEBUG, "lo_open(ino=%" PRIu64 ", flags=%d)\n", ino,
> > > > fi->flags);
> > > >
> > > > - update_open_flags(lo->writeback, lo->allow_direct_io, fi);
> > > > -
> > > > - sprintf(buf, "%i", lo_fd(req, ino));
> > > > - fd = openat(lo->proc_self_fd, buf, fi->flags & ~O_NOFOLLOW);
> > > > - if (fd == -1) {
> > > > - return (void)fuse_reply_err(req, errno);
> > > > - }
> > > > -
> > > > - pthread_mutex_lock(&lo->mutex);
> > > > - fh = lo_add_fd_mapping(req, fd);
> > > > - pthread_mutex_unlock(&lo->mutex);
> > > > - if (fh == -1) {
> > > > - close(fd);
> > > > - fuse_reply_err(req, ENOMEM);
> > > > + if (!inode) {
> > > > + fuse_reply_err(req, EBADF);
> > > > return;
> > > > }
> > > >
> > > > - fi->fh = fh;
> > > > - if (lo->cache == CACHE_NONE) {
> > > > - fi->direct_io = 1;
> > > > - } else if (lo->cache == CACHE_ALWAYS) {
> > > > - fi->keep_cache = 1;
> > > > + err = lo_do_open(lo, inode, fi);
> > > > + lo_inode_put(lo, &inode);
> > > > + if (err) {
> > > > + fuse_reply_err(req, err);
> > > > + } else {
> > > > + fuse_reply_open(req, fi);
> > > > }
> > > > - fuse_reply_open(req, fi);
> > > > }
> > > >
> > > > static void lo_release(fuse_req_t req, fuse_ino_t ino,
> > >
>
>
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v4 3/3] virtiofsd: prevent opening of special files (CVE-2020-35517)
2021-02-03 17:05 ` [Virtio-fs] " Stefan Hajnoczi
@ 2021-02-03 18:05 ` Dr. David Alan Gilbert
-1 siblings, 0 replies; 48+ messages in thread
From: Dr. David Alan Gilbert @ 2021-02-03 18:05 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: mszeredi, Daniel Berrange, slp, Greg Kurz, qemu-devel, virtio-fs,
Alex Xu, P J P, Laszlo Ersek, Vivek Goyal
* Stefan Hajnoczi (stefanha@redhat.com) wrote:
> On Wed, Feb 03, 2021 at 11:08:58AM -0500, Vivek Goyal wrote:
> > On Wed, Feb 03, 2021 at 05:02:37PM +0100, Greg Kurz wrote:
> > > On Wed, 3 Feb 2021 10:28:50 -0500
> > > Vivek Goyal <vgoyal@redhat.com> wrote:
> > >
> > > > On Wed, Feb 03, 2021 at 11:37:19AM +0000, Stefan Hajnoczi wrote:
> > > >
> > > > [..]
> > > > > @@ -1727,36 +1764,38 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
> > > > >
> > > > > update_open_flags(lo->writeback, lo->allow_direct_io, fi);
> > > > >
> > > > > - fd = openat(parent_inode->fd, name, (fi->flags | O_CREAT) & ~O_NOFOLLOW,
> > > > > - mode);
> > > > > + /* Try to create a new file but don't open existing files */
> > > > > + fd = openat(parent_inode->fd, name, fi->flags | O_CREAT | O_EXCL, mode);
> > > > > err = fd == -1 ? errno : 0;
> > > > > +
> > > > > lo_restore_cred(&old);
> > > > >
> > > > > - if (!err) {
> > > > > - ssize_t fh;
> > > > > -
> > > > > - pthread_mutex_lock(&lo->mutex);
> > > > > - fh = lo_add_fd_mapping(lo, fd);
> > > > > - pthread_mutex_unlock(&lo->mutex);
> > > > > - if (fh == -1) {
> > > > > - close(fd);
> > > > > - err = ENOMEM;
> > > > > - goto out;
> > > > > - }
> > > > > + /* Ignore the error if file exists and O_EXCL was not given */
> > > > > + if (err && !(err == EEXIST && !(fi->flags & O_EXCL))) {
> > > >
> > > > Can this check be simplified to.
> > > > if (err && (err == EEXIST && (fi->flags & O_EXCL)) {
> > >
> > > I guess you meant :
> > >
> > > if (err && (err != EEXIST || fi->flags & O_EXCL) {
> >
> > This sounds correct. I forgot to take into account that if error is
> > not -EEXIST, we still want to bail out irrespective of O_EXCL.
>
> I thought about De Morgan's law too but found the OR expression is not
> easier to read than the AND expression :(. If you prefer it written this
> way I can change it though.
I think the version that you put, matches your comment well; although
it's a bit of a weird case where nesting a pair of ! makes sense.
Dave
> Stefan
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [Virtio-fs] [PATCH v4 3/3] virtiofsd: prevent opening of special files (CVE-2020-35517)
@ 2021-02-03 18:05 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 48+ messages in thread
From: Dr. David Alan Gilbert @ 2021-02-03 18:05 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Daniel Berrange, qemu-devel, virtio-fs, Alex Xu, P J P,
Laszlo Ersek, Vivek Goyal
* Stefan Hajnoczi (stefanha@redhat.com) wrote:
> On Wed, Feb 03, 2021 at 11:08:58AM -0500, Vivek Goyal wrote:
> > On Wed, Feb 03, 2021 at 05:02:37PM +0100, Greg Kurz wrote:
> > > On Wed, 3 Feb 2021 10:28:50 -0500
> > > Vivek Goyal <vgoyal@redhat.com> wrote:
> > >
> > > > On Wed, Feb 03, 2021 at 11:37:19AM +0000, Stefan Hajnoczi wrote:
> > > >
> > > > [..]
> > > > > @@ -1727,36 +1764,38 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
> > > > >
> > > > > update_open_flags(lo->writeback, lo->allow_direct_io, fi);
> > > > >
> > > > > - fd = openat(parent_inode->fd, name, (fi->flags | O_CREAT) & ~O_NOFOLLOW,
> > > > > - mode);
> > > > > + /* Try to create a new file but don't open existing files */
> > > > > + fd = openat(parent_inode->fd, name, fi->flags | O_CREAT | O_EXCL, mode);
> > > > > err = fd == -1 ? errno : 0;
> > > > > +
> > > > > lo_restore_cred(&old);
> > > > >
> > > > > - if (!err) {
> > > > > - ssize_t fh;
> > > > > -
> > > > > - pthread_mutex_lock(&lo->mutex);
> > > > > - fh = lo_add_fd_mapping(lo, fd);
> > > > > - pthread_mutex_unlock(&lo->mutex);
> > > > > - if (fh == -1) {
> > > > > - close(fd);
> > > > > - err = ENOMEM;
> > > > > - goto out;
> > > > > - }
> > > > > + /* Ignore the error if file exists and O_EXCL was not given */
> > > > > + if (err && !(err == EEXIST && !(fi->flags & O_EXCL))) {
> > > >
> > > > Can this check be simplified to.
> > > > if (err && (err == EEXIST && (fi->flags & O_EXCL)) {
> > >
> > > I guess you meant :
> > >
> > > if (err && (err != EEXIST || fi->flags & O_EXCL) {
> >
> > This sounds correct. I forgot to take into account that if error is
> > not -EEXIST, we still want to bail out irrespective of O_EXCL.
>
> I thought about De Morgan's law too but found the OR expression is not
> easier to read than the AND expression :(. If you prefer it written this
> way I can change it though.
I think the version that you put, matches your comment well; although
it's a bit of a weird case where nesting a pair of ! makes sense.
Dave
> Stefan
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v4 3/3] virtiofsd: prevent opening of special files (CVE-2020-35517)
2021-02-03 17:05 ` [Virtio-fs] " Stefan Hajnoczi
@ 2021-02-03 21:14 ` Vivek Goyal
-1 siblings, 0 replies; 48+ messages in thread
From: Vivek Goyal @ 2021-02-03 21:14 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: mszeredi, Daniel Berrange, slp, qemu-devel, Greg Kurz,
Dr. David Alan Gilbert, virtio-fs, Alex Xu, P J P, Laszlo Ersek
On Wed, Feb 03, 2021 at 05:05:14PM +0000, Stefan Hajnoczi wrote:
> On Wed, Feb 03, 2021 at 11:08:58AM -0500, Vivek Goyal wrote:
> > On Wed, Feb 03, 2021 at 05:02:37PM +0100, Greg Kurz wrote:
> > > On Wed, 3 Feb 2021 10:28:50 -0500
> > > Vivek Goyal <vgoyal@redhat.com> wrote:
> > >
> > > > On Wed, Feb 03, 2021 at 11:37:19AM +0000, Stefan Hajnoczi wrote:
> > > >
> > > > [..]
> > > > > @@ -1727,36 +1764,38 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
> > > > >
> > > > > update_open_flags(lo->writeback, lo->allow_direct_io, fi);
> > > > >
> > > > > - fd = openat(parent_inode->fd, name, (fi->flags | O_CREAT) & ~O_NOFOLLOW,
> > > > > - mode);
> > > > > + /* Try to create a new file but don't open existing files */
> > > > > + fd = openat(parent_inode->fd, name, fi->flags | O_CREAT | O_EXCL, mode);
> > > > > err = fd == -1 ? errno : 0;
> > > > > +
> > > > > lo_restore_cred(&old);
> > > > >
> > > > > - if (!err) {
> > > > > - ssize_t fh;
> > > > > -
> > > > > - pthread_mutex_lock(&lo->mutex);
> > > > > - fh = lo_add_fd_mapping(lo, fd);
> > > > > - pthread_mutex_unlock(&lo->mutex);
> > > > > - if (fh == -1) {
> > > > > - close(fd);
> > > > > - err = ENOMEM;
> > > > > - goto out;
> > > > > - }
> > > > > + /* Ignore the error if file exists and O_EXCL was not given */
> > > > > + if (err && !(err == EEXIST && !(fi->flags & O_EXCL))) {
> > > >
> > > > Can this check be simplified to.
> > > > if (err && (err == EEXIST && (fi->flags & O_EXCL)) {
> > >
> > > I guess you meant :
> > >
> > > if (err && (err != EEXIST || fi->flags & O_EXCL) {
> >
> > This sounds correct. I forgot to take into account that if error is
> > not -EEXIST, we still want to bail out irrespective of O_EXCL.
>
> I thought about De Morgan's law too but found the OR expression is not
> easier to read than the AND expression :(. If you prefer it written this
> way I can change it though.
I personally find this one to read. And not because of AND but because
of double logical negation (!x) in previous expression.
But I am not particular about it. If you don't find it easier to
read, I can live with previous one.
Vivek
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [Virtio-fs] [PATCH v4 3/3] virtiofsd: prevent opening of special files (CVE-2020-35517)
@ 2021-02-03 21:14 ` Vivek Goyal
0 siblings, 0 replies; 48+ messages in thread
From: Vivek Goyal @ 2021-02-03 21:14 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Daniel Berrange, qemu-devel, virtio-fs, Alex Xu, P J P, Laszlo Ersek
On Wed, Feb 03, 2021 at 05:05:14PM +0000, Stefan Hajnoczi wrote:
> On Wed, Feb 03, 2021 at 11:08:58AM -0500, Vivek Goyal wrote:
> > On Wed, Feb 03, 2021 at 05:02:37PM +0100, Greg Kurz wrote:
> > > On Wed, 3 Feb 2021 10:28:50 -0500
> > > Vivek Goyal <vgoyal@redhat.com> wrote:
> > >
> > > > On Wed, Feb 03, 2021 at 11:37:19AM +0000, Stefan Hajnoczi wrote:
> > > >
> > > > [..]
> > > > > @@ -1727,36 +1764,38 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
> > > > >
> > > > > update_open_flags(lo->writeback, lo->allow_direct_io, fi);
> > > > >
> > > > > - fd = openat(parent_inode->fd, name, (fi->flags | O_CREAT) & ~O_NOFOLLOW,
> > > > > - mode);
> > > > > + /* Try to create a new file but don't open existing files */
> > > > > + fd = openat(parent_inode->fd, name, fi->flags | O_CREAT | O_EXCL, mode);
> > > > > err = fd == -1 ? errno : 0;
> > > > > +
> > > > > lo_restore_cred(&old);
> > > > >
> > > > > - if (!err) {
> > > > > - ssize_t fh;
> > > > > -
> > > > > - pthread_mutex_lock(&lo->mutex);
> > > > > - fh = lo_add_fd_mapping(lo, fd);
> > > > > - pthread_mutex_unlock(&lo->mutex);
> > > > > - if (fh == -1) {
> > > > > - close(fd);
> > > > > - err = ENOMEM;
> > > > > - goto out;
> > > > > - }
> > > > > + /* Ignore the error if file exists and O_EXCL was not given */
> > > > > + if (err && !(err == EEXIST && !(fi->flags & O_EXCL))) {
> > > >
> > > > Can this check be simplified to.
> > > > if (err && (err == EEXIST && (fi->flags & O_EXCL)) {
> > >
> > > I guess you meant :
> > >
> > > if (err && (err != EEXIST || fi->flags & O_EXCL) {
> >
> > This sounds correct. I forgot to take into account that if error is
> > not -EEXIST, we still want to bail out irrespective of O_EXCL.
>
> I thought about De Morgan's law too but found the OR expression is not
> easier to read than the AND expression :(. If you prefer it written this
> way I can change it though.
I personally find this one to read. And not because of AND but because
of double logical negation (!x) in previous expression.
But I am not particular about it. If you don't find it easier to
read, I can live with previous one.
Vivek
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v4 2/3] virtiofsd: optionally return inode pointer from lo_do_lookup()
2021-02-03 17:00 ` [Virtio-fs] " Stefan Hajnoczi
@ 2021-02-04 8:25 ` Greg Kurz
-1 siblings, 0 replies; 48+ messages in thread
From: Greg Kurz @ 2021-02-04 8:25 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: mszeredi, Daniel Berrange, slp, qemu-devel,
Dr. David Alan Gilbert, virtio-fs, P J P, Laszlo Ersek, vgoyal
[-- Attachment #1: Type: text/plain, Size: 3101 bytes --]
On Wed, 3 Feb 2021 17:00:06 +0000
Stefan Hajnoczi <stefanha@redhat.com> wrote:
> On Wed, Feb 03, 2021 at 03:20:14PM +0100, Greg Kurz wrote:
> > On Wed, 3 Feb 2021 11:37:18 +0000
> > Stefan Hajnoczi <stefanha@redhat.com> wrote:
> >
> > > lo_do_lookup() finds an existing inode or allocates a new one. It
> > > increments nlookup so that the inode stays alive until the client
> > > releases it.
> > >
> > > Existing callers don't need the struct lo_inode so the function doesn't
> > > return it. Extend the function to optionally return the inode. The next
> > > commit will need it.
> > >
> > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > ---
> > > tools/virtiofsd/passthrough_ll.c | 29 +++++++++++++++++++++--------
> > > 1 file changed, 21 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> > > index e63cbd3fb7..c87a1f3d72 100644
> > > --- a/tools/virtiofsd/passthrough_ll.c
> > > +++ b/tools/virtiofsd/passthrough_ll.c
> > > @@ -831,11 +831,13 @@ static int do_statx(struct lo_data *lo, int dirfd, const char *pathname,
> > > }
> > >
> > > /*
> > > - * Increments nlookup and caller must release refcount using
> > > - * lo_inode_put(&parent).
> > > + * Increments nlookup on the inode on success. unref_inode_lolocked() must be
> > > + * called eventually to decrement nlookup again. If inodep is non-NULL, the
> > > + * inode pointer is stored and the caller must call lo_inode_put().
> > > */
> > > static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
> > > - struct fuse_entry_param *e)
> > > + struct fuse_entry_param *e,
> > > + struct lo_inode **inodep)
> > > {
> > > int newfd;
> > > int res;
> > > @@ -845,6 +847,10 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
> > > struct lo_inode *inode = NULL;
> > > struct lo_inode *dir = lo_inode(req, parent);
> > >
> > > + if (inodep) {
> > > + *inodep = NULL;
> > > + }
> > > +
> >
> > Is this side-effect needed ? If lo_do_lookup() returns an error, it
> > rather seems that the caller shouldn't expect anything to be written
> > here, i.e. the content of *inodep still belongs to the caller and
> > whatever value it previously put in there (as patch 3/3 does) should
> > be preserved IMHO.
> >
> > Apart from that LGTM.
>
> I like this approach because it prevents accessing uninitialized memory
> in the caller:
>
> struct lo_inode *inode;
>
> if (lo_do_lookup(..., &inodep) != 0) {
> goto err;
> }
> ...
>
> err:
> lo_inode_put(&inode); <-- uninitialized in the error case!
My point is that it is the caller's business to ensure that inode
doesn't contain garbage if it is to be used irrespective of the
outcome of lo_do_lookup(). This is precisely what patch 3/3 does,
so I don't understand the ultimate purpose of nullifying the
inode pointer _again_ in lo_do_lookup()...
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [Virtio-fs] [PATCH v4 2/3] virtiofsd: optionally return inode pointer from lo_do_lookup()
@ 2021-02-04 8:25 ` Greg Kurz
0 siblings, 0 replies; 48+ messages in thread
From: Greg Kurz @ 2021-02-04 8:25 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Daniel Berrange, qemu-devel, virtio-fs, P J P, Laszlo Ersek, vgoyal
[-- Attachment #1: Type: text/plain, Size: 3101 bytes --]
On Wed, 3 Feb 2021 17:00:06 +0000
Stefan Hajnoczi <stefanha@redhat.com> wrote:
> On Wed, Feb 03, 2021 at 03:20:14PM +0100, Greg Kurz wrote:
> > On Wed, 3 Feb 2021 11:37:18 +0000
> > Stefan Hajnoczi <stefanha@redhat.com> wrote:
> >
> > > lo_do_lookup() finds an existing inode or allocates a new one. It
> > > increments nlookup so that the inode stays alive until the client
> > > releases it.
> > >
> > > Existing callers don't need the struct lo_inode so the function doesn't
> > > return it. Extend the function to optionally return the inode. The next
> > > commit will need it.
> > >
> > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > ---
> > > tools/virtiofsd/passthrough_ll.c | 29 +++++++++++++++++++++--------
> > > 1 file changed, 21 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> > > index e63cbd3fb7..c87a1f3d72 100644
> > > --- a/tools/virtiofsd/passthrough_ll.c
> > > +++ b/tools/virtiofsd/passthrough_ll.c
> > > @@ -831,11 +831,13 @@ static int do_statx(struct lo_data *lo, int dirfd, const char *pathname,
> > > }
> > >
> > > /*
> > > - * Increments nlookup and caller must release refcount using
> > > - * lo_inode_put(&parent).
> > > + * Increments nlookup on the inode on success. unref_inode_lolocked() must be
> > > + * called eventually to decrement nlookup again. If inodep is non-NULL, the
> > > + * inode pointer is stored and the caller must call lo_inode_put().
> > > */
> > > static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
> > > - struct fuse_entry_param *e)
> > > + struct fuse_entry_param *e,
> > > + struct lo_inode **inodep)
> > > {
> > > int newfd;
> > > int res;
> > > @@ -845,6 +847,10 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
> > > struct lo_inode *inode = NULL;
> > > struct lo_inode *dir = lo_inode(req, parent);
> > >
> > > + if (inodep) {
> > > + *inodep = NULL;
> > > + }
> > > +
> >
> > Is this side-effect needed ? If lo_do_lookup() returns an error, it
> > rather seems that the caller shouldn't expect anything to be written
> > here, i.e. the content of *inodep still belongs to the caller and
> > whatever value it previously put in there (as patch 3/3 does) should
> > be preserved IMHO.
> >
> > Apart from that LGTM.
>
> I like this approach because it prevents accessing uninitialized memory
> in the caller:
>
> struct lo_inode *inode;
>
> if (lo_do_lookup(..., &inodep) != 0) {
> goto err;
> }
> ...
>
> err:
> lo_inode_put(&inode); <-- uninitialized in the error case!
My point is that it is the caller's business to ensure that inode
doesn't contain garbage if it is to be used irrespective of the
outcome of lo_do_lookup(). This is precisely what patch 3/3 does,
so I don't understand the ultimate purpose of nullifying the
inode pointer _again_ in lo_do_lookup()...
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v4 2/3] virtiofsd: optionally return inode pointer from lo_do_lookup()
2021-02-04 8:25 ` [Virtio-fs] " Greg Kurz
@ 2021-02-04 9:45 ` Stefan Hajnoczi
-1 siblings, 0 replies; 48+ messages in thread
From: Stefan Hajnoczi @ 2021-02-04 9:45 UTC (permalink / raw)
To: Greg Kurz
Cc: mszeredi, Daniel Berrange, slp, qemu-devel,
Dr. David Alan Gilbert, virtio-fs, P J P, Laszlo Ersek, vgoyal
[-- Attachment #1: Type: text/plain, Size: 3846 bytes --]
On Thu, Feb 04, 2021 at 09:25:28AM +0100, Greg Kurz wrote:
> On Wed, 3 Feb 2021 17:00:06 +0000
> Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> > On Wed, Feb 03, 2021 at 03:20:14PM +0100, Greg Kurz wrote:
> > > On Wed, 3 Feb 2021 11:37:18 +0000
> > > Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > >
> > > > lo_do_lookup() finds an existing inode or allocates a new one. It
> > > > increments nlookup so that the inode stays alive until the client
> > > > releases it.
> > > >
> > > > Existing callers don't need the struct lo_inode so the function doesn't
> > > > return it. Extend the function to optionally return the inode. The next
> > > > commit will need it.
> > > >
> > > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > > ---
> > > > tools/virtiofsd/passthrough_ll.c | 29 +++++++++++++++++++++--------
> > > > 1 file changed, 21 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> > > > index e63cbd3fb7..c87a1f3d72 100644
> > > > --- a/tools/virtiofsd/passthrough_ll.c
> > > > +++ b/tools/virtiofsd/passthrough_ll.c
> > > > @@ -831,11 +831,13 @@ static int do_statx(struct lo_data *lo, int dirfd, const char *pathname,
> > > > }
> > > >
> > > > /*
> > > > - * Increments nlookup and caller must release refcount using
> > > > - * lo_inode_put(&parent).
> > > > + * Increments nlookup on the inode on success. unref_inode_lolocked() must be
> > > > + * called eventually to decrement nlookup again. If inodep is non-NULL, the
> > > > + * inode pointer is stored and the caller must call lo_inode_put().
> > > > */
> > > > static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
> > > > - struct fuse_entry_param *e)
> > > > + struct fuse_entry_param *e,
> > > > + struct lo_inode **inodep)
> > > > {
> > > > int newfd;
> > > > int res;
> > > > @@ -845,6 +847,10 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
> > > > struct lo_inode *inode = NULL;
> > > > struct lo_inode *dir = lo_inode(req, parent);
> > > >
> > > > + if (inodep) {
> > > > + *inodep = NULL;
> > > > + }
> > > > +
> > >
> > > Is this side-effect needed ? If lo_do_lookup() returns an error, it
> > > rather seems that the caller shouldn't expect anything to be written
> > > here, i.e. the content of *inodep still belongs to the caller and
> > > whatever value it previously put in there (as patch 3/3 does) should
> > > be preserved IMHO.
> > >
> > > Apart from that LGTM.
> >
> > I like this approach because it prevents accessing uninitialized memory
> > in the caller:
> >
> > struct lo_inode *inode;
> >
> > if (lo_do_lookup(..., &inodep) != 0) {
> > goto err;
> > }
> > ...
> >
> > err:
> > lo_inode_put(&inode); <-- uninitialized in the error case!
>
> My point is that it is the caller's business to ensure that inode
> doesn't contain garbage if it is to be used irrespective of the
> outcome of lo_do_lookup(). This is precisely what patch 3/3 does,
> so I don't understand the ultimate purpose of nullifying the
> inode pointer _again_ in lo_do_lookup()...
APIs should be designed to eliminate classes of errors where possible
IMO. Taking care regarding the uninitialized pointer in the error case
could be the caller's responsibility, but what's the advantage?
(There's a related thing with lo_inode_put(&inode) where it sets *inode
= NULL to eliminate use-after-free bugs in callers. It would have been
possible to use the same approach as free(3) where it's the caller's
responsiblity, but that API design decision in free(3) has caused
many bugs in applications.)
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [Virtio-fs] [PATCH v4 2/3] virtiofsd: optionally return inode pointer from lo_do_lookup()
@ 2021-02-04 9:45 ` Stefan Hajnoczi
0 siblings, 0 replies; 48+ messages in thread
From: Stefan Hajnoczi @ 2021-02-04 9:45 UTC (permalink / raw)
To: Greg Kurz
Cc: Daniel Berrange, qemu-devel, virtio-fs, P J P, Laszlo Ersek, vgoyal
[-- Attachment #1: Type: text/plain, Size: 3846 bytes --]
On Thu, Feb 04, 2021 at 09:25:28AM +0100, Greg Kurz wrote:
> On Wed, 3 Feb 2021 17:00:06 +0000
> Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> > On Wed, Feb 03, 2021 at 03:20:14PM +0100, Greg Kurz wrote:
> > > On Wed, 3 Feb 2021 11:37:18 +0000
> > > Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > >
> > > > lo_do_lookup() finds an existing inode or allocates a new one. It
> > > > increments nlookup so that the inode stays alive until the client
> > > > releases it.
> > > >
> > > > Existing callers don't need the struct lo_inode so the function doesn't
> > > > return it. Extend the function to optionally return the inode. The next
> > > > commit will need it.
> > > >
> > > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > > ---
> > > > tools/virtiofsd/passthrough_ll.c | 29 +++++++++++++++++++++--------
> > > > 1 file changed, 21 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> > > > index e63cbd3fb7..c87a1f3d72 100644
> > > > --- a/tools/virtiofsd/passthrough_ll.c
> > > > +++ b/tools/virtiofsd/passthrough_ll.c
> > > > @@ -831,11 +831,13 @@ static int do_statx(struct lo_data *lo, int dirfd, const char *pathname,
> > > > }
> > > >
> > > > /*
> > > > - * Increments nlookup and caller must release refcount using
> > > > - * lo_inode_put(&parent).
> > > > + * Increments nlookup on the inode on success. unref_inode_lolocked() must be
> > > > + * called eventually to decrement nlookup again. If inodep is non-NULL, the
> > > > + * inode pointer is stored and the caller must call lo_inode_put().
> > > > */
> > > > static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
> > > > - struct fuse_entry_param *e)
> > > > + struct fuse_entry_param *e,
> > > > + struct lo_inode **inodep)
> > > > {
> > > > int newfd;
> > > > int res;
> > > > @@ -845,6 +847,10 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
> > > > struct lo_inode *inode = NULL;
> > > > struct lo_inode *dir = lo_inode(req, parent);
> > > >
> > > > + if (inodep) {
> > > > + *inodep = NULL;
> > > > + }
> > > > +
> > >
> > > Is this side-effect needed ? If lo_do_lookup() returns an error, it
> > > rather seems that the caller shouldn't expect anything to be written
> > > here, i.e. the content of *inodep still belongs to the caller and
> > > whatever value it previously put in there (as patch 3/3 does) should
> > > be preserved IMHO.
> > >
> > > Apart from that LGTM.
> >
> > I like this approach because it prevents accessing uninitialized memory
> > in the caller:
> >
> > struct lo_inode *inode;
> >
> > if (lo_do_lookup(..., &inodep) != 0) {
> > goto err;
> > }
> > ...
> >
> > err:
> > lo_inode_put(&inode); <-- uninitialized in the error case!
>
> My point is that it is the caller's business to ensure that inode
> doesn't contain garbage if it is to be used irrespective of the
> outcome of lo_do_lookup(). This is precisely what patch 3/3 does,
> so I don't understand the ultimate purpose of nullifying the
> inode pointer _again_ in lo_do_lookup()...
APIs should be designed to eliminate classes of errors where possible
IMO. Taking care regarding the uninitialized pointer in the error case
could be the caller's responsibility, but what's the advantage?
(There's a related thing with lo_inode_put(&inode) where it sets *inode
= NULL to eliminate use-after-free bugs in callers. It would have been
possible to use the same approach as free(3) where it's the caller's
responsiblity, but that API design decision in free(3) has caused
many bugs in applications.)
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v4 3/3] virtiofsd: prevent opening of special files (CVE-2020-35517)
2021-02-03 21:14 ` [Virtio-fs] " Vivek Goyal
@ 2021-02-04 9:47 ` Stefan Hajnoczi
-1 siblings, 0 replies; 48+ messages in thread
From: Stefan Hajnoczi @ 2021-02-04 9:47 UTC (permalink / raw)
To: Vivek Goyal
Cc: mszeredi, Daniel Berrange, slp, qemu-devel, Greg Kurz,
Dr. David Alan Gilbert, virtio-fs, Alex Xu, P J P, Laszlo Ersek
[-- Attachment #1: Type: text/plain, Size: 2603 bytes --]
On Wed, Feb 03, 2021 at 04:14:41PM -0500, Vivek Goyal wrote:
> On Wed, Feb 03, 2021 at 05:05:14PM +0000, Stefan Hajnoczi wrote:
> > On Wed, Feb 03, 2021 at 11:08:58AM -0500, Vivek Goyal wrote:
> > > On Wed, Feb 03, 2021 at 05:02:37PM +0100, Greg Kurz wrote:
> > > > On Wed, 3 Feb 2021 10:28:50 -0500
> > > > Vivek Goyal <vgoyal@redhat.com> wrote:
> > > >
> > > > > On Wed, Feb 03, 2021 at 11:37:19AM +0000, Stefan Hajnoczi wrote:
> > > > >
> > > > > [..]
> > > > > > @@ -1727,36 +1764,38 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
> > > > > >
> > > > > > update_open_flags(lo->writeback, lo->allow_direct_io, fi);
> > > > > >
> > > > > > - fd = openat(parent_inode->fd, name, (fi->flags | O_CREAT) & ~O_NOFOLLOW,
> > > > > > - mode);
> > > > > > + /* Try to create a new file but don't open existing files */
> > > > > > + fd = openat(parent_inode->fd, name, fi->flags | O_CREAT | O_EXCL, mode);
> > > > > > err = fd == -1 ? errno : 0;
> > > > > > +
> > > > > > lo_restore_cred(&old);
> > > > > >
> > > > > > - if (!err) {
> > > > > > - ssize_t fh;
> > > > > > -
> > > > > > - pthread_mutex_lock(&lo->mutex);
> > > > > > - fh = lo_add_fd_mapping(lo, fd);
> > > > > > - pthread_mutex_unlock(&lo->mutex);
> > > > > > - if (fh == -1) {
> > > > > > - close(fd);
> > > > > > - err = ENOMEM;
> > > > > > - goto out;
> > > > > > - }
> > > > > > + /* Ignore the error if file exists and O_EXCL was not given */
> > > > > > + if (err && !(err == EEXIST && !(fi->flags & O_EXCL))) {
> > > > >
> > > > > Can this check be simplified to.
> > > > > if (err && (err == EEXIST && (fi->flags & O_EXCL)) {
> > > >
> > > > I guess you meant :
> > > >
> > > > if (err && (err != EEXIST || fi->flags & O_EXCL) {
> > >
> > > This sounds correct. I forgot to take into account that if error is
> > > not -EEXIST, we still want to bail out irrespective of O_EXCL.
> >
> > I thought about De Morgan's law too but found the OR expression is not
> > easier to read than the AND expression :(. If you prefer it written this
> > way I can change it though.
>
> I personally find this one to read. And not because of AND but because
> of double logical negation (!x) in previous expression.
>
> But I am not particular about it. If you don't find it easier to
> read, I can live with previous one.
Okay, I'll convert it. Dave also mentioned he finds the AND version
strange.
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [Virtio-fs] [PATCH v4 3/3] virtiofsd: prevent opening of special files (CVE-2020-35517)
@ 2021-02-04 9:47 ` Stefan Hajnoczi
0 siblings, 0 replies; 48+ messages in thread
From: Stefan Hajnoczi @ 2021-02-04 9:47 UTC (permalink / raw)
To: Vivek Goyal
Cc: Daniel Berrange, qemu-devel, virtio-fs, Alex Xu, P J P, Laszlo Ersek
[-- Attachment #1: Type: text/plain, Size: 2603 bytes --]
On Wed, Feb 03, 2021 at 04:14:41PM -0500, Vivek Goyal wrote:
> On Wed, Feb 03, 2021 at 05:05:14PM +0000, Stefan Hajnoczi wrote:
> > On Wed, Feb 03, 2021 at 11:08:58AM -0500, Vivek Goyal wrote:
> > > On Wed, Feb 03, 2021 at 05:02:37PM +0100, Greg Kurz wrote:
> > > > On Wed, 3 Feb 2021 10:28:50 -0500
> > > > Vivek Goyal <vgoyal@redhat.com> wrote:
> > > >
> > > > > On Wed, Feb 03, 2021 at 11:37:19AM +0000, Stefan Hajnoczi wrote:
> > > > >
> > > > > [..]
> > > > > > @@ -1727,36 +1764,38 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
> > > > > >
> > > > > > update_open_flags(lo->writeback, lo->allow_direct_io, fi);
> > > > > >
> > > > > > - fd = openat(parent_inode->fd, name, (fi->flags | O_CREAT) & ~O_NOFOLLOW,
> > > > > > - mode);
> > > > > > + /* Try to create a new file but don't open existing files */
> > > > > > + fd = openat(parent_inode->fd, name, fi->flags | O_CREAT | O_EXCL, mode);
> > > > > > err = fd == -1 ? errno : 0;
> > > > > > +
> > > > > > lo_restore_cred(&old);
> > > > > >
> > > > > > - if (!err) {
> > > > > > - ssize_t fh;
> > > > > > -
> > > > > > - pthread_mutex_lock(&lo->mutex);
> > > > > > - fh = lo_add_fd_mapping(lo, fd);
> > > > > > - pthread_mutex_unlock(&lo->mutex);
> > > > > > - if (fh == -1) {
> > > > > > - close(fd);
> > > > > > - err = ENOMEM;
> > > > > > - goto out;
> > > > > > - }
> > > > > > + /* Ignore the error if file exists and O_EXCL was not given */
> > > > > > + if (err && !(err == EEXIST && !(fi->flags & O_EXCL))) {
> > > > >
> > > > > Can this check be simplified to.
> > > > > if (err && (err == EEXIST && (fi->flags & O_EXCL)) {
> > > >
> > > > I guess you meant :
> > > >
> > > > if (err && (err != EEXIST || fi->flags & O_EXCL) {
> > >
> > > This sounds correct. I forgot to take into account that if error is
> > > not -EEXIST, we still want to bail out irrespective of O_EXCL.
> >
> > I thought about De Morgan's law too but found the OR expression is not
> > easier to read than the AND expression :(. If you prefer it written this
> > way I can change it though.
>
> I personally find this one to read. And not because of AND but because
> of double logical negation (!x) in previous expression.
>
> But I am not particular about it. If you don't find it easier to
> read, I can live with previous one.
Okay, I'll convert it. Dave also mentioned he finds the AND version
strange.
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v4 2/3] virtiofsd: optionally return inode pointer from lo_do_lookup()
2021-02-04 9:45 ` [Virtio-fs] " Stefan Hajnoczi
@ 2021-02-04 11:19 ` Greg Kurz
-1 siblings, 0 replies; 48+ messages in thread
From: Greg Kurz @ 2021-02-04 11:19 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: mszeredi, Daniel Berrange, slp, qemu-devel,
Dr. David Alan Gilbert, virtio-fs, P J P, Laszlo Ersek, vgoyal
[-- Attachment #1: Type: text/plain, Size: 4543 bytes --]
On Thu, 4 Feb 2021 09:45:37 +0000
Stefan Hajnoczi <stefanha@redhat.com> wrote:
> On Thu, Feb 04, 2021 at 09:25:28AM +0100, Greg Kurz wrote:
> > On Wed, 3 Feb 2021 17:00:06 +0000
> > Stefan Hajnoczi <stefanha@redhat.com> wrote:
> >
> > > On Wed, Feb 03, 2021 at 03:20:14PM +0100, Greg Kurz wrote:
> > > > On Wed, 3 Feb 2021 11:37:18 +0000
> > > > Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > > >
> > > > > lo_do_lookup() finds an existing inode or allocates a new one. It
> > > > > increments nlookup so that the inode stays alive until the client
> > > > > releases it.
> > > > >
> > > > > Existing callers don't need the struct lo_inode so the function doesn't
> > > > > return it. Extend the function to optionally return the inode. The next
> > > > > commit will need it.
> > > > >
> > > > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > > > ---
> > > > > tools/virtiofsd/passthrough_ll.c | 29 +++++++++++++++++++++--------
> > > > > 1 file changed, 21 insertions(+), 8 deletions(-)
> > > > >
> > > > > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> > > > > index e63cbd3fb7..c87a1f3d72 100644
> > > > > --- a/tools/virtiofsd/passthrough_ll.c
> > > > > +++ b/tools/virtiofsd/passthrough_ll.c
> > > > > @@ -831,11 +831,13 @@ static int do_statx(struct lo_data *lo, int dirfd, const char *pathname,
> > > > > }
> > > > >
> > > > > /*
> > > > > - * Increments nlookup and caller must release refcount using
> > > > > - * lo_inode_put(&parent).
> > > > > + * Increments nlookup on the inode on success. unref_inode_lolocked() must be
> > > > > + * called eventually to decrement nlookup again. If inodep is non-NULL, the
> > > > > + * inode pointer is stored and the caller must call lo_inode_put().
> > > > > */
> > > > > static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
> > > > > - struct fuse_entry_param *e)
> > > > > + struct fuse_entry_param *e,
> > > > > + struct lo_inode **inodep)
> > > > > {
> > > > > int newfd;
> > > > > int res;
> > > > > @@ -845,6 +847,10 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
> > > > > struct lo_inode *inode = NULL;
> > > > > struct lo_inode *dir = lo_inode(req, parent);
> > > > >
> > > > > + if (inodep) {
> > > > > + *inodep = NULL;
> > > > > + }
> > > > > +
> > > >
> > > > Is this side-effect needed ? If lo_do_lookup() returns an error, it
> > > > rather seems that the caller shouldn't expect anything to be written
> > > > here, i.e. the content of *inodep still belongs to the caller and
> > > > whatever value it previously put in there (as patch 3/3 does) should
> > > > be preserved IMHO.
> > > >
> > > > Apart from that LGTM.
> > >
> > > I like this approach because it prevents accessing uninitialized memory
> > > in the caller:
> > >
> > > struct lo_inode *inode;
> > >
> > > if (lo_do_lookup(..., &inodep) != 0) {
> > > goto err;
> > > }
> > > ...
> > >
> > > err:
> > > lo_inode_put(&inode); <-- uninitialized in the error case!
> >
> > My point is that it is the caller's business to ensure that inode
> > doesn't contain garbage if it is to be used irrespective of the
> > outcome of lo_do_lookup(). This is precisely what patch 3/3 does,
> > so I don't understand the ultimate purpose of nullifying the
> > inode pointer _again_ in lo_do_lookup()...
>
> APIs should be designed to eliminate classes of errors where possible
> IMO. Taking care regarding the uninitialized pointer in the error case
> could be the caller's responsibility, but what's the advantage?
>
Because it is more explicit. FWIW caller is still responsible since it
calls lo_inode_put() in the end : initializing inode to NULL like patch
3/3 does warrants that no matter what happens in lo_do_lookup() or even
if it isn't called at all, inode can be safely passed to lo_inode_put().
But this change doesn't hurt, especially with the benefits of the rest
of this series, so:
Reviewed-by: Greg Kurz <groug@kaod.org>
> (There's a related thing with lo_inode_put(&inode) where it sets *inode
> = NULL to eliminate use-after-free bugs in callers. It would have been
> possible to use the same approach as free(3) where it's the caller's
> responsiblity, but that API design decision in free(3) has caused
> many bugs in applications.)
>
> Stefan
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [Virtio-fs] [PATCH v4 2/3] virtiofsd: optionally return inode pointer from lo_do_lookup()
@ 2021-02-04 11:19 ` Greg Kurz
0 siblings, 0 replies; 48+ messages in thread
From: Greg Kurz @ 2021-02-04 11:19 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Daniel Berrange, qemu-devel, virtio-fs, P J P, Laszlo Ersek, vgoyal
[-- Attachment #1: Type: text/plain, Size: 4543 bytes --]
On Thu, 4 Feb 2021 09:45:37 +0000
Stefan Hajnoczi <stefanha@redhat.com> wrote:
> On Thu, Feb 04, 2021 at 09:25:28AM +0100, Greg Kurz wrote:
> > On Wed, 3 Feb 2021 17:00:06 +0000
> > Stefan Hajnoczi <stefanha@redhat.com> wrote:
> >
> > > On Wed, Feb 03, 2021 at 03:20:14PM +0100, Greg Kurz wrote:
> > > > On Wed, 3 Feb 2021 11:37:18 +0000
> > > > Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > > >
> > > > > lo_do_lookup() finds an existing inode or allocates a new one. It
> > > > > increments nlookup so that the inode stays alive until the client
> > > > > releases it.
> > > > >
> > > > > Existing callers don't need the struct lo_inode so the function doesn't
> > > > > return it. Extend the function to optionally return the inode. The next
> > > > > commit will need it.
> > > > >
> > > > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > > > ---
> > > > > tools/virtiofsd/passthrough_ll.c | 29 +++++++++++++++++++++--------
> > > > > 1 file changed, 21 insertions(+), 8 deletions(-)
> > > > >
> > > > > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> > > > > index e63cbd3fb7..c87a1f3d72 100644
> > > > > --- a/tools/virtiofsd/passthrough_ll.c
> > > > > +++ b/tools/virtiofsd/passthrough_ll.c
> > > > > @@ -831,11 +831,13 @@ static int do_statx(struct lo_data *lo, int dirfd, const char *pathname,
> > > > > }
> > > > >
> > > > > /*
> > > > > - * Increments nlookup and caller must release refcount using
> > > > > - * lo_inode_put(&parent).
> > > > > + * Increments nlookup on the inode on success. unref_inode_lolocked() must be
> > > > > + * called eventually to decrement nlookup again. If inodep is non-NULL, the
> > > > > + * inode pointer is stored and the caller must call lo_inode_put().
> > > > > */
> > > > > static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
> > > > > - struct fuse_entry_param *e)
> > > > > + struct fuse_entry_param *e,
> > > > > + struct lo_inode **inodep)
> > > > > {
> > > > > int newfd;
> > > > > int res;
> > > > > @@ -845,6 +847,10 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name,
> > > > > struct lo_inode *inode = NULL;
> > > > > struct lo_inode *dir = lo_inode(req, parent);
> > > > >
> > > > > + if (inodep) {
> > > > > + *inodep = NULL;
> > > > > + }
> > > > > +
> > > >
> > > > Is this side-effect needed ? If lo_do_lookup() returns an error, it
> > > > rather seems that the caller shouldn't expect anything to be written
> > > > here, i.e. the content of *inodep still belongs to the caller and
> > > > whatever value it previously put in there (as patch 3/3 does) should
> > > > be preserved IMHO.
> > > >
> > > > Apart from that LGTM.
> > >
> > > I like this approach because it prevents accessing uninitialized memory
> > > in the caller:
> > >
> > > struct lo_inode *inode;
> > >
> > > if (lo_do_lookup(..., &inodep) != 0) {
> > > goto err;
> > > }
> > > ...
> > >
> > > err:
> > > lo_inode_put(&inode); <-- uninitialized in the error case!
> >
> > My point is that it is the caller's business to ensure that inode
> > doesn't contain garbage if it is to be used irrespective of the
> > outcome of lo_do_lookup(). This is precisely what patch 3/3 does,
> > so I don't understand the ultimate purpose of nullifying the
> > inode pointer _again_ in lo_do_lookup()...
>
> APIs should be designed to eliminate classes of errors where possible
> IMO. Taking care regarding the uninitialized pointer in the error case
> could be the caller's responsibility, but what's the advantage?
>
Because it is more explicit. FWIW caller is still responsible since it
calls lo_inode_put() in the end : initializing inode to NULL like patch
3/3 does warrants that no matter what happens in lo_do_lookup() or even
if it isn't called at all, inode can be safely passed to lo_inode_put().
But this change doesn't hurt, especially with the benefits of the rest
of this series, so:
Reviewed-by: Greg Kurz <groug@kaod.org>
> (There's a related thing with lo_inode_put(&inode) where it sets *inode
> = NULL to eliminate use-after-free bugs in callers. It would have been
> possible to use the same approach as free(3) where it's the caller's
> responsiblity, but that API design decision in free(3) has caused
> many bugs in applications.)
>
> Stefan
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 48+ messages in thread
end of thread, other threads:[~2021-02-04 11:21 UTC | newest]
Thread overview: 48+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-03 11:37 [PATCH v4 0/3] virtiofsd: prevent opening of special files (CVE-2020-35517) Stefan Hajnoczi
2021-02-03 11:37 ` [Virtio-fs] " Stefan Hajnoczi
2021-02-03 11:37 ` [PATCH v4 1/3] virtiofsd: extract lo_do_open() from lo_open() Stefan Hajnoczi
2021-02-03 11:37 ` [Virtio-fs] " Stefan Hajnoczi
2021-02-03 14:20 ` Greg Kurz
2021-02-03 14:20 ` [Virtio-fs] " Greg Kurz
2021-02-03 14:47 ` Dr. David Alan Gilbert
2021-02-03 14:47 ` [Virtio-fs] " Dr. David Alan Gilbert
2021-02-03 15:45 ` Greg Kurz
2021-02-03 15:45 ` [Virtio-fs] " Greg Kurz
2021-02-03 17:47 ` Greg Kurz
2021-02-03 17:47 ` [Virtio-fs] " Greg Kurz
2021-02-03 16:57 ` Stefan Hajnoczi
2021-02-03 16:57 ` [Virtio-fs] " Stefan Hajnoczi
2021-02-03 11:37 ` [PATCH v4 2/3] virtiofsd: optionally return inode pointer from lo_do_lookup() Stefan Hajnoczi
2021-02-03 11:37 ` [Virtio-fs] " Stefan Hajnoczi
2021-02-03 14:20 ` Greg Kurz
2021-02-03 14:20 ` [Virtio-fs] " Greg Kurz
2021-02-03 17:00 ` Stefan Hajnoczi
2021-02-03 17:00 ` [Virtio-fs] " Stefan Hajnoczi
2021-02-04 8:25 ` Greg Kurz
2021-02-04 8:25 ` [Virtio-fs] " Greg Kurz
2021-02-04 9:45 ` Stefan Hajnoczi
2021-02-04 9:45 ` [Virtio-fs] " Stefan Hajnoczi
2021-02-04 11:19 ` Greg Kurz
2021-02-04 11:19 ` [Virtio-fs] " Greg Kurz
2021-02-03 11:37 ` [PATCH v4 3/3] virtiofsd: prevent opening of special files (CVE-2020-35517) Stefan Hajnoczi
2021-02-03 11:37 ` [Virtio-fs] " Stefan Hajnoczi
2021-02-03 15:28 ` Vivek Goyal
2021-02-03 15:28 ` [Virtio-fs] " Vivek Goyal
2021-02-03 16:02 ` Greg Kurz
2021-02-03 16:02 ` [Virtio-fs] " Greg Kurz
2021-02-03 16:08 ` Vivek Goyal
2021-02-03 16:08 ` [Virtio-fs] " Vivek Goyal
2021-02-03 17:05 ` Stefan Hajnoczi
2021-02-03 17:05 ` [Virtio-fs] " Stefan Hajnoczi
2021-02-03 18:05 ` Dr. David Alan Gilbert
2021-02-03 18:05 ` [Virtio-fs] " Dr. David Alan Gilbert
2021-02-03 21:14 ` Vivek Goyal
2021-02-03 21:14 ` [Virtio-fs] " Vivek Goyal
2021-02-04 9:47 ` Stefan Hajnoczi
2021-02-04 9:47 ` [Virtio-fs] " Stefan Hajnoczi
2021-02-03 15:57 ` Greg Kurz
2021-02-03 15:57 ` [Virtio-fs] " Greg Kurz
2021-02-03 17:06 ` Stefan Hajnoczi
2021-02-03 17:06 ` [Virtio-fs] " Stefan Hajnoczi
2021-02-03 11:46 ` [PATCH v4 0/3] " no-reply
2021-02-03 11:46 ` [Virtio-fs] " no-reply
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.