[PATCH v3] 9pfs: Fix segfault in do_readdir_many caused by struct dirent overread

[PATCH v3] 9pfs: Fix segfault in do_readdir_many caused by struct dirent overread

[Vitaly Chikunov]

`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);
+}
-- 
2.33.0

Re: [PATCH v3] 9pfs: Fix segfault in do_readdir_many caused by struct dirent overread

[Christian Schoenebeck]

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);
> +}

Re: [PATCH v3] 9pfs: Fix segfault in do_readdir_many caused by struct dirent overread

[Dmitry V. Levin]

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

Re: [PATCH v3] 9pfs: Fix segfault in do_readdir_many caused by struct dirent overread

[Dmitry V. Levin]

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

Re: [PATCH v3] 9pfs: Fix segfault in do_readdir_many caused by struct dirent overread

[Philippe Mathieu-Daudé via]

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);
}

Re: [PATCH v3] 9pfs: Fix segfault in do_readdir_many caused by struct dirent overread

[Christian Schoenebeck]

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

Re: [PATCH v3] 9pfs: Fix segfault in do_readdir_many caused by struct dirent overread

[Greg Kurz]

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
> 
> 

Re: [PATCH v3] 9pfs: Fix segfault in do_readdir_many caused by struct dirent overread

[Vitaly Chikunov]

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
> > 
> > 

Re: [PATCH v3] 9pfs: Fix segfault in do_readdir_many caused by struct dirent overread

[Christian Schoenebeck]

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

Re: [PATCH v3] 9pfs: Fix segfault in do_readdir_many caused by struct dirent overread

[Dmitry V. Levin]

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

Re: [PATCH v3] 9pfs: Fix segfault in do_readdir_many caused by struct dirent overread

[Philippe Mathieu-Daudé via]

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?

Re: [PATCH v3] 9pfs: Fix segfault in do_readdir_many caused by struct dirent overread

[Christian Schoenebeck]

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

Re: [PATCH v3] 9pfs: Fix segfault in do_readdir_many caused by struct dirent overread

[Dmitry V. Levin]

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

Re: [PATCH v3] 9pfs: Fix segfault in do_readdir_many caused by struct dirent overread

[Philippe Mathieu-Daudé via]

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.

Re: [PATCH v3] 9pfs: Fix segfault in do_readdir_many caused by struct dirent overread

[Vitaly Chikunov]

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

Re: [PATCH v3] 9pfs: Fix segfault in do_readdir_many caused by struct dirent overread

[Greg Kurz]

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

> 

Re: [PATCH v3] 9pfs: Fix segfault in do_readdir_many caused by struct dirent overread

[Christian Schoenebeck]

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);
> +}

Re: [PATCH v3] 9pfs: Fix segfault in do_readdir_many caused by struct dirent overread

[Philippe Mathieu-Daudé via]

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.

>> +}
> 
> 
>