| Message ID | E1t8GnN-002RS8-E2@kylie.crudebyte.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | 9pfs: fix crash on 'Treaddir' request | expand |
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 :-) Reviewed-by: Greg Kurz <groug@kaod.org> > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c > index af636cfb2d..9a291d1b51 100644 > --- a/hw/9pfs/9p.c > +++ b/hw/9pfs/9p.c > @@ -2587,6 +2587,11 @@ static void coroutine_fn v9fs_readdir(void *opaque) > retval = -EINVAL; > goto out_nofid; > } > + if (fidp->fid_type != P9_FID_DIR) { > + warn_report_once("9p: bad client: T_readdir on non-directory stream"); > + retval = -ENOTDIR; > + goto out; > + } > if (!fidp->fs.dir.stream) { > retval = -EINVAL; > goto out;
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
On Tuesday, November 5, 2024 11:25:26 AM CET Christian Schoenebeck 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> Queued on 9p.next: https://github.com/cschoenebeck/qemu/commits/9p.next I'll send a PR tomorrow. Thanks! /Christian
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c index af636cfb2d..9a291d1b51 100644 --- a/hw/9pfs/9p.c +++ b/hw/9pfs/9p.c @@ -2587,6 +2587,11 @@ static void coroutine_fn v9fs_readdir(void *opaque) retval = -EINVAL; goto out_nofid; } + if (fidp->fid_type != P9_FID_DIR) { + warn_report_once("9p: bad client: T_readdir on non-directory stream"); + retval = -ENOTDIR; + goto out; + } if (!fidp->fs.dir.stream) { retval = -EINVAL; goto out;
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(+)