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

[Greg Kurz <groug@kaod.org>]

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;



-- 
Greg