[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[Xen-devel] [PATCH] linux-2.6.18/gntdev: add range check for IOCTL_GNTDEV_UNMAP_GRANT_REF arguments,



..., 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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.