[Qemu-devel] [PATCH] 9pfs: fix NULL pointer dereference in v9fs_version

[Qemu-devel] [PATCH] 9pfs: fix NULL pointer dereference in v9fs_version

[Li Qiang]

From: Li Qiang <liqiang6-s@360.cn>

In 9pfs get version dispatch function, a guest can provide a NULL
version string thus causing an NULL pointer dereference issue.
This patch fix this issue.

Signed-off-by: Li Qiang <liqiang6-s@360.cn>
---
 hw/9pfs/9p.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 119ee58..dd3145c 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -955,6 +955,11 @@ static void v9fs_version(void *opaque)
         offset = err;
         goto out;
     }
+
+    if (!version.data) {
+        offset = -EINVAL;
+        goto out;
+    }
     trace_v9fs_version(pdu->tag, pdu->id, s->msize, version.data);
 
     virtfs_reset(pdu);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] 9pfs: fix NULL pointer dereference in v9fs_version

[Greg Kurz]

On Mon, 26 Sep 2016 21:38:48 -0700
Li Qiang <liq3ea@gmail.com> wrote:

> From: Li Qiang <liqiang6-s@360.cn>
> 
> In 9pfs get version dispatch function, a guest can provide a NULL
> version string thus causing an NULL pointer dereference issue.

The guest doesn't provide NULL, but an empty string actually.

> This patch fix this issue.
> 
> Signed-off-by: Li Qiang <liqiang6-s@360.cn>
> ---
>  hw/9pfs/9p.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index 119ee58..dd3145c 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -955,6 +955,11 @@ static void v9fs_version(void *opaque)
>          offset = err;
>          goto out;
>      }
> +
> +    if (!version.data) {
> +        offset = -EINVAL;
> +        goto out;
> +    }

If a client sends a Tversion message with an unrecognized version (which is
obviously the case with an empty string), the server should send back a
Rversion response with 'unknown'... while with this patch it will send Rerror.

http://man.cat-v.org/plan_9/5/version

>      trace_v9fs_version(pdu->tag, pdu->id, s->msize, version.data);
>  
>      virtfs_reset(pdu);

All guests originated strings come from v9fs_iov_vunmarshal() actually, which
does this:

                str->data = g_malloc(str->size + 1);
                copied = v9fs_unpack(str->data, out_sg, out_num, offset,
                                     str->size);
                if (copied > 0) {
                    str->data[str->size] = 0;
                } else {
                    v9fs_string_free(str);
                }

It makes sense to free the buffer when v9fs_unpack() fails, because the error is
propagated and the caller isn't supposed to derefence str->data in this case.
But it looks wrong to do the same with an empty string, which is expected to be
usable by the caller.

The fix is to check copied >= 0.

Cheers.

--
Greg