[PATCH] USB: usbfs: fix mmap dma mismatch

[PATCH] USB: usbfs: fix mmap dma mismatch

[Greg Kroah-Hartman]

In commit 2bef9aed6f0e ("usb: usbfs: correct kernel->user page attribute
mismatch") we switched from always calling remap_pfn_range() to call
dma_mmap_coherent() to handle issues with systems with non-coherent USB host
controller drivers.  Unfortunatly, as syzbot quickly told us, not all the world
is host controllers with DMA support, so we need to check what host controller
we are attempting to talk to before doing this type of allocation.

Thanks to Christoph for the quick idea of how to fix this.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Hillf Danton <hdanton@sina.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Jeremy Linton <jeremy.linton@arm.com>
Reported-by: syzbot+353be47c9ce21b68b7ed@syzkaller.appspotmail.com
Fixes: 2bef9aed6f0e ("usb: usbfs: correct kernel->user page attribute mismatch")
Cc: stable <stable@vger.kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/usb/core/devio.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index b9db9812d6c5..d93d94d7ff50 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -251,9 +251,19 @@ static int usbdev_mmap(struct file *file, struct vm_area_struct *vma)
 	usbm->vma_use_count = 1;
 	INIT_LIST_HEAD(&usbm->memlist);
 
-	if (dma_mmap_coherent(hcd->self.sysdev, vma, mem, dma_handle, size)) {
-		dec_usb_memory_use_count(usbm, &usbm->vma_use_count);
-		return -EAGAIN;
+	if (hcd->localmem_pool || !hcd_uses_dma(hcd)) {
+		if (remap_pfn_range(vma, vma->vm_start,
+				    virt_to_phys(usbm->mem) >> PAGE_SHIFT,
+				    size, vma->vm_page_prot) < 0) {
+			dec_usb_memory_use_count(usbm, &usbm->vma_use_count);
+			return -EAGAIN;
+		}
+	} else {
+		if (dma_mmap_coherent(hcd->self.sysdev, vma, mem, dma_handle,
+				      size)) {
+			dec_usb_memory_use_count(usbm, &usbm->vma_use_count);
+			return -EAGAIN;
+		}
 	}
 
 	vma->vm_flags |= VM_IO;
-- 
2.26.2

Re: [PATCH] USB: usbfs: fix mmap dma mismatch

[Jeremy Linton]

Hi,

So looking at hcd_buffer_alloc() again, there are 4 cases, 
localmem_pool, hcd_uses_dma, dma_pool_alloc and dma_alloc_coherent 
directly. The dma_pool_alloc appears to just be using 
dma_alloc_coherent, so its really three cases.

Those three cases appear to be handled below:

So:

Reviewed-by: Jeremy Linton <jeremy.linton@arm.com>

I'm testing it now...


Thanks,



On 5/14/20 6:27 AM, Greg Kroah-Hartman wrote:
> In commit 2bef9aed6f0e ("usb: usbfs: correct kernel->user page attribute
> mismatch") we switched from always calling remap_pfn_range() to call
> dma_mmap_coherent() to handle issues with systems with non-coherent USB host
> controller drivers.  Unfortunatly, as syzbot quickly told us, not all the world
> is host controllers with DMA support, so we need to check what host controller
> we are attempting to talk to before doing this type of allocation.
> 
> Thanks to Christoph for the quick idea of how to fix this.
> 
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Hillf Danton <hdanton@sina.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Jeremy Linton <jeremy.linton@arm.com>
> Reported-by: syzbot+353be47c9ce21b68b7ed@syzkaller.appspotmail.com
> Fixes: 2bef9aed6f0e ("usb: usbfs: correct kernel->user page attribute mismatch")
> Cc: stable <stable@vger.kernel.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>   drivers/usb/core/devio.c | 16 +++++++++++++---
>   1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
> index b9db9812d6c5..d93d94d7ff50 100644
> --- a/drivers/usb/core/devio.c
> +++ b/drivers/usb/core/devio.c
> @@ -251,9 +251,19 @@ static int usbdev_mmap(struct file *file, struct vm_area_struct *vma)
>   	usbm->vma_use_count = 1;
>   	INIT_LIST_HEAD(&usbm->memlist);
>   
> -	if (dma_mmap_coherent(hcd->self.sysdev, vma, mem, dma_handle, size)) {
> -		dec_usb_memory_use_count(usbm, &usbm->vma_use_count);
> -		return -EAGAIN;
> +	if (hcd->localmem_pool || !hcd_uses_dma(hcd)) {
> +		if (remap_pfn_range(vma, vma->vm_start,
> +				    virt_to_phys(usbm->mem) >> PAGE_SHIFT,
> +				    size, vma->vm_page_prot) < 0) {
> +			dec_usb_memory_use_count(usbm, &usbm->vma_use_count);
> +			return -EAGAIN;
> +		}
> +	} else {
> +		if (dma_mmap_coherent(hcd->self.sysdev, vma, mem, dma_handle,
> +				      size)) {
> +			dec_usb_memory_use_count(usbm, &usbm->vma_use_count);
> +			return -EAGAIN;
> +		}
>   	}
>   
>   	vma->vm_flags |= VM_IO;
> 

Re: [PATCH] USB: usbfs: fix mmap dma mismatch

[Christoph Hellwig]

On Thu, May 14, 2020 at 01:27:11PM +0200, Greg Kroah-Hartman wrote:
> +	if (hcd->localmem_pool || !hcd_uses_dma(hcd)) {
> +		if (remap_pfn_range(vma, vma->vm_start,
> +				    virt_to_phys(usbm->mem) >> PAGE_SHIFT,
> +				    size, vma->vm_page_prot) < 0) {
> +			dec_usb_memory_use_count(usbm, &usbm->vma_use_count);
> +			return -EAGAIN;
> +		}
> +	} else {
> +		if (dma_mmap_coherent(hcd->self.sysdev, vma, mem, dma_handle,
> +				      size)) {
> +			dec_usb_memory_use_count(usbm, &usbm->vma_use_count);
> +			return -EAGAIN;
> +		}

What about a goto label to share the error handling path?

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH] USB: usbfs: fix mmap dma mismatch

[Greg Kroah-Hartman]

On Thu, May 14, 2020 at 01:58:29PM +0200, Christoph Hellwig wrote:
> On Thu, May 14, 2020 at 01:27:11PM +0200, Greg Kroah-Hartman wrote:
> > +	if (hcd->localmem_pool || !hcd_uses_dma(hcd)) {
> > +		if (remap_pfn_range(vma, vma->vm_start,
> > +				    virt_to_phys(usbm->mem) >> PAGE_SHIFT,
> > +				    size, vma->vm_page_prot) < 0) {
> > +			dec_usb_memory_use_count(usbm, &usbm->vma_use_count);
> > +			return -EAGAIN;
> > +		}
> > +	} else {
> > +		if (dma_mmap_coherent(hcd->self.sysdev, vma, mem, dma_handle,
> > +				      size)) {
> > +			dec_usb_memory_use_count(usbm, &usbm->vma_use_count);
> > +			return -EAGAIN;
> > +		}
> 
> What about a goto label to share the error handling path?

I thought about that, but that's a bit messier than the duplicated lines
here :)

> Otherwise looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>

thanks for the review, and the help with this.

greg k-h

Re: [PATCH] USB: usbfs: fix mmap dma mismatch

[Christoph Hellwig]

On Thu, May 14, 2020 at 02:09:44PM +0200, Greg Kroah-Hartman wrote:
> > > +		if (dma_mmap_coherent(hcd->self.sysdev, vma, mem, dma_handle,
> > > +				      size)) {
> > > +			dec_usb_memory_use_count(usbm, &usbm->vma_use_count);
> > > +			return -EAGAIN;
> > > +		}
> > 
> > What about a goto label to share the error handling path?
> 
> I thought about that, but that's a bit messier than the duplicated lines
> here :)

Actually the error handling looks weird, we can just use normal unwinding
here with an extra call to usb_free_coherent.  Also -EAGAIN is a strange
error to return in this case, as it is simply incorrect.  I think passing
through the errors from dma_mmap_coherent and remap_pfn_range would make
a lot more sense.

Last but not least I wonder if this is the right place to open code
the localmem and has_dma checks - from a layering POV it should be
a usb_mmap_coherent helper at the same level as usb_alloc_coherent.