From: Vivek Goyal <vgoyal@redhat.com>
If qemu guest asked to drop CAP_FSETID upon write, send that info
to qemu in SLAVE_FS_IO message so that qemu can drop capability
before WRITE. This is to make sure that any setuid bit is killed
on fd (if there is one set).
Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
tools/virtiofsd/buffer.c | 10 ++++++----
tools/virtiofsd/fuse_common.h | 6 +++++-
tools/virtiofsd/fuse_lowlevel.h | 6 +++++-
tools/virtiofsd/fuse_virtio.c | 5 ++++-
tools/virtiofsd/passthrough_ll.c | 2 +-
5 files changed, 21 insertions(+), 8 deletions(-)
diff --git a/tools/virtiofsd/buffer.c b/tools/virtiofsd/buffer.c
index 8135d52d2a..b4cda7db9a 100644
--- a/tools/virtiofsd/buffer.c
+++ b/tools/virtiofsd/buffer.c
@@ -203,7 +203,7 @@ static ssize_t fuse_buf_fd_to_fd(const struct fuse_buf *dst, size_t dst_off,
static ssize_t fuse_buf_copy_one(fuse_req_t req,
const struct fuse_buf *dst, size_t dst_off,
const struct fuse_buf *src, size_t src_off,
- size_t len)
+ size_t len, bool dropped_cap_fsetid)
{
int src_is_fd = src->flags & FUSE_BUF_IS_FD;
int dst_is_fd = dst->flags & FUSE_BUF_IS_FD;
@@ -211,7 +211,8 @@ static ssize_t fuse_buf_copy_one(fuse_req_t req,
int dst_is_phys = src->flags & FUSE_BUF_PHYS_ADDR;
if (src_is_phys && !src_is_fd && dst_is_fd) {
- return fuse_virtio_write(req, dst, dst_off, src, src_off, len);
+ return fuse_virtio_write(req, dst, dst_off, src, src_off, len,
+ dropped_cap_fsetid);
}
assert(!src_is_phys && !dst_is_phys);
if (!src_is_fd && !dst_is_fd) {
@@ -267,7 +268,7 @@ static int fuse_bufvec_advance(struct fuse_bufvec *bufv, size_t len)
}
ssize_t fuse_buf_copy(fuse_req_t req, struct fuse_bufvec *dstv,
- struct fuse_bufvec *srcv)
+ struct fuse_bufvec *srcv, bool dropped_cap_fsetid)
{
size_t copied = 0, i;
@@ -309,7 +310,8 @@ ssize_t fuse_buf_copy(fuse_req_t req, struct fuse_bufvec *dstv,
dst_len = dst->size - dstv->off;
len = min_size(src_len, dst_len);
- res = fuse_buf_copy_one(req, dst, dstv->off, src, srcv->off, len);
+ res = fuse_buf_copy_one(req, dst, dstv->off, src, srcv->off, len,
+ dropped_cap_fsetid);
if (res < 0) {
if (!copied) {
return res;
diff --git a/tools/virtiofsd/fuse_common.h b/tools/virtiofsd/fuse_common.h
index beed03aa93..8a75729be9 100644
--- a/tools/virtiofsd/fuse_common.h
+++ b/tools/virtiofsd/fuse_common.h
@@ -733,10 +733,14 @@ size_t fuse_buf_size(const struct fuse_bufvec *bufv);
* @param req The request this copy is part of
* @param dst destination buffer vector
* @param src source buffer vector
+ * @param dropped_cap_fsetid Caller has dropped CAP_FSETID. If work is handed
+ * over to a different thread/process, CAP_FSETID needs to be dropped
+ * there as well.
* @return actual number of bytes copied or -errno on error
*/
ssize_t fuse_buf_copy(fuse_req_t req,
- struct fuse_bufvec *dst, struct fuse_bufvec *src);
+ struct fuse_bufvec *dst, struct fuse_bufvec *src,
+ bool dropped_cap_fsetid);
/**
* Memory buffer iterator
diff --git a/tools/virtiofsd/fuse_lowlevel.h b/tools/virtiofsd/fuse_lowlevel.h
index 24e580aafe..dfd7e1525c 100644
--- a/tools/virtiofsd/fuse_lowlevel.h
+++ b/tools/virtiofsd/fuse_lowlevel.h
@@ -2030,9 +2030,13 @@ int64_t fuse_virtio_io(struct fuse_session *se, VhostUserFSSlaveMsg *msg,
* @param src The source (memory) buffer
* @param src_off The GPA
* @param len Length in bytes
+ * @param dropped_cap_fsetid Caller dropped CAP_FSETID. If it is being handed
+ * over to different thread/process, CAP_FSETID needs to be dropped
+ * before write.
*/
ssize_t fuse_virtio_write(fuse_req_t req, const struct fuse_buf *dst,
size_t dst_off, const struct fuse_buf *src,
- size_t src_off, size_t len);
+ size_t src_off, size_t len,
+ bool dropped_cap_fsetid);
#endif /* FUSE_LOWLEVEL_H_ */
diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
index c6ea2bd2a1..9f3d38942a 100644
--- a/tools/virtiofsd/fuse_virtio.c
+++ b/tools/virtiofsd/fuse_virtio.c
@@ -1291,7 +1291,7 @@ int64_t fuse_virtio_io(struct fuse_session *se, VhostUserFSSlaveMsg *msg,
*/
ssize_t fuse_virtio_write(fuse_req_t req, const struct fuse_buf *dst,
size_t dst_off, const struct fuse_buf *src,
- size_t src_off, size_t len)
+ size_t src_off, size_t len, bool dropped_cap_fsetid)
{
VhostUserFSSlaveMsg *msg = g_malloc0(sizeof(VhostUserFSSlaveMsg) +
sizeof(VhostUserFSSlaveMsgEntry));
@@ -1311,6 +1311,9 @@ ssize_t fuse_virtio_write(fuse_req_t req, const struct fuse_buf *dst,
msg->entries[0].c_offset = (uintptr_t)src->mem + src_off;
msg->entries[0].len = len;
msg->entries[0].flags = VHOST_USER_FS_FLAG_MAP_W;
+ if (dropped_cap_fsetid) {
+ msg->flags |= VHOST_USER_FS_GENFLAG_DROP_FSETID;
+ }
int64_t result = fuse_virtio_io(req->se, msg, dst->fd);
fuse_log(FUSE_LOG_DEBUG, "%s: result=%" PRId64 " \n", __func__, result);
diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index c5b8a1f5b1..b76d878509 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -2301,7 +2301,7 @@ static void lo_write_buf(fuse_req_t req, fuse_ino_t ino,
}
}
- res = fuse_buf_copy(req, &out_buf, in_buf);
+ res = fuse_buf_copy(req, &out_buf, in_buf, fi->kill_priv);
if (res < 0) {
fuse_reply_err(req, -res);
} else {
--
2.31.1
On Wed, Apr 28, 2021 at 12:01:00PM +0100, Dr. David Alan Gilbert (git) wrote: > From: Vivek Goyal <vgoyal@redhat.com> > > If qemu guest asked to drop CAP_FSETID upon write, send that info > to qemu in SLAVE_FS_IO message so that qemu can drop capability > before WRITE. This is to make sure that any setuid bit is killed > on fd (if there is one set). > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com> I'm not sure if the QEMU FSETID patches make sense. QEMU shouldn't be running with FSETID because QEMU is untrusted. FSETGID would allow QEMU to create setgid files, thereby potentially allowing an attacker to gain any GID. I think it's better not to implement QEMU FSETID functionality at all and to handle it another way. In the worst case I/O requests should just fail, it seems like a rare case anyway: I/O to a setuid/setgid file with a memory buffer that is not mapped in virtiofsd. Stefan
On Thu, May 06, 2021 at 04:37:04PM +0100, Stefan Hajnoczi wrote: > On Wed, Apr 28, 2021 at 12:01:00PM +0100, Dr. David Alan Gilbert (git) wrote: > > From: Vivek Goyal <vgoyal@redhat.com> > > > > If qemu guest asked to drop CAP_FSETID upon write, send that info > > to qemu in SLAVE_FS_IO message so that qemu can drop capability > > before WRITE. This is to make sure that any setuid bit is killed > > on fd (if there is one set). > > > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com> > > I'm not sure if the QEMU FSETID patches make sense. QEMU shouldn't be > running with FSETID because QEMU is untrusted. FSETGID would allow QEMU > to create setgid files, thereby potentially allowing an attacker to gain > any GID. Sure, its not recommended to run QEMU as root, but we don't block that either and I do regularly test with qemu running as root. > > I think it's better not to implement QEMU FSETID functionality at all > and to handle it another way. One way could be that virtiofsd tries to clear setuid bit after I/O has finished. But that will be non-atomic operation and it is filled with perils as it requires virtiofsd to know what all kernel will do if this write has been done with CAP_FSETID dropped. > In the worst case I/O requests should just > fail, it seems like a rare case anyway: Is there a way for virtiofsd to know if qemu is running with CAP_FSETID or not. If there is one, it might be reasonable to error out. If we don't know, then we can't fail all the operations. > I/O to a setuid/setgid file with > a memory buffer that is not mapped in virtiofsd. With DAX it is easily triggerable. User has to append to a setuid file in virtiofs and this path will trigger. I am fine with not supporting this patch but will also need a reaosonable alternative solution. Vivek
On Thu, May 06, 2021 at 12:02:23PM -0400, Vivek Goyal wrote: > On Thu, May 06, 2021 at 04:37:04PM +0100, Stefan Hajnoczi wrote: > > On Wed, Apr 28, 2021 at 12:01:00PM +0100, Dr. David Alan Gilbert (git) wrote: > > > From: Vivek Goyal <vgoyal@redhat.com> > > > > > > If qemu guest asked to drop CAP_FSETID upon write, send that info > > > to qemu in SLAVE_FS_IO message so that qemu can drop capability > > > before WRITE. This is to make sure that any setuid bit is killed > > > on fd (if there is one set). > > > > > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com> > > > > I'm not sure if the QEMU FSETID patches make sense. QEMU shouldn't be > > running with FSETID because QEMU is untrusted. FSETGID would allow QEMU > > to create setgid files, thereby potentially allowing an attacker to gain > > any GID. > > Sure, its not recommended to run QEMU as root, but we don't block that > either and I do regularly test with qemu running as root. > > > > > I think it's better not to implement QEMU FSETID functionality at all > > and to handle it another way. > > One way could be that virtiofsd tries to clear setuid bit after I/O > has finished. But that will be non-atomic operation and it is filled > with perils as it requires virtiofsd to know what all kernel will > do if this write has been done with CAP_FSETID dropped. > > > In the worst case I/O requests should just > > fail, it seems like a rare case anyway: > > Is there a way for virtiofsd to know if qemu is running with CAP_FSETID > or not. If there is one, it might be reasonable to error out. If we > don't know, then we can't fail all the operations. > > > I/O to a setuid/setgid file with > > a memory buffer that is not mapped in virtiofsd. > > With DAX it is easily triggerable. User has to append to a setuid file > in virtiofs and this path will trigger. > > I am fine with not supporting this patch but will also need a reaosonable > alternative solution. One way to avoid this problem is by introducing DMA read/write functions into the vhost-user protocol that can be used by all device types, not just virtio-fs. Today virtio-fs uses the IO slave request when it cannot access a region of guest memory. It sends the file descriptor to QEMU and QEMU performs the pread(2)/pwrite(2) on behalf of virtiofsd. I mentioned in the past that this solution is over-specialized. It doesn't solve the larger problem that vhost-user processes do not have full access to the guest memory space (e.g. DAX window). Instead of sending file I/O requests over to QEMU, the vhost-user protocol should offer DMA read/write requests so any vhost-user process can access the guest memory space where vhost's shared memory mechanism is insufficient. Here is how it would work: 1. Drop the IO slave request, replace it with DMA read/write slave requests. Note that these new requests can also be used in environments where maximum vIOMMU isolation is needed for security reasons and sharing all of guest RAM with the vhost-user process is considered unacceptable. 2. When virtqueue buffer mapping fails, send DMA read/write slave requests to transfer the data from/to QEMU. virtiofsd calls pread(2)/pwrite(2) itself with virtiofsd's Linux capabilities. Stefan
On Mon, May 10, 2021 at 10:05:09AM +0100, Stefan Hajnoczi wrote: > On Thu, May 06, 2021 at 12:02:23PM -0400, Vivek Goyal wrote: > > On Thu, May 06, 2021 at 04:37:04PM +0100, Stefan Hajnoczi wrote: > > > On Wed, Apr 28, 2021 at 12:01:00PM +0100, Dr. David Alan Gilbert (git) wrote: > > > > From: Vivek Goyal <vgoyal@redhat.com> > > > > > > > > If qemu guest asked to drop CAP_FSETID upon write, send that info > > > > to qemu in SLAVE_FS_IO message so that qemu can drop capability > > > > before WRITE. This is to make sure that any setuid bit is killed > > > > on fd (if there is one set). > > > > > > > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com> > > > > > > I'm not sure if the QEMU FSETID patches make sense. QEMU shouldn't be > > > running with FSETID because QEMU is untrusted. FSETGID would allow QEMU > > > to create setgid files, thereby potentially allowing an attacker to gain > > > any GID. > > > > Sure, its not recommended to run QEMU as root, but we don't block that > > either and I do regularly test with qemu running as root. > > > > > > > > I think it's better not to implement QEMU FSETID functionality at all > > > and to handle it another way. > > > > One way could be that virtiofsd tries to clear setuid bit after I/O > > has finished. But that will be non-atomic operation and it is filled > > with perils as it requires virtiofsd to know what all kernel will > > do if this write has been done with CAP_FSETID dropped. > > > > > In the worst case I/O requests should just > > > fail, it seems like a rare case anyway: > > > > Is there a way for virtiofsd to know if qemu is running with CAP_FSETID > > or not. If there is one, it might be reasonable to error out. If we > > don't know, then we can't fail all the operations. > > > > > I/O to a setuid/setgid file with > > > a memory buffer that is not mapped in virtiofsd. > > > > With DAX it is easily triggerable. User has to append to a setuid file > > in virtiofs and this path will trigger. > > > > I am fine with not supporting this patch but will also need a reaosonable > > alternative solution. > > One way to avoid this problem is by introducing DMA read/write functions > into the vhost-user protocol that can be used by all device types, not > just virtio-fs. > > Today virtio-fs uses the IO slave request when it cannot access a region > of guest memory. It sends the file descriptor to QEMU and QEMU performs > the pread(2)/pwrite(2) on behalf of virtiofsd. > > I mentioned in the past that this solution is over-specialized. It > doesn't solve the larger problem that vhost-user processes do not have > full access to the guest memory space (e.g. DAX window). > > Instead of sending file I/O requests over to QEMU, the vhost-user > protocol should offer DMA read/write requests so any vhost-user process > can access the guest memory space where vhost's shared memory mechanism > is insufficient. > > Here is how it would work: > > 1. Drop the IO slave request, replace it with DMA read/write slave > requests. > > Note that these new requests can also be used in environments where > maximum vIOMMU isolation is needed for security reasons and sharing > all of guest RAM with the vhost-user process is considered > unacceptable. > > 2. When virtqueue buffer mapping fails, send DMA read/write slave > requests to transfer the data from/to QEMU. virtiofsd calls > pread(2)/pwrite(2) itself with virtiofsd's Linux capabilities. Can you elaborate a bit more how will this new DMA read/write vhost-user commands can be implemented. I am assuming its not a real DMA and just sort of emulation of DMA. Effectively we have two processes and one process needs to read/write to/from address space of other process. We were also wondering if we can make use of process_vm_readv() and process_vm_write() syscalls to achieve this. But this atleast requires virtiofsd to be more priviliged than qemu and also virtiofsd needs to know where DAX mapping window is. We briefly discussed this here. https://lore.kernel.org/qemu-devel/20210421200746.GH1579961@redhat.com/ Vivek
On Mon, May 10, 2021 at 11:23:24AM -0400, Vivek Goyal wrote: > On Mon, May 10, 2021 at 10:05:09AM +0100, Stefan Hajnoczi wrote: > > On Thu, May 06, 2021 at 12:02:23PM -0400, Vivek Goyal wrote: > > > On Thu, May 06, 2021 at 04:37:04PM +0100, Stefan Hajnoczi wrote: > > > > On Wed, Apr 28, 2021 at 12:01:00PM +0100, Dr. David Alan Gilbert (git) wrote: > > > > > From: Vivek Goyal <vgoyal@redhat.com> > > > > > > > > > > If qemu guest asked to drop CAP_FSETID upon write, send that info > > > > > to qemu in SLAVE_FS_IO message so that qemu can drop capability > > > > > before WRITE. This is to make sure that any setuid bit is killed > > > > > on fd (if there is one set). > > > > > > > > > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com> > > > > > > > > I'm not sure if the QEMU FSETID patches make sense. QEMU shouldn't be > > > > running with FSETID because QEMU is untrusted. FSETGID would allow QEMU > > > > to create setgid files, thereby potentially allowing an attacker to gain > > > > any GID. > > > > > > Sure, its not recommended to run QEMU as root, but we don't block that > > > either and I do regularly test with qemu running as root. > > > > > > > > > > > I think it's better not to implement QEMU FSETID functionality at all > > > > and to handle it another way. > > > > > > One way could be that virtiofsd tries to clear setuid bit after I/O > > > has finished. But that will be non-atomic operation and it is filled > > > with perils as it requires virtiofsd to know what all kernel will > > > do if this write has been done with CAP_FSETID dropped. > > > > > > > In the worst case I/O requests should just > > > > fail, it seems like a rare case anyway: > > > > > > Is there a way for virtiofsd to know if qemu is running with CAP_FSETID > > > or not. If there is one, it might be reasonable to error out. If we > > > don't know, then we can't fail all the operations. > > > > > > > I/O to a setuid/setgid file with > > > > a memory buffer that is not mapped in virtiofsd. > > > > > > With DAX it is easily triggerable. User has to append to a setuid file > > > in virtiofs and this path will trigger. > > > > > > I am fine with not supporting this patch but will also need a reaosonable > > > alternative solution. > > > > One way to avoid this problem is by introducing DMA read/write functions > > into the vhost-user protocol that can be used by all device types, not > > just virtio-fs. > > > > Today virtio-fs uses the IO slave request when it cannot access a region > > of guest memory. It sends the file descriptor to QEMU and QEMU performs > > the pread(2)/pwrite(2) on behalf of virtiofsd. > > > > I mentioned in the past that this solution is over-specialized. It > > doesn't solve the larger problem that vhost-user processes do not have > > full access to the guest memory space (e.g. DAX window). > > > > Instead of sending file I/O requests over to QEMU, the vhost-user > > protocol should offer DMA read/write requests so any vhost-user process > > can access the guest memory space where vhost's shared memory mechanism > > is insufficient. > > > > Here is how it would work: > > > > 1. Drop the IO slave request, replace it with DMA read/write slave > > requests. > > > > Note that these new requests can also be used in environments where > > maximum vIOMMU isolation is needed for security reasons and sharing > > all of guest RAM with the vhost-user process is considered > > unacceptable. > > > > 2. When virtqueue buffer mapping fails, send DMA read/write slave > > requests to transfer the data from/to QEMU. virtiofsd calls > > pread(2)/pwrite(2) itself with virtiofsd's Linux capabilities. > > Can you elaborate a bit more how will this new DMA read/write vhost-user > commands can be implemented. I am assuming its not a real DMA and just > sort of emulation of DMA. Effectively we have two processes and one > process needs to read/write to/from address space of other process. > > We were also wondering if we can make use of process_vm_readv() > and process_vm_write() syscalls to achieve this. But this atleast > requires virtiofsd to be more priviliged than qemu and also virtiofsd > needs to know where DAX mapping window is. We briefly discussed this here. > > https://lore.kernel.org/qemu-devel/20210421200746.GH1579961@redhat.com/ I wasn't thinking of directly allowing QEMU virtual memory access via process_vm_readv/writev(). That would be more efficient but requires privileges and also exposes internals of QEMU's virtual memory layout and vIOMMU translation to the vhost-user process. Instead I was thinking about VHOST_USER_DMA_READ/WRITE messages containing the address (a device IOVA, it could just be a guest physical memory address in most cases) and the length. The WRITE message would also contain the data that the vhost-user device wishes to write. The READ message reply would contain the data that the device read from QEMU. QEMU would implement this using QEMU's address_space_read/write() APIs. So basically just a new vhost-user protocol message to do a memcpy(), but with guest addresses and vIOMMU support :). The vhost-user device will need to do bounce buffering so using these new messages is slower than zero-copy I/O to shared guest RAM. Stefan
© 2016 - 2026 Red Hat, Inc.