| Message ID | 6aefc5ee17ba6ac636de56bba8e7f24fd0262187.1475990063.git.liqiang6-s@360.cn |
|---|---|
| State | New |
| Headers | show |
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:
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: > >
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.
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 > >
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
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 > > > >
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: