WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

[Xen-devel] [PATCH] linux-2.6.18/gntdev: add range check for IOCTL_GNTDE

To: "xen-devel@xxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxx>
Subject: [Xen-devel] [PATCH] linux-2.6.18/gntdev: add range check for IOCTL_GNTDEV_UNMAP_GRANT_REF arguments,
From: "Jan Beulich" <JBeulich@xxxxxxxxxx>
Date: Fri, 17 Dec 2010 09:48:24 +0000
Delivery-date: Fri, 17 Dec 2010 01:49:10 -0800
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
..., move all user memory accesses out of semaphore protected region,
simplify, clean up, and remove some bogus printk()-s in gntdev_ioctl()
and its helpers.

The purpose of free_list_sem seems questionable: It is only ever being
acquired for write access, and always with grants_sem already held
in write mode.

Signed-off-by: Jan Beulich <jbeulich@xxxxxxxxxx>

--- a/drivers/xen/gntdev/gntdev.c
+++ b/drivers/xen/gntdev/gntdev.c
@@ -203,19 +203,12 @@ static inline unsigned long get_kernel_v
  * value of *offset to the offset that should be mmap()-ed in order to map the
  * grant reference.
  */
-static int add_grant_reference(struct file *flip,
+static int add_grant_reference(gntdev_file_private_data_t *private_data,
                               struct ioctl_gntdev_grant_ref *op,
                               uint64_t *offset)
 {
-       gntdev_file_private_data_t *private_data 
-               = (gntdev_file_private_data_t *) flip->private_data;
-
        uint32_t slot_index;
 
-       if (unlikely(private_data->free_list_size == 0)) {
-               return -ENOMEM;
-       }
-
        slot_index = private_data->free_list[--private_data->free_list_size];
        private_data->free_list[private_data->free_list_size]
                = GNTDEV_FREE_LIST_INVALID;
@@ -239,19 +232,17 @@ static int add_grant_reference(struct fi
  * previous invocation of find_contiguous_free_range(), during the same
  * invocation of the driver.
  */
-static int add_grant_references(struct file *flip,
-                               int count,
+static int add_grant_references(gntdev_file_private_data_t *private_data,
+                               uint32_t count,
                                struct ioctl_gntdev_grant_ref *ops,
                                uint32_t first_slot)
 {
-       gntdev_file_private_data_t *private_data 
-               = (gntdev_file_private_data_t *) flip->private_data;
-       int i;
+       uint32_t i;
        
        for (i = 0; i < count; ++i) {
 
                /* First, mark the slot's entry in the free list as invalid. */
-               int free_list_index = 
+               uint32_t free_list_index =
                        private_data->grants[first_slot+i].u.free_list_index;
                private_data->free_list[free_list_index] = 
                        GNTDEV_FREE_LIST_INVALID;
@@ -271,16 +262,16 @@ static int add_grant_references(struct f
  * GNTDEV_SLOT_INVALID. This will reduce the recorded size of the free list to
  * the number of valid entries.
  */
-static void compress_free_list(struct file *flip) 
+static void compress_free_list(gntdev_file_private_data_t *private_data)
 {
-       gntdev_file_private_data_t *private_data 
-               = (gntdev_file_private_data_t *) flip->private_data;
-       int i, j = 0, old_size, slot_index;
+       uint32_t i, j = 0, old_size;
        
        old_size = private_data->free_list_size;
        for (i = 0; i < old_size; ++i) {
                if (private_data->free_list[i] != GNTDEV_FREE_LIST_INVALID) {
                        if (i > j) {
+                               int32_t slot_index;
+
                                slot_index = private_data->free_list[i];
                                private_data->free_list[j] = slot_index;
                                private_data->grants[slot_index].u
@@ -300,19 +291,11 @@ static void compress_free_list(struct fi
  *
  * Returns the index of the first slot if a range is found, otherwise -ENOMEM.
  */
-static int find_contiguous_free_range(struct file *flip,
+static int find_contiguous_free_range(gntdev_file_private_data_t *private_data,
                                      uint32_t num_slots) 
 {
-       gntdev_file_private_data_t *private_data 
-               = (gntdev_file_private_data_t *) flip->private_data;
-       
-       int i;
-       int start_index = private_data->next_fit_index;
-       int range_start = 0, range_length;
-
-       if (private_data->free_list_size < num_slots) {
-               return -ENOMEM;
-       }
+       uint32_t i, start_index = private_data->next_fit_index;
+       uint32_t range_start = 0, range_length;
 
        /* First search from the start_index to the end of the array. */
        range_length = 0;
@@ -872,89 +855,82 @@ private_data_initialised:
        case IOCTL_GNTDEV_MAP_GRANT_REF:
        {
                struct ioctl_gntdev_map_grant_ref op;
+               struct ioctl_gntdev_grant_ref *refs = NULL;
+
+               if (copy_from_user(&op, (void __user *)arg, sizeof(op)))
+                       return -EFAULT;
+               if (unlikely(op.count <= 0))
+                       return -EINVAL;
+
+               if (op.count > 1 && op.count <= private_data->grants_size) {
+                       struct ioctl_gntdev_grant_ref *u;
+
+                       refs = kmalloc(op.count * sizeof(*refs), GFP_KERNEL);
+                       if (!refs)
+                               return -ENOMEM;
+                       u = ((struct ioctl_gntdev_map_grant_ref *)arg)->refs;
+                       if (copy_from_user(refs, (void __user *)u,
+                                          sizeof(*refs) * op.count)) {
+                               kfree(refs);
+                               return -EFAULT;
+                       }
+               }
+
                down_write(&private_data->grants_sem);
                down_write(&private_data->free_list_sem);
 
-               if ((rc = copy_from_user(&op, (void __user *) arg, 
-                                        sizeof(op)))) {
-                       rc = -EFAULT;
-                       goto map_out;
-               }
-               if (unlikely(op.count <= 0)) {
-                       rc = -EINVAL;
+               if (unlikely(op.count > private_data->free_list_size)) {
+                       rc = -ENOMEM;
                        goto map_out;
                }
 
                if (op.count == 1) {
-                       if ((rc = add_grant_reference(flip, &op.refs[0],
+                       if ((rc = add_grant_reference(private_data, op.refs,
                                                      &op.index)) < 0) {
                                printk(KERN_ERR "Adding grant reference "
                                       "failed (%d).\n", rc);
                                goto map_out;
                        }
                } else {
-                       struct ioctl_gntdev_grant_ref *refs, *u;
-                       refs = kmalloc(op.count * sizeof(*refs), GFP_KERNEL);
-                       if (!refs) {
-                               rc = -ENOMEM;
-                               goto map_out;
-                       }
-                       u = ((struct ioctl_gntdev_map_grant_ref *)arg)->refs;
-                       if ((rc = copy_from_user(refs,
-                                                (void __user *)u,
-                                                sizeof(*refs) * op.count))) {
-                               printk(KERN_ERR "Copying refs from user failed"
-                                      " (%d).\n", rc);
-                               rc = -EINVAL;
-                               goto map_out;
-                       }
-                       if ((rc = find_contiguous_free_range(flip, op.count))
-                           < 0) {
+                       if ((rc = find_contiguous_free_range(private_data,
+                                                            op.count)) < 0) {
                                printk(KERN_ERR "Finding contiguous range "
                                       "failed (%d).\n", rc);
-                               kfree(refs);
                                goto map_out;
                        }
                        op.index = rc << PAGE_SHIFT;
-                       if ((rc = add_grant_references(flip, op.count,
+                       if ((rc = add_grant_references(private_data, op.count,
                                                       refs, rc))) {
                                printk(KERN_ERR "Adding grant references "
                                       "failed (%d).\n", rc);
-                               kfree(refs);
                                goto map_out;
                        }
-                       compress_free_list(flip);
-                       kfree(refs);
-               }
-               if ((rc = copy_to_user((void __user *) arg, 
-                                      &op, 
-                                      sizeof(op)))) {
-                       printk(KERN_ERR "Copying result back to user failed "
-                              "(%d)\n", rc);
-                       rc = -EFAULT;
-                       goto map_out;
+                       compress_free_list(private_data);
                }
+
        map_out:
-               up_write(&private_data->grants_sem);
                up_write(&private_data->free_list_sem);
+               up_write(&private_data->grants_sem);
+
+               kfree(refs);
+
+               if (!rc && copy_to_user((void __user *)arg, &op, sizeof(op)))
+                       rc = -EFAULT;
                return rc;
        }
        case IOCTL_GNTDEV_UNMAP_GRANT_REF:
        {
                struct ioctl_gntdev_unmap_grant_ref op;
-               int i, start_index;
+               uint32_t i, start_index;
 
-               down_write(&private_data->grants_sem);
-               down_write(&private_data->free_list_sem);
-
-               if ((rc = copy_from_user(&op, 
-                                        (void __user *) arg, 
-                                        sizeof(op)))) {
-                       rc = -EFAULT;
-                       goto unmap_out;
-               }
+               if (copy_from_user(&op, (void __user *)arg, sizeof(op)))
+                       return -EFAULT;
 
                start_index = op.index >> PAGE_SHIFT;
+               if (start_index + op.count > private_data->grants_size)
+                       return -EINVAL;
+
+               down_write(&private_data->grants_sem);
 
                /* First, check that all pages are in the NOT_YET_MAPPED
                 * state.
@@ -984,6 +960,8 @@ private_data_initialised:
                        }
                }
 
+               down_write(&private_data->free_list_sem);
+
                /* Unmap pages and add them to the free list.
                 */
                for (i = 0; i < op.count; ++i) {
@@ -996,9 +974,9 @@ private_data_initialised:
                        ++private_data->free_list_size;
                }
 
+               up_write(&private_data->free_list_sem);
        unmap_out:
                up_write(&private_data->grants_sem);
-               up_write(&private_data->free_list_sem);
                return rc;
        }
        case IOCTL_GNTDEV_GET_OFFSET_FOR_VADDR:
@@ -1007,25 +985,16 @@ private_data_initialised:
                struct vm_area_struct *vma;
                unsigned long vaddr;
 
-               if ((rc = copy_from_user(&op, 
-                                        (void __user *) arg, 
-                                        sizeof(op)))) {
-                       rc = -EFAULT;
-                       goto get_offset_out;
-               }
+               if (copy_from_user(&op, (void __user *)arg, sizeof(op)))
+                       return -EFAULT;
+
                vaddr = (unsigned long)op.vaddr;
 
                down_read(&current->mm->mmap_sem);              
                vma = find_vma(current->mm, vaddr);
-               if (vma == NULL) {
-                       rc = -EFAULT;
-                       goto get_offset_unlock_out;
-               }
-               if ((!vma->vm_ops) || (vma->vm_ops != &gntdev_vmops)) {
-                       printk(KERN_ERR "The vaddr specified does not belong "
-                              "to a gntdev instance: %#lx\n", vaddr);
+               if (!vma || vma->vm_ops != &gntdev_vmops) {
                        rc = -EFAULT;
-                       goto get_offset_unlock_out;
+                       goto get_offset_out;
                }
                if (vma->vm_start != vaddr) {
                        printk(KERN_ERR "The vaddr specified in an "
@@ -1034,45 +1003,31 @@ private_data_initialised:
                               "%#lx; vaddr = %#lx\n",
                               vma->vm_start, vaddr);
                        rc = -EFAULT;
-                       goto get_offset_unlock_out;
+                       goto get_offset_out;
                }
                op.offset = vma->vm_pgoff << PAGE_SHIFT;
                op.count = (vma->vm_end - vma->vm_start) >> PAGE_SHIFT;
+       get_offset_out:
                up_read(&current->mm->mmap_sem);
-               if ((rc = copy_to_user((void __user *) arg, 
-                                      &op, 
-                                      sizeof(op)))) {
+               if (!rc && copy_to_user((void __user *)arg, &op, sizeof(op)))
                        rc = -EFAULT;
-                       goto get_offset_out;
-               }
-               goto get_offset_out;
-       get_offset_unlock_out:
-               up_read(&current->mm->mmap_sem);
-       get_offset_out:
                return rc;
        }
        case IOCTL_GNTDEV_SET_MAX_GRANTS:
        {
                struct ioctl_gntdev_set_max_grants op;
-               if ((rc = copy_from_user(&op, 
-                                        (void __user *) arg, 
-                                        sizeof(op)))) {
-                       rc = -EFAULT;
-                       goto set_max_out;
-               }
+
+               if (copy_from_user(&op, (void __user *)arg, sizeof(op)))
+                       return -EFAULT;
+               if (op.count > MAX_GRANTS_LIMIT)
+                       return -EINVAL;
+
                down_write(&private_data->grants_sem);
-               if (private_data->grants) {
+               if (unlikely(private_data->grants))
                        rc = -EBUSY;
-                       goto set_max_unlock_out;
-               }
-               if (op.count > MAX_GRANTS_LIMIT) {
-                       rc = -EINVAL;
-                       goto set_max_unlock_out;
-               }                                                
-               rc = init_private_data(private_data, op.count);
-       set_max_unlock_out:
+               else
+                       rc = init_private_data(private_data, op.count);
                up_write(&private_data->grants_sem);
-       set_max_out:
                return rc;
        }
        default:


Attachment: xen-gntdev-ioctl-checks.patch
Description: Text document

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
<Prev in Thread] Current Thread [Next in Thread>
  • [Xen-devel] [PATCH] linux-2.6.18/gntdev: add range check for IOCTL_GNTDEV_UNMAP_GRANT_REF arguments,, Jan Beulich <=