linux-kernel - Re: [PATCH] dma-mapping: skip USB devices when configuring DMA during probe

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

Message-ID: <f203fd95-886b-adf6-33f9-e85a9198711f@arm.com>
Date:   Thu, 3 Aug 2017 12:50:20 +0100
From:   Robin Murphy <robin.murphy@....com>
To:     Johan Hovold <johan@...nel.org>, Christoph Hellwig <hch@....de>,
        Marek Szyprowski <m.szyprowski@...sung.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     Andreas Färber <afaerber@...e.de>,
        linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-rpi-kernel@...ts.infradead.org,
        stable <stable@...r.kernel.org>,
        Sricharan R <sricharan@...eaurora.org>,
        Stefan Wahren <stefan.wahren@...e.com>
Subject: Re: [PATCH] dma-mapping: skip USB devices when configuring DMA during
 probe

Hi Johan,

On 03/08/17 11:05, Johan Hovold wrote:
> USB devices use the DMA mask and offset of the controller, which have
> already been setup when a device is probed. Note that modifying the mask
> of a USB device would change the mask for the controller (and all
> devices on the bus) as the mask is literally shared.

If I understand the situation correctly, some USB devices have to
pretend to be DMA masters in order to interact properly with subsystems
that make DMA mapping calls directly, because those subsystems don't
have access to the 'real' DMA master sysdev (and shouldn't have to know
anyway). If that is the case, then there's really a much more general
problem, because correct DMA API operation can depend on other things
like IOMMU stuff and archdata which cannot be arbitrarily copied or
shared between devices.

What with things like the DWC3 glue layer device mess as well, the
feeling I'm getting is that this probably needs addressing at the driver
core/DMA API level - if we had some flag in struct device to say "I can
effectively do DMA, but proxied through my parent", the DMA API could
pick that up and internally chase down the correct DMA master device,
and we could then probably clean up a lot of the "sysdev"-type gunk in
various driver layers as well.

For expedience of keeping the existing hack working, though, I think
this patch is OK.

> Since commit 2bf698671205 ("USB: of: fix root-hub device-tree node
> handling"), of_dma_configure() would be called also for root hubs, which
> use the device node of the controller. A separate bug that makes
> of_dma_configure() generate a 30-bit DMA mask from the RPI3's
> "dma-ranges" would thus set a broken mask also for the controller. This
> in turn leads to USB devices failing to enumerate when control transfers
> fail:

If we're calling out that (long-standing) bug in particular, it might be
worth clarifying that it's benign for the HCD itself because the dwc2
driver still sets a 32-bit mask afterwards on probe - the breakage is
specifically from the subsequent of_dma_configure() call with the root
hub resetting the HCD's mask again *after* the HCD driver has configured
it (and started to perform DMA), which is just fundamentally wrong
regardless of what it ends up set to.

> 	dwc2 3f980000.usb: Cannot do DMA to address 0x000000003a166a00
> 
> Fix this, and similar future problems, by simply skipping USB devices
> when dma_configure() is called during probe.
> 
> Fixes: 09515ef5ddad ("of/acpi: Configure dma operations at probe time for platform/amba/pci bus devices")
> Cc: stable <stable@...r.kernel.org>	# 4.12
> Cc: Robin Murphy <robin.murphy@....com>
> Cc: Sricharan R <sricharan@...eaurora.org>
> Cc: Stefan Wahren <stefan.wahren@...e.com>
> Reported-by: Hans Verkuil <hverkuil@...all.nl>
> Signed-off-by: Johan Hovold <johan@...nel.org>
> ---
>  drivers/base/dma-mapping.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/base/dma-mapping.c b/drivers/base/dma-mapping.c
> index b555ff9dd8fc..c6cde7e79599 100644
> --- a/drivers/base/dma-mapping.c
> +++ b/drivers/base/dma-mapping.c
> @@ -13,6 +13,7 @@
>  #include <linux/gfp.h>
>  #include <linux/of_device.h>
>  #include <linux/slab.h>
> +#include <linux/usb.h>
>  #include <linux/vmalloc.h>
>  
>  /*
> @@ -345,6 +346,10 @@ int dma_configure(struct device *dev)
>  	enum dev_dma_attr attr;
>  	int ret = 0;
>  
> +	/* USB devices share the controller's mask. */
> +	if (dev->bus == &usb_bus_type)

Maybe a dev_is_usb() helper to mimic dev_is_pci()?

Robin.

> +		return 0;
> +
>  	if (dev_is_pci(dev)) {
>  		bridge = pci_get_host_bridge_device(to_pci_dev(dev));
>  		dma_dev = bridge;
> @@ -369,6 +374,9 @@ int dma_configure(struct device *dev)
>  
>  void dma_deconfigure(struct device *dev)
>  {
> +	if (dev->bus == &usb_bus_type)
> +		return;
> +
>  	of_dma_deconfigure(dev);
>  	acpi_dma_deconfigure(dev);
>  }
>