[PATCH] vfio iommu type1: no need to check task->mm if task has been destroyed

[PATCH] vfio iommu type1: no need to check task->mm if task has been destroyed

[Xu Yandong]

The task structure in vfio_dma struct used to identify the same
task who map it or other task who shares same adress space is
allowed to unmap. But if the task who map it has exited, mm of
the task has been set to null, we should unmap the vfio dma directly.

Signed-off-by: Xu Yandong <xuyandong2@huawei.com>
---
Hi all,
When I unplug a vcpu from a VM lanched with a VFIO hostdev device,
I found that the *vfio_dma* mapped by this vcpu task could not be unmaped
in the future, so I send this patch to unmap vfio_dma directly if the
task who mapped it has exited. 

Howerver this patch may introduce a new security risk because any task can 
unmap the *vfio_dma* if the mapper task has exited.  

---
 drivers/vfio/vfio_iommu_type1.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 5c212bf..601a353 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -947,7 +947,7 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
 		 * Task with same address space who mapped this iova range is
 		 * allowed to unmap the iova range.
 		 */
-		if (dma->task->mm != current->mm)
+		if (dma->task->mm && (dma->task->mm != current->mm))
 			break;
 
 		if (!RB_EMPTY_ROOT(&dma->pfn_list)) {
-- 
1.8.3.1

Re: [PATCH] vfio iommu type1: no need to check task->mm if task has been destroyed

[Alex Williamson]

[cc +Kirti]

On Wed, 18 Apr 2018 18:55:45 +0800
Xu Yandong <xuyandong2@huawei.com> wrote:

> The task structure in vfio_dma struct used to identify the same
> task who map it or other task who shares same adress space is
> allowed to unmap. But if the task who map it has exited, mm of
> the task has been set to null, we should unmap the vfio dma directly.
> 
> Signed-off-by: Xu Yandong <xuyandong2@huawei.com>
> ---
> Hi all,
> When I unplug a vcpu from a VM lanched with a VFIO hostdev device,
> I found that the *vfio_dma* mapped by this vcpu task could not be unmaped
> in the future, so I send this patch to unmap vfio_dma directly if the
> task who mapped it has exited. 
> 
> Howerver this patch may introduce a new security risk because any task can 
> unmap the *vfio_dma* if the mapper task has exited.  

Well that's unexpected, but adding some debugging code I can clearly
see that the map and unmap ioctls are typically called by the various
processor threads, which all share the same mm_struct (so accounting is
correct regardless of which CPU does the unmap).  I don't think the fix
below is correct though, it's not for a security risk, but for
accounting issue and correctness issues.  The pages are mapped and
accounted against the users locked memory limits, if we simply bail
out, both the IOMMU mappings and the limit accounting are wrong.
Perhaps rather than referencing the calling task_struct in the vfio_dma
on mapping, we should traverse to the highest parent task sharing the
same mm_struct.  Kirti, any thoughts since this code originated for
mdev support?  Thanks,

Alex
 
> ---
>  drivers/vfio/vfio_iommu_type1.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index 5c212bf..601a353 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -947,7 +947,7 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
>  		 * Task with same address space who mapped this iova range is
>  		 * allowed to unmap the iova range.
>  		 */
> -		if (dma->task->mm != current->mm)
> +		if (dma->task->mm && (dma->task->mm != current->mm))
>  			break;
>  
>  		if (!RB_EMPTY_ROOT(&dma->pfn_list)) {

Re: [PATCH] vfio iommu type1: no need to check task->mm if task has been destroyed

[Alex Williamson]

On Thu, 19 Apr 2018 10:19:26 -0600
Alex Williamson <alex.williamson@redhat.com> wrote:

> [cc +Kirti]
> 
> On Wed, 18 Apr 2018 18:55:45 +0800
> Xu Yandong <xuyandong2@huawei.com> wrote:
> 
> > The task structure in vfio_dma struct used to identify the same
> > task who map it or other task who shares same adress space is
> > allowed to unmap. But if the task who map it has exited, mm of
> > the task has been set to null, we should unmap the vfio dma directly.
> > 
> > Signed-off-by: Xu Yandong <xuyandong2@huawei.com>
> > ---
> > Hi all,
> > When I unplug a vcpu from a VM lanched with a VFIO hostdev device,
> > I found that the *vfio_dma* mapped by this vcpu task could not be unmaped
> > in the future, so I send this patch to unmap vfio_dma directly if the
> > task who mapped it has exited. 
> > 
> > Howerver this patch may introduce a new security risk because any task can 
> > unmap the *vfio_dma* if the mapper task has exited.    
> 
> Well that's unexpected, but adding some debugging code I can clearly
> see that the map and unmap ioctls are typically called by the various
> processor threads, which all share the same mm_struct (so accounting is
> correct regardless of which CPU does the unmap).  I don't think the fix
> below is correct though, it's not for a security risk, but for
> accounting issue and correctness issues.  The pages are mapped and
> accounted against the users locked memory limits, if we simply bail
> out, both the IOMMU mappings and the limit accounting are wrong.
> Perhaps rather than referencing the calling task_struct in the vfio_dma
> on mapping, we should traverse to the highest parent task sharing the
> same mm_struct.  Kirti, any thoughts since this code originated for
> mdev support?  Thanks,

I think something like below is a better solution.  More research
required on group_leader and if it needs to be sanity tested or if
testing mm_struct is redundant, but I think it should resolve the
failing test case, all mappings reference the same task regardless of
which vCPU triggers it.  Thanks,

Alex

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 5c212bf29640..3a1d3695c3fb 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -1093,6 +1093,7 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
 	int ret = 0, prot = 0;
 	uint64_t mask;
 	struct vfio_dma *dma;
+	struct task_struct *task;
 
 	/* Verify that none of our __u64 fields overflow */
 	if (map->size != size || map->vaddr != vaddr || map->iova != iova)
@@ -1131,8 +1132,12 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
 	dma->iova = iova;
 	dma->vaddr = vaddr;
 	dma->prot = prot;
-	get_task_struct(current);
-	dma->task = current;
+
+	task = (current->mm == current->group_leader->mm ?
+		current->group_leader : current);
+	get_task_struct(task);
+	dma->task = task;
+
 	dma->pfn_list = RB_ROOT;
 
 	/* Insert zero-sized and grow as we map chunks of it */

 

RE: [PATCH] vfio iommu type1: no need to check task->mm if task has been destroyed

[xuyandong]

Hi Alex,
 After days of intensive test, the bug did not show up any more.
 So I think the new patch does solve this problem.

> -----Original Message-----
> From: Alex Williamson [mailto:alex.williamson@redhat.com]
> Sent: Friday, April 20, 2018 3:55 AM
> To: xuyandong <xuyandong2@huawei.com>
> Cc: kvm@vger.kernel.org; linux-kernel@vger.kernel.org; Zhanghailiang
> <zhang.zhanghailiang@huawei.com>; wangxin (U)
> <wangxinxin.wang@huawei.com>; Kirti Wankhede
> <kwankhede@nvidia.com>
> Subject: Re: [PATCH] vfio iommu type1: no need to check task->mm if task
> has been destroyed
> 
> On Thu, 19 Apr 2018 10:19:26 -0600
> Alex Williamson <alex.williamson@redhat.com> wrote:
> 
> > [cc +Kirti]
> >
> > On Wed, 18 Apr 2018 18:55:45 +0800
> > Xu Yandong <xuyandong2@huawei.com> wrote:
> >
> > > The task structure in vfio_dma struct used to identify the same task
> > > who map it or other task who shares same adress space is allowed to
> > > unmap. But if the task who map it has exited, mm of the task has
> > > been set to null, we should unmap the vfio dma directly.
> > >
> > > Signed-off-by: Xu Yandong <xuyandong2@huawei.com>
> > > ---
> > > Hi all,
> > > When I unplug a vcpu from a VM lanched with a VFIO hostdev device, I
> > > found that the *vfio_dma* mapped by this vcpu task could not be
> > > unmaped in the future, so I send this patch to unmap vfio_dma
> > > directly if the task who mapped it has exited.
> > >
> > > Howerver this patch may introduce a new security risk because any task
> can
> > > unmap the *vfio_dma* if the mapper task has exited.
> >
> > Well that's unexpected, but adding some debugging code I can clearly
> > see that the map and unmap ioctls are typically called by the various
> > processor threads, which all share the same mm_struct (so accounting
> > is correct regardless of which CPU does the unmap).  I don't think the
> > fix below is correct though, it's not for a security risk, but for
> > accounting issue and correctness issues.  The pages are mapped and
> > accounted against the users locked memory limits, if we simply bail
> > out, both the IOMMU mappings and the limit accounting are wrong.
> > Perhaps rather than referencing the calling task_struct in the
> > vfio_dma on mapping, we should traverse to the highest parent task
> > sharing the same mm_struct.  Kirti, any thoughts since this code
> > originated for mdev support?  Thanks,
> 
> I think something like below is a better solution.  More research required on
> group_leader and if it needs to be sanity tested or if testing mm_struct is
> redundant, but I think it should resolve the failing test case, all mappings
> reference the same task regardless of which vCPU triggers it.  Thanks,
> 
> Alex
> 
> diff --git a/drivers/vfio/vfio_iommu_type1.c
> b/drivers/vfio/vfio_iommu_type1.c index 5c212bf29640..3a1d3695c3fb
> 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -1093,6 +1093,7 @@ static int vfio_dma_do_map(struct vfio_iommu
> *iommu,
>  	int ret = 0, prot = 0;
>  	uint64_t mask;
>  	struct vfio_dma *dma;
> +	struct task_struct *task;
> 
>  	/* Verify that none of our __u64 fields overflow */
>  	if (map->size != size || map->vaddr != vaddr || map->iova != iova)
> @@ -1131,8 +1132,12 @@ static int vfio_dma_do_map(struct vfio_iommu
> *iommu,
>  	dma->iova = iova;
>  	dma->vaddr = vaddr;
>  	dma->prot = prot;
> -	get_task_struct(current);
> -	dma->task = current;
> +
> +	task = (current->mm == current->group_leader->mm ?
> +		current->group_leader : current);
> +	get_task_struct(task);
> +	dma->task = task;
> +
>  	dma->pfn_list = RB_ROOT;
> 
>  	/* Insert zero-sized and grow as we map chunks of it */
> 
>