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 <[email protected]>
---
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,
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 <[email protected]>
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<[email protected]>
>> ---
>>
>> 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
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<[email protected]>
> ---
>
> 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,
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
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
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.