[Qemu-devel] [PATCH] 9pfs: make unmarshal V9fsString more robust

[Qemu-devel] [PATCH] 9pfs: make unmarshal V9fsString more robust

[Li Qiang]

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

In 9pfs function v9fs_iov_vunmarshal, it will not allocate space
for empty string. This will cause several NULL pointer dereference
issues. this patch fix this issue.

Signed-off-by: Li Qiang <liqiang6-s@360.cn>
---
 fsdev/9p-iov-marshal.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fsdev/9p-iov-marshal.c b/fsdev/9p-iov-marshal.c
index 663cad5..1d16f8d 100644
--- a/fsdev/9p-iov-marshal.c
+++ b/fsdev/9p-iov-marshal.c
@@ -125,7 +125,7 @@ ssize_t v9fs_iov_vunmarshal(struct iovec *out_sg, int out_num, size_t offset,
                 str->data = g_malloc(str->size + 1);
                 copied = v9fs_unpack(str->data, out_sg, out_num, offset,
                                      str->size);
-                if (copied > 0) {
+                if (copied >= 0) {
                     str->data[str->size] = 0;
                 } else {
                     v9fs_string_free(str);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] 9pfs: make unmarshal V9fsString more robust

[Greg Kurz]

On Tue, 27 Sep 2016 04:44:11 -0700
Li Qiang <liq3ea@gmail.com> wrote:

> From: Li Qiang <liqiang6-s@360.cn>
> 
> In 9pfs function v9fs_iov_vunmarshal, it will not allocate space
> for empty string. This will cause several NULL pointer dereference
> issues. this patch fix this issue.
> 
> Signed-off-by: Li Qiang <liqiang6-s@360.cn>
> ---

Talking about robustness was appropriate for your previous patches, but
it does not really apply here since v9fs_iov_vunmarshal() does not have
any issue with empty strings actually.

I've changed the title to:

9pfs: allocate space for guest originated empty strings

And while here, I've updated the changelog to provide a more detailed
justification:

    If a guest sends an empty string paramater to any 9P operation, the current
    code unmarshals it into a V9fsString equal to { .size = 0, .data = NULL }.
    
    This is unfortunate because it can cause NULL pointer dereference to happen
    at various locations in the 9pfs code. And we don't want to check str->data
    everywhere we pass it to strcmp() or any other function which expects a
    dereferenceable pointer.
    
    This patch enforces the allocation of genuine C empty strings instead, so
    callers don't have to bother.

Thanks.

--
Greg

>  fsdev/9p-iov-marshal.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fsdev/9p-iov-marshal.c b/fsdev/9p-iov-marshal.c
> index 663cad5..1d16f8d 100644
> --- a/fsdev/9p-iov-marshal.c
> +++ b/fsdev/9p-iov-marshal.c
> @@ -125,7 +125,7 @@ ssize_t v9fs_iov_vunmarshal(struct iovec *out_sg, int out_num, size_t offset,
>                  str->data = g_malloc(str->size + 1);
>                  copied = v9fs_unpack(str->data, out_sg, out_num, offset,
>                                       str->size);
> -                if (copied > 0) {
> +                if (copied >= 0) {
>                      str->data[str->size] = 0;
>                  } else {
>                      v9fs_string_free(str);

Re: [Qemu-devel] [PATCH] 9pfs: make unmarshal V9fsString more robust

[李强]

On 2016-09-28 0:40 GMT+08:00 Greg Kurz <groug@kaod.org> wrote:

>
> Talking about robustness was appropriate for your previous patches, but
> it does not really apply here since v9fs_iov_vunmarshal() does not have
> any issue with empty strings actually.
>
> I've changed the title to:
>
> 9pfs: allocate space for guest originated empty strings
>
> And while here, I've updated the changelog to provide a more detailed
> justification:
>
> ...

Thanks very much to point out the mistakes, I will do more next time.

BTW, need I resend this patch formally?

Thanks.

Re: [Qemu-devel] [PATCH] 9pfs: make unmarshal V9fsString more robust

[Greg Kurz]

On Wed, 28 Sep 2016 10:14:00 +0800
李强 <liq3ea@gmail.com> wrote:

> On 2016-09-28 0:40 GMT+08:00 Greg Kurz <groug@kaod.org> wrote:
> 
> >
> > Talking about robustness was appropriate for your previous patches, but
> > it does not really apply here since v9fs_iov_vunmarshal() does not have
> > any issue with empty strings actually.
> >
> > I've changed the title to:
> >
> > 9pfs: allocate space for guest originated empty strings
> >
> > And while here, I've updated the changelog to provide a more detailed
> > justification:
> >
> > ...
> 
> Thanks very much to point out the mistakes, I will do more next time.
> 
> BTW, need I resend this patch formally?
> 

No, that's ok.

> Thanks.