qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Greg Kurz
Subject: Re: [Qemu-devel] [PATCH] 9pfs: fix NULL pointer dereference in v9fs_version
Date: Tue, 27 Sep 2016 09:35:33 +0200

On Mon, 26 Sep 2016 21:38:48 -0700
Li Qiang <address@hidden> wrote:

> From: Li Qiang <address@hidden>
> 
> 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 <address@hidden>
> ---
>  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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]