hw/9pfs/codir.c | 3 +-- include/qemu/osdep.h | 13 +++++++++++++ util/osdep.c | 16 ++++++++++++++++ 3 files changed, 30 insertions(+), 2 deletions(-)
`struct dirent' returned from readdir(3) could be shorter than
`sizeof(struct dirent)', thus memcpy of sizeof length will overread
into unallocated page causing SIGSEGV. Example stack trace:
#0 0x00005555559ebeed v9fs_co_readdir_many (/usr/bin/qemu-system-x86_64 + 0x497eed)
#1 0x00005555559ec2e9 v9fs_readdir (/usr/bin/qemu-system-x86_64 + 0x4982e9)
#2 0x0000555555eb7983 coroutine_trampoline (/usr/bin/qemu-system-x86_64 + 0x963983)
#3 0x00007ffff73e0be0 n/a (n/a + 0x0)
While fixing, provide a helper for any future `struct dirent' cloning.
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/841
Cc: qemu-stable@nongnu.org
Co-authored-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
---
Tested on x86-64 Linux.
Changes since v1:
- Update commentary text.
- Remove hanging of g_malloc "errors".
- Simplify qemu_dirent_dup.
hw/9pfs/codir.c | 3 +--
include/qemu/osdep.h | 13 +++++++++++++
util/osdep.c | 16 ++++++++++++++++
3 files changed, 30 insertions(+), 2 deletions(-)
diff --git a/hw/9pfs/codir.c b/hw/9pfs/codir.c
index 032cce04c4..c0873bde16 100644
--- a/hw/9pfs/codir.c
+++ b/hw/9pfs/codir.c
@@ -143,8 +143,7 @@ static int do_readdir_many(V9fsPDU *pdu, V9fsFidState *fidp,
} else {
e = e->next = g_malloc0(sizeof(V9fsDirEnt));
}
- e->dent = g_malloc0(sizeof(struct dirent));
- memcpy(e->dent, dent, sizeof(struct dirent));
+ e->dent = qemu_dirent_dup(dent);
/* perform a full stat() for directory entry if requested by caller */
if (dostat) {
diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index d1660d67fa..ce12f64853 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -805,6 +805,19 @@ static inline int platform_does_not_support_system(const char *command)
}
#endif /* !HAVE_SYSTEM_FUNCTION */
+/**
+ * Duplicate directory entry @dent.
+ *
+ * It is highly recommended to use this function instead of open coding
+ * duplication of @c dirent objects, because the actual @c struct @c dirent
+ * size may be bigger or shorter than @c sizeof(struct dirent) and correct
+ * handling is platform specific (see gitlab issue #841).
+ *
+ * @dent - original directory entry to be duplicated
+ * @returns duplicated directory entry which should be freed with g_free()
+ */
+struct dirent *qemu_dirent_dup(struct dirent *dent);
+
#ifdef __cplusplus
}
#endif
diff --git a/util/osdep.c b/util/osdep.c
index 42a0a4986a..0bc7ec1e22 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -33,6 +33,7 @@
extern int madvise(char *, size_t, int);
#endif
+#include <dirent.h>
#include "qemu-common.h"
#include "qemu/cutils.h"
#include "qemu/sockets.h"
@@ -615,3 +616,18 @@ writev(int fd, const struct iovec *iov, int iov_cnt)
return readv_writev(fd, iov, iov_cnt, true);
}
#endif
+
+struct dirent *
+qemu_dirent_dup(struct dirent *dent)
+{
+#if defined _DIRENT_HAVE_D_RECLEN
+ /* Avoid use of strlen() if there's d_reclen. */
+ const size_t sz = dent->d_reclen;
+#else
+ /* Fallback to a most portable way. */
+ const size_t sz = offsetof(struct dirent, d_name) +
+ strlen(dent->d_name) + 1;
+#endif
+ struct dirent *dst = g_malloc(sz);
+ return memcpy(dst, dent, sz);
+}
--
2.33.0
On Freitag, 28. Januar 2022 23:33:26 CET Vitaly Chikunov wrote:
> `struct dirent' returned from readdir(3) could be shorter than
> `sizeof(struct dirent)', thus memcpy of sizeof length will overread
> into unallocated page causing SIGSEGV. Example stack trace:
I actually suggested to make it clear in the commit log comment here that it
could be both, shorter or longer, but OK, the API doc comment on
qemu_dirent_dup() sais it, so ...
Reviewed-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
>
> #0 0x00005555559ebeed v9fs_co_readdir_many (/usr/bin/qemu-system-x86_64 +
> 0x497eed) #1 0x00005555559ec2e9 v9fs_readdir (/usr/bin/qemu-system-x86_64
> + 0x4982e9) #2 0x0000555555eb7983 coroutine_trampoline
> (/usr/bin/qemu-system-x86_64 + 0x963983) #3 0x00007ffff73e0be0 n/a (n/a +
> 0x0)
>
> While fixing, provide a helper for any future `struct dirent' cloning.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/841
> Cc: qemu-stable@nongnu.org
> Co-authored-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
> ---
> Tested on x86-64 Linux.
>
> Changes since v1:
> - Update commentary text.
> - Remove hanging of g_malloc "errors".
> - Simplify qemu_dirent_dup.
>
> hw/9pfs/codir.c | 3 +--
> include/qemu/osdep.h | 13 +++++++++++++
> util/osdep.c | 16 ++++++++++++++++
> 3 files changed, 30 insertions(+), 2 deletions(-)
>
> diff --git a/hw/9pfs/codir.c b/hw/9pfs/codir.c
> index 032cce04c4..c0873bde16 100644
> --- a/hw/9pfs/codir.c
> +++ b/hw/9pfs/codir.c
> @@ -143,8 +143,7 @@ static int do_readdir_many(V9fsPDU *pdu, V9fsFidState
> *fidp, } else {
> e = e->next = g_malloc0(sizeof(V9fsDirEnt));
> }
> - e->dent = g_malloc0(sizeof(struct dirent));
> - memcpy(e->dent, dent, sizeof(struct dirent));
> + e->dent = qemu_dirent_dup(dent);
>
> /* perform a full stat() for directory entry if requested by caller
> */ if (dostat) {
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index d1660d67fa..ce12f64853 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -805,6 +805,19 @@ static inline int
> platform_does_not_support_system(const char *command) }
> #endif /* !HAVE_SYSTEM_FUNCTION */
>
> +/**
> + * Duplicate directory entry @dent.
> + *
> + * It is highly recommended to use this function instead of open coding
> + * duplication of @c dirent objects, because the actual @c struct @c dirent
> + * size may be bigger or shorter than @c sizeof(struct dirent) and correct
> + * handling is platform specific (see gitlab issue #841).
> + *
> + * @dent - original directory entry to be duplicated
> + * @returns duplicated directory entry which should be freed with g_free()
> + */
> +struct dirent *qemu_dirent_dup(struct dirent *dent);
> +
> #ifdef __cplusplus
> }
> #endif
> diff --git a/util/osdep.c b/util/osdep.c
> index 42a0a4986a..0bc7ec1e22 100644
> --- a/util/osdep.c
> +++ b/util/osdep.c
> @@ -33,6 +33,7 @@
> extern int madvise(char *, size_t, int);
> #endif
>
> +#include <dirent.h>
> #include "qemu-common.h"
> #include "qemu/cutils.h"
> #include "qemu/sockets.h"
> @@ -615,3 +616,18 @@ writev(int fd, const struct iovec *iov, int iov_cnt)
> return readv_writev(fd, iov, iov_cnt, true);
> }
> #endif
> +
> +struct dirent *
> +qemu_dirent_dup(struct dirent *dent)
> +{
> +#if defined _DIRENT_HAVE_D_RECLEN
> + /* Avoid use of strlen() if there's d_reclen. */
> + const size_t sz = dent->d_reclen;
> +#else
> + /* Fallback to a most portable way. */
> + const size_t sz = offsetof(struct dirent, d_name) +
> + strlen(dent->d_name) + 1;
> +#endif
> + struct dirent *dst = g_malloc(sz);
> + return memcpy(dst, dent, sz);
> +}
On Freitag, 28. Januar 2022 23:33:26 CET Vitaly Chikunov wrote:
> `struct dirent' returned from readdir(3) could be shorter than
> `sizeof(struct dirent)', thus memcpy of sizeof length will overread
> into unallocated page causing SIGSEGV. Example stack trace:
>
> #0 0x00005555559ebeed v9fs_co_readdir_many (/usr/bin/qemu-system-x86_64 +
> 0x497eed) #1 0x00005555559ec2e9 v9fs_readdir (/usr/bin/qemu-system-x86_64
> + 0x4982e9) #2 0x0000555555eb7983 coroutine_trampoline
> (/usr/bin/qemu-system-x86_64 + 0x963983) #3 0x00007ffff73e0be0 n/a (n/a +
> 0x0)
>
> While fixing, provide a helper for any future `struct dirent' cloning.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/841
> Cc: qemu-stable@nongnu.org
> Co-authored-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
> ---
> Tested on x86-64 Linux.
I was too optimistic. Looks like this needs more work. With this patch applied
the 9p test cases [1] are crashing now:
$ gdb --args tests/qtest/qos-test -m slow
...
# Start of flush tests
ok 50 /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/virtio-9p-tests/synth/flush/success
ok 51 /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/virtio-9p-tests/synth/flush/ignored
# End of flush tests
# Start of readdir tests
Broken pipe
Thread 1 "qos-test" received signal SIGABRT, Aborted.
__GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
50 ../sysdeps/unix/sysv/linux/raise.c: No such file or directory.
(gdb) bt
#0 __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
#1 0x00007ffff7b7d537 in __GI_abort () at abort.c:79
#2 0x00005555555ba495 in qtest_client_socket_recv_line (s=0x5555557663c0) at ../tests/qtest/libqtest.c:503
#3 0x00005555555ba5b3 in qtest_rsp_args (s=0x5555557663c0, expected_args=2) at ../tests/qtest/libqtest.c:523
#4 0x00005555555bbdb4 in qtest_clock_rsp (s=0x5555557663c0) at ../tests/qtest/libqtest.c:970
#5 0x00005555555bbe55 in qtest_clock_step (s=0x5555557663c0, step=100) at ../tests/qtest/libqtest.c:985
#6 0x00005555555cdc21 in qvirtio_wait_used_elem (qts=0x5555557663c0, d=0x555555779b48, vq=0x5555557b0480, desc_idx=8, len=0x0, timeout_us=10000000)
at ../tests/qtest/libqos/virtio.c:220
#7 0x00005555555ae79f in v9fs_req_wait_for_reply (req=0x5555557899a0, len=0x0) at ../tests/qtest/virtio-9p-test.c:278
#8 0x00005555555b03bf in fs_readdir (obj=0x555555779bb0, data=0x0, t_alloc=0x5555557448b8) at ../tests/qtest/virtio-9p-test.c:851
#9 0x00005555555990c4 in run_one_test (arg=0x5555557ac600) at ../tests/qtest/qos-test.c:182
#10 0x00007ffff7f02b9e in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#11 0x00007ffff7f0299b in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#12 0x00007ffff7f0299b in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#13 0x00007ffff7f0299b in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#14 0x00007ffff7f0299b in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#15 0x00007ffff7f0299b in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#16 0x00007ffff7f0299b in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#17 0x00007ffff7f0299b in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#18 0x00007ffff7f0299b in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#19 0x00007ffff7f0299b in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#20 0x00007ffff7f0299b in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#21 0x00007ffff7f0308a in g_test_run_suite () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#22 0x00007ffff7f030a1 in g_test_run () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#23 0x00005555555995a3 in main (argc=1, argv=0x7fffffffe508, envp=0x7fffffffe528) at ../tests/qtest/qos-test.c:338
(gdb)
[1] https://wiki.qemu.org/Documentation/9p#Test_Cases
Best regards,
Christian Schoenebeck
Christian, On Wed, Feb 02, 2022 at 05:55:45PM +0100, Christian Schoenebeck wrote: > On Freitag, 28. Januar 2022 23:33:26 CET Vitaly Chikunov wrote: > > `struct dirent' returned from readdir(3) could be shorter than > > `sizeof(struct dirent)', thus memcpy of sizeof length will overread > > into unallocated page causing SIGSEGV. Example stack trace: > > > > #0 0x00005555559ebeed v9fs_co_readdir_many (/usr/bin/qemu-system-x86_64 + > > 0x497eed) #1 0x00005555559ec2e9 v9fs_readdir (/usr/bin/qemu-system-x86_64 > > + 0x4982e9) #2 0x0000555555eb7983 coroutine_trampoline > > (/usr/bin/qemu-system-x86_64 + 0x963983) #3 0x00007ffff73e0be0 n/a (n/a + > > 0x0) > > > > While fixing, provide a helper for any future `struct dirent' cloning. > > > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/841 > > Cc: qemu-stable@nongnu.org > > Co-authored-by: Christian Schoenebeck <qemu_oss@crudebyte.com> > > Signed-off-by: Vitaly Chikunov <vt@altlinux.org> > > --- > > Tested on x86-64 Linux. > > I was too optimistic. Looks like this needs more work. With this patch applied > the 9p test cases [1] are crashing now: > > $ gdb --args tests/qtest/qos-test -m slow > ... > # Start of flush tests > ok 50 /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/virtio-9p-tests/synth/flush/success > ok 51 /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/virtio-9p-tests/synth/flush/ignored > # End of flush tests > # Start of readdir tests I changed implementation from the one using dent->d_reclen to the one using strlen(dent->d_name) and it's passed readdir tests, but failed later: # Start of readdir tests ok 53 /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/virtio-9p-tests/synth/readdir/basic ok 54 /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/virtio-9p-tests/synth/readdir/split_512 ok 55 /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/virtio-9p-tests/synth/readdir/split_256 ok 56 /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/virtio-9p-tests/synth/readdir/split_128 # End of readdir tests # End of synth tests # Start of local tests # starting QEMU: exec x86_64-softmmu/qemu-system-x86_64 -qtest unix:/tmp/qtest-2822967.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-2822967.qmp,id=char0 -mon chardev=char0,mode=control -display none -M pc -fsdev local,id=fsdev0,path='/usr/src/RPM/BUILD/qemu-6.2.0/build-dynamic/qtest-9p-local-NpcZCR',security_model=mapped-xattr -device virtio-9p-pci,fsdev=fsdev0,addr=04.0,mount_tag=qtest -accel qtest # GLib-DEBUG: setenv()/putenv() are not thread-safe and should not be used after threads are created ok 57 /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/virtio-9p-tests/local/config Received response 7 (RLERROR) instead of 73 (RMKDIR) Rlerror has errno 95 (Operation not supported) ** ERROR:../tests/qtest/virtio-9p-test.c:305:v9fs_req_recv: assertion failed (hdr.id == id): (7 == 73) Bail out! ERROR:../tests/qtest/virtio-9p-test.c:305:v9fs_req_recv: assertion failed (hdr.id == id): (7 == 73) Aborted Thanks, > Broken pipe > > Thread 1 "qos-test" received signal SIGABRT, Aborted. > __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50 > 50 ../sysdeps/unix/sysv/linux/raise.c: No such file or directory. > (gdb) bt > #0 __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50 > #1 0x00007ffff7b7d537 in __GI_abort () at abort.c:79 > #2 0x00005555555ba495 in qtest_client_socket_recv_line (s=0x5555557663c0) at ../tests/qtest/libqtest.c:503 > #3 0x00005555555ba5b3 in qtest_rsp_args (s=0x5555557663c0, expected_args=2) at ../tests/qtest/libqtest.c:523 > #4 0x00005555555bbdb4 in qtest_clock_rsp (s=0x5555557663c0) at ../tests/qtest/libqtest.c:970 > #5 0x00005555555bbe55 in qtest_clock_step (s=0x5555557663c0, step=100) at ../tests/qtest/libqtest.c:985 > #6 0x00005555555cdc21 in qvirtio_wait_used_elem (qts=0x5555557663c0, d=0x555555779b48, vq=0x5555557b0480, desc_idx=8, len=0x0, timeout_us=10000000) > at ../tests/qtest/libqos/virtio.c:220 > #7 0x00005555555ae79f in v9fs_req_wait_for_reply (req=0x5555557899a0, len=0x0) at ../tests/qtest/virtio-9p-test.c:278 > #8 0x00005555555b03bf in fs_readdir (obj=0x555555779bb0, data=0x0, t_alloc=0x5555557448b8) at ../tests/qtest/virtio-9p-test.c:851 > #9 0x00005555555990c4 in run_one_test (arg=0x5555557ac600) at ../tests/qtest/qos-test.c:182 > #10 0x00007ffff7f02b9e in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0 > #11 0x00007ffff7f0299b in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0 > #12 0x00007ffff7f0299b in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0 > #13 0x00007ffff7f0299b in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0 > #14 0x00007ffff7f0299b in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0 > #15 0x00007ffff7f0299b in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0 > #16 0x00007ffff7f0299b in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0 > #17 0x00007ffff7f0299b in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0 > #18 0x00007ffff7f0299b in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0 > #19 0x00007ffff7f0299b in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0 > #20 0x00007ffff7f0299b in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0 > #21 0x00007ffff7f0308a in g_test_run_suite () from /lib/x86_64-linux-gnu/libglib-2.0.so.0 > #22 0x00007ffff7f030a1 in g_test_run () from /lib/x86_64-linux-gnu/libglib-2.0.so.0 > #23 0x00005555555995a3 in main (argc=1, argv=0x7fffffffe508, envp=0x7fffffffe528) at ../tests/qtest/qos-test.c:338 > (gdb) > > [1] https://wiki.qemu.org/Documentation/9p#Test_Cases > > Best regards, > Christian Schoenebeck > >
On Thu, Feb 03, 2022 at 07:55:41AM +0300, Vitaly Chikunov wrote:
> Christian,
>
> On Wed, Feb 02, 2022 at 05:55:45PM +0100, Christian Schoenebeck wrote:
> > On Freitag, 28. Januar 2022 23:33:26 CET Vitaly Chikunov wrote:
> > > `struct dirent' returned from readdir(3) could be shorter than
> > > `sizeof(struct dirent)', thus memcpy of sizeof length will overread
> > > into unallocated page causing SIGSEGV. Example stack trace:
> > >
> > > #0 0x00005555559ebeed v9fs_co_readdir_many (/usr/bin/qemu-system-x86_64 +
> > > 0x497eed) #1 0x00005555559ec2e9 v9fs_readdir (/usr/bin/qemu-system-x86_64
> > > + 0x4982e9) #2 0x0000555555eb7983 coroutine_trampoline
> > > (/usr/bin/qemu-system-x86_64 + 0x963983) #3 0x00007ffff73e0be0 n/a (n/a +
> > > 0x0)
> > >
> > > While fixing, provide a helper for any future `struct dirent' cloning.
> > >
> > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/841
> > > Cc: qemu-stable@nongnu.org
> > > Co-authored-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > > Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
> > > ---
> > > Tested on x86-64 Linux.
> >
> > I was too optimistic. Looks like this needs more work. With this patch applied
> > the 9p test cases [1] are crashing now:
> >
> > $ gdb --args tests/qtest/qos-test -m slow
> > ...
> > # Start of flush tests
> > ok 50 /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/virtio-9p-tests/synth/flush/success
> > ok 51 /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/virtio-9p-tests/synth/flush/ignored
> > # End of flush tests
> > # Start of readdir tests
>
> I changed implementation from the one using dent->d_reclen to the one using
> strlen(dent->d_name) and it's passed readdir tests, but failed later:
>
> # Start of readdir tests
> ok 53 /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/virtio-9p-tests/synth/readdir/basic
> ok 54 /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/virtio-9p-tests/synth/readdir/split_512
> ok 55 /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/virtio-9p-tests/synth/readdir/split_256
> ok 56 /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/virtio-9p-tests/synth/readdir/split_128
> # End of readdir tests
> # End of synth tests
> # Start of local tests
> # starting QEMU: exec x86_64-softmmu/qemu-system-x86_64 -qtest unix:/tmp/qtest-2822967.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-2822967.qmp,id=char0 -mon chardev=char0,mode=control -display none -M pc -fsdev local,id=fsdev0,path='/usr/src/RPM/BUILD/qemu-6.2.0/build-dynamic/qtest-9p-local-NpcZCR',security_model=mapped-xattr -device virtio-9p-pci,fsdev=fsdev0,addr=04.0,mount_tag=qtest -accel qtest
> # GLib-DEBUG: setenv()/putenv() are not thread-safe and should not be used after threads are created
> ok 57 /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/virtio-9p-tests/local/config
> Received response 7 (RLERROR) instead of 73 (RMKDIR)
> Rlerror has errno 95 (Operation not supported)
> **
> ERROR:../tests/qtest/virtio-9p-test.c:305:v9fs_req_recv: assertion failed (hdr.id == id): (7 == 73)
> Bail out! ERROR:../tests/qtest/virtio-9p-test.c:305:v9fs_req_recv: assertion failed (hdr.id == id): (7 == 73)
> Aborted
I added some debugging output and in the bug case `d_reclen` is 0. Thus
this is not readdir's struct dirent, but something else (test-only
simulated dirent without accounting that we now have d_reclen logic).
This maybe related to the other bug in
static void synth_direntry(V9fsSynthNode *node,
struct dirent *entry, off_t off)
{
strcpy(entry->d_name, node->name);
entry->d_ino = node->attr->inode;
entry->d_off = off + 1;
}
Where `d_reclen` is not updated.
Thanks,
>
> Thanks,
>
> > Broken pipe
> >
> > Thread 1 "qos-test" received signal SIGABRT, Aborted.
> > __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
> > 50 ../sysdeps/unix/sysv/linux/raise.c: No such file or directory.
> > (gdb) bt
> > #0 __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
> > #1 0x00007ffff7b7d537 in __GI_abort () at abort.c:79
> > #2 0x00005555555ba495 in qtest_client_socket_recv_line (s=0x5555557663c0) at ../tests/qtest/libqtest.c:503
> > #3 0x00005555555ba5b3 in qtest_rsp_args (s=0x5555557663c0, expected_args=2) at ../tests/qtest/libqtest.c:523
> > #4 0x00005555555bbdb4 in qtest_clock_rsp (s=0x5555557663c0) at ../tests/qtest/libqtest.c:970
> > #5 0x00005555555bbe55 in qtest_clock_step (s=0x5555557663c0, step=100) at ../tests/qtest/libqtest.c:985
> > #6 0x00005555555cdc21 in qvirtio_wait_used_elem (qts=0x5555557663c0, d=0x555555779b48, vq=0x5555557b0480, desc_idx=8, len=0x0, timeout_us=10000000)
> > at ../tests/qtest/libqos/virtio.c:220
> > #7 0x00005555555ae79f in v9fs_req_wait_for_reply (req=0x5555557899a0, len=0x0) at ../tests/qtest/virtio-9p-test.c:278
> > #8 0x00005555555b03bf in fs_readdir (obj=0x555555779bb0, data=0x0, t_alloc=0x5555557448b8) at ../tests/qtest/virtio-9p-test.c:851
> > #9 0x00005555555990c4 in run_one_test (arg=0x5555557ac600) at ../tests/qtest/qos-test.c:182
> > #10 0x00007ffff7f02b9e in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
> > #11 0x00007ffff7f0299b in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
> > #12 0x00007ffff7f0299b in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
> > #13 0x00007ffff7f0299b in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
> > #14 0x00007ffff7f0299b in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
> > #15 0x00007ffff7f0299b in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
> > #16 0x00007ffff7f0299b in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
> > #17 0x00007ffff7f0299b in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
> > #18 0x00007ffff7f0299b in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
> > #19 0x00007ffff7f0299b in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
> > #20 0x00007ffff7f0299b in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
> > #21 0x00007ffff7f0308a in g_test_run_suite () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
> > #22 0x00007ffff7f030a1 in g_test_run () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
> > #23 0x00005555555995a3 in main (argc=1, argv=0x7fffffffe508, envp=0x7fffffffe528) at ../tests/qtest/qos-test.c:338
> > (gdb)
> >
> > [1] https://wiki.qemu.org/Documentation/9p#Test_Cases
> >
> > Best regards,
> > Christian Schoenebeck
> >
> >
On Donnerstag, 3. Februar 2022 07:20:05 CET Vitaly Chikunov wrote:
> On Thu, Feb 03, 2022 at 07:55:41AM +0300, Vitaly Chikunov wrote:
> > Christian,
> >
> > On Wed, Feb 02, 2022 at 05:55:45PM +0100, Christian Schoenebeck wrote:
> > > On Freitag, 28. Januar 2022 23:33:26 CET Vitaly Chikunov wrote:
> > > > `struct dirent' returned from readdir(3) could be shorter than
> > > > `sizeof(struct dirent)', thus memcpy of sizeof length will overread
> > > >
> > > > into unallocated page causing SIGSEGV. Example stack trace:
> > > > #0 0x00005555559ebeed v9fs_co_readdir_many
> > > > (/usr/bin/qemu-system-x86_64 +
> > > >
> > > > 0x497eed) #1 0x00005555559ec2e9 v9fs_readdir
> > > > (/usr/bin/qemu-system-x86_64
> > > > + 0x4982e9) #2 0x0000555555eb7983 coroutine_trampoline
> > > > (/usr/bin/qemu-system-x86_64 + 0x963983) #3 0x00007ffff73e0be0 n/a
> > > > (n/a +
> > > > 0x0)
> > > >
> > > > While fixing, provide a helper for any future `struct dirent' cloning.
> > > >
> > > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/841
> > > > Cc: qemu-stable@nongnu.org
> > > > Co-authored-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > > > Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
> > > > ---
> > > > Tested on x86-64 Linux.
> > >
> > > I was too optimistic. Looks like this needs more work. With this patch
> > > applied the 9p test cases [1] are crashing now:
> > >
> > > $ gdb --args tests/qtest/qos-test -m slow
> > > ...
> > > # Start of flush tests
> > > ok 50
> > > /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/vi
> > > rtio-9p-tests/synth/flush/success ok 51
> > > /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/vi
> > > rtio-9p-tests/synth/flush/ignored # End of flush tests
> > > # Start of readdir tests
> >
> > I changed implementation from the one using dent->d_reclen to the one
> > using
> >
> > strlen(dent->d_name) and it's passed readdir tests, but failed later:
> > # Start of readdir tests
> > ok 53
> > /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/vi
> > rtio-9p-tests/synth/readdir/basic ok 54
> > /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/vi
> > rtio-9p-tests/synth/readdir/split_512 ok 55
> > /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/vi
> > rtio-9p-tests/synth/readdir/split_256 ok 56
> > /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/vi
> > rtio-9p-tests/synth/readdir/split_128 # End of readdir tests
> > # End of synth tests
> > # Start of local tests
> > # starting QEMU: exec x86_64-softmmu/qemu-system-x86_64 -qtest
> > unix:/tmp/qtest-2822967.sock -qtest-log /dev/null -chardev
> > socket,path=/tmp/qtest-2822967.qmp,id=char0 -mon
> > chardev=char0,mode=control -display none -M pc -fsdev
> > local,id=fsdev0,path='/usr/src/RPM/BUILD/qemu-6.2.0/build-dynamic/qtest
> > -9p-local-NpcZCR',security_model=mapped-xattr -device
> > virtio-9p-pci,fsdev=fsdev0,addr=04.0,mount_tag=qtest -accel qtest #
> > GLib-DEBUG: setenv()/putenv() are not thread-safe and should not be
> > used after threads are created ok 57
> > /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/vi
> > rtio-9p-tests/local/config Received response 7 (RLERROR) instead of 73
> > (RMKDIR)
> > Rlerror has errno 95 (Operation not supported)
> > **
> > ERROR:../tests/qtest/virtio-9p-test.c:305:v9fs_req_recv: assertion
> > failed (hdr.id == id): (7 == 73) Bail out!
> > ERROR:../tests/qtest/virtio-9p-test.c:305:v9fs_req_recv: assertion
> > failed (hdr.id == id): (7 == 73) Aborted
>
> I added some debugging output and in the bug case `d_reclen` is 0. Thus
> this is not readdir's struct dirent, but something else (test-only
> simulated dirent without accounting that we now have d_reclen logic).
>
> This maybe related to the other bug in
>
> static void synth_direntry(V9fsSynthNode *node,
> struct dirent *entry, off_t off)
> {
> strcpy(entry->d_name, node->name);
> entry->d_ino = node->attr->inode;
> entry->d_off = off + 1;
> }
>
> Where `d_reclen` is not updated.
The synth driver (used by the 'synth' 9p tests) intentionally just simulates a
filesystem. The synth driver does not call any real fs syscalls, it just has
its own very simple in-RAM-only structures that are used to simulate a fs and
therefore the synth driver populates the dirent structure by itself.
Could you try to resolve this issue in the synth driver and send a v3 of this
patch? I am currently busy with other tasks right now.
Best regards,
Christian Schoenebeck
On Freitag, 28. Januar 2022 23:33:26 CET Vitaly Chikunov wrote:
> `struct dirent' returned from readdir(3) could be shorter than
> `sizeof(struct dirent)', thus memcpy of sizeof length will overread
> into unallocated page causing SIGSEGV. Example stack trace:
>
> #0 0x00005555559ebeed v9fs_co_readdir_many (/usr/bin/qemu-system-x86_64 +
> 0x497eed) #1 0x00005555559ec2e9 v9fs_readdir (/usr/bin/qemu-system-x86_64
> + 0x4982e9) #2 0x0000555555eb7983 coroutine_trampoline
> (/usr/bin/qemu-system-x86_64 + 0x963983) #3 0x00007ffff73e0be0 n/a (n/a +
> 0x0)
>
> While fixing, provide a helper for any future `struct dirent' cloning.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/841
> Cc: qemu-stable@nongnu.org
> Co-authored-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
> ---
> Tested on x86-64 Linux.
>
> Changes since v1:
> - Update commentary text.
> - Remove hanging of g_malloc "errors".
> - Simplify qemu_dirent_dup.
>
> hw/9pfs/codir.c | 3 +--
> include/qemu/osdep.h | 13 +++++++++++++
> util/osdep.c | 16 ++++++++++++++++
> 3 files changed, 30 insertions(+), 2 deletions(-)
>
> diff --git a/hw/9pfs/codir.c b/hw/9pfs/codir.c
> index 032cce04c4..c0873bde16 100644
> --- a/hw/9pfs/codir.c
> +++ b/hw/9pfs/codir.c
> @@ -143,8 +143,7 @@ static int do_readdir_many(V9fsPDU *pdu, V9fsFidState
> *fidp, } else {
> e = e->next = g_malloc0(sizeof(V9fsDirEnt));
> }
> - e->dent = g_malloc0(sizeof(struct dirent));
> - memcpy(e->dent, dent, sizeof(struct dirent));
> + e->dent = qemu_dirent_dup(dent);
>
> /* perform a full stat() for directory entry if requested by caller
> */ if (dostat) {
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index d1660d67fa..ce12f64853 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -805,6 +805,19 @@ static inline int
> platform_does_not_support_system(const char *command) }
> #endif /* !HAVE_SYSTEM_FUNCTION */
>
> +/**
> + * Duplicate directory entry @dent.
> + *
> + * It is highly recommended to use this function instead of open coding
> + * duplication of @c dirent objects, because the actual @c struct @c dirent
> + * size may be bigger or shorter than @c sizeof(struct dirent) and correct
> + * handling is platform specific (see gitlab issue #841).
> + *
> + * @dent - original directory entry to be duplicated
> + * @returns duplicated directory entry which should be freed with g_free()
> + */
> +struct dirent *qemu_dirent_dup(struct dirent *dent);
> +
> #ifdef __cplusplus
> }
> #endif
> diff --git a/util/osdep.c b/util/osdep.c
> index 42a0a4986a..0bc7ec1e22 100644
> --- a/util/osdep.c
> +++ b/util/osdep.c
> @@ -33,6 +33,7 @@
> extern int madvise(char *, size_t, int);
> #endif
>
> +#include <dirent.h>
> #include "qemu-common.h"
> #include "qemu/cutils.h"
> #include "qemu/sockets.h"
> @@ -615,3 +616,18 @@ writev(int fd, const struct iovec *iov, int iov_cnt)
> return readv_writev(fd, iov, iov_cnt, true);
> }
> #endif
> +
> +struct dirent *
> +qemu_dirent_dup(struct dirent *dent)
> +{
> +#if defined _DIRENT_HAVE_D_RECLEN
> + /* Avoid use of strlen() if there's d_reclen. */
> + const size_t sz = dent->d_reclen;
> +#else
> + /* Fallback to a most portable way. */
> + const size_t sz = offsetof(struct dirent, d_name) +
> + strlen(dent->d_name) + 1;
> +#endif
From the experience we just made, I would add in v3 something like
assert(sz > 0);
here.
> + struct dirent *dst = g_malloc(sz);
> + return memcpy(dst, dent, sz);
> +}
Best regards,
Christian Schoenebeck
Christian,
On Thu, Feb 03, 2022 at 01:42:19PM +0100, Christian Schoenebeck wrote:
> On Freitag, 28. Januar 2022 23:33:26 CET Vitaly Chikunov wrote:
> > `struct dirent' returned from readdir(3) could be shorter than
> > `sizeof(struct dirent)', thus memcpy of sizeof length will overread
> > into unallocated page causing SIGSEGV. Example stack trace:
> >
> > #0 0x00005555559ebeed v9fs_co_readdir_many (/usr/bin/qemu-system-x86_64 +
> > 0x497eed) #1 0x00005555559ec2e9 v9fs_readdir (/usr/bin/qemu-system-x86_64
> > + 0x4982e9) #2 0x0000555555eb7983 coroutine_trampoline
> > (/usr/bin/qemu-system-x86_64 + 0x963983) #3 0x00007ffff73e0be0 n/a (n/a +
> > 0x0)
> >
> > While fixing, provide a helper for any future `struct dirent' cloning.
> >
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/841
> > Cc: qemu-stable@nongnu.org
> > Co-authored-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
> > ---
> > Tested on x86-64 Linux.
> >
> > Changes since v1:
> > - Update commentary text.
> > - Remove hanging of g_malloc "errors".
> > - Simplify qemu_dirent_dup.
> >
> > hw/9pfs/codir.c | 3 +--
> > include/qemu/osdep.h | 13 +++++++++++++
> > util/osdep.c | 16 ++++++++++++++++
> > 3 files changed, 30 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/9pfs/codir.c b/hw/9pfs/codir.c
> > index 032cce04c4..c0873bde16 100644
> > --- a/hw/9pfs/codir.c
> > +++ b/hw/9pfs/codir.c
> > @@ -143,8 +143,7 @@ static int do_readdir_many(V9fsPDU *pdu, V9fsFidState
> > *fidp, } else {
> > e = e->next = g_malloc0(sizeof(V9fsDirEnt));
> > }
> > - e->dent = g_malloc0(sizeof(struct dirent));
> > - memcpy(e->dent, dent, sizeof(struct dirent));
> > + e->dent = qemu_dirent_dup(dent);
> >
> > /* perform a full stat() for directory entry if requested by caller
> > */ if (dostat) {
> > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> > index d1660d67fa..ce12f64853 100644
> > --- a/include/qemu/osdep.h
> > +++ b/include/qemu/osdep.h
> > @@ -805,6 +805,19 @@ static inline int
> > platform_does_not_support_system(const char *command) }
> > #endif /* !HAVE_SYSTEM_FUNCTION */
> >
> > +/**
> > + * Duplicate directory entry @dent.
> > + *
> > + * It is highly recommended to use this function instead of open coding
> > + * duplication of @c dirent objects, because the actual @c struct @c dirent
> > + * size may be bigger or shorter than @c sizeof(struct dirent) and correct
> > + * handling is platform specific (see gitlab issue #841).
> > + *
> > + * @dent - original directory entry to be duplicated
> > + * @returns duplicated directory entry which should be freed with g_free()
> > + */
> > +struct dirent *qemu_dirent_dup(struct dirent *dent);
> > +
> > #ifdef __cplusplus
> > }
> > #endif
> > diff --git a/util/osdep.c b/util/osdep.c
> > index 42a0a4986a..0bc7ec1e22 100644
> > --- a/util/osdep.c
> > +++ b/util/osdep.c
> > @@ -33,6 +33,7 @@
> > extern int madvise(char *, size_t, int);
> > #endif
> >
> > +#include <dirent.h>
> > #include "qemu-common.h"
> > #include "qemu/cutils.h"
> > #include "qemu/sockets.h"
> > @@ -615,3 +616,18 @@ writev(int fd, const struct iovec *iov, int iov_cnt)
> > return readv_writev(fd, iov, iov_cnt, true);
> > }
> > #endif
> > +
> > +struct dirent *
> > +qemu_dirent_dup(struct dirent *dent)
> > +{
> > +#if defined _DIRENT_HAVE_D_RECLEN
> > + /* Avoid use of strlen() if there's d_reclen. */
> > + const size_t sz = dent->d_reclen;
> > +#else
> > + /* Fallback to a most portable way. */
> > + const size_t sz = offsetof(struct dirent, d_name) +
> > + strlen(dent->d_name) + 1;
> > +#endif
>
> >From the experience we just made, I would add in v3 something like
>
> assert(sz > 0);
>
> here.
Yes but this will cause another abort() call. I am thinking about v3 fix
like this:
struct dirent *
qemu_dirent_dup(struct dirent *dent)
{
size_t sz = 0;
#if defined _DIRENT_HAVE_D_RECLEN
/* Avoid use of strlen() if there's d_reclen. */
sz = dent->d_reclen;
#endif
if (sz == 0) {
/* Fallback to a most portable way. */
sz = offsetof(struct dirent, d_name) +
strlen(dent->d_name) + 1;
}
struct dirent *dst = g_malloc(sz);
return memcpy(dst, dent, sz);
}
Thus it will use strlen for simulated dirents and d_reclen for real ones
(on Linux).
Thanks,
>
> > + struct dirent *dst = g_malloc(sz);
> > + return memcpy(dst, dent, sz);
> > +}
>
> Best regards,
> Christian Schoenebeck
>
On Fri, Feb 04, 2022 at 03:15:16AM +0300, Vitaly Chikunov wrote:
[...]
> Yes but this will cause another abort() call. I am thinking about v3 fix
> like this:
>
> struct dirent *
> qemu_dirent_dup(struct dirent *dent)
> {
> size_t sz = 0;
> #if defined _DIRENT_HAVE_D_RECLEN
> /* Avoid use of strlen() if there's d_reclen. */
> sz = dent->d_reclen;
> #endif
> if (sz == 0) {
> /* Fallback to the most portable way. */
> sz = offsetof(struct dirent, d_name) +
> strlen(dent->d_name) + 1;
> }
> struct dirent *dst = g_malloc(sz);
> return memcpy(dst, dent, sz);
> }
>
> Thus it will use strlen for simulated dirents and d_reclen for real ones
Makes sense.
--
ldv
On Freitag, 4. Februar 2022 01:22:38 CET Dmitry V. Levin wrote:
> On Fri, Feb 04, 2022 at 03:15:16AM +0300, Vitaly Chikunov wrote:
> [...]
>
> > Yes but this will cause another abort() call. I am thinking about v3 fix
> >
> > like this:
> > struct dirent *
> > qemu_dirent_dup(struct dirent *dent)
> > {
> >
> > size_t sz = 0;
> >
> > #if defined _DIRENT_HAVE_D_RECLEN
> >
> > /* Avoid use of strlen() if there's d_reclen. */
> > sz = dent->d_reclen;
> >
> > #endif
> >
> > if (sz == 0) {
> >
> > /* Fallback to the most portable way. */
> > sz = offsetof(struct dirent, d_name) +
> >
> > strlen(dent->d_name) + 1;
> >
> > }
> > struct dirent *dst = g_malloc(sz);
> > return memcpy(dst, dent, sz);
> >
> > }
> >
> > Thus it will use strlen for simulated dirents and d_reclen for real ones
>
> Makes sense.
Then maybe consider to leave your reviewed-by tag on today's v3 Dmitry,
thanks! :)
Best regards,
Christian Schoenebeck
© 2016 - 2024 Red Hat, Inc.