[PATCH] usb/mcs7830: Don't use buffers from stack for USB transfers

[PATCH] usb/mcs7830: Don't use buffers from stack for USB transfers

[Christian Eggers]

From: Christian Eggers <christian.eggers@kathrein.de>

mcs7830_set_reg() and mcs7830_get_reg() are called with buffers
from stack which must not be used directly for USB transfers.
This causes corruption of the stack particulary on non x86
architectures because DMA may be used for these transfers.

Signed-off-by: Christian Eggers <christian.eggers@kathrein.de>
---

This is my first patch submission for Linux. I hope everything is fine.

diff -uprN linux-2.6.28.1.orig/drivers/net/usb/mcs7830.c linux-2.6.28.1/drivers/net/usb/mcs7830.c
--- linux-2.6.28.1.orig/drivers/net/usb/mcs7830.c       2009-01-18 19:45:37.000000000 +0100
+++ linux-2.6.28.1/drivers/net/usb/mcs7830.c    2009-01-20 20:53:59.000000000 +0100
@@ -94,10 +94,18 @@ static int mcs7830_get_reg(struct usbnet
 {
        struct usb_device *xdev = dev->udev;
        int ret;
+       void *buffer;
+
+       buffer = kmalloc(size, GFP_NOIO);
+       if (buffer == NULL)
+               return -ENOMEM;

        ret = usb_control_msg(xdev, usb_rcvctrlpipe(xdev, 0), MCS7830_RD_BREQ,
-                             MCS7830_RD_BMREQ, 0x0000, index, data,
+                             MCS7830_RD_BMREQ, 0x0000, index, buffer,
                              size, MCS7830_CTRL_TIMEOUT);
+       memcpy(data, buffer, size);
+       kfree(buffer);
+
        return ret;
 }

@@ -105,10 +113,18 @@ static int mcs7830_set_reg(struct usbnet
 {
        struct usb_device *xdev = dev->udev;
        int ret;
+       void *buffer;
+
+       buffer = kmalloc(size, GFP_NOIO);
+       if (buffer == NULL)
+               return -ENOMEM;
+
+       memcpy(buffer, data, size);

        ret = usb_control_msg(xdev, usb_sndctrlpipe(xdev, 0), MCS7830_WR_BREQ,
-                             MCS7830_WR_BMREQ, 0x0000, index, data,
+                             MCS7830_WR_BMREQ, 0x0000, index, buffer,
                              size, MCS7830_CTRL_TIMEOUT);
+       kfree(buffer);
        return ret;
 }

Re: [PATCH] usb/mcs7830: Don't use buffers from stack for USB transfers

[David Miller]

From: "Christian Eggers" <ceggers@gmx.de>
Date: Tue, 20 Jan 2009 21:29:21 +0100 (CET)

> This is my first patch submission for Linux. I hope everything is fine.

Unfortuntately not, your email client has corrupted the
patch changing tab characters into spaces.

Please resubmit this after correcting this problem.  See:

	Documentation/email-clients.txt

in the kernel source for some help with this issue.

Thank you.

Re: [PATCH] usb/mcs7830: Don't use buffers from stack for USB transfers

[Arnd Bergmann]

On Tuesday 20 January 2009, you wrote:
> From: Christian Eggers <christian.eggers@kathrein.de>
> 
> mcs7830_set_reg() and mcs7830_get_reg() are called with buffers
> from stack which must not be used directly for USB transfers.
> This causes corruption of the stack particulary on non x86
> architectures because DMA may be used for these transfers.
> 
> Signed-off-by: Christian Eggers <christian.eggers@kathrein.de>

Have you observed problems with this, or just suspected trouble?

When I wrote this code, I looked at other code doing the same
and assumed it was ok, because usb_control_msg waits for the
DMA to complete before returning.

Is the problem only on systems that have noncoherent DMA, or
something else?

> This is my first patch submission for Linux. I hope everything is fine.

I was about to say that you should have Cc:'d me, but then I noticed
that I'm not listed in the driver as maintainer, nor in the MAINTAINERS
file, so I can't really complain here ;-)

	Arnd <><

Re: [PATCH] usb/mcs7830: Don't use buffers from stack for USB transfers

[David Miller]

From: Arnd Bergmann <arnd@arndb.de>
Date: Tue, 20 Jan 2009 23:45:47 +0100

> On Tuesday 20 January 2009, you wrote:
> > From: Christian Eggers <christian.eggers@kathrein.de>
> > 
> > mcs7830_set_reg() and mcs7830_get_reg() are called with buffers
> > from stack which must not be used directly for USB transfers.
> > This causes corruption of the stack particulary on non x86
> > architectures because DMA may be used for these transfers.
> > 
> > Signed-off-by: Christian Eggers <christian.eggers@kathrein.de>
> 
> Have you observed problems with this, or just suspected trouble?

Yes, this is in response to SH platform failures.

You cannot DMA from/to the kernel stack, because it might not be in
the aliased linear mapping of physical memory.  It could even be
vmalloc()'d memory on some platforms.

Re: [PATCH] usb/mcs7830: Don't use buffers from stack for USB transfers

[Arnd Bergmann]

On Tuesday 20 January 2009, David Miller wrote:
> You cannot DMA from/to the kernel stack, because it might not be in
> the aliased linear mapping of physical memory.  It could even be
> vmalloc()'d memory on some platforms.

Ok, I see. It seems a bit of a waste to do a kmalloc for something
that is guaranteed to be just a few bytes, but allocating the
buffer per-device would mean another mutex, which has about the
same overhead, so I'm basically ok with the patch.

> +       buffer = kmalloc(size, GFP_NOIO);

GFP_NOIO seems out of place in a network driver: there is nothing
wrong with waiting for I/O here, so plain GFP_KERNEL should be fine.

> +       if (buffer == NULL)
> +               return -ENOMEM;

I'd prefer to write 

	if (!buffer)

here, as I do elsewhere in the driver.

Otherwise,

Acked-by: Arnd Bergmann <arnd@arndb.de>

Re: [PATCH] usb/mcs7830: Don't use buffers from stack for USB transfers

[David Miller]

From: Arnd Bergmann <arnd@arndb.de>
Date: Wed, 21 Jan 2009 00:17:18 +0100

> On Tuesday 20 January 2009, David Miller wrote:
> > +       buffer = kmalloc(size, GFP_NOIO);
> 
> GFP_NOIO seems out of place in a network driver: there is nothing
> wrong with waiting for I/O here, so plain GFP_KERNEL should be fine.

There seems to be a large precendence for this in other USB drivers,
both for networking and storage.  Probably a mutex or other locking
hierarchy issue.

Really, I would just apply this patch as-is.  It works, it's pretty
clean, and every retort has been a misunderstanding or extreme
nit-picking :-)

Re: [PATCH] usb/mcs7830: Don't use buffers from stack for USB transfers

[Oliver Neukum]

Am Wednesday 21 January 2009 00:23:45 schrieb David Miller:
> > GFP_NOIO seems out of place in a network driver: there is nothing
> > wrong with waiting for I/O here, so plain GFP_KERNEL should be fine.
> 
> There seems to be a large precendence for this in other USB drivers,
> both for networking and storage.  Probably a mutex or other locking
> hierarchy issue.

Usb storage uses a lot of infrastructure in error handling. As a storage
interface and another interface can share a device and be reset only
together. All drivers' reset handling must be written as if they were
block devices. Therefore you see a lot of GFP_NOIO in USB.

	Regards
		Oliver

Re: [PATCH] usb/mcs7830: Don't use buffers from stack for USB transfers

[Arnd Bergmann]

On Wednesday 21 January 2009, David Miller wrote:
> There seems to be a large precendence for this in other USB drivers,
> both for networking and storage.  Probably a mutex or other locking
> hierarchy issue.
> 
> Really, I would just apply this patch as-is.  It works, it's pretty
> clean, and every retort has been a misunderstanding or extreme
> nit-picking :-)

Ok, fair enough. Please add my Acked-by then.

On a related topic, can we put something in place that can check for
this error at run-time, like a WARN_ON(is_kernel_stack(addr)) in
dma_map_single?
Since I only copied this code from elsewhere, I would suspect that
there are lots of similar bugs that never get found on common hardware
otherwise.

	Arnd <><

Re: [PATCH] usb/mcs7830: Don't use buffers from stack for USB transfers

[David Miller]

From: Arnd Bergmann <arnd@arndb.de>
Date: Wed, 21 Jan 2009 00:37:51 +0100

> On a related topic, can we put something in place that can check for
> this error at run-time, like a WARN_ON(is_kernel_stack(addr)) in
> dma_map_single?

That would be something for the DMA API debugging patches that
have been floating around lately.

Re: [PATCH] usb/mcs7830: Don't use buffers from stack for USB transfers

[Oliver Neukum]

Am Wednesday 21 January 2009 00:17:18 schrieb Arnd Bergmann:
> > +       buffer = kmalloc(size, GFP_NOIO);
> 
> GFP_NOIO seems out of place in a network driver: there is nothing
> wrong with waiting for I/O here, so plain GFP_KERNEL should be fine.

What happens if you run nfs over that link?

	Regards
		Oliver

Re: [PATCH] usb/mcs7830: Don't use buffers from stack for USB transfers

[Oliver Neukum]

Am Tuesday 20 January 2009 23:45:47 schrieb Arnd Bergmann:
> Have you observed problems with this, or just suspected trouble?
> 
> When I wrote this code, I looked at other code doing the same
> and assumed it was ok, because usb_control_msg waits for the
> DMA to complete before returning.
> 
> Is the problem only on systems that have noncoherent DMA, or
> something else?

That's not enough. Tasks can leave pointers to variables on the
stack to other tasks. You must under no circumstances do DMA
on the stack if the driver may run on system that have noncoherent
DMA.

	Regards
		Oliver