[RFC] usb: Check results of dma_map_single

[RFC] usb: Check results of dma_map_single

[Larry Finger]

At http://marc.info/?l=linux-wireless&m=125695331205062&w=2, a problem
with DMA buffer processing was corrected for the libertas driver. Because
routine usb_fill_bulk_urb() does not check that DMA is possible when
dma_map_single() is called, this condition was not detected until the buffer
was unmapped. By this time memory corruption had occurred.

The situation is fixed by testing the returned DMA address. If not a legal
address, a WARN_ON(1) is executed to provide traceback and the error is
returned.

Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
---

Index: linux-2.6/drivers/usb/core/hcd.c
===================================================================
--- linux-2.6.orig/drivers/usb/core/hcd.c
+++ linux-2.6/drivers/usb/core/hcd.c
@@ -1281,6 +1281,13 @@ static int map_urb_for_dma(struct usb_hc
 					urb->setup_packet,
 					sizeof(struct usb_ctrlrequest),
 					DMA_TO_DEVICE);
+			ret = dma_mapping_error(hcd->self.controller,
+						urb->setup_dma);
+			/* warn if DMA mapping failed */
+			if (ret) {
+				WARN_ON(1);
+				return ret;
+			}
 		else if (hcd->driver->flags & HCD_LOCAL_MEM)
 			ret = hcd_alloc_coherent(
 					urb->dev->bus, mem_flags,
@@ -1299,6 +1306,13 @@ static int map_urb_for_dma(struct usb_hc
 					urb->transfer_buffer,
 					urb->transfer_buffer_length,
 					dir);
+			ret = dma_mapping_error(hcd->self.controller,
+						urb->transfer_dma);
+			/* warn if DMA mapping failed */
+			if (ret) {
+				WARN_ON(1);
+				return ret;
+			}
 		else if (hcd->driver->flags & HCD_LOCAL_MEM) {
 			ret = hcd_alloc_coherent(
 					urb->dev->bus, mem_flags,

Re: [RFC] usb: Check results of dma_map_single

[Paulius Zaleckas]

On 11/04/2009 07:48 AM, Larry Finger wrote:
> At http://marc.info/?l=linux-wireless&m=125695331205062&w=2, a problem
> with DMA buffer processing was corrected for the libertas driver. Because
> routine usb_fill_bulk_urb() does not check that DMA is possible when
> dma_map_single() is called, this condition was not detected until the buffer
> was unmapped. By this time memory corruption had occurred.
>
> The situation is fixed by testing the returned DMA address. If not a legal
> address, a WARN_ON(1) is executed to provide traceback and the error is
> returned.
>
> Signed-off-by: Larry Finger<Larry.Finger-tQ5ms3gMjBLk1uMJSBkQmQ@public.gmane.org>
> ---
>
> Index: linux-2.6/drivers/usb/core/hcd.c
> ===================================================================
> --- linux-2.6.orig/drivers/usb/core/hcd.c
> +++ linux-2.6/drivers/usb/core/hcd.c
> @@ -1281,6 +1281,13 @@ static int map_urb_for_dma(struct usb_hc
>   					urb->setup_packet,
>   					sizeof(struct usb_ctrlrequest),
>   					DMA_TO_DEVICE);
> +			ret = dma_mapping_error(hcd->self.controller,
> +						urb->setup_dma);
> +			/* warn if DMA mapping failed */
> +			if (ret) {
> +				WARN_ON(1);
> +				return ret;
> +			}

First of all you forgot to add { } around everything under if statement.

I don't think WARN_ON is needed...
dma_mapping_error under most architectures return 0 or 1 so it would be
better to make some real error value. EAGAIN seems to be proper error,
since documentation says that driver should try again later.

I would write this error handler like this:

if (dma_mapping_error(hcd->self.controller, urb->setup_dma))
	ret = -EAGAIN;

>   		else if (hcd->driver->flags&  HCD_LOCAL_MEM)
>   			ret = hcd_alloc_coherent(
>   					urb->dev->bus, mem_flags,
> @@ -1299,6 +1306,13 @@ static int map_urb_for_dma(struct usb_hc
>   					urb->transfer_buffer,
>   					urb->transfer_buffer_length,
>   					dir);
> +			ret = dma_mapping_error(hcd->self.controller,
> +						urb->transfer_dma);
> +			/* warn if DMA mapping failed */
> +			if (ret) {
> +				WARN_ON(1);
> +				return ret;
> +			}

ditto

>   		else if (hcd->driver->flags&  HCD_LOCAL_MEM) {
>   			ret = hcd_alloc_coherent(
>   					urb->dev->bus, mem_flags,

Re: [RFC] usb: Check results of dma_map_single

[Larry Finger]

Paulius Zaleckas wrote:
> On 11/04/2009 07:48 AM, Larry Finger wrote:
>> At http://marc.info/?l=linux-wireless&m=125695331205062&w=2, a problem
>> with DMA buffer processing was corrected for the libertas driver. Because
>> routine usb_fill_bulk_urb() does not check that DMA is possible when
>> dma_map_single() is called, this condition was not detected until the
>> buffer
>> was unmapped. By this time memory corruption had occurred.
>>
>> The situation is fixed by testing the returned DMA address. If not a
>> legal
>> address, a WARN_ON(1) is executed to provide traceback and the error is
>> returned.
>>
>> Signed-off-by: Larry
>> Finger<Larry.Finger-tQ5ms3gMjBLk1uMJSBkQmQ@public.gmane.org>
>> ---
>>
>> Index: linux-2.6/drivers/usb/core/hcd.c
>> ===================================================================
>> --- linux-2.6.orig/drivers/usb/core/hcd.c
>> +++ linux-2.6/drivers/usb/core/hcd.c
>> @@ -1281,6 +1281,13 @@ static int map_urb_for_dma(struct usb_hc
>>                       urb->setup_packet,
>>                       sizeof(struct usb_ctrlrequest),
>>                       DMA_TO_DEVICE);
>> +            ret = dma_mapping_error(hcd->self.controller,
>> +                        urb->setup_dma);
>> +            /* warn if DMA mapping failed */
>> +            if (ret) {
>> +                WARN_ON(1);
>> +                return ret;
>> +            }
> 
> First of all you forgot to add { } around everything under if statement.
> 
> I don't think WARN_ON is needed...
> dma_mapping_error under most architectures return 0 or 1 so it would be
> better to make some real error value. EAGAIN seems to be proper error,
> since documentation says that driver should try again later.
> 
> I would write this error handler like this:
> 
> if (dma_mapping_error(hcd->self.controller, urb->setup_dma))
>     ret = -EAGAIN;
> 
>>           else if (hcd->driver->flags&  HCD_LOCAL_MEM)
>>               ret = hcd_alloc_coherent(
>>                       urb->dev->bus, mem_flags,
>> @@ -1299,6 +1306,13 @@ static int map_urb_for_dma(struct usb_hc
>>                       urb->transfer_buffer,
>>                       urb->transfer_buffer_length,
>>                       dir);
>> +            ret = dma_mapping_error(hcd->self.controller,
>> +                        urb->transfer_dma);
>> +            /* warn if DMA mapping failed */
>> +            if (ret) {
>> +                WARN_ON(1);
>> +                return ret;
>> +            }
> 
> ditto
> 
>>           else if (hcd->driver->flags&  HCD_LOCAL_MEM) {
>>               ret = hcd_alloc_coherent(
>>                       urb->dev->bus, mem_flags,
> 

Thank you for your review and comments. I'll wait a bit to see what
other comments are offered, but I have implemented all your changes.

Larry

Re: [RFC] usb: Check results of dma_map_single

[Michael Buesch]

On Wednesday 04 November 2009 15:06:25 Larry Finger wrote:
> Thank you for your review and comments. I'll wait a bit to see what
> other comments are offered, but I have implemented all your changes.

If you use a WARN_ON, please use WARN_ON_ONCE to avoid possible spamming of the logs.

-- 
Greetings, Michael.

Re: [RFC] usb: Check results of dma_map_single

[Larry Finger]

On 11/04/2009 08:51 AM, Michael Buesch wrote:
> On Wednesday 04 November 2009 15:06:25 Larry Finger wrote:
>> Thank you for your review and comments. I'll wait a bit to see what
>> other comments are offered, but I have implemented all your changes.
> 
> If you use a WARN_ON, please use WARN_ON_ONCE to avoid possible spamming of the logs.

Good point.

Larry

Re: [RFC] usb: Check results of dma_map_single

[Christian Lamparter]

On Wednesday 04 November 2009 06:48:51 Larry Finger wrote:
> At http://marc.info/?l=linux-wireless&m=125695331205062&w=2, a problem
> with DMA buffer processing was corrected for the libertas driver. Because
> routine usb_fill_bulk_urb() does not check that DMA is possible when
           ^^^
hmm, usb_fill_bulk_urb? No, that should be usb_submit_urb :)

http://osdir.com/ml/linux-wireless/2009-10/msg01182.html

> dma_map_single() is called, this condition was not detected until the buffer
> was unmapped. By this time memory corruption had occurred.
> 
> The situation is fixed by testing the returned DMA address. If not a legal
> address, a WARN_ON(1) is executed to provide traceback and the error is
> returned.
> 
> Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>

Re: [RFC] usb: Check results of dma_map_single

[Larry Finger]

On 11/04/2009 01:49 PM, Christian Lamparter wrote:
> On Wednesday 04 November 2009 06:48:51 Larry Finger wrote:
>> At http://marc.info/?l=linux-wireless&m=125695331205062&w=2, a problem
>> with DMA buffer processing was corrected for the libertas driver. Because
>> routine usb_fill_bulk_urb() does not check that DMA is possible when
>            ^^^
> hmm, usb_fill_bulk_urb? No, that should be usb_submit_urb :)

Thanks. The actual routine modified is map_urb_for_dma.

Larry