[Qemu-devel] [Patch] virtio: security patch

[Qemu-devel] [Patch] virtio: security patch

[Supriya Kannery]

For security purpose, convert  'int i' to 'unsigned int i' in
virtio functions, so that range of index is restricted
to positive value.

Signed-off-by: Supriya Kannery (supriyak@linux.vnet.ibm.com)

---
 hw/virtio.c |   27 +++++++++++++++++----------
 hw/virtio.h |    3 ++-
 2 files changed, 19 insertions(+), 11 deletions(-)

Index: qemu/hw/virtio.c
===================================================================
--- qemu.orig/hw/virtio.c
+++ qemu/hw/virtio.c
@@ -101,28 +101,32 @@ static void virtqueue_init(VirtQueue *vq
                                  VIRTIO_PCI_VRING_ALIGN);
 }
 
-static inline uint64_t vring_desc_addr(target_phys_addr_t desc_pa, int i)
+static inline uint64_t vring_desc_addr(target_phys_addr_t desc_pa,
+                                       unsigned int i)
 {
     target_phys_addr_t pa;
     pa = desc_pa + sizeof(VRingDesc) * i + offsetof(VRingDesc, addr);
     return ldq_phys(pa);
 }
 
-static inline uint32_t vring_desc_len(target_phys_addr_t desc_pa, int i)
+static inline uint32_t vring_desc_len(target_phys_addr_t desc_pa,
+                                      unsigned int i)
 {
     target_phys_addr_t pa;
     pa = desc_pa + sizeof(VRingDesc) * i + offsetof(VRingDesc, len);
     return ldl_phys(pa);
 }
 
-static inline uint16_t vring_desc_flags(target_phys_addr_t desc_pa, int i)
+static inline uint16_t vring_desc_flags(target_phys_addr_t desc_pa,
+                                        unsigned int i)
 {
     target_phys_addr_t pa;
     pa = desc_pa + sizeof(VRingDesc) * i + offsetof(VRingDesc, flags);
     return lduw_phys(pa);
 }
 
-static inline uint16_t vring_desc_next(target_phys_addr_t desc_pa, int i)
+static inline uint16_t vring_desc_next(target_phys_addr_t desc_pa,
+                                       unsigned int i)
 {
     target_phys_addr_t pa;
     pa = desc_pa + sizeof(VRingDesc) * i + offsetof(VRingDesc, next);
@@ -143,7 +147,7 @@ static inline uint16_t vring_avail_idx(V
     return lduw_phys(pa);
 }
 
-static inline uint16_t vring_avail_ring(VirtQueue *vq, int i)
+static inline uint16_t vring_avail_ring(VirtQueue *vq, unsigned int i)
 {
     target_phys_addr_t pa;
     pa = vq->vring.avail + offsetof(VRingAvail, ring[i]);
@@ -155,14 +159,16 @@ static inline uint16_t vring_used_event(
     return vring_avail_ring(vq, vq->vring.num);
 }
 
-static inline void vring_used_ring_id(VirtQueue *vq, int i, uint32_t val)
+static inline void vring_used_ring_id(VirtQueue *vq, unsigned int i,
+                                      uint32_t val)
 {
     target_phys_addr_t pa;
     pa = vq->vring.used + offsetof(VRingUsed, ring[i].id);
     stl_phys(pa, val);
 }
 
-static inline void vring_used_ring_len(VirtQueue *vq, int i, uint32_t val)
+static inline void vring_used_ring_len(VirtQueue *vq, unsigned int i,
+                                       uint32_t val)
 {
     target_phys_addr_t pa;
     pa = vq->vring.used + offsetof(VRingUsed, ring[i].len);
@@ -334,10 +340,11 @@ static unsigned virtqueue_next_desc(targ
     return next;
 }
 
-int virtqueue_avail_bytes(VirtQueue *vq, int in_bytes, int out_bytes)
+int virtqueue_avail_bytes(VirtQueue *vq, unsigned int in_bytes,
+                          unsigned int out_bytes)
 {
     unsigned int idx;
-    int total_bufs, in_total, out_total;
+    unsigned int total_bufs, in_total, out_total;
 
     idx = vq->last_avail_idx;
 
@@ -345,7 +352,7 @@ int virtqueue_avail_bytes(VirtQueue *vq,
     while (virtqueue_num_heads(vq, idx)) {
         unsigned int max, num_bufs, indirect = 0;
         target_phys_addr_t desc_pa;
-        int i;
+        unsigned int i;
 
         max = vq->vring.num;
         num_bufs = total_bufs;
Index: qemu/hw/virtio.h
===================================================================
--- qemu.orig/hw/virtio.h
+++ qemu/hw/virtio.h
@@ -156,7 +156,8 @@ void virtqueue_fill(VirtQueue *vq, const
 void virtqueue_map_sg(struct iovec *sg, target_phys_addr_t *addr,
     size_t num_sg, int is_write);
 int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem);
-int virtqueue_avail_bytes(VirtQueue *vq, int in_bytes, int out_bytes);
+int virtqueue_avail_bytes(VirtQueue *vq, unsigned int in_bytes,
+                          unsigned int out_bytes);
 
 void virtio_notify(VirtIODevice *vdev, VirtQueue *vq);
 

Re: [Qemu-devel] [Patch] virtio: security patch

[Stefan Hajnoczi]

On Wed, Aug 3, 2011 at 8:56 AM, Supriya Kannery
<supriyak@linux.vnet.ibm.com> wrote:
> For security purpose, convert  'int i' to 'unsigned int i' in
> virtio functions, so that range of index is restricted
> to positive value.
>
> Signed-off-by: Supriya Kannery (supriyak@linux.vnet.ibm.com)
>
> ---
>  hw/virtio.c |   27 +++++++++++++++++----------
>  hw/virtio.h |    3 ++-
>  2 files changed, 19 insertions(+), 11 deletions(-)

I think this change is good because the ints are accidents waiting to happen.

The commit message should be descriptive though: "virtio: make indices
unsigned".  There is currently no bug in the code AFAICT.  This is not
a security fix, just a cleanup to make the code safer.

Stefan