Re: [PATCH] 9pfs: fix crash on 'Treaddir' request

[Christian Schoenebeck <qemu_oss@crudebyte.com>]

On Tuesday, November 5, 2024 1:13:14 PM CET Greg Kurz wrote:
> On Tue, 5 Nov 2024 11:25:26 +0100
> Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> 
> > A bad (broken or malicious) 9p client (guest) could cause QEMU host to
> > crash by sending a 9p 'Treaddir' request with a numeric file ID (FID) that
> > was previously opened for a file instead of an expected directory:
> > 
> >   #0  0x0000762aff8f4919 in __GI___rewinddir (dirp=0xf) at
> >     ../sysdeps/unix/sysv/linux/rewinddir.c:29
> >   #1  0x0000557b7625fb40 in do_readdir_many (pdu=0x557bb67d2eb0,
> >     fidp=0x557bb67955b0, entries=0x762afe9fff58, offset=0, maxsize=131072,
> >     dostat=<optimized out>) at ../hw/9pfs/codir.c:101
> >   #2  v9fs_co_readdir_many (pdu=pdu@entry=0x557bb67d2eb0,
> >     fidp=fidp@entry=0x557bb67955b0, entries=entries@entry=0x762afe9fff58,
> >     offset=0, maxsize=131072, dostat=false) at ../hw/9pfs/codir.c:226
> >   #3  0x0000557b7625c1f9 in v9fs_do_readdir (pdu=0x557bb67d2eb0,
> >     fidp=0x557bb67955b0, offset=<optimized out>,
> >     max_count=<optimized out>) at ../hw/9pfs/9p.c:2488
> >   #4  v9fs_readdir (opaque=0x557bb67d2eb0) at ../hw/9pfs/9p.c:2602
> > 
> > That's because V9fsFidOpenState was declared as union type. So the
> > same memory region is used for either an open POSIX file handle (int),
> > or a POSIX DIR* pointer, etc., so 9p server incorrectly used the
> > previously opened (valid) POSIX file handle (0xf) as DIR* pointer,
> > eventually causing a crash in glibc's rewinddir() function.
> > 
> > Root cause was therefore a missing check in 9p server's 'Treaddir'
> > request handler, which must ensure that the client supplied FID was
> > really opened as directory stream before trying to access the
> > aforementioned union and its DIR* member.
> > 
> > Cc: qemu-stable@nongnu.org
> > Fixes: d62dbb51f7 ("virtio-9p: Add fidtype so that we can do type ...")
> > Reported-by: Akihiro Suda <suda.kyoto@gmail.com>
> > Tested-by: Akihiro Suda <suda.kyoto@gmail.com>
> > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > ---
> >  hw/9pfs/9p.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> 
> Wow ! And this is there since pretty much always... I'm amazed how
> it went unnoticed for so long :-)

Yeah, a true hero, lasted for amazing 14 years. :)

Probably a good wake-up call for looking into that fuzzing environment
somebody made for 9pfs. I never tried it myself.

> Reviewed-by: Greg Kurz <groug@kaod.org>

Thanks!

/Christian