[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2] 9pfs: Fix segfault in do_readdir_many caused by struct di
From: |
Christian Schoenebeck |
Subject: |
Re: [PATCH v2] 9pfs: Fix segfault in do_readdir_many caused by struct dirent overread |
Date: |
Thu, 03 Feb 2022 13:31:58 +0100 |
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