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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <7CECC2DFC21538489F72729DF5EFB4D9BF9F0B@dggemm501-mbs.china.huawei.com>
Date:   Fri, 20 Apr 2018 11:52:56 +0000
From:   xuyandong <xuyandong2@...wei.com>
To:     Alex Williamson <alex.williamson@...hat.com>
CC:     "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Zhanghailiang <zhang.zhanghailiang@...wei.com>,
        "wangxin (U)" <wangxinxin.wang@...wei.com>,
        Kirti Wankhede <kwankhede@...dia.com>
Subject: RE: [PATCH] vfio iommu type1: no need to check task->mm if task has
 been destroyed

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@...hat.com]
> Sent: Friday, April 20, 2018 3:55 AM
> To: xuyandong <xuyandong2@...wei.com>
> Cc: kvm@...r.kernel.org; linux-kernel@...r.kernel.org; Zhanghailiang
> <zhang.zhanghailiang@...wei.com>; wangxin (U)
> <wangxinxin.wang@...wei.com>; Kirti Wankhede
> <kwankhede@...dia.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@...hat.com> wrote:
> 
> > [cc +Kirti]
> >
> > On Wed, 18 Apr 2018 18:55:45 +0800
> > Xu Yandong <xuyandong2@...wei.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@...wei.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 */
> 
>