linux-kernel - Re: [RFC PATCH] vfio: type1: fix kthread use case

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>] [day] [month] [year] [list]

Date:   Tue, 7 Jul 2020 08:31:41 +0800
From:   Yan Zhao <yan.y.zhao@...el.com>
To:     Hillf Danton <hdanton@...a.com>
Cc:     Alex Williamson <alex.williamson@...hat.com>,
        iommu@...ts.linux-foundation.org,
        Kevin Tian <kevin.tian@...el.com>,
        Joerg Roedel <joro@...tes.org>, linux-kernel@...r.kernel.org,
        Markus Elfring <Markus.Elfring@....de>,
        Christoph Hellwig <hch@....de>
Subject: Re: [RFC PATCH] vfio: type1: fix kthread use case

On Mon, Jul 06, 2020 at 06:49:15PM +0800, Hillf Danton wrote:
> 
> It's incorrect to tell out if a task is kthread without checking
> PF_KTHREAD at all.
> 
> What's also fixed, if no need to be in a seperate patch, is to
> invoke kthread_use_mm() without checking the current mm first as
> the kthread may hold a mm struct atm and it's not the right place
> to switch mm.
> 
> Fixes: 8d46c0cca5f4 ("vfio: introduce vfio_dma_rw to read/write a range of IOVAs")
> Cc: Yan Zhao <yan.y.zhao@...el.com>
> Cc: Markus Elfring <Markus.Elfring@....de>
> Cc: Christoph Hellwig <hch@....de>
> Signed-off-by: Hillf Danton <hdanton@...a.com>
> ---
> 
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -2798,7 +2798,8 @@ static int vfio_iommu_type1_dma_rw_chunk
>  	struct mm_struct *mm;
>  	unsigned long vaddr;
>  	struct vfio_dma *dma;
> -	bool kthread = current->mm == NULL;
> +	bool kthread = current->flags & PF_KTHREAD;
> +	bool use_mm = current->mm == NULL;
is it acceptable to just rename "kthread" to "kthread_no_use_mm"?

I think "current->mm == NULL" in itself implies kthread and not use_mm,
as a user thread is not able to have "current->mm == NULL", right?


Thanks
Yan

>  	size_t offset;
>  
>  	*copied = 0;
> @@ -2812,11 +2813,10 @@ static int vfio_iommu_type1_dma_rw_chunk
>  		return -EPERM;
>  
>  	mm = get_task_mm(dma->task);
> -
>  	if (!mm)
>  		return -EPERM;
>  
> -	if (kthread)
> +	if (kthread && use_mm)
>  		kthread_use_mm(mm);
>  	else if (current->mm != mm)
>  		goto out;
> @@ -2843,7 +2843,7 @@ static int vfio_iommu_type1_dma_rw_chunk
>  	} else
>  		*copied = copy_from_user(data, (void __user *)vaddr,
>  					   count) ? 0 : count;
> -	if (kthread)
> +	if (kthread && use_mm)
>  		kthread_unuse_mm(mm);
>  out:
>  	mmput(mm);
> --
>