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  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  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 Wed, 12 Oct 2016 15:49:46 -0500
Eric Blake  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




pgpUPo0OeukPa.pgp
Description: OpenPGP digital signature

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  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

[Eric Blake]

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



signature.asc
Description: OpenPGP digital signature

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  wrote:

> On Sat,  8 Oct 2016 22:26:51 -0700
> Li Qiang  wrote:
> 
> > From: Li Qiang 
> > 
> > 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 
> > ---  
> 
> 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 
> 

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(_fidp->fs.xattr.name);
> >  v9fs_string_copy(_fidp->fs.xattr.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 Sat,  8 Oct 2016 22:26:51 -0700
Li Qiang  wrote:

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

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 

>  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(_fidp->fs.xattr.name);
>  v9fs_string_copy(_fidp->fs.xattr.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:

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

[Li Qiang]

From: Li Qiang 

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 
---
 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(_fidp->fs.xattr.name);
 v9fs_string_copy(_fidp->fs.xattr.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