[Xen-changelog] [linux-2.6.18-xen] blktap: don't access deallocated data

["Xen patchbot-linux-2.6.18-xen" <patchbot-linux-2.6.18-xen@xxxxxxxxxxxxxxxxxxx>]

# HG changeset patch
# User Keir Fraser <keir.fraser@xxxxxxxxxx>
# Date 1239969802 -3600
# Node ID 464a925d73f141d8ca568026ee2e6635489affa2
# Parent  dfd2adc5874021b52c13d317df1f55b46ec38e3d
blktap: don't access deallocated data

Dereferencing filp->private_data->vma in the file_operations.release
actor isn't permitted, as the vma generally has been destroyed by that
time. The kfree()ing of vma->vm_private_data must be done in the
vm_operations.close actor, and the call to zap_page_range() seems
redundant with the caller of that actor altogether.

Signed-off-by: Jan Beulich <jbeulich@xxxxxxxxxx>
---
 drivers/xen/blktap/blktap.c |   69 +++++++++++++++++++++-----------------------
 1 files changed, 33 insertions(+), 36 deletions(-)

diff -r dfd2adc58740 -r 464a925d73f1 drivers/xen/blktap/blktap.c
--- a/drivers/xen/blktap/blktap.c       Thu Apr 16 11:47:44 2009 +0100
+++ b/drivers/xen/blktap/blktap.c       Fri Apr 17 13:03:22 2009 +0100
@@ -293,6 +293,10 @@ static inline int OFFSET_TO_SEG(int offs
 /******************************************************************
  * BLKTAP VM OPS
  */
+struct tap_vma_priv {
+       tap_blkif_t *info;
+       struct page *map[];
+};
 
 static struct page *blktap_nopage(struct vm_area_struct *vma,
                                  unsigned long address,
@@ -315,7 +319,7 @@ static pte_t blktap_clear_pte(struct vm_
        int offset, seg, usr_idx, pending_idx, mmap_idx;
        unsigned long uvstart = vma->vm_start + (RING_PAGES << PAGE_SHIFT);
        unsigned long kvaddr;
-       struct page **map;
+       struct tap_vma_priv *priv;
        struct page *pg;
        struct grant_handle_pair *khandle;
        struct gnttab_unmap_grant_ref unmap[2];
@@ -330,12 +334,12 @@ static pte_t blktap_clear_pte(struct vm_
                                               ptep, is_fullmm);
 
        info = vma->vm_file->private_data;
-       map = vma->vm_private_data;
+       priv = vma->vm_private_data;
 
        /* TODO Should these be changed to if statements? */
        BUG_ON(!info);
        BUG_ON(!info->idx_map);
-       BUG_ON(!map);
+       BUG_ON(!priv);
 
        offset = (int) ((uvaddr - uvstart) >> PAGE_SHIFT);
        usr_idx = OFFSET_TO_USR_IDX(offset);
@@ -347,7 +351,7 @@ static pte_t blktap_clear_pte(struct vm_
        kvaddr = idx_to_kaddr(mmap_idx, pending_idx, seg);
        pg = pfn_to_page(__pa(kvaddr) >> PAGE_SHIFT);
        ClearPageReserved(pg);
-       map[offset + RING_PAGES] = NULL;
+       priv->map[offset + RING_PAGES] = NULL;
 
        khandle = &pending_handle(mmap_idx, pending_idx, seg);
 
@@ -388,9 +392,20 @@ static pte_t blktap_clear_pte(struct vm_
        return copy;
 }
 
+static void blktap_vma_close(struct vm_area_struct *vma)
+{
+       struct tap_vma_priv *priv = vma->vm_private_data;
+
+       if (priv) {
+               priv->info->vma = NULL;
+               kfree(priv);
+       }
+}
+
 struct vm_operations_struct blktap_vm_ops = {
        nopage:   blktap_nopage,
        zap_pte:  blktap_clear_pte,
+       close:    blktap_vma_close,
 };
 
 /******************************************************************
@@ -608,21 +623,6 @@ static int blktap_release(struct inode *
        /* Free the ring page. */
        ClearPageReserved(virt_to_page(info->ufe_ring.sring));
        free_page((unsigned long) info->ufe_ring.sring);
-
-       /* Clear any active mappings and free foreign map table */
-       if (info->vma) {
-               struct mm_struct *mm = info->vma->vm_mm;
-
-               down_write(&mm->mmap_sem);
-               zap_page_range(
-                       info->vma, info->vma->vm_start, 
-                       info->vma->vm_end - info->vma->vm_start, NULL);
-               up_write(&mm->mmap_sem);
-
-               kfree(info->vma->vm_private_data);
-
-               info->vma = NULL;
-       }
 
        if (info->idx_map) {
                kfree(info->idx_map);
@@ -662,8 +662,7 @@ static int blktap_mmap(struct file *filp
 static int blktap_mmap(struct file *filp, struct vm_area_struct *vma)
 {
        int size;
-       struct page **map;
-       int i;
+       struct tap_vma_priv *priv;
        tap_blkif_t *info = filp->private_data;
        int ret;
 
@@ -700,18 +699,16 @@ static int blktap_mmap(struct file *filp
        }
 
        /* Mark this VM as containing foreign pages, and set up mappings. */
-       map = kzalloc(((vma->vm_end - vma->vm_start) >> PAGE_SHIFT)
-                     * sizeof(struct page *),
-                     GFP_KERNEL);
-       if (map == NULL) {
+       priv = kzalloc(sizeof(*priv) + ((vma->vm_end - vma->vm_start)
+                                       >> PAGE_SHIFT) * sizeof(*priv->map),
+                      GFP_KERNEL);
+       if (priv == NULL) {
                WPRINTK("Couldn't alloc VM_FOREIGN map.\n");
                goto fail;
        }
-
-       for (i = 0; i < ((vma->vm_end - vma->vm_start) >> PAGE_SHIFT); i++)
-               map[i] = NULL;
-    
-       vma->vm_private_data = map;
+       priv->info = info;
+
+       vma->vm_private_data = priv;
        vma->vm_flags |= VM_FOREIGN;
        vma->vm_flags |= VM_DONTCOPY;
 
@@ -1190,7 +1187,7 @@ static int blktap_read_ufe_ring(tap_blki
                for (j = 0; j < pending_req->nr_pages; j++) {
 
                        unsigned long kvaddr, uvaddr;
-                       struct page **map = info->vma->vm_private_data;
+                       struct tap_vma_priv *priv = info->vma->vm_private_data;
                        struct page *pg;
                        int offset;
 
@@ -1201,7 +1198,7 @@ static int blktap_read_ufe_ring(tap_blki
                        ClearPageReserved(pg);
                        offset = (uvaddr - info->vma->vm_start) 
                                >> PAGE_SHIFT;
-                       map[offset] = NULL;
+                       priv->map[offset] = NULL;
                }
                fast_flush_area(pending_req, pending_idx, usr_idx, info->minor);
                info->idx_map[usr_idx] = INVALID_REQ;
@@ -1359,6 +1356,7 @@ static void dispatch_rw_block_io(blkif_t
        unsigned int nseg;
        int ret, i, nr_sects = 0;
        tap_blkif_t *info;
+       struct tap_vma_priv *priv;
        blkif_request_t *target;
        int pending_idx = RTN_PEND_IDX(pending_req,pending_req->mem_idx);
        int usr_idx;
@@ -1407,6 +1405,7 @@ static void dispatch_rw_block_io(blkif_t
        pending_req->status    = BLKIF_RSP_OKAY;
        pending_req->nr_pages  = nseg;
        op = 0;
+       priv = info->vma->vm_private_data;
        mm = info->vma->vm_mm;
        if (!xen_feature(XENFEAT_auto_translated_physmap))
                down_write(&mm->mmap_sem);
@@ -1490,8 +1489,7 @@ static void dispatch_rw_block_io(blkif_t
                                                          >> PAGE_SHIFT));
                        offset = (uvaddr - info->vma->vm_start) >> PAGE_SHIFT;
                        pg = pfn_to_page(__pa(kvaddr) >> PAGE_SHIFT);
-                       ((struct page **)info->vma->vm_private_data)[offset] =
-                               pg;
+                       priv->map[offset] = pg;
                }
        } else {
                for (i = 0; i < nseg; i++) {
@@ -1518,8 +1516,7 @@ static void dispatch_rw_block_io(blkif_t
 
                        offset = (uvaddr - info->vma->vm_start) >> PAGE_SHIFT;
                        pg = pfn_to_page(__pa(kvaddr) >> PAGE_SHIFT);
-                       ((struct page **)info->vma->vm_private_data)[offset] =
-                               pg;
+                       priv->map[offset] = pg;
                }
        }
 

_______________________________________________
Xen-changelog mailing list
Xen-changelog@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-changelog