LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] vfio iommu type1: no need to check task->mm if task has been destroyed
@ 2018-04-18 10:55 Xu Yandong
  2018-04-19 16:19 ` Alex Williamson
  0 siblings, 1 reply; 4+ messages in thread
From: Xu Yandong @ 2018-04-18 10:55 UTC (permalink / raw)
  To: alex.williamson
  Cc: kvm, linux-kernel, zhang.zhanghailiang, wangxinxin.wang, 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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] vfio iommu type1: no need to check task->mm if task has been destroyed
  2018-04-18 10:55 [PATCH] vfio iommu type1: no need to check task->mm if task has been destroyed Xu Yandong
@ 2018-04-19 16:19 ` Alex Williamson
  2018-04-19 19:54   ` Alex Williamson
  0 siblings, 1 reply; 4+ messages in thread
From: Alex Williamson @ 2018-04-19 16:19 UTC (permalink / raw)
  To: Xu Yandong
  Cc: kvm, linux-kernel, zhang.zhanghailiang, wangxinxin.wang, Kirti Wankhede

[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)) {

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] vfio iommu type1: no need to check task->mm if task has been destroyed
  2018-04-19 16:19 ` Alex Williamson
@ 2018-04-19 19:54   ` Alex Williamson
  2018-04-20 11:52     ` xuyandong
  0 siblings, 1 reply; 4+ messages in thread
From: Alex Williamson @ 2018-04-19 19:54 UTC (permalink / raw)
  To: Xu Yandong
  Cc: kvm, linux-kernel, zhang.zhanghailiang, wangxinxin.wang, Kirti Wankhede

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 */

 

^ permalink raw reply	[flat|nested] 4+ messages in thread

* RE: [PATCH] vfio iommu type1: no need to check task->mm if task has been destroyed
  2018-04-19 19:54   ` Alex Williamson
@ 2018-04-20 11:52     ` xuyandong
  0 siblings, 0 replies; 4+ messages in thread
From: xuyandong @ 2018-04-20 11:52 UTC (permalink / raw)
  To: Alex Williamson
  Cc: kvm, linux-kernel, Zhanghailiang, wangxin (U), Kirti Wankhede

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 */
> 
> 

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2018-04-20 11:53 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-18 10:55 [PATCH] vfio iommu type1: no need to check task->mm if task has been destroyed Xu Yandong
2018-04-19 16:19 ` Alex Williamson
2018-04-19 19:54   ` Alex Williamson
2018-04-20 11:52     ` xuyandong

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).