Re: [PATCH v2] dma-mapping: Move vmap address checks into dma_map_single()

[Florian Fainelli]

On 10/4/19 2:28 PM, Kees Cook wrote:
> As we've seen from USB and other areas, we need to always do runtime
> checks for DMA operating on memory regions that might be remapped. This
> moves the existing checks from USB into dma_map_single(), but leaves
> the slightly heavier checks as they are.
> 
> Suggested-by: Laura Abbott <labbott@xxxxxxxxxx>
> Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>

Nearly the same might be necessary for
drivers/usb/gadget/udc/core.c::usb_gadget_map_request_by_dev(). You
might also want to check other subsystems, SPI tries to be fairly smart
about vmalloc'd and kmap'd buffer, some I2C drivers have explicit checks
(they should probably rely on the core), and finally MTD is an area
where this has also been historically dealt with.

> ---
> v2: Only add is_vmalloc_addr()
> v1: 201910021341.7819A660@keescook">https://lore.kernel.org/lkml/201910021341.7819A660@keescook
> ---
>  drivers/usb/core/hcd.c      | 8 +-------
>  include/linux/dma-mapping.h | 7 +++++++
>  2 files changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index f225eaa98ff8..281568d464f9 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -1410,10 +1410,7 @@ int usb_hcd_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
>  		if (hcd->self.uses_pio_for_control)
>  			return ret;
>  		if (hcd_uses_dma(hcd)) {
> -			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)) {
> +			if (object_is_on_stack(urb->setup_packet)) {
>  				WARN_ONCE(1, "setup packet is on stack\n");
>  				return -EAGAIN;
>  			}
> @@ -1479,9 +1476,6 @@ 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)) {
> -				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;
> diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> index 4a1c4fca475a..12dbd07f74f2 100644
> --- a/include/linux/dma-mapping.h
> +++ b/include/linux/dma-mapping.h
> @@ -583,6 +583,13 @@ static inline unsigned long dma_get_merge_boundary(struct device *dev)
>  static inline dma_addr_t dma_map_single_attrs(struct device *dev, void *ptr,
>  		size_t size, enum dma_data_direction dir, unsigned long attrs)
>  {
> +	/* DMA must never operate on areas that might be remapped. */
> +	if (WARN_ONCE(is_vmalloc_addr(ptr),
> +		      "%s %s: driver maps %lu bytes from vmalloc area\n",
> +		      dev ? dev_driver_string(dev) : "unknown driver",
> +		      dev ? dev_name(dev) : "unknown device", size))
> +		return DMA_MAPPING_ERROR;
> +
>  	debug_dma_map_single(dev, ptr, size);
>  	return dma_map_page_attrs(dev, virt_to_page(ptr), offset_in_page(ptr),
>  			size, dir, attrs);
> 


-- 
Florian