[Qemu-devel] [PATCH 0/4] fix several bugs about use-after-free and an api abuse

[Qemu-devel] [PATCH 0/4] fix several bugs about use-after-free and an api abuse

[zhanghailiang]

zhanghailiang (4):
  l2cap: fix access freed memory
  monitor: fix access freed memory
  virtio-blk: fix reference a pointer which might be freed
  ivshmem: check the value returned by fstat()

 hw/block/virtio-blk.c | 5 +++--
 hw/bt/l2cap.c         | 2 +-
 hw/misc/ivshmem.c     | 5 ++++-
 monitor.c             | 4 +++-
 4 files changed, 11 insertions(+), 5 deletions(-)

-- 
1.7.12.4

[Qemu-devel] [PATCH 1/4] l2cap: fix access freed memory

[zhanghailiang]

Pointer 'ch' will be used in function 'l2cap_channel_open_req_msg' after
it was previously freed in 'l2cap_channel_open'.
Assigned it to NULL after it is freed.

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
---
 hw/bt/l2cap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/bt/l2cap.c b/hw/bt/l2cap.c
index 2301d6f..591e047 100644
--- a/hw/bt/l2cap.c
+++ b/hw/bt/l2cap.c
@@ -429,7 +429,7 @@ static struct l2cap_chan_s *l2cap_channel_open(struct l2cap_instance_s *l2cap,
                 status = L2CAP_CS_NO_INFO;
             } else {
                 g_free(ch);
-
+                ch = NULL;
                 result = L2CAP_CR_NO_MEM;
                 status = L2CAP_CS_NO_INFO;
             }
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH 1/4] l2cap: fix access freed memory

[Alex Bennée]

zhanghailiang writes:

> Pointer 'ch' will be used in function 'l2cap_channel_open_req_msg' after
> it was previously freed in 'l2cap_channel_open'.
> Assigned it to NULL after it is freed.
>
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> ---
>  hw/bt/l2cap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/bt/l2cap.c b/hw/bt/l2cap.c
> index 2301d6f..591e047 100644
> --- a/hw/bt/l2cap.c
> +++ b/hw/bt/l2cap.c
> @@ -429,7 +429,7 @@ static struct l2cap_chan_s *l2cap_channel_open(struct l2cap_instance_s *l2cap,
>                  status = L2CAP_CS_NO_INFO;
>              } else {
>                  g_free(ch);
> -
> +                ch = NULL;
>                  result = L2CAP_CR_NO_MEM;
>                  status = L2CAP_CS_NO_INFO;
>              }

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée

[Qemu-devel] [PATCH 2/4] monitor: fix access freed memory

[zhanghailiang]

The function monitor_fdset_dup_fd_find_remove() references member of 'mon_fdset'
which may be freed in function monitor_fdset_cleanup()

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
---
 monitor.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/monitor.c b/monitor.c
index 5bc70a6..41e46a6 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2532,8 +2532,10 @@ static int monitor_fdset_dup_fd_find_remove(int dup_fd, bool remove)
 {
     MonFdset *mon_fdset;
     MonFdsetFd *mon_fdset_fd_dup;
+    int64_t id = -1;
 
     QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
+        id = mon_fdset->id;
         QLIST_FOREACH(mon_fdset_fd_dup, &mon_fdset->dup_fds, next) {
             if (mon_fdset_fd_dup->fd == dup_fd) {
                 if (remove) {
@@ -2542,7 +2544,7 @@ static int monitor_fdset_dup_fd_find_remove(int dup_fd, bool remove)
                         monitor_fdset_cleanup(mon_fdset);
                     }
                 }
-                return mon_fdset->id;
+                return id;
             }
         }
     }
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH 2/4] monitor: fix access freed memory

[Alex Bennée]

zhanghailiang writes:

> The function monitor_fdset_dup_fd_find_remove() references member of 'mon_fdset'
> which may be freed in function monitor_fdset_cleanup()
>
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> ---
>  monitor.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/monitor.c b/monitor.c
> index 5bc70a6..41e46a6 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -2532,8 +2532,10 @@ static int monitor_fdset_dup_fd_find_remove(int dup_fd, bool remove)
>  {
>      MonFdset *mon_fdset;
>      MonFdsetFd *mon_fdset_fd_dup;
> +    int64_t id = -1;
>  
>      QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
> +        id = mon_fdset->id;
>          QLIST_FOREACH(mon_fdset_fd_dup, &mon_fdset->dup_fds, next) {
>              if (mon_fdset_fd_dup->fd == dup_fd) {
>                  if (remove) {
> @@ -2542,7 +2544,7 @@ static int monitor_fdset_dup_fd_find_remove(int dup_fd, bool remove)
>                          monitor_fdset_cleanup(mon_fdset);
>                      }
>                  }
> -                return mon_fdset->id;
> +                return id;
>              }
>          }
>      }

If monitor_fdset_cleanup closes the FD won't you now be passing an
invalid fd to the calling function?


-- 
Alex Bennée

Re: [Qemu-devel] [PATCH 2/4] monitor: fix access freed memory

[zhanghailiang]

Hi Alex,
Thanks for your quick reply.
On 2014/8/4 17:01, Alex Bennée wrote:
>
> zhanghailiang writes:
>
>> The function monitor_fdset_dup_fd_find_remove() references member of 'mon_fdset'
>> which may be freed in function monitor_fdset_cleanup()
>>
>> Signed-off-by: zhanghailiang<zhang.zhanghailiang@huawei.com>
>> ---
>>   monitor.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/monitor.c b/monitor.c
>> index 5bc70a6..41e46a6 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -2532,8 +2532,10 @@ static int monitor_fdset_dup_fd_find_remove(int dup_fd, bool remove)
>>   {
>>       MonFdset *mon_fdset;
>>       MonFdsetFd *mon_fdset_fd_dup;
>> +    int64_t id = -1;
>>
>>       QLIST_FOREACH(mon_fdset,&mon_fdsets, next) {
>> +        id = mon_fdset->id;
>>           QLIST_FOREACH(mon_fdset_fd_dup,&mon_fdset->dup_fds, next) {
>>               if (mon_fdset_fd_dup->fd == dup_fd) {
>>                   if (remove) {
>> @@ -2542,7 +2544,7 @@ static int monitor_fdset_dup_fd_find_remove(int dup_fd, bool remove)
>>                           monitor_fdset_cleanup(mon_fdset);
>>                       }
>>                   }
>> -                return mon_fdset->id;
>> +                return id;
>>               }
>>           }
>>       }
>
> If monitor_fdset_cleanup closes the FD won't you now be passing an
							  ^^^^^^^^^^
> invalid fd to the calling function?
^^^^^^^^^^^^^
Sorry for my bad english, do you mean return an invalid id to the caller?
I think there is no problem, and the calling process is:
qemu_close()
(1)|--->fdset_id = monitor_fdset_dup_fd_find
        |--->monitor_fdset_dup_fd_find_remove(dup_fd, false)
            |--->monitor_fdset_dup_fd_find_remove
                |--->>>remove=false, no problem to return id or -1.
(2)|--->(ingore return value)monitor_fdset_dup_fd_remove
        |--->monitor_fdset_dup_fd_find_remove(dup_fd, true)
            |--->monitor_fdset_dup_fd_find_remove
                |--->>>remove=true, mon_fdset may be freed, and fault to 
return mon_fdset->id
As you see above, monitor_fdset_dup_fd_find_remove() has two 
capabilities (find/remove) which distinguished by 'bool remove'. It may 
free the mon_fdset only when remove=true, as show step(2), return value 
id will be ignored, so it is no problem to return an invaild id in this 
case.
Here we just solve the bug of accessing freed memory but not change its 
original logic.

Thanks,
zhanghailiang

[Qemu-devel] [PATCH 3/4] virtio-blk: fix reference a pointer which might be freed

[zhanghailiang]

In function virtio_blk_handle_request, it may freed memory pointed by req,
So do not access member of req after calling this function.

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
---
 hw/block/virtio-blk.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index c241c50..54a853a 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -458,7 +458,7 @@ static void virtio_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq)
 static void virtio_blk_dma_restart_bh(void *opaque)
 {
     VirtIOBlock *s = opaque;
-    VirtIOBlockReq *req = s->rq;
+    VirtIOBlockReq *req = s->rq, *next = NULL;
     MultiReqBuffer mrb = {
         .num_writes = 0,
     };
@@ -469,8 +469,9 @@ static void virtio_blk_dma_restart_bh(void *opaque)
     s->rq = NULL;
 
     while (req) {
+        next = req->next;
         virtio_blk_handle_request(req, &mrb);
-        req = req->next;
+        req = next;
     }
 
     virtio_submit_multiwrite(s->bs, &mrb);
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH 3/4] virtio-blk: fix reference a pointer which might be freed

[Stefan Hajnoczi]

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

On Mon, Aug 04, 2014 at 04:25:43PM +0800, zhanghailiang wrote:
> In function virtio_blk_handle_request, it may freed memory pointed by req,
> So do not access member of req after calling this function.
> 
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> ---
>  hw/block/virtio-blk.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

[Qemu-devel] [PATCH 4/4] ivshmem: check the value returned by fstat()

[zhanghailiang]

The function fstat() may fail, so check its return value.

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
---
 hw/misc/ivshmem.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index 768e528..2667e9f 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -324,7 +324,10 @@ static int check_shm_size(IVShmemState *s, int fd) {
 
     struct stat buf;
 
-    fstat(fd, &buf);
+    if (fstat(fd, &buf) < 0) {
+        fprintf(stderr, "Cannot stat IVSHMEM: %s\n", strerror(errno));
+        return -1;
+    }
 
     if (s->ivshmem_size > buf.st_size) {
         fprintf(stderr,
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH 4/4] ivshmem: check the value returned by fstat()

[Michael S. Tsirkin]

On Mon, Aug 04, 2014 at 04:25:44PM +0800, zhanghailiang wrote:
> The function fstat() may fail, so check its return value.
> 
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> ---
>  hw/misc/ivshmem.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index 768e528..2667e9f 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -324,7 +324,10 @@ static int check_shm_size(IVShmemState *s, int fd) {
>  
>      struct stat buf;
>  
> -    fstat(fd, &buf);
> +    if (fstat(fd, &buf) < 0) {
> +        fprintf(stderr, "Cannot stat IVSHMEM: %s\n", strerror(errno));
> +        return -1;
> +    }
>

That's a confusing error message:
1. You don't stat ivshmem. You stat a shmem fd. Also best to print fd #.
2. Tell the user what action was taken, e.g. IVSHMEM failed to start.
  
>      if (s->ivshmem_size > buf.st_size) {
>          fprintf(stderr,
> -- 
> 1.7.12.4
> 

Re: [Qemu-devel] [PATCH 4/4] ivshmem: check the value returned by fstat()

[zhanghailiang]

Hi Michael,
Thanks for your review of this patch!

> On Mon, Aug 04, 2014 at 04:25:44PM +0800, zhanghailiang wrote:
>> The function fstat() may fail, so check its return value.
>>
>> Signed-off-by: zhanghailiang<zhang.zhanghailiang@huawei.com>
>> ---
>>   hw/misc/ivshmem.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
>> index 768e528..2667e9f 100644
>> --- a/hw/misc/ivshmem.c
>> +++ b/hw/misc/ivshmem.c
>> @@ -324,7 +324,10 @@ static int check_shm_size(IVShmemState *s, int fd) {
>>
>>       struct stat buf;
>>
>> -    fstat(fd,&buf);
>> +    if (fstat(fd,&buf)<  0) {
>> +        fprintf(stderr, "Cannot stat IVSHMEM: %s\n", strerror(errno));
>> +        return -1;
>> +    }
>>
>
> That's a confusing error message:
> 1. You don't stat ivshmem. You stat a shmem fd. Also best to print fd #.
I will add this in next version of the patch.
> 2. Tell the user what action was taken, e.g. IVSHMEM failed to start.
>
>>       if (s->ivshmem_size>  buf.st_size) {
>>           fprintf(stderr,
>> --
I have check the places of calling this function, And found that, if 
this function return -1, qemu will call exit(-1). One of the callers is 
ivshmem_read(), the purpose of check_shm_size() is to forbid guest to 
map more memory than the object has allocated. So here is it suitable to 
return -1 if fstat() failed? Or just give a warning message and return 0?
what's your opinion? Thanks.
>> 1.7.12.4
>>
>
> .
>

Best regards,
zhanghailiang

Re: [Qemu-devel] [PATCH 4/4] ivshmem: check the value returned by fstat()

[Levente Kurusa]

----- Original Message -----
> Hi Michael,
> Thanks for your review of this patch!
> 
> > On Mon, Aug 04, 2014 at 04:25:44PM +0800, zhanghailiang wrote:
> >> The function fstat() may fail, so check its return value.
> >>
> >> Signed-off-by: zhanghailiang<zhang.zhanghailiang@huawei.com>
> >> ---
> >>   hw/misc/ivshmem.c | 5 ++++-
> >>   1 file changed, 4 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> >> index 768e528..2667e9f 100644
> >> --- a/hw/misc/ivshmem.c
> >> +++ b/hw/misc/ivshmem.c
> >> @@ -324,7 +324,10 @@ static int check_shm_size(IVShmemState *s, int fd) {
> >>
> >>       struct stat buf;
> >>
> >> -    fstat(fd,&buf);
> >> +    if (fstat(fd,&buf)<  0) {
> >> +        fprintf(stderr, "Cannot stat IVSHMEM: %s\n", strerror(errno));
> >> +        return -1;
> >> +    }
> >>
> >
> > That's a confusing error message:
> > 1. You don't stat ivshmem. You stat a shmem fd. Also best to print fd #.
> I will add this in next version of the patch.
> > 2. Tell the user what action was taken, e.g. IVSHMEM failed to start.
> >
> >>       if (s->ivshmem_size>  buf.st_size) {
> >>           fprintf(stderr,
> >> --
> I have check the places of calling this function, And found that, if
> this function return -1, qemu will call exit(-1). One of the callers is
> ivshmem_read(), the purpose of check_shm_size() is to forbid guest to
> map more memory than the object has allocated. So here is it suitable to
> return -1 if fstat() failed? Or just give a warning message and return 0?
> what's your opinion? Thanks.

Yes, please return -1. All the errors fstat(2) can return in this scenario
will be fatal to ivshmem.

Exiting is probably not the best way to go, but I'm working on a fix for
that at the moment, and for now, let's just exit, like all the other error
paths do.

Thanks,
Levente Kurusa

Re: [Qemu-devel] [PATCH 4/4] ivshmem: check the value returned by fstat()

[zhanghailiang]

On 2014/8/5 14:50, Levente Kurusa wrote:
> ----- Original Message -----
>> Hi Michael,
>> Thanks for your review of this patch!
>>
>>> On Mon, Aug 04, 2014 at 04:25:44PM +0800, zhanghailiang wrote:
>>>> The function fstat() may fail, so check its return value.
>>>>
>>>> Signed-off-by: zhanghailiang<zhang.zhanghailiang@huawei.com>
>>>> ---
>>>>    hw/misc/ivshmem.c | 5 ++++-
>>>>    1 file changed, 4 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
>>>> index 768e528..2667e9f 100644
>>>> --- a/hw/misc/ivshmem.c
>>>> +++ b/hw/misc/ivshmem.c
>>>> @@ -324,7 +324,10 @@ static int check_shm_size(IVShmemState *s, int fd) {
>>>>
>>>>        struct stat buf;
>>>>
>>>> -    fstat(fd,&buf);
>>>> +    if (fstat(fd,&buf)<   0) {
>>>> +        fprintf(stderr, "Cannot stat IVSHMEM: %s\n", strerror(errno));
>>>> +        return -1;
>>>> +    }
>>>>
>>>
>>> That's a confusing error message:
>>> 1. You don't stat ivshmem. You stat a shmem fd. Also best to print fd #.
>> I will add this in next version of the patch.
>>> 2. Tell the user what action was taken, e.g. IVSHMEM failed to start.
>>>
>>>>        if (s->ivshmem_size>   buf.st_size) {
>>>>            fprintf(stderr,
>>>> --
>> I have check the places of calling this function, And found that, if
>> this function return -1, qemu will call exit(-1). One of the callers is
>> ivshmem_read(), the purpose of check_shm_size() is to forbid guest to
>> map more memory than the object has allocated. So here is it suitable to
>> return -1 if fstat() failed? Or just give a warning message and return 0?
>> what's your opinion? Thanks.
>
> Yes, please return -1. All the errors fstat(2) can return in this scenario
> will be fatal to ivshmem.
>
> Exiting is probably not the best way to go, but I'm working on a fix for
> that at the moment, and for now, let's just exit, like all the other error
> paths do.
>
> Thanks,
> Levente Kurusa
>

OK,thanks!

Best regards,
zhanghailiang

Re: [Qemu-devel] [PATCH 4/4] ivshmem: check the value returned by fstat()

[Michael S. Tsirkin]

On Tue, Aug 05, 2014 at 10:00:41AM +0800, zhanghailiang wrote:
> Hi Michael,
> Thanks for your review of this patch!
> 
> >On Mon, Aug 04, 2014 at 04:25:44PM +0800, zhanghailiang wrote:
> >>The function fstat() may fail, so check its return value.
> >>
> >>Signed-off-by: zhanghailiang<zhang.zhanghailiang@huawei.com>
> >>---
> >>  hw/misc/ivshmem.c | 5 ++++-
> >>  1 file changed, 4 insertions(+), 1 deletion(-)
> >>
> >>diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> >>index 768e528..2667e9f 100644
> >>--- a/hw/misc/ivshmem.c
> >>+++ b/hw/misc/ivshmem.c
> >>@@ -324,7 +324,10 @@ static int check_shm_size(IVShmemState *s, int fd) {
> >>
> >>      struct stat buf;
> >>
> >>-    fstat(fd,&buf);
> >>+    if (fstat(fd,&buf)<  0) {
> >>+        fprintf(stderr, "Cannot stat IVSHMEM: %s\n", strerror(errno));
> >>+        return -1;
> >>+    }
> >>
> >
> >That's a confusing error message:
> >1. You don't stat ivshmem. You stat a shmem fd. Also best to print fd #.
> I will add this in next version of the patch.
> >2. Tell the user what action was taken, e.g. IVSHMEM failed to start.
> >
> >>      if (s->ivshmem_size>  buf.st_size) {
> >>          fprintf(stderr,
> >>--
> I have check the places of calling this function, And found that, if this
> function return -1, qemu will call exit(-1). One of the callers is
> ivshmem_read(), the purpose of check_shm_size() is to forbid guest to map
> more memory than the object has allocated. So here is it suitable to return
> -1 if fstat() failed? Or just give a warning message and return 0?
> what's your opinion? Thanks.

So put "exiting" In the error message then.

> >>1.7.12.4
> >>
> >
> >.
> >
> 
> Best regards,
> zhanghailiang