* Re: [PATCH v3] 9pfs: Fix segfault in do_readdir_many caused by struct dirent overread
2022-02-04 5:06 [PATCH v3] 9pfs: Fix segfault in do_readdir_many caused by struct dirent overread Vitaly Chikunov
@ 2022-02-04 12:08 ` Christian Schoenebeck
2022-02-04 12:15 ` Dmitry V. Levin
` (2 subsequent siblings)
3 siblings, 0 replies; 18+ messages in thread
From: Christian Schoenebeck @ 2022-02-04 12:08 UTC (permalink / raw)
To: qemu-devel; +Cc: Vitaly Chikunov, Greg Kurz, qemu-stable, ldv
On Freitag, 4. Februar 2022 06:06:09 CET Vitaly Chikunov wrote:
> `struct dirent' returned from readdir(3) could be shorter (or longer)
> 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 again.
Tests are passing now, no other issues encountered during some quick tests on
my side. So looks good to me, thanks!
Reviewed-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
Best regards,
Christian Schoenebeck
>
> Changes from v2:
> - Make it work with a simulated dirent where d_reclen is 0, which was
> caused abort in readdir qos-test, by using fallback at runtime.
>
> hw/9pfs/codir.c | 3 +--
> include/qemu/osdep.h | 13 +++++++++++++
> util/osdep.c | 18 ++++++++++++++++++
> 3 files changed, 32 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..2c80528a61 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,20 @@ 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)
> +{
> + 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);
> +}
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v3] 9pfs: Fix segfault in do_readdir_many caused by struct dirent overread
2022-02-04 5:06 [PATCH v3] 9pfs: Fix segfault in do_readdir_many caused by struct dirent overread Vitaly Chikunov
2022-02-04 12:08 ` Christian Schoenebeck
@ 2022-02-04 12:15 ` Dmitry V. Levin
2022-02-04 12:17 ` Dmitry V. Levin
2022-02-04 13:55 ` Philippe Mathieu-Daudé via
2022-02-05 11:36 ` Christian Schoenebeck
3 siblings, 1 reply; 18+ messages in thread
From: Dmitry V. Levin @ 2022-02-04 12:15 UTC (permalink / raw)
To: Vitaly Chikunov; +Cc: qemu-stable, Christian Schoenebeck, Greg Kurz, qemu-devel
On Fri, Feb 04, 2022 at 08:06:09AM +0300, Vitaly Chikunov wrote:
> `struct dirent' returned from readdir(3) could be shorter (or longer)
> 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 again.
>
> Changes from v2:
> - Make it work with a simulated dirent where d_reclen is 0, which was
> caused abort in readdir qos-test, by using fallback at runtime.
>
> hw/9pfs/codir.c | 3 +--
> include/qemu/osdep.h | 13 +++++++++++++
> util/osdep.c | 18 ++++++++++++++++++
> 3 files changed, 32 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..2c80528a61 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,20 @@ 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)
> +{
> + 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);
> +}
Reviewed-by: Dmitry V. Levin" <ldv@altlinux.org>
--
ldv
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v3] 9pfs: Fix segfault in do_readdir_many caused by struct dirent overread
2022-02-04 12:15 ` Dmitry V. Levin
@ 2022-02-04 12:17 ` Dmitry V. Levin
0 siblings, 0 replies; 18+ messages in thread
From: Dmitry V. Levin @ 2022-02-04 12:17 UTC (permalink / raw)
To: Vitaly Chikunov; +Cc: qemu-stable, Christian Schoenebeck, Greg Kurz, qemu-devel
On Fri, Feb 04, 2022 at 03:15:38PM +0300, Dmitry V. Levin wrote:
> On Fri, Feb 04, 2022 at 08:06:09AM +0300, Vitaly Chikunov wrote:
> > `struct dirent' returned from readdir(3) could be shorter (or longer)
> > 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 again.
> >
> > Changes from v2:
> > - Make it work with a simulated dirent where d_reclen is 0, which was
> > caused abort in readdir qos-test, by using fallback at runtime.
> >
> > hw/9pfs/codir.c | 3 +--
> > include/qemu/osdep.h | 13 +++++++++++++
> > util/osdep.c | 18 ++++++++++++++++++
> > 3 files changed, 32 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..2c80528a61 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,20 @@ 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)
> > +{
> > + 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);
> > +}
>
> Reviewed-by: Dmitry V. Levin" <ldv@altlinux.org>
Reviewed-by: Dmitry V. Levin <ldv@altlinux.org>
There should be no '"', sorry about that. :)
--
ldv
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3] 9pfs: Fix segfault in do_readdir_many caused by struct dirent overread
2022-02-04 5:06 [PATCH v3] 9pfs: Fix segfault in do_readdir_many caused by struct dirent overread Vitaly Chikunov
2022-02-04 12:08 ` Christian Schoenebeck
2022-02-04 12:15 ` Dmitry V. Levin
@ 2022-02-04 13:55 ` Philippe Mathieu-Daudé via
2022-02-04 14:12 ` Christian Schoenebeck
2022-02-04 16:19 ` Dmitry V. Levin
2022-02-05 11:36 ` Christian Schoenebeck
3 siblings, 2 replies; 18+ messages in thread
From: Philippe Mathieu-Daudé via @ 2022-02-04 13:55 UTC (permalink / raw)
To: Vitaly Chikunov, Greg Kurz, Christian Schoenebeck, qemu-devel
Cc: qemu-stable, ldv
On 4/2/22 06:06, Vitaly Chikunov wrote:
> `struct dirent' returned from readdir(3) could be shorter (or longer)
> 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 again.
>
> Changes from v2:
> - Make it work with a simulated dirent where d_reclen is 0, which was
> caused abort in readdir qos-test, by using fallback at runtime.
>
> hw/9pfs/codir.c | 3 +--
> include/qemu/osdep.h | 13 +++++++++++++
> util/osdep.c | 18 ++++++++++++++++++
> 3 files changed, 32 insertions(+), 2 deletions(-)
> +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) {
If _DIRENT_HAVE_D_RECLEN is defined, this case is unlikely...
> + /* 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);
> +}
What about this?
struct dirent *
qemu_dirent_dup(struct dirent *dent)
{
size_t sz;
#if defined _DIRENT_HAVE_D_RECLEN
/* Avoid use of strlen() if there's d_reclen. */
sz = dent->d_reclen;
#else
/* Fallback to the most portable way. */
sz = offsetof(struct dirent, d_name) +
strlen(dent->d_name) + 1;
#endif
return g_memdup(dent, sz);
}
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v3] 9pfs: Fix segfault in do_readdir_many caused by struct dirent overread
2022-02-04 13:55 ` Philippe Mathieu-Daudé via
@ 2022-02-04 14:12 ` Christian Schoenebeck
2022-02-04 15:16 ` Greg Kurz
2022-02-04 16:19 ` Dmitry V. Levin
1 sibling, 1 reply; 18+ messages in thread
From: Christian Schoenebeck @ 2022-02-04 14:12 UTC (permalink / raw)
To: qemu-devel, Philippe Mathieu-Daudé
Cc: Vitaly Chikunov, Greg Kurz, qemu-stable, ldv
On Freitag, 4. Februar 2022 14:55:45 CET Philippe Mathieu-Daudé via wrote:
> On 4/2/22 06:06, Vitaly Chikunov wrote:
> > `struct dirent' returned from readdir(3) could be shorter (or longer)
> > 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 again.
> >
> > Changes from v2:
> > - Make it work with a simulated dirent where d_reclen is 0, which was
> >
> > caused abort in readdir qos-test, by using fallback at runtime.
> >
> > hw/9pfs/codir.c | 3 +--
> > include/qemu/osdep.h | 13 +++++++++++++
> > util/osdep.c | 18 ++++++++++++++++++
> > 3 files changed, 32 insertions(+), 2 deletions(-)
> >
> > +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) {
>
> If _DIRENT_HAVE_D_RECLEN is defined, this case is unlikely...
>
> > + /* 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);
> > +}
>
> What about this?
>
> struct dirent *
> qemu_dirent_dup(struct dirent *dent)
> {
> size_t sz;
>
> #if defined _DIRENT_HAVE_D_RECLEN
> /* Avoid use of strlen() if there's d_reclen. */
> sz = dent->d_reclen;
> #else
> /* Fallback to the most portable way. */
> sz = offsetof(struct dirent, d_name) +
> strlen(dent->d_name) + 1;
> #endif
>
> return g_memdup(dent, sz);
> }
That was the intentional change v2 -> v3 Philippe. Previous v2 crashed the
9p 'synth' tests:
https://lore.kernel.org/qemu-devel/2627488.0S70g7mNYN@silver/T/#ma09bedde59a91e6435443e151f7e49fef8616e4c
You might argue that the 9p 'synth' driver should instead populate d_reclen
instead, but this v3 is a simpler solution and guarantees to always work. So
I'd prefer to go with Vitaly's v3 for now, especially as this patch would go
to the stable branches as well.
Best regards,
Christian Schoenebeck
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v3] 9pfs: Fix segfault in do_readdir_many caused by struct dirent overread
2022-02-04 14:12 ` Christian Schoenebeck
@ 2022-02-04 15:16 ` Greg Kurz
2022-02-04 15:32 ` Vitaly Chikunov
2022-02-04 15:33 ` Christian Schoenebeck
0 siblings, 2 replies; 18+ messages in thread
From: Greg Kurz @ 2022-02-04 15:16 UTC (permalink / raw)
To: Christian Schoenebeck
Cc: Vitaly Chikunov, qemu-stable, ldv, qemu-devel,
Philippe Mathieu-Daudé
On Fri, 04 Feb 2022 15:12:18 +0100
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> On Freitag, 4. Februar 2022 14:55:45 CET Philippe Mathieu-Daudé via wrote:
> > On 4/2/22 06:06, Vitaly Chikunov wrote:
> > > `struct dirent' returned from readdir(3) could be shorter (or longer)
> > > 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 again.
> > >
> > > Changes from v2:
> > > - Make it work with a simulated dirent where d_reclen is 0, which was
> > >
> > > caused abort in readdir qos-test, by using fallback at runtime.
> > >
> > > hw/9pfs/codir.c | 3 +--
> > > include/qemu/osdep.h | 13 +++++++++++++
> > > util/osdep.c | 18 ++++++++++++++++++
> > > 3 files changed, 32 insertions(+), 2 deletions(-)
> > >
> > > +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) {
> >
> > If _DIRENT_HAVE_D_RECLEN is defined, this case is unlikely...
> >
> > > + /* 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);
> > > +}
> >
> > What about this?
> >
> > struct dirent *
> > qemu_dirent_dup(struct dirent *dent)
> > {
> > size_t sz;
> >
> > #if defined _DIRENT_HAVE_D_RECLEN
> > /* Avoid use of strlen() if there's d_reclen. */
> > sz = dent->d_reclen;
> > #else
> > /* Fallback to the most portable way. */
> > sz = offsetof(struct dirent, d_name) +
> > strlen(dent->d_name) + 1;
> > #endif
> >
> > return g_memdup(dent, sz);
> > }
>
> That was the intentional change v2 -> v3 Philippe. Previous v2 crashed the
> 9p 'synth' tests:
>
> https://lore.kernel.org/qemu-devel/2627488.0S70g7mNYN@silver/T/#ma09bedde59a91e6435443e151f7e49fef8616e4c
>
> You might argue that the 9p 'synth' driver should instead populate d_reclen
> instead, but this v3 is a simpler solution and guarantees to always work. So
> I'd prefer to go with Vitaly's v3 for now, especially as this patch would go
> to the stable branches as well.
>
This really is a band-aid. Anyone reading this will react as Philippe,
and this is common code, not just 9pfs. With correctly populated dirents,
the helper could be as simple as:
struct dirent *
qemu_dirent_dup(struct dirent *dent)
{
size_t sz = offsetof(struct dirent, d_name) + _D_EXACT_NAMLEN(dent) + 1;
return g_memdup(dent, sz);
}
If this is a cycles problem, I can help reviewing the fixes on
the synth driver, or alternatively you can keep this code
somewhere under 9pfs but please don't put that in common utils.
Cheers,
--
Greg
> Best regards,
> Christian Schoenebeck
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v3] 9pfs: Fix segfault in do_readdir_many caused by struct dirent overread
2022-02-04 15:16 ` Greg Kurz
@ 2022-02-04 15:32 ` Vitaly Chikunov
2022-02-04 15:50 ` Dmitry V. Levin
2022-02-04 15:33 ` Christian Schoenebeck
1 sibling, 1 reply; 18+ messages in thread
From: Vitaly Chikunov @ 2022-02-04 15:32 UTC (permalink / raw)
To: Greg Kurz
Cc: qemu-stable, Dmitry V. Levin, Christian Schoenebeck, qemu-devel,
Philippe Mathieu-Daudé
Greg,
On Fri, Feb 04, 2022 at 04:16:06PM +0100, Greg Kurz wrote:
> On Fri, 04 Feb 2022 15:12:18 +0100
> Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
>
> > On Freitag, 4. Februar 2022 14:55:45 CET Philippe Mathieu-Daudé via wrote:
> > > On 4/2/22 06:06, Vitaly Chikunov wrote:
> > > > `struct dirent' returned from readdir(3) could be shorter (or longer)
> > > > 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 again.
> > > >
> > > > Changes from v2:
> > > > - Make it work with a simulated dirent where d_reclen is 0, which was
> > > >
> > > > caused abort in readdir qos-test, by using fallback at runtime.
> > > >
> > > > hw/9pfs/codir.c | 3 +--
> > > > include/qemu/osdep.h | 13 +++++++++++++
> > > > util/osdep.c | 18 ++++++++++++++++++
> > > > 3 files changed, 32 insertions(+), 2 deletions(-)
> > > >
> > > > +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) {
> > >
> > > If _DIRENT_HAVE_D_RECLEN is defined, this case is unlikely...
> > >
> > > > + /* 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);
> > > > +}
> > >
> > > What about this?
> > >
> > > struct dirent *
> > > qemu_dirent_dup(struct dirent *dent)
> > > {
> > > size_t sz;
> > >
> > > #if defined _DIRENT_HAVE_D_RECLEN
> > > /* Avoid use of strlen() if there's d_reclen. */
> > > sz = dent->d_reclen;
> > > #else
> > > /* Fallback to the most portable way. */
> > > sz = offsetof(struct dirent, d_name) +
> > > strlen(dent->d_name) + 1;
> > > #endif
> > >
> > > return g_memdup(dent, sz);
> > > }
> >
> > That was the intentional change v2 -> v3 Philippe. Previous v2 crashed the
> > 9p 'synth' tests:
> >
> > https://lore.kernel.org/qemu-devel/2627488.0S70g7mNYN@silver/T/#ma09bedde59a91e6435443e151f7e49fef8616e4c
> >
> > You might argue that the 9p 'synth' driver should instead populate d_reclen
> > instead, but this v3 is a simpler solution and guarantees to always work. So
> > I'd prefer to go with Vitaly's v3 for now, especially as this patch would go
> > to the stable branches as well.
> >
>
> This really is a band-aid. Anyone reading this will react as Philippe,
> and this is common code, not just 9pfs. With correctly populated dirents,
> the helper could be as simple as:
>
> struct dirent *
> qemu_dirent_dup(struct dirent *dent)
> {
> size_t sz = offsetof(struct dirent, d_name) + _D_EXACT_NAMLEN(dent) + 1;
But d_namlen is not populated by synth_direntry, so this will lead to
a bug too. Idea is that qemu_dirent_dup handles real dirents and
simulated (underpopulated) dirents.
Also Linux does not have d_namlen AFAIK, thus this code will not provide
any speed up in most cases (and always fallback to strlen), unlike if we
use d_reclen.
Also, I m not sure if _D_EXACT_NAMLEN is defined on all systems, so this
needs ifdefs too.
>
> return g_memdup(dent, sz);
> }
>
> If this is a cycles problem,
If you don't like d_reclen speed ups, we can always just use most portable
strlen method.
Vitaly,
> I can help reviewing the fixes on
> the synth driver, or alternatively you can keep this code
> somewhere under 9pfs but please don't put that in common utils.
>
> Cheers,
>
> --
> Greg
>
> > Best regards,
> > Christian Schoenebeck
> >
> >
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v3] 9pfs: Fix segfault in do_readdir_many caused by struct dirent overread
2022-02-04 15:32 ` Vitaly Chikunov
@ 2022-02-04 15:50 ` Dmitry V. Levin
2022-02-04 15:54 ` Philippe Mathieu-Daudé via
0 siblings, 1 reply; 18+ messages in thread
From: Dmitry V. Levin @ 2022-02-04 15:50 UTC (permalink / raw)
To: Vitaly Chikunov, Greg Kurz
Cc: qemu-stable, Christian Schoenebeck, Philippe Mathieu-Daudé,
qemu-devel
On Fri, Feb 04, 2022 at 06:32:07PM +0300, Vitaly Chikunov wrote:
[...]
> > struct dirent *
> > qemu_dirent_dup(struct dirent *dent)
> > {
> > size_t sz = offsetof(struct dirent, d_name) + _D_EXACT_NAMLEN(dent) + 1;
>
> But d_namlen is not populated by synth_direntry, so this will lead to
> a bug too. Idea is that qemu_dirent_dup handles real dirents and
> simulated (underpopulated) dirents.
>
> Also Linux does not have d_namlen AFAIK, thus this code will not provide
> any speed up in most cases (and always fallback to strlen), unlike if we
> use d_reclen.
>
> Also, I m not sure if _D_EXACT_NAMLEN is defined on all systems, so this
> needs ifdefs too.
Yes, _D_EXACT_NAMLEN() is a GNU extension, it was introduced in glibc
back in 1996 but some popular libcs available for Linux do not provide
this macro.
--
ldv
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v3] 9pfs: Fix segfault in do_readdir_many caused by struct dirent overread
2022-02-04 15:50 ` Dmitry V. Levin
@ 2022-02-04 15:54 ` Philippe Mathieu-Daudé via
2022-02-04 16:04 ` Christian Schoenebeck
0 siblings, 1 reply; 18+ messages in thread
From: Philippe Mathieu-Daudé via @ 2022-02-04 15:54 UTC (permalink / raw)
To: Dmitry V. Levin, Vitaly Chikunov, Greg Kurz
Cc: Christian Schoenebeck, qemu-devel, qemu-stable
On 4/2/22 16:50, Dmitry V. Levin wrote:
> On Fri, Feb 04, 2022 at 06:32:07PM +0300, Vitaly Chikunov wrote:
> [...]
>>> struct dirent *
>>> qemu_dirent_dup(struct dirent *dent)
>>> {
>>> size_t sz = offsetof(struct dirent, d_name) + _D_EXACT_NAMLEN(dent) + 1;
>>
>> But d_namlen is not populated by synth_direntry, so this will lead to
>> a bug too. Idea is that qemu_dirent_dup handles real dirents and
>> simulated (underpopulated) dirents.
>>
>> Also Linux does not have d_namlen AFAIK, thus this code will not provide
>> any speed up in most cases (and always fallback to strlen), unlike if we
>> use d_reclen.
>>
>> Also, I m not sure if _D_EXACT_NAMLEN is defined on all systems, so this
>> needs ifdefs too.
>
> Yes, _D_EXACT_NAMLEN() is a GNU extension, it was introduced in glibc
> back in 1996 but some popular libcs available for Linux do not provide
> this macro.
Can't we define _D_EXACT_NAMLEN() if not available?
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v3] 9pfs: Fix segfault in do_readdir_many caused by struct dirent overread
2022-02-04 15:54 ` Philippe Mathieu-Daudé via
@ 2022-02-04 16:04 ` Christian Schoenebeck
2022-02-04 19:31 ` Philippe Mathieu-Daudé via
0 siblings, 1 reply; 18+ messages in thread
From: Christian Schoenebeck @ 2022-02-04 16:04 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Dmitry V. Levin, Vitaly Chikunov, Greg Kurz, qemu-devel, qemu-stable
On Freitag, 4. Februar 2022 16:54:12 CET Philippe Mathieu-Daudé wrote:
> On 4/2/22 16:50, Dmitry V. Levin wrote:
> > On Fri, Feb 04, 2022 at 06:32:07PM +0300, Vitaly Chikunov wrote:
> > [...]
> >
> >>> struct dirent *
> >>> qemu_dirent_dup(struct dirent *dent)
> >>> {
> >>>
> >>> size_t sz = offsetof(struct dirent, d_name) + _D_EXACT_NAMLEN(dent)
> >>> + 1;
> >>
> >> But d_namlen is not populated by synth_direntry, so this will lead to
> >> a bug too. Idea is that qemu_dirent_dup handles real dirents and
> >> simulated (underpopulated) dirents.
> >>
> >> Also Linux does not have d_namlen AFAIK, thus this code will not provide
> >> any speed up in most cases (and always fallback to strlen), unlike if we
> >> use d_reclen.
> >>
> >> Also, I m not sure if _D_EXACT_NAMLEN is defined on all systems, so this
> >> needs ifdefs too.
> >
> > Yes, _D_EXACT_NAMLEN() is a GNU extension, it was introduced in glibc
> > back in 1996 but some popular libcs available for Linux do not provide
> > this macro.
>
> Can't we define _D_EXACT_NAMLEN() if not available?
It is not that trivial.
With recent macOS patch set in mind: macOS does not have any of these macros
either. It does have d_namlen and d_reclen though. Keep in mind though that
macOS also has d_seekoff which is almost always zero though.
So please, don't blindly define something, test it! On doubt I stick with
Vitaly's solution, because it just works^TM.
On the long term we can still adjust this to make all people happy, but this
is about fixing a crash, so I am fine with what Greg called "band-aid".
Best regards,
Christian Schoenebeck
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v3] 9pfs: Fix segfault in do_readdir_many caused by struct dirent overread
2022-02-04 16:04 ` Christian Schoenebeck
@ 2022-02-04 19:31 ` Philippe Mathieu-Daudé via
0 siblings, 0 replies; 18+ messages in thread
From: Philippe Mathieu-Daudé via @ 2022-02-04 19:31 UTC (permalink / raw)
To: Christian Schoenebeck
Cc: Dmitry V. Levin, Vitaly Chikunov, Greg Kurz, qemu-devel, qemu-stable
On 4/2/22 17:04, Christian Schoenebeck wrote:
> On Freitag, 4. Februar 2022 16:54:12 CET Philippe Mathieu-Daudé wrote:
>> On 4/2/22 16:50, Dmitry V. Levin wrote:
>>> On Fri, Feb 04, 2022 at 06:32:07PM +0300, Vitaly Chikunov wrote:
>>> [...]
>>>
>>>>> struct dirent *
>>>>> qemu_dirent_dup(struct dirent *dent)
>>>>> {
>>>>>
>>>>> size_t sz = offsetof(struct dirent, d_name) + _D_EXACT_NAMLEN(dent)
>>>>> + 1;
>>>>
>>>> But d_namlen is not populated by synth_direntry, so this will lead to
>>>> a bug too. Idea is that qemu_dirent_dup handles real dirents and
>>>> simulated (underpopulated) dirents.
>>>>
>>>> Also Linux does not have d_namlen AFAIK, thus this code will not provide
>>>> any speed up in most cases (and always fallback to strlen), unlike if we
>>>> use d_reclen.
>>>>
>>>> Also, I m not sure if _D_EXACT_NAMLEN is defined on all systems, so this
>>>> needs ifdefs too.
>>>
>>> Yes, _D_EXACT_NAMLEN() is a GNU extension, it was introduced in glibc
>>> back in 1996 but some popular libcs available for Linux do not provide
>>> this macro.
>>
>> Can't we define _D_EXACT_NAMLEN() if not available?
>
> It is not that trivial.
>
> With recent macOS patch set in mind: macOS does not have any of these macros
> either. It does have d_namlen and d_reclen though. Keep in mind though that
> macOS also has d_seekoff which is almost always zero though.
>
> So please, don't blindly define something, test it! On doubt I stick with
> Vitaly's solution, because it just works^TM.
Note I haven't NAck'ed this approach, I am simply looking at a better
alternative if possible.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3] 9pfs: Fix segfault in do_readdir_many caused by struct dirent overread
2022-02-04 15:16 ` Greg Kurz
2022-02-04 15:32 ` Vitaly Chikunov
@ 2022-02-04 15:33 ` Christian Schoenebeck
1 sibling, 0 replies; 18+ messages in thread
From: Christian Schoenebeck @ 2022-02-04 15:33 UTC (permalink / raw)
To: Greg Kurz
Cc: qemu-devel, Philippe Mathieu-Daudé,
Vitaly Chikunov, qemu-stable, ldv
On Freitag, 4. Februar 2022 16:16:06 CET Greg Kurz wrote:
> On Fri, 04 Feb 2022 15:12:18 +0100
>
> Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > On Freitag, 4. Februar 2022 14:55:45 CET Philippe Mathieu-Daudé via wrote:
> > > On 4/2/22 06:06, Vitaly Chikunov wrote:
> > > > `struct dirent' returned from readdir(3) could be shorter (or longer)
> > > > 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 again.
> > > >
> > > > Changes from v2:
> > > > - Make it work with a simulated dirent where d_reclen is 0, which was
> > > >
> > > > caused abort in readdir qos-test, by using fallback at runtime.
> > > >
> > > > hw/9pfs/codir.c | 3 +--
> > > > include/qemu/osdep.h | 13 +++++++++++++
> > > > util/osdep.c | 18 ++++++++++++++++++
> > > > 3 files changed, 32 insertions(+), 2 deletions(-)
> > > >
> > > > +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) {
> > >
> > > If _DIRENT_HAVE_D_RECLEN is defined, this case is unlikely...
> > >
> > > > + /* 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);
> > > > +}
> > >
> > > What about this?
> > >
> > > struct dirent *
> > > qemu_dirent_dup(struct dirent *dent)
> > > {
> > >
> > > size_t sz;
> > >
> > > #if defined _DIRENT_HAVE_D_RECLEN
> > >
> > > /* Avoid use of strlen() if there's d_reclen. */
> > > sz = dent->d_reclen;
> > >
> > > #else
> > >
> > > /* Fallback to the most portable way. */
> > > sz = offsetof(struct dirent, d_name) +
> > >
> > > strlen(dent->d_name) + 1;
> > >
> > > #endif
> > >
> > > return g_memdup(dent, sz);
> > >
> > > }
> >
> > That was the intentional change v2 -> v3 Philippe. Previous v2 crashed the
> > 9p 'synth' tests:
> >
> > https://lore.kernel.org/qemu-devel/2627488.0S70g7mNYN@silver/T/#ma09bedde5
> > 9a91e6435443e151f7e49fef8616e4c
> >
> > You might argue that the 9p 'synth' driver should instead populate
> > d_reclen
> > instead, but this v3 is a simpler solution and guarantees to always work.
> > So I'd prefer to go with Vitaly's v3 for now, especially as this patch
> > would go to the stable branches as well.
>
> This really is a band-aid. Anyone reading this will react as Philippe,
> and this is common code, not just 9pfs. With correctly populated dirents,
> the helper could be as simple as:
>
> struct dirent *
> qemu_dirent_dup(struct dirent *dent)
> {
> size_t sz = offsetof(struct dirent, d_name) + _D_EXACT_NAMLEN(dent) + 1;
>
> return g_memdup(dent, sz);
> }
Which requires _D_EXACT_NAMLEN being defined, whereas Vitaly's solution
ensures to work on all systems.
> If this is a cycles problem, I can help reviewing the fixes on
> the synth driver, or alternatively you can keep this code
> somewhere under 9pfs but please don't put that in common utils.
Who cares, sends a patch, and makes sure that it works at least as reliably as
the currently suggested solution (tested with guest and test cases).
Thanks!
Best regards,
Christian Schoenebeck
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3] 9pfs: Fix segfault in do_readdir_many caused by struct dirent overread
2022-02-04 13:55 ` Philippe Mathieu-Daudé via
2022-02-04 14:12 ` Christian Schoenebeck
@ 2022-02-04 16:19 ` Dmitry V. Levin
2022-02-05 3:23 ` Vitaly Chikunov
2022-02-05 5:58 ` Greg Kurz
1 sibling, 2 replies; 18+ messages in thread
From: Dmitry V. Levin @ 2022-02-04 16:19 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Vitaly Chikunov, qemu-stable, Christian Schoenebeck, Greg Kurz,
qemu-devel
On Fri, Feb 04, 2022 at 02:55:45PM +0100, Philippe Mathieu-Daudé wrote:
> On 4/2/22 06:06, Vitaly Chikunov wrote:
> > `struct dirent' returned from readdir(3) could be shorter (or longer)
> > 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 again.
> >
> > Changes from v2:
> > - Make it work with a simulated dirent where d_reclen is 0, which was
> > caused abort in readdir qos-test, by using fallback at runtime.
> >
> > hw/9pfs/codir.c | 3 +--
> > include/qemu/osdep.h | 13 +++++++++++++
> > util/osdep.c | 18 ++++++++++++++++++
> > 3 files changed, 32 insertions(+), 2 deletions(-)
>
> > +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) {
>
> If _DIRENT_HAVE_D_RECLEN is defined, this case is unlikely...
If it was so easy to be misled this way,
I'd suggest adding an explicit comment, e.g.
/*
* Test sz for non-zero even if d_reclen is available
* because some drivers may set d_reclen to zero.
*/
--
ldv
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v3] 9pfs: Fix segfault in do_readdir_many caused by struct dirent overread
2022-02-04 16:19 ` Dmitry V. Levin
@ 2022-02-05 3:23 ` Vitaly Chikunov
2022-02-05 5:58 ` Greg Kurz
1 sibling, 0 replies; 18+ messages in thread
From: Vitaly Chikunov @ 2022-02-05 3:23 UTC (permalink / raw)
To: Dmitry V. Levin
Cc: qemu-devel, qemu-stable, Christian Schoenebeck,
Philippe Mathieu-Daudé,
Greg Kurz
On Fri, Feb 04, 2022 at 07:19:39PM +0300, Dmitry V. Levin wrote:
> On Fri, Feb 04, 2022 at 02:55:45PM +0100, Philippe Mathieu-Daudé wrote:
> > On 4/2/22 06:06, Vitaly Chikunov wrote:
> > > `struct dirent' returned from readdir(3) could be shorter (or longer)
> > > 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 again.
> > >
> > > Changes from v2:
> > > - Make it work with a simulated dirent where d_reclen is 0, which was
> > > caused abort in readdir qos-test, by using fallback at runtime.
> > >
> > > hw/9pfs/codir.c | 3 +--
> > > include/qemu/osdep.h | 13 +++++++++++++
> > > util/osdep.c | 18 ++++++++++++++++++
> > > 3 files changed, 32 insertions(+), 2 deletions(-)
> >
> > > +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) {
> >
> > If _DIRENT_HAVE_D_RECLEN is defined, this case is unlikely...
>
> If it was so easy to be misled this way,
> I'd suggest adding an explicit comment, e.g.
>
> /*
> * Test sz for non-zero even if d_reclen is available
> * because some drivers may set d_reclen to zero.
> */
Sounds good, to avoid misunderstanding. I can send v4, with g_memdup
(like Greg suggested).
Thanks,
>
>
> --
> ldv
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v3] 9pfs: Fix segfault in do_readdir_many caused by struct dirent overread
2022-02-04 16:19 ` Dmitry V. Levin
2022-02-05 3:23 ` Vitaly Chikunov
@ 2022-02-05 5:58 ` Greg Kurz
1 sibling, 0 replies; 18+ messages in thread
From: Greg Kurz @ 2022-02-05 5:58 UTC (permalink / raw)
To: Dmitry V. Levin
Cc: Vitaly Chikunov, qemu-stable, Christian Schoenebeck,
Philippe Mathieu-Daudé,
qemu-devel
On Fri, 4 Feb 2022 19:19:39 +0300
"Dmitry V. Levin" <ldv@altlinux.org> wrote:
> On Fri, Feb 04, 2022 at 02:55:45PM +0100, Philippe Mathieu-Daudé wrote:
> > On 4/2/22 06:06, Vitaly Chikunov wrote:
> > > `struct dirent' returned from readdir(3) could be shorter (or longer)
> > > 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 again.
> > >
> > > Changes from v2:
> > > - Make it work with a simulated dirent where d_reclen is 0, which was
> > > caused abort in readdir qos-test, by using fallback at runtime.
> > >
> > > hw/9pfs/codir.c | 3 +--
> > > include/qemu/osdep.h | 13 +++++++++++++
> > > util/osdep.c | 18 ++++++++++++++++++
> > > 3 files changed, 32 insertions(+), 2 deletions(-)
> >
> > > +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) {
> >
> > If _DIRENT_HAVE_D_RECLEN is defined, this case is unlikely...
>
> If it was so easy to be misled this way,
> I'd suggest adding an explicit comment, e.g.
>
> /*
> * Test sz for non-zero even if d_reclen is available
> * because some drivers may set d_reclen to zero.
> */
>
+1
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3] 9pfs: Fix segfault in do_readdir_many caused by struct dirent overread
2022-02-04 5:06 [PATCH v3] 9pfs: Fix segfault in do_readdir_many caused by struct dirent overread Vitaly Chikunov
` (2 preceding siblings ...)
2022-02-04 13:55 ` Philippe Mathieu-Daudé via
@ 2022-02-05 11:36 ` Christian Schoenebeck
2022-02-05 11:58 ` Philippe Mathieu-Daudé via
3 siblings, 1 reply; 18+ messages in thread
From: Christian Schoenebeck @ 2022-02-05 11:36 UTC (permalink / raw)
To: qemu-devel, Philippe Mathieu-Daudé via
Cc: Vitaly Chikunov, Greg Kurz, qemu-stable, ldv
On Freitag, 4. Februar 2022 06:06:09 CET Vitaly Chikunov wrote:
> `struct dirent' returned from readdir(3) could be shorter (or longer)
> 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 again.
>
> Changes from v2:
> - Make it work with a simulated dirent where d_reclen is 0, which was
> caused abort in readdir qos-test, by using fallback at runtime.
>
> hw/9pfs/codir.c | 3 +--
> include/qemu/osdep.h | 13 +++++++++++++
> util/osdep.c | 18 ++++++++++++++++++
> 3 files changed, 32 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..2c80528a61 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,20 @@ 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)
> +{
> + 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) {
Philippe, Greg, apart from the additional comment, do you want to see this
check to be changed in v4 to this?
if (unlikely(sz == 0)) {
Best regards,
Christian Schoenebeck
> + /* 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);
> +}
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v3] 9pfs: Fix segfault in do_readdir_many caused by struct dirent overread
2022-02-05 11:36 ` Christian Schoenebeck
@ 2022-02-05 11:58 ` Philippe Mathieu-Daudé via
0 siblings, 0 replies; 18+ messages in thread
From: Philippe Mathieu-Daudé via @ 2022-02-05 11:58 UTC (permalink / raw)
To: Christian Schoenebeck, qemu-devel
Cc: Vitaly Chikunov, Greg Kurz, qemu-stable, ldv
On 5/2/22 12:36, Christian Schoenebeck wrote:
> On Freitag, 4. Februar 2022 06:06:09 CET Vitaly Chikunov wrote:
>> `struct dirent' returned from readdir(3) could be shorter (or longer)
>> 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 again.
>>
>> Changes from v2:
>> - Make it work with a simulated dirent where d_reclen is 0, which was
>> caused abort in readdir qos-test, by using fallback at runtime.
>>
>> hw/9pfs/codir.c | 3 +--
>> include/qemu/osdep.h | 13 +++++++++++++
>> util/osdep.c | 18 ++++++++++++++++++
>> 3 files changed, 32 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..2c80528a61 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,20 @@ 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)
>> +{
>> + 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) {
>
> Philippe, Greg, apart from the additional comment, do you want to see this
> check to be changed in v4 to this?
>
> if (unlikely(sz == 0)) {
This is not an hot path so probably not very useful.
>
> Best regards,
> Christian Schoenebeck
>
>> + /* 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);
However 'return g_memdup(dent, sz)' is simpler to review.
>> +}
>
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread