[Qemu-devel] [PATCH] linux-user: fix emulation of getdents
In case when TARGET_ABI_BITS == 32 && HOST_LONG_BITS == 64, the last
byte of the target dirent structure (aka d_type byte) was never copied
from the native dirent structure, thus breaking everything that relies
on valid d_type value, e.g. glob(3).
Signed-off-by: Dmitry V. Levin
---
linux-user/syscall.c | 7 +++
linux-user/syscall_defs.h | 3 ++-
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 41c869b..2b6025b 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -7030,10 +7030,9 @@ abi_long do_syscall(void *cpu_env, int num, abi_long
arg1,
tde->d_ino = tswapal(de->d_ino);
tde->d_off = tswapal(de->d_off);
tnamelen = treclen - (2 * sizeof(abi_long) + 2);
- if (tnamelen > 256)
-tnamelen = 256;
-/* XXX: may not be correct */
-pstrcpy(tde->d_name, tnamelen, de->d_name);
+ if (tnamelen > sizeof(tde->d_name))
+tnamelen = sizeof(tde->d_name);
+memcpy(tde->d_name, de->d_name, tnamelen);
de = (struct linux_dirent *)((char *)de + reclen);
len -= reclen;
tde = (struct target_dirent *)((char *)tde + treclen);
diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
index 2cfda5a..f27a8d4 100644
--- a/linux-user/syscall_defs.h
+++ b/linux-user/syscall_defs.h
@@ -258,7 +258,8 @@ struct target_dirent {
abi_longd_ino;
abi_longd_off;
unsigned short d_reclen;
- chard_name[256]; /* We must not include limits.h! */
+ chard_name[257];/* We must not include limits.h! */
+ /* 257 = NAME_MAX + '\0' + d_type */
};
struct target_dirent64 {
--
ldv
[Qemu-devel] [PATCH v2] linux-user: fix emulation of getdents
In case when TARGET_ABI_BITS == 32 && HOST_LONG_BITS == 64, the last
byte of the target dirent structure (aka d_type byte) was never copied
from the host dirent structure, thus breaking everything that relies
on valid d_type value, e.g. glob(3).
Signed-off-by: Dmitry V. Levin
---
linux-user/syscall.c | 8
linux-user/syscall_defs.h | 3 ++-
2 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 41c869b..adc3270 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -7030,10 +7030,10 @@ abi_long do_syscall(void *cpu_env, int num, abi_long
arg1,
tde->d_ino = tswapal(de->d_ino);
tde->d_off = tswapal(de->d_off);
tnamelen = treclen - (2 * sizeof(abi_long) + 2);
- if (tnamelen > 256)
-tnamelen = 256;
-/* XXX: may not be correct */
-pstrcpy(tde->d_name, tnamelen, de->d_name);
+if (tnamelen > sizeof(tde->d_name)) {
+tnamelen = sizeof(tde->d_name);
+}
+memcpy(tde->d_name, de->d_name, tnamelen);
de = (struct linux_dirent *)((char *)de + reclen);
len -= reclen;
tde = (struct target_dirent *)((char *)tde + treclen);
diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
index 2cfda5a..19fe414 100644
--- a/linux-user/syscall_defs.h
+++ b/linux-user/syscall_defs.h
@@ -258,7 +258,8 @@ struct target_dirent {
abi_longd_ino;
abi_longd_off;
unsigned short d_reclen;
- chard_name[256]; /* We must not include limits.h! */
+ chard_name[257];/* 257 = NAME_MAX + '\0' + d_type, */
+/* we must not include limits.h! */
};
struct target_dirent64 {
--
ldv
[Qemu-devel] [PATCH v3] linux-user: fix emulation of getdents
In case when TARGET_ABI_BITS == 32 && HOST_LONG_BITS == 64, the last
byte of the target dirent structure (aka d_type byte) was never copied
from the host dirent structure, thus breaking everything that relies
on valid d_type value, e.g. glob(3).
Signed-off-by: Dmitry V. Levin
---
linux-user/syscall.c | 11 +--
linux-user/syscall_defs.h | 2 +-
2 files changed, 6 insertions(+), 7 deletions(-)
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 41c869b..8a8aaca 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -7025,15 +7025,14 @@ abi_long do_syscall(void *cpu_env, int num, abi_long
arg1,
tde = target_dirp;
while (len > 0) {
reclen = de->d_reclen;
- treclen = reclen - (2 * (sizeof(long) - sizeof(abi_long)));
+tnamelen = reclen - offsetof(struct linux_dirent, d_name);
+assert(tnamelen >= 0);
+treclen = tnamelen + offsetof(struct target_dirent,
d_name);
+assert(count1 + treclen <= count);
tde->d_reclen = tswap16(treclen);
tde->d_ino = tswapal(de->d_ino);
tde->d_off = tswapal(de->d_off);
- tnamelen = treclen - (2 * sizeof(abi_long) + 2);
- if (tnamelen > 256)
-tnamelen = 256;
-/* XXX: may not be correct */
-pstrcpy(tde->d_name, tnamelen, de->d_name);
+memcpy(tde->d_name, de->d_name, tnamelen);
de = (struct linux_dirent *)((char *)de + reclen);
len -= reclen;
tde = (struct target_dirent *)((char *)tde + treclen);
diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
index 2cfda5a..f7271fa 100644
--- a/linux-user/syscall_defs.h
+++ b/linux-user/syscall_defs.h
@@ -258,7 +258,7 @@ struct target_dirent {
abi_longd_ino;
abi_longd_off;
unsigned short d_reclen;
- chard_name[256]; /* We must not include limits.h! */
+ chard_name[];
};
struct target_dirent64 {
--
ldv
Re: [Qemu-devel] [PATCH v4] linux-user: fix emulation of getdents
In case when TARGET_ABI_BITS == 32 && HOST_LONG_BITS == 64, the last
byte of the target dirent structure (aka d_type byte) was never copied
from the host dirent structure, thus breaking everything that relies
on valid d_type value, e.g. glob(3).
Signed-off-by: Dmitry V. Levin
---
linux-user/syscall.c | 11 +--
linux-user/syscall_defs.h | 8
2 files changed, 9 insertions(+), 10 deletions(-)
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 41c869b..8a8aaca 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -7025,15 +7025,14 @@ abi_long do_syscall(void *cpu_env, int num, abi_long
arg1,
tde = target_dirp;
while (len > 0) {
reclen = de->d_reclen;
- treclen = reclen - (2 * (sizeof(long) - sizeof(abi_long)));
+tnamelen = reclen - offsetof(struct linux_dirent, d_name);
+assert(tnamelen >= 0);
+treclen = tnamelen + offsetof(struct target_dirent,
d_name);
+assert(count1 + treclen <= count);
tde->d_reclen = tswap16(treclen);
tde->d_ino = tswapal(de->d_ino);
tde->d_off = tswapal(de->d_off);
- tnamelen = treclen - (2 * sizeof(abi_long) + 2);
- if (tnamelen > 256)
-tnamelen = 256;
-/* XXX: may not be correct */
-pstrcpy(tde->d_name, tnamelen, de->d_name);
+memcpy(tde->d_name, de->d_name, tnamelen);
de = (struct linux_dirent *)((char *)de + reclen);
len -= reclen;
tde = (struct target_dirent *)((char *)tde + treclen);
diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
index 2cfda5a..a98cbf7 100644
--- a/linux-user/syscall_defs.h
+++ b/linux-user/syscall_defs.h
@@ -255,10 +255,10 @@ struct kernel_statfs {
};
struct target_dirent {
- abi_longd_ino;
- abi_longd_off;
- unsigned short d_reclen;
- chard_name[256]; /* We must not include limits.h! */
+abi_longd_ino;
+abi_longd_off;
+unsigned short d_reclen;
+chard_name[];
};
struct target_dirent64 {
--
ldv
Re: [PATCH v2] 9pfs: Fix segfault in do_readdir_many caused by struct dirent overread
On Fri, Feb 04, 2022 at 03:15:16AM +0300, Vitaly Chikunov wrote:
[...]
> Yes but this will cause another abort() call. I am thinking about v3 fix
> like this:
>
> struct dirent *
> qemu_dirent_dup(struct dirent *dent)
> {
> size_t sz = 0;
> #if defined _DIRENT_HAVE_D_RECLEN
> /* Avoid use of strlen() if there's d_reclen. */
> sz = dent->d_reclen;
> #endif
> if (sz == 0) {
> /* Fallback to the most portable way. */
> sz = offsetof(struct dirent, d_name) +
> strlen(dent->d_name) + 1;
> }
> struct dirent *dst = g_malloc(sz);
> return memcpy(dst, dent, sz);
> }
>
> Thus it will use strlen for simulated dirents and d_reclen for real ones
Makes sense.
--
ldv
Re: [PATCH v3] 9pfs: Fix segfault in do_readdir_many caused by struct dirent overread
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 0x559ebeed v9fs_co_readdir_many (/usr/bin/qemu-system-x86_64 +
> > 0x497eed)
> > #1 0x559ec2e9 v9fs_readdir (/usr/bin/qemu-system-x86_64 +
> > 0x4982e9)
> > #2 0x55eb7983 coroutine_trampoline (/usr/bin/qemu-system-x86_64 +
> > 0x963983)
> > #3 0x773e0be0 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-sta...@nongnu.org
> > Co-authored-by: Christian Schoenebeck
> > Signed-off-by: Vitaly Chikunov
> > ---
> > 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
> > #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"
Reviewed-by: Dmitry V. Levin
There should be no '"', sorry about that. :)
--
ldv
Re: [PATCH v3] 9pfs: Fix segfault in do_readdir_many caused by struct dirent overread
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 0x559ebeed v9fs_co_readdir_many (/usr/bin/qemu-system-x86_64 +
> 0x497eed)
> #1 0x559ec2e9 v9fs_readdir (/usr/bin/qemu-system-x86_64 + 0x4982e9)
> #2 0x55eb7983 coroutine_trampoline (/usr/bin/qemu-system-x86_64 +
> 0x963983)
> #3 0x773e0be0 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-sta...@nongnu.org
> Co-authored-by: Christian Schoenebeck
> Signed-off-by: Vitaly Chikunov
> ---
> 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
> #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
Re: [PATCH v3] 9pfs: Fix segfault in do_readdir_many caused by struct dirent overread
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
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 0x559ebeed v9fs_co_readdir_many (/usr/bin/qemu-system-x86_64
> > + 0x497eed)
> > #1 0x559ec2e9 v9fs_readdir (/usr/bin/qemu-system-x86_64 +
> > 0x4982e9)
> > #2 0x55eb7983 coroutine_trampoline (/usr/bin/qemu-system-x86_64
> > + 0x963983)
> > #3 0x773e0be0 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-sta...@nongnu.org
> > Co-authored-by: Christian Schoenebeck
> > Signed-off-by: Vitaly Chikunov
> > ---
> > 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: [Qemu-devel] d_off field in struct dirent and 32-on-64 emulation
On Thu, Dec 27, 2018 at 06:18:19PM +0100, Florian Weimer wrote:
> We have a bit of an interesting problem with respect to the d_off
> field in struct dirent.
>
> When running a 64-bit kernel on certain file systems, notably ext4,
> this field uses the full 63 bits even for small directories (strace -v
> output, wrapped here for readability):
>
> getdents(3, [
> {d_ino=1494304, d_off=3901177228673045825, d_reclen=40,
> d_name="authorized_keys", d_type=DT_REG},
> {d_ino=1494277, d_off=7491915799041650922, d_reclen=24, d_name=".",
> d_type=DT_DIR},
> {d_ino=1314655, d_off=9223372036854775807, d_reclen=24, d_name="..",
> d_type=DT_DIR}
> ], 32768) = 88
>
> When running in 32-bit compat mode, this value is somehow truncated to
> 31 bits, for both the getdents and the getdents64 (!) system call (at
> least on i386).
Why getdents64 system call is affected by this truncation,
isn't it a kernel bug that has to be fixed in the kernel instead?
--
ldv
signature.asc
Description: PGP signature
