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