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

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

[zhanghailiang]

v1 -> v2:
-ivshmem: modified the log message according to reviewing suggestion of Michael

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     | 6 +++++-
 monitor.c             | 4 +++-
 4 files changed, 12 insertions(+), 5 deletions(-)

-- 
1.7.12.4

[Qemu-devel] [PATCH v2 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.

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
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

[Qemu-devel] [PATCH v2 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

[Qemu-devel] [PATCH v2 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.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
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

[Qemu-devel] [PATCH v2 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 | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

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

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

[Levente Kurusa]

> The function fstat() may fail, so check its return value.
> 
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> ---
>  hw/misc/ivshmem.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index 768e528..5d939d2 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -324,7 +324,11 @@ static int check_shm_size(IVShmemState *s, int fd) {
>  
>      struct stat buf;
>  
> -    fstat(fd, &buf);
> +    if (fstat(fd, &buf) < 0) {
> +        fprintf(stderr, "ivshmem: exiting for fstat fd '%d' failed: %s\n",
> +                fd, strerror(errno));

exiting for fstat?

I would go with something like this:

"ivshmem: exiting: fstat on fd %d failed: %s"

... or something among the lines.

Thanks!
Levente Kurusa

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

[zhanghailiang]

On 2014/8/6 20:30, Levente Kurusa wrote:
>> The function fstat() may fail, so check its return value.
>>
>> Signed-off-by: zhanghailiang<zhang.zhanghailiang@huawei.com>
>> ---
>>   hw/misc/ivshmem.c | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
>> index 768e528..5d939d2 100644
>> --- a/hw/misc/ivshmem.c
>> +++ b/hw/misc/ivshmem.c
>> @@ -324,7 +324,11 @@ static int check_shm_size(IVShmemState *s, int fd) {
>>
>>       struct stat buf;
>>
>> -    fstat(fd,&buf);
>> +    if (fstat(fd,&buf)<  0) {
>> +        fprintf(stderr, "ivshmem: exiting for fstat fd '%d' failed: %s\n",
>> +                fd, strerror(errno));
>
> exiting for fstat?
>
> I would go with something like this:
>
> "ivshmem: exiting: fstat on fd %d failed: %s"
>
> ... or something among the lines.

Hmmm, this is more clear, i will change it.Thanks!