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

[Christian Schoenebeck <qemu_oss@crudebyte.com>]

On Donnerstag, 3. Februar 2022 07:20:05 CET Vitaly Chikunov wrote:
> On Thu, Feb 03, 2022 at 07:55:41AM +0300, Vitaly Chikunov wrote:
> > Christian,
> > 
> > On Wed, Feb 02, 2022 at 05:55:45PM +0100, Christian Schoenebeck wrote:
> > > On Freitag, 28. Januar 2022 23:33:26 CET Vitaly Chikunov wrote:
> > > > `struct dirent' returned from readdir(3) could be shorter than
> > > > `sizeof(struct dirent)', thus memcpy of sizeof length will overread
> > > > 
> > > > into unallocated page causing SIGSEGV. Example stack trace:
> > > >  #0  0x00005555559ebeed v9fs_co_readdir_many
> > > >  (/usr/bin/qemu-system-x86_64 +
> > > > 
> > > > 0x497eed) #1  0x00005555559ec2e9 v9fs_readdir
> > > > (/usr/bin/qemu-system-x86_64
> > > > + 0x4982e9) #2  0x0000555555eb7983 coroutine_trampoline
> > > > (/usr/bin/qemu-system-x86_64 + 0x963983) #3  0x00007ffff73e0be0 n/a
> > > > (n/a +
> > > > 0x0)
> > > > 
> > > > While fixing, provide a helper for any future `struct dirent' cloning.
> > > > 
> > > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/841
> > > > Cc: qemu-stable@nongnu.org
> > > > Co-authored-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > > > Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
> > > > ---
> > > > Tested on x86-64 Linux.
> > > 
> > > I was too optimistic. Looks like this needs more work. With this patch
> > > applied the 9p test cases [1] are crashing now:
> > > 
> > > $ gdb --args tests/qtest/qos-test -m slow
> > > ...
> > > # Start of flush tests
> > > ok 50
> > > /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/vi
> > > rtio-9p-tests/synth/flush/success ok 51
> > > /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/vi
> > > rtio-9p-tests/synth/flush/ignored # End of flush tests
> > > # Start of readdir tests
> > 
> > I changed implementation from the one using dent->d_reclen to the one
> > using
> > 
> > strlen(dent->d_name) and it's passed readdir tests, but failed later:
> >   # Start of readdir tests
> >   ok 53
> >   /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/vi
> >   rtio-9p-tests/synth/readdir/basic ok 54
> >   /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/vi
> >   rtio-9p-tests/synth/readdir/split_512 ok 55
> >   /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/vi
> >   rtio-9p-tests/synth/readdir/split_256 ok 56
> >   /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/vi
> >   rtio-9p-tests/synth/readdir/split_128 # End of readdir tests
> >   # End of synth tests
> >   # Start of local tests
> >   # starting QEMU: exec x86_64-softmmu/qemu-system-x86_64 -qtest
> >   unix:/tmp/qtest-2822967.sock -qtest-log /dev/null -chardev
> >   socket,path=/tmp/qtest-2822967.qmp,id=char0 -mon
> >   chardev=char0,mode=control -display none -M pc  -fsdev
> >   local,id=fsdev0,path='/usr/src/RPM/BUILD/qemu-6.2.0/build-dynamic/qtest
> >   -9p-local-NpcZCR',security_model=mapped-xattr -device
> >   virtio-9p-pci,fsdev=fsdev0,addr=04.0,mount_tag=qtest -accel qtest #
> >   GLib-DEBUG: setenv()/putenv() are not thread-safe and should not be
> >   used after threads are created ok 57
> >   /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/vi
> >   rtio-9p-tests/local/config Received response 7 (RLERROR) instead of 73
> >   (RMKDIR)
> >   Rlerror has errno 95 (Operation not supported)
> >   **
> >   ERROR:../tests/qtest/virtio-9p-test.c:305:v9fs_req_recv: assertion
> >   failed (hdr.id == id): (7 == 73) Bail out!
> >   ERROR:../tests/qtest/virtio-9p-test.c:305:v9fs_req_recv: assertion
> >   failed (hdr.id == id): (7 == 73) Aborted
> 
> I added some debugging output and in the bug case `d_reclen` is 0. Thus
> this is not readdir's struct dirent, but something else (test-only
> simulated dirent without accounting that we now have d_reclen logic).
> 
> This maybe related to the other bug in
> 
>   static void synth_direntry(V9fsSynthNode *node,
> 				  struct dirent *entry, off_t off)
>   {
>       strcpy(entry->d_name, node->name);
>       entry->d_ino = node->attr->inode;
>       entry->d_off = off + 1;
>   }
> 
> Where `d_reclen` is not updated.

The synth driver (used by the 'synth' 9p tests) intentionally just simulates a 
filesystem. The synth driver does not call any real fs syscalls, it just has 
its own very simple in-RAM-only structures that are used to simulate a fs and 
therefore the synth driver populates the dirent structure by itself.

Could you try to resolve this issue in the synth driver and send a v3 of this 
patch? I am currently busy with other tasks right now.

Best regards,
Christian Schoenebeck