[Qemu-devel] [PATCH 0/2] 9pfs: fix xattr issues

[Qemu-devel] [PATCH 0/2] 9pfs: fix xattr issues

[Li Qiang]

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

Hello,

This series fix two security issues, the first issue is caused by 
uninitialized heap, and the other is caused by no considering free the
buffer allocated previously.

Li Qiang (2):
  9pfs: fix information leak in xattr read
  9pfs: fix memory leak about xattr value

 hw/9pfs/9p.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/2] 9pfs: fix information leak in xattr read

[Li Qiang]

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

9pfs uses g_malloc() to allocate the xattr memory space, if the guest
reads this memory before writing to it, this will leak host heap memory
to the guest. This patch avoid this.

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

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 119ee58..8751c19 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -3282,7 +3282,7 @@ static void v9fs_xattrcreate(void *opaque)
     xattr_fidp->fs.xattr.flags = flags;
     v9fs_string_init(&xattr_fidp->fs.xattr.name);
     v9fs_string_copy(&xattr_fidp->fs.xattr.name, &name);
-    xattr_fidp->fs.xattr.value = g_malloc(size);
+    xattr_fidp->fs.xattr.value = g_malloc0(size);
     err = offset;
     put_fid(pdu, file_fidp);
 out_nofid:
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 1/2] 9pfs: fix information leak in xattr read

[Greg Kurz]

On Sat,  8 Oct 2016 22:26:51 -0700
Li Qiang <liq3ea@gmail.com> wrote:

> From: Li Qiang <liqiang6-s@360.cn>
> 
> 9pfs uses g_malloc() to allocate the xattr memory space, if the guest
> reads this memory before writing to it, this will leak host heap memory
> to the guest. This patch avoid this.
> 
> Signed-off-by: Li Qiang <liqiang6-s@360.cn>
> ---

I've looked again and we could theorically defer allocation until
v9fs_xattr_write() is called, and only allow v9fs_xattr_read() to
return bytes previously written by the client. But this would
result in rather complex code to handle partial writes and reads.

So this patch looks like the way to go.

Reviewed-by: Greg Kurz <groug@kaod.org>

>  hw/9pfs/9p.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index 119ee58..8751c19 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -3282,7 +3282,7 @@ static void v9fs_xattrcreate(void *opaque)
>      xattr_fidp->fs.xattr.flags = flags;
>      v9fs_string_init(&xattr_fidp->fs.xattr.name);
>      v9fs_string_copy(&xattr_fidp->fs.xattr.name, &name);
> -    xattr_fidp->fs.xattr.value = g_malloc(size);
> +    xattr_fidp->fs.xattr.value = g_malloc0(size);
>      err = offset;
>      put_fid(pdu, file_fidp);
>  out_nofid:

Re: [Qemu-devel] [PATCH 1/2] 9pfs: fix information leak in xattr read

[Greg Kurz]

On Mon, 10 Oct 2016 10:56:03 +0200
Greg Kurz <groug@kaod.org> wrote:

> On Sat,  8 Oct 2016 22:26:51 -0700
> Li Qiang <liq3ea@gmail.com> wrote:
> 
> > From: Li Qiang <liqiang6-s@360.cn>
> > 
> > 9pfs uses g_malloc() to allocate the xattr memory space, if the guest
> > reads this memory before writing to it, this will leak host heap memory
> > to the guest. This patch avoid this.
> > 
> > Signed-off-by: Li Qiang <liqiang6-s@360.cn>
> > ---  
> 
> I've looked again and we could theorically defer allocation until
> v9fs_xattr_write() is called, and only allow v9fs_xattr_read() to
> return bytes previously written by the client. But this would
> result in rather complex code to handle partial writes and reads.
> 
> So this patch looks like the way to go.
> 
> Reviewed-by: Greg Kurz <groug@kaod.org>
> 

But in fact, I'm afraid we have a more serious problem here... size
comes from the guest and could cause g_malloc() to abort if QEMU has
reached some RLIMIT... we need to call g_try_malloc0() and return
ENOMEM if the allocation fails.

Since this is yet another issue, I suggest you send another patch
on top of this one... and maybe check other locations in the code
where this could happen.

Cheers.

--
Greg

> >  hw/9pfs/9p.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > index 119ee58..8751c19 100644
> > --- a/hw/9pfs/9p.c
> > +++ b/hw/9pfs/9p.c
> > @@ -3282,7 +3282,7 @@ static void v9fs_xattrcreate(void *opaque)
> >      xattr_fidp->fs.xattr.flags = flags;
> >      v9fs_string_init(&xattr_fidp->fs.xattr.name);
> >      v9fs_string_copy(&xattr_fidp->fs.xattr.name, &name);
> > -    xattr_fidp->fs.xattr.value = g_malloc(size);
> > +    xattr_fidp->fs.xattr.value = g_malloc0(size);
> >      err = offset;
> >      put_fid(pdu, file_fidp);
> >  out_nofid:  
> 
> 

Re: [Qemu-devel] [PATCH 1/2] 9pfs: fix information leak in xattr read

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 669 bytes --]

On 10/12/2016 08:23 AM, Greg Kurz wrote:
> 
> But in fact, I'm afraid we have a more serious problem here... size
> comes from the guest and could cause g_malloc() to abort if QEMU has
> reached some RLIMIT... we need to call g_try_malloc0() and return
> ENOMEM if the allocation fails.

Even if it does not cause an ENOMEM failure right away, the guest can
also use this to chew up lots of host resources. It may also be worth
putting a reasonable cap at the maximum the guest can allocate, rather
than just trying to malloc every possible size.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 1/2] 9pfs: fix information leak in xattr read

[Li Qiang]

Yes, I think the limit to apply to xattr size in 9pfs is the same as the
Linux xattr size limit, I will try to find this limit.

Thanks.

On 2016-10-13 4:49 GMT+08:00 Eric Blake <eblake@redhat.com> wrote:

> On 10/12/2016 08:23 AM, Greg Kurz wrote:
> >
> > But in fact, I'm afraid we have a more serious problem here... size
> > comes from the guest and could cause g_malloc() to abort if QEMU has
> > reached some RLIMIT... we need to call g_try_malloc0() and return
> > ENOMEM if the allocation fails.
>
> Even if it does not cause an ENOMEM failure right away, the guest can
> also use this to chew up lots of host resources. It may also be worth
> putting a reasonable cap at the maximum the guest can allocate, rather
> than just trying to malloc every possible size.
>
> --
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>
>

Re: [Qemu-devel] [PATCH 1/2] 9pfs: fix information leak in xattr read

[Greg Kurz]

On Thu, 13 Oct 2016 11:30:08 +0800
Li Qiang <liq3ea@gmail.com> wrote:

> Yes, I think the limit to apply to xattr size in 9pfs is the same as the
> Linux xattr size limit, I will try to find this limit.
> 

/usr/include/linux/limits.h:#define XATTR_SIZE_MAX 65536        /* size of an extended attribute value (64k) */

> Thanks.
> 
> On 2016-10-13 4:49 GMT+08:00 Eric Blake <eblake@redhat.com> wrote:
> 
> > On 10/12/2016 08:23 AM, Greg Kurz wrote:
> > >
> > > But in fact, I'm afraid we have a more serious problem here... size
> > > comes from the guest and could cause g_malloc() to abort if QEMU has
> > > reached some RLIMIT... we need to call g_try_malloc0() and return
> > > ENOMEM if the allocation fails.
> >
> > Even if it does not cause an ENOMEM failure right away, the guest can
> > also use this to chew up lots of host resources. It may also be worth
> > putting a reasonable cap at the maximum the guest can allocate, rather
> > than just trying to malloc every possible size.
> >
> > --
> > Eric Blake   eblake redhat com    +1-919-301-3266
> > Libvirt virtualization library http://libvirt.org
> >
> >

Re: [Qemu-devel] [PATCH 1/2] 9pfs: fix information leak in xattr read

[Greg Kurz]

[-- Attachment #1: Type: text/plain, Size: 1229 bytes --]

On Wed, 12 Oct 2016 15:49:46 -0500
Eric Blake <eblake@redhat.com> wrote:

> On 10/12/2016 08:23 AM, Greg Kurz wrote:
> > 
> > But in fact, I'm afraid we have a more serious problem here... size
> > comes from the guest and could cause g_malloc() to abort if QEMU has
> > reached some RLIMIT... we need to call g_try_malloc0() and return
> > ENOMEM if the allocation fails.  
> 
> Even if it does not cause an ENOMEM failure right away, the guest can
> also use this to chew up lots of host resources. It may also be worth
> putting a reasonable cap at the maximum the guest can allocate, rather
> than just trying to malloc every possible size.
> 

In the case of v9fs_xattrcreate(), the allocation of the xattr value only
happens if a fid with a specific id was created. This function alone cannot
be used to chew up memory, but it can certainly be used to crash QEMU if the
guest passes an insanely great value.

I fully agree that guest triggered allocations should be capped though,
and the more I look the more I realize the 9p code is fragile on this
matter... This will require more analysis and fixing, which goes far
beyond the scope of preventing an immediate crash.

Cheers.

--
Greg



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[Qemu-devel] [PATCH 2/2] 9pfs: fix memory leak about xattr value

[Li Qiang]

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

The 'fs.xattr.value' field in V9fsFidState object doesn't consider
the situation that this field has been allocated previously. Every
time, it will be allocated directly. This leads a  host memory leak
issue. This patch fix this.

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

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 8751c19..3af23f9 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -350,6 +350,7 @@ free_out:
     v9fs_string_free(&fidp->fs.xattr.name);
 free_value:
     g_free(fidp->fs.xattr.value);
+    fidp->fs.xattr.value = NULL;
     return retval;
 }
 
@@ -3191,7 +3192,8 @@ static void v9fs_xattrwalk(void *opaque)
         xattr_fidp->fid_type = P9_FID_XATTR;
         xattr_fidp->fs.xattr.copied_len = -1;
         if (size) {
-            xattr_fidp->fs.xattr.value = g_malloc(size);
+            xattr_fidp->fs.xattr.value = g_realloc(
+                xattr_fidp->fs.xattr.value, size);
             err = v9fs_co_llistxattr(pdu, &xattr_fidp->path,
                                      xattr_fidp->fs.xattr.value,
                                      xattr_fidp->fs.xattr.len);
@@ -3224,7 +3226,8 @@ static void v9fs_xattrwalk(void *opaque)
         xattr_fidp->fid_type = P9_FID_XATTR;
         xattr_fidp->fs.xattr.copied_len = -1;
         if (size) {
-            xattr_fidp->fs.xattr.value = g_malloc(size);
+            xattr_fidp->fs.xattr.value = g_realloc(
+                xattr_fidp->fs.xattr.value, size);
             err = v9fs_co_lgetxattr(pdu, &xattr_fidp->path,
                                     &name, xattr_fidp->fs.xattr.value,
                                     xattr_fidp->fs.xattr.len);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 2/2] 9pfs: fix memory leak about xattr value

[Greg Kurz]

On Sat,  8 Oct 2016 22:27:08 -0700
Li Qiang <liq3ea@gmail.com> wrote:

> From: Li Qiang <liqiang6-s@360.cn>
> 
> The 'fs.xattr.value' field in V9fsFidState object doesn't consider
> the situation that this field has been allocated previously. Every
> time, it will be allocated directly. This leads a  host memory leak
> issue. This patch fix this.
> 

This cannot happen because v9fs_xattrwalk() always allocates this fid:

    xattr_fidp = alloc_fid(s, newfid);
    if (xattr_fidp == NULL) {
        err = -EINVAL;
        goto out;
    }

> Signed-off-by: Li Qiang <liqiang6-s@360.cn>
> ---
>  hw/9pfs/9p.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index 8751c19..3af23f9 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -350,6 +350,7 @@ free_out:
>      v9fs_string_free(&fidp->fs.xattr.name);
>  free_value:
>      g_free(fidp->fs.xattr.value);
> +    fidp->fs.xattr.value = NULL;
>      return retval;
>  }
>  
> @@ -3191,7 +3192,8 @@ static void v9fs_xattrwalk(void *opaque)
>          xattr_fidp->fid_type = P9_FID_XATTR;
>          xattr_fidp->fs.xattr.copied_len = -1;
>          if (size) {
> -            xattr_fidp->fs.xattr.value = g_malloc(size);
> +            xattr_fidp->fs.xattr.value = g_realloc(
> +                xattr_fidp->fs.xattr.value, size);
>              err = v9fs_co_llistxattr(pdu, &xattr_fidp->path,
>                                       xattr_fidp->fs.xattr.value,
>                                       xattr_fidp->fs.xattr.len);
> @@ -3224,7 +3226,8 @@ static void v9fs_xattrwalk(void *opaque)
>          xattr_fidp->fid_type = P9_FID_XATTR;
>          xattr_fidp->fs.xattr.copied_len = -1;
>          if (size) {
> -            xattr_fidp->fs.xattr.value = g_malloc(size);
> +            xattr_fidp->fs.xattr.value = g_realloc(
> +                xattr_fidp->fs.xattr.value, size);
>              err = v9fs_co_lgetxattr(pdu, &xattr_fidp->path,
>                                      &name, xattr_fidp->fs.xattr.value,
>                                      xattr_fidp->fs.xattr.len);

Re: [Qemu-devel] [PATCH 2/2] 9pfs: fix memory leak about xattr value

[Li Qiang]

Oops! I have made this patch to dress the memory leak in v9fs_xattrcreate
but forget this and make another mistake.

I mean we should call g_free for xattr.value first in xattr create.

This avoid the memory leak.

static void v9fs_xattrcreate(void *opaque)
{
  ...
    g_free(xattr_fidp->fs.xattr.value);
    xattr_fidp->fs.xattr.value = g_malloc(size);
    err = offset;

}

Should I resend this path series?

2016-10-10 17:06 GMT+08:00 Greg Kurz <groug@kaod.org>:

> On Sat,  8 Oct 2016 22:27:08 -0700
> Li Qiang <liq3ea@gmail.com> wrote:
>
> > From: Li Qiang <liqiang6-s@360.cn>
> >
> > The 'fs.xattr.value' field in V9fsFidState object doesn't consider
> > the situation that this field has been allocated previously. Every
> > time, it will be allocated directly. This leads a  host memory leak
> > issue. This patch fix this.
> >
>
> This cannot happen because v9fs_xattrwalk() always allocates this fid:
>
>     xattr_fidp = alloc_fid(s, newfid);
>     if (xattr_fidp == NULL) {
>         err = -EINVAL;
>         goto out;
>     }
>
> > Signed-off-by: Li Qiang <liqiang6-s@360.cn>
> > ---
> >  hw/9pfs/9p.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > index 8751c19..3af23f9 100644
> > --- a/hw/9pfs/9p.c
> > +++ b/hw/9pfs/9p.c
> > @@ -350,6 +350,7 @@ free_out:
> >      v9fs_string_free(&fidp->fs.xattr.name);
> >  free_value:
> >      g_free(fidp->fs.xattr.value);
> > +    fidp->fs.xattr.value = NULL;
> >      return retval;
> >  }
> >
> > @@ -3191,7 +3192,8 @@ static void v9fs_xattrwalk(void *opaque)
> >          xattr_fidp->fid_type = P9_FID_XATTR;
> >          xattr_fidp->fs.xattr.copied_len = -1;
> >          if (size) {
> > -            xattr_fidp->fs.xattr.value = g_malloc(size);
> > +            xattr_fidp->fs.xattr.value = g_realloc(
> > +                xattr_fidp->fs.xattr.value, size);
> >              err = v9fs_co_llistxattr(pdu, &xattr_fidp->path,
> >                                       xattr_fidp->fs.xattr.value,
> >                                       xattr_fidp->fs.xattr.len);
> > @@ -3224,7 +3226,8 @@ static void v9fs_xattrwalk(void *opaque)
> >          xattr_fidp->fid_type = P9_FID_XATTR;
> >          xattr_fidp->fs.xattr.copied_len = -1;
> >          if (size) {
> > -            xattr_fidp->fs.xattr.value = g_malloc(size);
> > +            xattr_fidp->fs.xattr.value = g_realloc(
> > +                xattr_fidp->fs.xattr.value, size);
> >              err = v9fs_co_lgetxattr(pdu, &xattr_fidp->path,
> >                                      &name, xattr_fidp->fs.xattr.value,
> >                                      xattr_fidp->fs.xattr.len);
>
>

Re: [Qemu-devel] [PATCH 2/2] 9pfs: fix memory leak about xattr value

[Greg Kurz]

On Mon, 10 Oct 2016 17:15:10 +0800
Li Qiang <liq3ea@gmail.com> wrote:

> Oops! I have made this patch to dress the memory leak in v9fs_xattrcreate

I guess you mean v9fs_xattrwalk().

There is no possible memory leak with v9fs_xattrwalk() because xattr_fidp
comes from alloc_fid(). The leak can only occur when xattr_fidp comes from
get_fid().

> but forget this and make another mistake.
> 
> I mean we should call g_free for xattr.value first in xattr create.
> 
> This avoid the memory leak.
> 
> static void v9fs_xattrcreate(void *opaque)
> {
>   ...
>     g_free(xattr_fidp->fs.xattr.value);
>     xattr_fidp->fs.xattr.value = g_malloc(size);
>     err = offset;
> 
> }
> 

Indeed we have a potential leak with v9fs_xattrcreate().

> Should I resend this path series?
> 

No, you just need to send a patch that fixes the leak in
v9fs_xattrcreate().

> 2016-10-10 17:06 GMT+08:00 Greg Kurz <groug@kaod.org>:
> 
> > On Sat,  8 Oct 2016 22:27:08 -0700
> > Li Qiang <liq3ea@gmail.com> wrote:
> >
> > > From: Li Qiang <liqiang6-s@360.cn>
> > >
> > > The 'fs.xattr.value' field in V9fsFidState object doesn't consider
> > > the situation that this field has been allocated previously. Every
> > > time, it will be allocated directly. This leads a  host memory leak
> > > issue. This patch fix this.
> > >
> >
> > This cannot happen because v9fs_xattrwalk() always allocates this fid:
> >
> >     xattr_fidp = alloc_fid(s, newfid);
> >     if (xattr_fidp == NULL) {
> >         err = -EINVAL;
> >         goto out;
> >     }
> >
> > > Signed-off-by: Li Qiang <liqiang6-s@360.cn>
> > > ---
> > >  hw/9pfs/9p.c | 7 +++++--
> > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > > index 8751c19..3af23f9 100644
> > > --- a/hw/9pfs/9p.c
> > > +++ b/hw/9pfs/9p.c
> > > @@ -350,6 +350,7 @@ free_out:
> > >      v9fs_string_free(&fidp->fs.xattr.name);
> > >  free_value:
> > >      g_free(fidp->fs.xattr.value);
> > > +    fidp->fs.xattr.value = NULL;
> > >      return retval;
> > >  }
> > >
> > > @@ -3191,7 +3192,8 @@ static void v9fs_xattrwalk(void *opaque)
> > >          xattr_fidp->fid_type = P9_FID_XATTR;
> > >          xattr_fidp->fs.xattr.copied_len = -1;
> > >          if (size) {
> > > -            xattr_fidp->fs.xattr.value = g_malloc(size);
> > > +            xattr_fidp->fs.xattr.value = g_realloc(
> > > +                xattr_fidp->fs.xattr.value, size);
> > >              err = v9fs_co_llistxattr(pdu, &xattr_fidp->path,
> > >                                       xattr_fidp->fs.xattr.value,
> > >                                       xattr_fidp->fs.xattr.len);
> > > @@ -3224,7 +3226,8 @@ static void v9fs_xattrwalk(void *opaque)
> > >          xattr_fidp->fid_type = P9_FID_XATTR;
> > >          xattr_fidp->fs.xattr.copied_len = -1;
> > >          if (size) {
> > > -            xattr_fidp->fs.xattr.value = g_malloc(size);
> > > +            xattr_fidp->fs.xattr.value = g_realloc(
> > > +                xattr_fidp->fs.xattr.value, size);
> > >              err = v9fs_co_lgetxattr(pdu, &xattr_fidp->path,
> > >                                      &name, xattr_fidp->fs.xattr.value,
> > >                                      xattr_fidp->fs.xattr.len);
> >
> >