[PATCH] xen/gntdev: fix deadlock between map ioctl and mmap

[PATCH] xen/gntdev: fix deadlock between map ioctl and mmap

[Andrey Misiurov]

Commit 45ca1afe2fd1 ("xen/gntdev: fix error handling in ioctl") moved
copy_to_user() in gntdev_ioctl_map_grant_ref() under priv->lock. A
fault in copy_to_user() takes mmap_lock, while gntdev_mmap() is called
with mmap_lock held and takes priv->lock:

  ioctl thread                        mmap thread
  mutex_lock(&priv->lock)
                                      mmap_write_lock(mm)
                                      gntdev_mmap()
                                        mutex_lock(&priv->lock)
  copy_to_user() faults
    mmap_read_lock(mm)

Reproduced on Debian 6.12.111 in a Xen guest, with the ioctl argument
on a MAP_PRIVATE file page that has not been written yet. sysrq-w:

  task:repro state:D stack:0 pid:4479 tgid:4477 ppid:1 flags:0x00000000
   rwsem_down_read_slowpath+0x25c/0x480
   down_read_killable+0x48/0xc0
   lock_mm_and_find_vma+0x96/0x250
   do_user_addr_fault+0x12d/0x620
   exc_page_fault+0x7e/0x180
   asm_exc_page_fault+0x26/0x30
   _copy_to_user+0x36/0x50
   gntdev_ioctl+0x3b9/0xa00 [xen_gntdev]
   __x64_sys_ioctl+0x97/0xc0
  task:repro state:D stack:0 pid:4480 tgid:4477 ppid:1 flags:0x00000000
   __mutex_lock.constprop.0+0x41b/0x720
   gntdev_mmap+0x4d/0x330 [xen_gntdev]
   __mmap_region+0x697/0xb10
   mmap_region+0x78/0xa0
   do_mmap+0x45d/0x630
   vm_mmap_pgoff+0xf8/0x1c0

Drop priv->lock before copy_to_user() again. On failure, re-take it
and remove the map by index, as IOCTL_GNTDEV_UNMAP_GRANT_REF does,
since another thread may have unmapped it meanwhile.

Fixes: 45ca1afe2fd1 ("xen/gntdev: fix error handling in ioctl")
Cc: stable@vger.kernel.org
Signed-off-by: Andrey Misiurov <andrey.misyrov@gmail.com>
---
 drivers/xen/gntdev.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
index b71f5ba..1ad0220 100644
--- a/drivers/xen/gntdev.c
+++ b/drivers/xen/gntdev.c
@@ -670,15 +670,19 @@ static long gntdev_ioctl_map_grant_ref(struct gntdev_priv *priv,
 	mutex_lock(&priv->lock);
 	gntdev_add_map(priv, map);
 	op.index = map->index << PAGE_SHIFT;
+	mutex_unlock(&priv->lock);
 
 	if (copy_to_user(u, &op, sizeof(op)) != 0) {
-		list_del(&map->next);
+		mutex_lock(&priv->lock);
+		map = gntdev_find_map_index(priv, op.index >> PAGE_SHIFT,
+					    op.count);
+		if (map)
+			list_del(&map->next);
 		mutex_unlock(&priv->lock);
 		gntdev_put_map(priv, map);
 		return -EFAULT;
 	}
 
-	mutex_unlock(&priv->lock);
 	return 0;
 }
 

base-commit: 61b6d1df1d6234c38a6ea7e4f58e7da035ef6ab4
-- 
2.43.7

Re: [PATCH] xen/gntdev: fix deadlock between map ioctl and mmap

[Juergen Gross]

[-- Attachment #1.1: Type: text/plain, Size: 1902 bytes --]

On 13.10.26 19:07, Andrey Misiurov wrote:
> Commit 45ca1afe2fd1 ("xen/gntdev: fix error handling in ioctl") moved
> copy_to_user() in gntdev_ioctl_map_grant_ref() under priv->lock. A
> fault in copy_to_user() takes mmap_lock, while gntdev_mmap() is called
> with mmap_lock held and takes priv->lock:
> 
>    ioctl thread                        mmap thread
>    mutex_lock(&priv->lock)
>                                        mmap_write_lock(mm)
>                                        gntdev_mmap()
>                                          mutex_lock(&priv->lock)
>    copy_to_user() faults
>      mmap_read_lock(mm)
> 
> Reproduced on Debian 6.12.111 in a Xen guest, with the ioctl argument
> on a MAP_PRIVATE file page that has not been written yet. sysrq-w:
> 
>    task:repro state:D stack:0 pid:4479 tgid:4477 ppid:1 flags:0x00000000
>     rwsem_down_read_slowpath+0x25c/0x480
>     down_read_killable+0x48/0xc0
>     lock_mm_and_find_vma+0x96/0x250
>     do_user_addr_fault+0x12d/0x620
>     exc_page_fault+0x7e/0x180
>     asm_exc_page_fault+0x26/0x30
>     _copy_to_user+0x36/0x50
>     gntdev_ioctl+0x3b9/0xa00 [xen_gntdev]
>     __x64_sys_ioctl+0x97/0xc0
>    task:repro state:D stack:0 pid:4480 tgid:4477 ppid:1 flags:0x00000000
>     __mutex_lock.constprop.0+0x41b/0x720
>     gntdev_mmap+0x4d/0x330 [xen_gntdev]
>     __mmap_region+0x697/0xb10
>     mmap_region+0x78/0xa0
>     do_mmap+0x45d/0x630
>     vm_mmap_pgoff+0xf8/0x1c0
> 
> Drop priv->lock before copy_to_user() again. On failure, re-take it
> and remove the map by index, as IOCTL_GNTDEV_UNMAP_GRANT_REF does,
> since another thread may have unmapped it meanwhile.
> 
> Fixes: 45ca1afe2fd1 ("xen/gntdev: fix error handling in ioctl")
> Cc: stable@vger.kernel.org
> Signed-off-by: Andrey Misiurov <andrey.misyrov@gmail.com>
Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]