* [PATCH 1/3] xen/gntdev: fix unsafe vma access
2013-01-02 22:57 ` oopsable race in xen-gntdev [PATCH 0/3] Daniel De Graaf
@ 2013-01-02 22:57 ` Daniel De Graaf
2013-01-02 22:57 ` [PATCH 2/3] xen/gntdev: correctly unmap unlinked maps in mmu notifier Daniel De Graaf
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Daniel De Graaf @ 2013-01-02 22:57 UTC (permalink / raw)
To: viro, konrad.wilk
Cc: david.vrabel, stefano.stabellini, xen-devel, linux-kernel,
Daniel De Graaf
In gntdev_ioctl_get_offset_for_vaddr, we need to hold mmap_sem while
calling find_vma() to avoid potentially having the result freed out from
under us. Similarly, the MMU notifier functions need to synchronize with
gntdev_vma_close to avoid map->vma being freed during their iteration.
Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
Reported-by: Al Viro <viro@ZenIV.linux.org.uk>
---
drivers/xen/gntdev.c | 29 ++++++++++++++++++++++++-----
1 file changed, 24 insertions(+), 5 deletions(-)
diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
index 2e22df2..cca62cc 100644
--- a/drivers/xen/gntdev.c
+++ b/drivers/xen/gntdev.c
@@ -378,7 +378,20 @@ static void gntdev_vma_close(struct vm_area_struct *vma)
struct grant_map *map = vma->vm_private_data;
pr_debug("gntdev_vma_close %p\n", vma);
- map->vma = NULL;
+ if (use_ptemod) {
+ struct file *file = vma->vm_file;
+ struct gntdev_priv *priv = file->private_data;
+ /* It is possible that an mmu notifier could be running
+ * concurrently, so take priv->lock to ensure that the vma won't
+ * vanishing during the unmap_grant_pages call, since we will
+ * spin here until that completes. Such a concurrent call will
+ * not do any unmapping, since that has been done prior to
+ * closing the vma, but it may still iterate the unmap_ops list.
+ */
+ spin_lock(&priv->lock);
+ map->vma = NULL;
+ spin_unlock(&priv->lock);
+ }
vma->vm_private_data = NULL;
gntdev_put_map(map);
}
@@ -579,25 +592,31 @@ static long gntdev_ioctl_get_offset_for_vaddr(struct gntdev_priv *priv,
struct ioctl_gntdev_get_offset_for_vaddr op;
struct vm_area_struct *vma;
struct grant_map *map;
+ int rv = -EINVAL;
if (copy_from_user(&op, u, sizeof(op)) != 0)
return -EFAULT;
pr_debug("priv %p, offset for vaddr %lx\n", priv, (unsigned long)op.vaddr);
+ down_read(¤t->mm->mmap_sem);
vma = find_vma(current->mm, op.vaddr);
if (!vma || vma->vm_ops != &gntdev_vmops)
- return -EINVAL;
+ goto out_unlock;
map = vma->vm_private_data;
if (!map)
- return -EINVAL;
+ goto out_unlock;
op.offset = map->index << PAGE_SHIFT;
op.count = map->count;
+ rv = 0;
- if (copy_to_user(u, &op, sizeof(op)) != 0)
+ out_unlock:
+ up_read(¤t->mm->mmap_sem);
+
+ if (rv == 0 && copy_to_user(u, &op, sizeof(op)) != 0)
return -EFAULT;
- return 0;
+ return rv;
}
static long gntdev_ioctl_notify(struct gntdev_priv *priv, void __user *u)
--
1.7.11.7
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 2/3] xen/gntdev: correctly unmap unlinked maps in mmu notifier
2013-01-02 22:57 ` oopsable race in xen-gntdev [PATCH 0/3] Daniel De Graaf
2013-01-02 22:57 ` [PATCH 1/3] xen/gntdev: fix unsafe vma access Daniel De Graaf
@ 2013-01-02 22:57 ` Daniel De Graaf
2013-01-02 22:57 ` [PATCH 3/3] xen/gntdev: remove erronous use of copy_to_user Daniel De Graaf
2013-01-11 17:40 ` oopsable race in xen-gntdev [PATCH 0/3] Konrad Rzeszutek Wilk
3 siblings, 0 replies; 6+ messages in thread
From: Daniel De Graaf @ 2013-01-02 22:57 UTC (permalink / raw)
To: viro, konrad.wilk
Cc: david.vrabel, stefano.stabellini, xen-devel, linux-kernel,
Daniel De Graaf
If gntdev_ioctl_unmap_grant_ref is called on a range before unmapping
it, the entry is removed from priv->maps and the later call to
mn_invl_range_start won't find it to do the unmapping. Fix this by
creating another list of freeable maps that the mmu notifier can search
and use to unmap grants.
Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
---
drivers/xen/gntdev.c | 92 +++++++++++++++++++++++++++++++++++-----------------
1 file changed, 63 insertions(+), 29 deletions(-)
diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
index cca62cc..9be3e5e 100644
--- a/drivers/xen/gntdev.c
+++ b/drivers/xen/gntdev.c
@@ -56,10 +56,15 @@ MODULE_PARM_DESC(limit, "Maximum number of grants that may be mapped by "
static atomic_t pages_mapped = ATOMIC_INIT(0);
static int use_ptemod;
+#define populate_freeable_maps use_ptemod
struct gntdev_priv {
+ /* maps with visible offsets in the file descriptor */
struct list_head maps;
- /* lock protects maps from concurrent changes */
+ /* maps that are not visible; will be freed on munmap.
+ * Only populated if populate_freeable_maps == 1 */
+ struct list_head freeable_maps;
+ /* lock protects maps and freeable_maps */
spinlock_t lock;
struct mm_struct *mm;
struct mmu_notifier mn;
@@ -193,7 +198,7 @@ static struct grant_map *gntdev_find_map_index(struct gntdev_priv *priv,
return NULL;
}
-static void gntdev_put_map(struct grant_map *map)
+static void gntdev_put_map(struct gntdev_priv *priv, struct grant_map *map)
{
if (!map)
return;
@@ -208,6 +213,12 @@ static void gntdev_put_map(struct grant_map *map)
evtchn_put(map->notify.event);
}
+ if (populate_freeable_maps && priv) {
+ spin_lock(&priv->lock);
+ list_del(&map->next);
+ spin_unlock(&priv->lock);
+ }
+
if (map->pages && !use_ptemod)
unmap_grant_pages(map, 0, map->count);
gntdev_free_map(map);
@@ -376,11 +387,11 @@ static void gntdev_vma_open(struct vm_area_struct *vma)
static void gntdev_vma_close(struct vm_area_struct *vma)
{
struct grant_map *map = vma->vm_private_data;
+ struct file *file = vma->vm_file;
+ struct gntdev_priv *priv = file->private_data;
pr_debug("gntdev_vma_close %p\n", vma);
if (use_ptemod) {
- struct file *file = vma->vm_file;
- struct gntdev_priv *priv = file->private_data;
/* It is possible that an mmu notifier could be running
* concurrently, so take priv->lock to ensure that the vma won't
* vanishing during the unmap_grant_pages call, since we will
@@ -393,7 +404,7 @@ static void gntdev_vma_close(struct vm_area_struct *vma)
spin_unlock(&priv->lock);
}
vma->vm_private_data = NULL;
- gntdev_put_map(map);
+ gntdev_put_map(priv, map);
}
static struct vm_operations_struct gntdev_vmops = {
@@ -403,33 +414,43 @@ static struct vm_operations_struct gntdev_vmops = {
/* ------------------------------------------------------------------ */
+static void unmap_if_in_range(struct grant_map *map,
+ unsigned long start, unsigned long end)
+{
+ unsigned long mstart, mend;
+ int err;
+
+ if (!map->vma)
+ return;
+ if (map->vma->vm_start >= end)
+ return;
+ if (map->vma->vm_end <= start)
+ return;
+ mstart = max(start, map->vma->vm_start);
+ mend = min(end, map->vma->vm_end);
+ pr_debug("map %d+%d (%lx %lx), range %lx %lx, mrange %lx %lx\n",
+ map->index, map->count,
+ map->vma->vm_start, map->vma->vm_end,
+ start, end, mstart, mend);
+ err = unmap_grant_pages(map,
+ (mstart - map->vma->vm_start) >> PAGE_SHIFT,
+ (mend - mstart) >> PAGE_SHIFT);
+ WARN_ON(err);
+}
+
static void mn_invl_range_start(struct mmu_notifier *mn,
struct mm_struct *mm,
unsigned long start, unsigned long end)
{
struct gntdev_priv *priv = container_of(mn, struct gntdev_priv, mn);
struct grant_map *map;
- unsigned long mstart, mend;
- int err;
spin_lock(&priv->lock);
list_for_each_entry(map, &priv->maps, next) {
- if (!map->vma)
- continue;
- if (map->vma->vm_start >= end)
- continue;
- if (map->vma->vm_end <= start)
- continue;
- mstart = max(start, map->vma->vm_start);
- mend = min(end, map->vma->vm_end);
- pr_debug("map %d+%d (%lx %lx), range %lx %lx, mrange %lx %lx\n",
- map->index, map->count,
- map->vma->vm_start, map->vma->vm_end,
- start, end, mstart, mend);
- err = unmap_grant_pages(map,
- (mstart - map->vma->vm_start) >> PAGE_SHIFT,
- (mend - mstart) >> PAGE_SHIFT);
- WARN_ON(err);
+ unmap_if_in_range(map, start, end);
+ }
+ list_for_each_entry(map, &priv->freeable_maps, next) {
+ unmap_if_in_range(map, start, end);
}
spin_unlock(&priv->lock);
}
@@ -458,6 +479,15 @@ static void mn_release(struct mmu_notifier *mn,
err = unmap_grant_pages(map, /* offset */ 0, map->count);
WARN_ON(err);
}
+ list_for_each_entry(map, &priv->freeable_maps, next) {
+ if (!map->vma)
+ continue;
+ pr_debug("map %d+%d (%lx %lx)\n",
+ map->index, map->count,
+ map->vma->vm_start, map->vma->vm_end);
+ err = unmap_grant_pages(map, /* offset */ 0, map->count);
+ WARN_ON(err);
+ }
spin_unlock(&priv->lock);
}
@@ -479,6 +509,7 @@ static int gntdev_open(struct inode *inode, struct file *flip)
return -ENOMEM;
INIT_LIST_HEAD(&priv->maps);
+ INIT_LIST_HEAD(&priv->freeable_maps);
spin_lock_init(&priv->lock);
if (use_ptemod) {
@@ -513,8 +544,9 @@ static int gntdev_release(struct inode *inode, struct file *flip)
while (!list_empty(&priv->maps)) {
map = list_entry(priv->maps.next, struct grant_map, next);
list_del(&map->next);
- gntdev_put_map(map);
+ gntdev_put_map(NULL /* already removed */, map);
}
+ WARN_ON(!list_empty(&priv->freeable_maps));
if (use_ptemod)
mmu_notifier_unregister(&priv->mn, priv->mm);
@@ -542,14 +574,14 @@ static long gntdev_ioctl_map_grant_ref(struct gntdev_priv *priv,
if (unlikely(atomic_add_return(op.count, &pages_mapped) > limit)) {
pr_debug("can't map: over limit\n");
- gntdev_put_map(map);
+ gntdev_put_map(NULL, map);
return err;
}
if (copy_from_user(map->grants, &u->refs,
sizeof(map->grants[0]) * op.count) != 0) {
- gntdev_put_map(map);
- return err;
+ gntdev_put_map(NULL, map);
+ return -EFAULT;
}
spin_lock(&priv->lock);
@@ -578,11 +610,13 @@ static long gntdev_ioctl_unmap_grant_ref(struct gntdev_priv *priv,
map = gntdev_find_map_index(priv, op.index >> PAGE_SHIFT, op.count);
if (map) {
list_del(&map->next);
+ if (populate_freeable_maps)
+ list_add_tail(&map->next, &priv->freeable_maps);
err = 0;
}
spin_unlock(&priv->lock);
if (map)
- gntdev_put_map(map);
+ gntdev_put_map(priv, map);
return err;
}
@@ -797,7 +831,7 @@ out_unlock_put:
out_put_map:
if (use_ptemod)
map->vma = NULL;
- gntdev_put_map(map);
+ gntdev_put_map(priv, map);
return err;
}
--
1.7.11.7
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: oopsable race in xen-gntdev [PATCH 0/3]
2013-01-02 22:57 ` oopsable race in xen-gntdev [PATCH 0/3] Daniel De Graaf
` (2 preceding siblings ...)
2013-01-02 22:57 ` [PATCH 3/3] xen/gntdev: remove erronous use of copy_to_user Daniel De Graaf
@ 2013-01-11 17:40 ` Konrad Rzeszutek Wilk
3 siblings, 0 replies; 6+ messages in thread
From: Konrad Rzeszutek Wilk @ 2013-01-11 17:40 UTC (permalink / raw)
To: Daniel De Graaf
Cc: viro, david.vrabel, stefano.stabellini, xen-devel, linux-kernel
On Wed, Jan 02, 2013 at 05:57:10PM -0500, Daniel De Graaf wrote:
> On 12/21/2012 03:18 PM, Konrad Rzeszutek Wilk wrote:
> > On Sat, Dec 15, 2012 at 06:12:11PM +0000, Al Viro wrote:
> >> 1) find_vma() is *not* safe without ->mmap_sem and its result may
> >> very well be freed just as it's returned to caller. IOW,
> >> gntdev_ioctl_get_offset_for_vaddr() is racy and may end up with
> >> dereferencing freed memory.
>
> I agree, this one should be fixed by taking mmap_sem in
> gntdev_ioctl_get_offset_for_vaddr. Iterating the grant_map list here
> will not work under HVM, where map->vma is not filled in.
>
> >> 2) gntdev_vma_close() is putting NULL into map->vma with only
> >> ->mmap_sem held by caller. Things like
> >> if (!map->vma)
> >> continue;
> >> if (map->vma->vm_start >= end)
> >> continue;
> >> if (map->vma->vm_end <= start)
> >> done with just priv->lock held are racy.
> >>
> >> I'm not familiar with the code, but it looks like we need to
> >> protect gntdev_vma_close() guts with the same spinlock and probably
> >> hold ->mmap_sem shared around the "find_vma()+get to map->{index,count}"
> >> in the ioctl. Or replace the logics in ioctl with search through the
> >> list of grant_map under the same spinlock...
> >>
> >> Comments?
>
> Although I don't think the mmu notifier is ever called without mmap_sem
> on this particular device file (we map only with VM_DONTCOPY and other
> paths like truncate generally aren't triggered), it's probably best not
> to rely on that behavior, so adding the spinlock in gntdev_vma_close
> seems to be the best solution.
>
> > Hey Al,
> >
> > Thank you for your analysis.
Are you OK with the patches or have comments about them? Thanks.
> >
> > CC-ing Daniel, David and Stefano. I recall we had some priv->lock movement
> > in the past and there is also interaction with another piece of code -
> > the balloon code so we better be circumspect of not blowing up.
> >
> > Al, it is around holidays and folks are mostly gone - so this will take
> > a bit of time to get sorted out.
>
> While I was digging in this code, I found a related bug in
> mn_invl_range_start: if gntdev_ioctl_unmap_grant_ref is called on
> a range before unmapping it, the entry is removed from priv->maps and
> the later call to mn_invl_range_start won't find it to do the unmapping.
> This could be fixed by using find_vma, but I don't think there's a safe
> way to do that from inside the mmu notifier, so instead I created a list
> of these unlinked but still mapped pages.
>
> The third patch is a fix to an unrelated bug that I found while testing
> the fixes in the other two patches.
>
> [PATCH 1/3] xen/gntdev: fix unsafe vma access
> [PATCH 2/3] xen/gntdev: correctly unmap unlinked maps in mmu
> [PATCH 3/3] xen/gntdev: remove erronous use of copy_to_user
^ permalink raw reply [flat|nested] 6+ messages in thread