[PATCH v3 0/2] usb: Check for DMA capable buffer sanity

[PATCH v3 0/2] usb: Check for DMA capable buffer sanity

[Florian Fainelli]

Changes in v3:

- added check in usb_gadget_map_request_by_dev (Felipe), new patch
- improved commit message description (Clemens)
- added additiona checks for urb->setup_packet (Alan)

Changes in v2:

- moved the check from usb_start_wait_urb() to usb_hcd_map_urb_for_dma()

Florian Fainelli (2):
  usb: core: Check URB setup_packet and transfer_buffer sanity
  usb: udc: core: Error if req->buf is either from vmalloc or stack

 drivers/usb/core/hcd.c        | 12 ++++++++++++
 drivers/usb/gadget/udc/core.c |  9 +++++++++
 2 files changed, 21 insertions(+)

-- 
2.9.3

[PATCH v3 1/2] usb: core: Check URB setup_packet and transfer_buffer sanity

[Florian Fainelli]

Update usb_hcd_map_urb_for_dma() to check for an URB's setup_packet and
transfer_buffer sanity. We first check that urb->setup_packet is neither
coming from vmalloc space nor is an on stack buffer, and if that's the
case, produce a warning and return an error. For urb->transfer_buffer
there is an existing is_vmalloc_addr() check so we just supplement that
with an object_is_on_stack() check, produce a warning if that is the case
and also return an error.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/usb/core/hcd.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 49550790a3cb..26d710eec7da 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -26,6 +26,7 @@
 #include <linux/module.h>
 #include <linux/version.h>
 #include <linux/kernel.h>
+#include <linux/sched/task_stack.h>
 #include <linux/slab.h>
 #include <linux/completion.h>
 #include <linux/utsname.h>
@@ -1523,6 +1524,14 @@ int usb_hcd_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
 		if (hcd->self.uses_pio_for_control)
 			return ret;
 		if (IS_ENABLED(CONFIG_HAS_DMA) && hcd->self.uses_dma) {
+			if (is_vmalloc_addr(urb->setup_packet)) {
+				WARN_ONCE(1, "setup packet is not dma capable\n");
+				return -EAGAIN;
+			} else if (object_is_on_stack(urb->setup_packet)) {
+				WARN_ONCE(1, "setup packet is on stack\n");
+				return -EAGAIN;
+			}
+
 			urb->setup_dma = dma_map_single(
 					hcd->self.sysdev,
 					urb->setup_packet,
@@ -1587,6 +1596,9 @@ int usb_hcd_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
 			} else if (is_vmalloc_addr(urb->transfer_buffer)) {
 				WARN_ONCE(1, "transfer buffer not dma capable\n");
 				ret = -EAGAIN;
+			} else if (object_is_on_stack(urb->transfer_buffer)) {
+				WARN_ONCE(1, "transfer buffer is on stack\n");
+				ret = -EAGAIN;
 			} else {
 				urb->transfer_dma = dma_map_single(
 						hcd->self.sysdev,
-- 
2.9.3

[PATCH v3 2/2] usb: udc: core: Error if req->buf is either from vmalloc or stack

[Florian Fainelli]

Check that req->buf is a valid DMA capable address, produce a warning
and return an error if it's either coming from vmalloc space or is an on
stack buffer.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/usb/gadget/udc/core.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
index efce68e9a8e0..a03aec574f64 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -23,6 +23,7 @@
 #include <linux/list.h>
 #include <linux/err.h>
 #include <linux/dma-mapping.h>
+#include <linux/sched/task_stack.h>
 #include <linux/workqueue.h>
 
 #include <linux/usb/ch9.h>
@@ -798,6 +799,14 @@ int usb_gadget_map_request_by_dev(struct device *dev,
 
 		req->num_mapped_sgs = mapped;
 	} else {
+		if (is_vmalloc_addr(req->buf)) {
+			dev_err(dev, "buffer is not dma capable\n");
+			return -EFAULT;
+		} else if (object_is_on_stack(req->buf)) {
+			dev_err(dev, "buffer is on stack\n");
+			return -EFAULT;
+		}
+
 		req->dma = dma_map_single(dev, req->buf, req->length,
 				is_in ? DMA_TO_DEVICE : DMA_FROM_DEVICE);
 
-- 
2.9.3

Re: [PATCH v3 0/2] usb: Check for DMA capable buffer sanity

[Florian Fainelli]

On 04/25/2017 05:56 PM, Florian Fainelli wrote:
> Changes in v3:
> 
> - added check in usb_gadget_map_request_by_dev (Felipe), new patch
> - improved commit message description (Clemens)
> - added additiona checks for urb->setup_packet (Alan)
> 
> Changes in v2:
> 
> - moved the check from usb_start_wait_urb() to usb_hcd_map_urb_for_dma()

Is this version looking good now? Thanks!

> 
> Florian Fainelli (2):
>   usb: core: Check URB setup_packet and transfer_buffer sanity
>   usb: udc: core: Error if req->buf is either from vmalloc or stack
> 
>  drivers/usb/core/hcd.c        | 12 ++++++++++++
>  drivers/usb/gadget/udc/core.c |  9 +++++++++
>  2 files changed, 21 insertions(+)
> 


-- 
Florian

Re: [PATCH v3 0/2] usb: Check for DMA capable buffer sanity

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 3565 bytes --]

On Fri, May 05, 2017 at 02:08:31PM -0700, Florian Fainelli wrote:
> On 04/25/2017 05:56 PM, Florian Fainelli wrote:
> > Changes in v3:
> > 
> > - added check in usb_gadget_map_request_by_dev (Felipe), new patch
> > - improved commit message description (Clemens)
> > - added additiona checks for urb->setup_packet (Alan)
> > 
> > Changes in v2:
> > 
> > - moved the check from usb_start_wait_urb() to usb_hcd_map_urb_for_dma()
> 
> Is this version looking good now? Thanks!

So, it seems I am in a similar situation with the I2C subsystem right
now. I need to check the message buffers if they are DMA capable.

Because you have basically the same checks in 3 different places, and I
need something similar for I2C, I wondered about a generic place to put
these checks. Especially since we want future improvements to these
checks applied everywhere immediately. Here is a small diff on what I
have now:

===

dma-mapping(?): introduce helper to check for DMA capable addresses

Introduce a helper to check if an address is DMA capable. Such a check
is subtle, so it is good to have a centralized place for it.

Note: I am absolutely not sure if dma-mapping.h is a good place for such
a function. I just couldn't think of a better one for now.

Second note: I am not even sure the checks are complete (kmapped mem?).
I am not an MM expert. But that just strengthens the argument of having
on centralized place IMO.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>


diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 5dea98358c05c4..777a37b395ff19 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -1584,7 +1584,7 @@ int usb_hcd_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
 					ret = -EAGAIN;
 				else
 					urb->transfer_flags |= URB_DMA_MAP_PAGE;
-			} else if (is_vmalloc_addr(urb->transfer_buffer)) {
+			} else if (!is_dma_capable_addr(urb->transfer_buffer)) {
 				WARN_ONCE(1, "transfer buffer not dma capable\n");
 				ret = -EAGAIN;
 			} else {
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index 4f3eecedca2d7c..da8c1230302505 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -10,6 +10,8 @@
 #include <linux/scatterlist.h>
 #include <linux/kmemcheck.h>
 #include <linux/bug.h>
+#include <linux/mm.h>
+#include <linux/sched/task_stack.h>
 
 /**
  * List of possible attributes associated with a DMA mapping. The semantics
@@ -818,4 +820,10 @@ static inline int dma_mmap_wc(struct device *dev,
 #define dma_unmap_len_set(PTR, LEN_NAME, VAL)    do { } while (0)
 #endif
 
+/* only works in process context because of stack detection */
+static inline bool is_dma_capable_addr(void *addr)
+{
+	return !(is_vmalloc_or_module_addr(addr) ||
+		 object_is_on_stack(addr));
+}
 #endif
diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index 34a1c3e46ed725..47de0c0a700e7c 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -261,6 +261,7 @@ int is_vmalloc_or_module_addr(const void *x)
 #endif
 	return is_vmalloc_addr(x);
 }
+EXPORT_SYMBOL_GPL(is_vmalloc_or_module_addr);
 
 /*
  * Walk a vmap address to the struct page it maps.

===

The WIP branch containing also the I2C parts can be found here:

git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/topic/i2c-core-dma

I think the whole series needs 1 or 2 days more before I send out an
RFC, but I thought I'll let you know about my idea already.

Thanks and kind regards,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [PATCH v3 0/2] usb: Check for DMA capable buffer sanity

[David Laight]

From: Wolfram Sang
> Sent: 28 May 2017 17:04
> 
> On Fri, May 05, 2017 at 02:08:31PM -0700, Florian Fainelli wrote:
> > On 04/25/2017 05:56 PM, Florian Fainelli wrote:
> > > Changes in v3:
> > >
> > > - added check in usb_gadget_map_request_by_dev (Felipe), new patch
> > > - improved commit message description (Clemens)
> > > - added additiona checks for urb->setup_packet (Alan)
> > >
> > > Changes in v2:
> > >
> > > - moved the check from usb_start_wait_urb() to usb_hcd_map_urb_for_dma()
> >
> > Is this version looking good now? Thanks!
> 
> So, it seems I am in a similar situation with the I2C subsystem right
> now. I need to check the message buffers if they are DMA capable.
> 
> Because you have basically the same checks in 3 different places, and I
> need something similar for I2C, I wondered about a generic place to put
> these checks. Especially since we want future improvements to these
> checks applied everywhere immediately. Here is a small diff on what I
> have now:
...
> +			} else if (!is_dma_capable_addr(urb->transfer_buffer)) {

For a generic function I'd pass the length as well.
It might be that buffers that don't cross page boundaries might be
deemed 'dma-able'.

Possibly more useful would be a variant of (IIRC) dma_map_for_device()
that will allocate a suitable bounce buffer for non-dma memory.
I think it can already do so for memory that is outside the address
range that the device can address (eg for a 32bit PCIe master in 64bit
system).

	David

Re: RE: [PATCH v3 0/2] usb: Check for DMA capable buffer sanity

[Vignesh R]

On Wednesday 31 May 2017 04:34 PM, David Laight wrote:
> From: Wolfram Sang
>> Sent: 28 May 2017 17:04
>>
>> On Fri, May 05, 2017 at 02:08:31PM -0700, Florian Fainelli wrote:
>>> On 04/25/2017 05:56 PM, Florian Fainelli wrote:
>>>> Changes in v3:
>>>>
>>>> - added check in usb_gadget_map_request_by_dev (Felipe), new patch
>>>> - improved commit message description (Clemens)
>>>> - added additiona checks for urb->setup_packet (Alan)
>>>>
>>>> Changes in v2:
>>>>
>>>> - moved the check from usb_start_wait_urb() to usb_hcd_map_urb_for_dma()
>>>
>>> Is this version looking good now? Thanks!
>>
>> So, it seems I am in a similar situation with the I2C subsystem right
>> now. I need to check the message buffers if they are DMA capable.
>>
>> Because you have basically the same checks in 3 different places, and I
>> need something similar for I2C, I wondered about a generic place to put
>> these checks. Especially since we want future improvements to these
>> checks applied everywhere immediately. Here is a small diff on what I
>> have now:
> ...
>> +			} else if (!is_dma_capable_addr(urb->transfer_buffer)) {
> 
> For a generic function I'd pass the length as well.
> It might be that buffers that don't cross page boundaries might be
> deemed 'dma-able'.
> 
> Possibly more useful would be a variant of (IIRC) dma_map_for_device()
> that will allocate a suitable bounce buffer for non-dma memory.
> I think it can already do so for memory that is outside the address
> range that the device can address (eg for a 32bit PCIe master in 64bit
> system).
> 

Such generic DMA API would be greatly useful!

I tried adding bounce buffers support to handle vmalloc'd buffers in
MTD/SPI subsystem. But, there was a need felt for generic DMA API that
can allocate bounce buffer for non-dma'able buffers that all drivers can
make use of[1][2]


[1] https://lkml.org/lkml/2017/3/1/488
[2] https://lkml.org/lkml/2017/4/25/278



-- 
Regards
Vignesh

Re: [PATCH v3 0/2] usb: Check for DMA capable buffer sanity

[Vignesh R]

On Sunday 28 May 2017 09:33 PM, Wolfram Sang wrote:
> On Fri, May 05, 2017 at 02:08:31PM -0700, Florian Fainelli wrote:
>> On 04/25/2017 05:56 PM, Florian Fainelli wrote:
>>> Changes in v3:
>>>
>>> - added check in usb_gadget_map_request_by_dev (Felipe), new patch
>>> - improved commit message description (Clemens)
>>> - added additiona checks for urb->setup_packet (Alan)
>>>
>>> Changes in v2:
>>>
>>> - moved the check from usb_start_wait_urb() to usb_hcd_map_urb_for_dma()
>>
>> Is this version looking good now? Thanks!
> 
> So, it seems I am in a similar situation with the I2C subsystem right
> now. I need to check the message buffers if they are DMA capable.
> 
> Because you have basically the same checks in 3 different places, and I
> need something similar for I2C, I wondered about a generic place to put
> these checks. Especially since we want future improvements to these
> checks applied everywhere immediately. Here is a small diff on what I
> have now:
> 
> ===
> 
> dma-mapping(?): introduce helper to check for DMA capable addresses
> 
> Introduce a helper to check if an address is DMA capable. Such a check
> is subtle, so it is good to have a centralized place for it.
> 
> Note: I am absolutely not sure if dma-mapping.h is a good place for such
> a function. I just couldn't think of a better one for now.
> 
> Second note: I am not even sure the checks are complete (kmapped mem?).
> I am not an MM expert. But that just strengthens the argument of having
> on centralized place IMO.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 5dea98358c05c4..777a37b395ff19 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -1584,7 +1584,7 @@ int usb_hcd_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
>  					ret = -EAGAIN;
>  				else
>  					urb->transfer_flags |= URB_DMA_MAP_PAGE;
> -			} else if (is_vmalloc_addr(urb->transfer_buffer)) {
> +			} else if (!is_dma_capable_addr(urb->transfer_buffer)) {
>  				WARN_ONCE(1, "transfer buffer not dma capable\n");
>  				ret = -EAGAIN;
>  			} else {
> diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> index 4f3eecedca2d7c..da8c1230302505 100644
> --- a/include/linux/dma-mapping.h
> +++ b/include/linux/dma-mapping.h
> @@ -10,6 +10,8 @@
>  #include <linux/scatterlist.h>
>  #include <linux/kmemcheck.h>
>  #include <linux/bug.h>
> +#include <linux/mm.h>
> +#include <linux/sched/task_stack.h>
>  
>  /**
>   * List of possible attributes associated with a DMA mapping. The semantics
> @@ -818,4 +820,10 @@ static inline int dma_mmap_wc(struct device *dev,
>  #define dma_unmap_len_set(PTR, LEN_NAME, VAL)    do { } while (0)
>  #endif
>  
> +/* only works in process context because of stack detection */
> +static inline bool is_dma_capable_addr(void *addr)
> +{
> +	return !(is_vmalloc_or_module_addr(addr) ||
> +		 object_is_on_stack(addr));

This does not catch kmap'ed buffers which are not directly DMA'able.
I would suggest to use virt_addr_valid() instead. Something like:

	return (virt_addr_valid(addr) && !object_is_on_stack(addr));

> +}
>  #endif
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index 34a1c3e46ed725..47de0c0a700e7c 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -261,6 +261,7 @@ int is_vmalloc_or_module_addr(const void *x)
>  #endif
>  	return is_vmalloc_addr(x);
>  }
> +EXPORT_SYMBOL_GPL(is_vmalloc_or_module_addr);
>  
>  /*
>   * Walk a vmap address to the struct page it maps.
> 
> ===
> 
> The WIP branch containing also the I2C parts can be found here:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/topic/i2c-core-dma
> 
> I think the whole series needs 1 or 2 days more before I send out an
> RFC, but I thought I'll let you know about my idea already.
> 
> Thanks and kind regards,
> 
>    Wolfram
> 

-- 
Regards
Vignesh

Re: [PATCH v3 0/2] usb: Check for DMA capable buffer sanity

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 704 bytes --]


> > +/* only works in process context because of stack detection */
> > +static inline bool is_dma_capable_addr(void *addr)
> > +{
> > +	return !(is_vmalloc_or_module_addr(addr) ||
> > +		 object_is_on_stack(addr));
> 
> This does not catch kmap'ed buffers which are not directly DMA'able.
> I would suggest to use virt_addr_valid() instead. Something like:
> 
> 	return (virt_addr_valid(addr) && !object_is_on_stack(addr));

Hehe, here is the part of the commit message I have for this code:

===

Second note: I am not even sure the checks complete (kmapped mem?). But
that just strengthens the argument of having on centralized place IMO :)

===

So, thanks for the heads up!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: RE: [PATCH v3 0/2] usb: Check for DMA capable buffer sanity

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1609 bytes --]


> > Possibly more useful would be a variant of (IIRC) dma_map_for_device()
> > that will allocate a suitable bounce buffer for non-dma memory.
> > I think it can already do so for memory that is outside the address
> > range that the device can address (eg for a 32bit PCIe master in 64bit
> > system).
> > 
> 
> Such generic DMA API would be greatly useful!
> 
> I tried adding bounce buffers support to handle vmalloc'd buffers in
> MTD/SPI subsystem. But, there was a need felt for generic DMA API that
> can allocate bounce buffer for non-dma'able buffers that all drivers can
> make use of[1][2]

Yes, I see this DMA API would make sense for subsystems like SPI, MTD or
USB. For I2C, I don't think it makes a lot of sense because DMA is
rarely used there. Most hardware doesn't even have DMA support and if
so, the drivers apply a threshold (say 8 bytes) because most I2C
transfers are smaller and setting up DMA for that simply doesn't pay
off. And we are still talking of a mostly 100 or 400 kHz bus here.

So, I'd prefer a lightweight helper function telling if DMA is
possible/feasible for a given I2C message. If so, do it. If not, falling
back to PIO might be good enough for now. We can implement bounce buffer
support in the above helper function later. I don't really want to
enforce DMA capable buffers for I2C transactions when DMA is so rarely
needed there.

So, if I2C is a bit different, then it might simply make sense to keep
the function local for I2C now? This seems like a sensible start to me
meanwhile.

Thanks to all for the helpful input here!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: RE: [PATCH v3 0/2] usb: Check for DMA capable buffer sanity

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 366 bytes --]


> So, if I2C is a bit different, then it might simply make sense to keep
> the function local for I2C now? This seems like a sensible start to me
> meanwhile.

Then again, the DMA API would be for drivers. If the USB core wants to
check for capable buffers, such a helper might be nice nonetheless.
Especially if the USB core needs the check in at least 3 places.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]