[patch] usb: io_edgeport: eliminate get_string()

[patch] usb: io_edgeport: eliminate get_string()

[Dan Carpenter]

Johan Hovold points out that get_string() is basically just a re-implimentation
of usb_string().  It is also buggy.  It does DMA on the stack and it doesn't
handle negative returns from usb_get_descriptor().  Plus unicode_to_ascii() is 
a rubbish function and moving to usb_string() avoids using it.

Let's eliminate get_string() entirely.

Reported-by: Johan Hovold <jhovold@gmail.com>
Signed-off-by: Dan Carpenter <error27@gmail.com>
Cc: stable@kernel.org

--- orig/drivers/usb/serial/io_edgeport.c	2009-12-30 17:20:35.000000000 +0300
+++ devel/drivers/usb/serial/io_edgeport.c	2010-01-25 08:27:41.000000000 +0300
@@ -364,42 +364,6 @@ static void update_edgeport_E2PROM(struc
 	release_firmware(fw);
 }
 
-
-/************************************************************************
- *									*
- *  Get string descriptor from device					*
- *									*
- ************************************************************************/
-static int get_string(struct usb_device *dev, int Id, char *string, int buflen)
-{
-	struct usb_string_descriptor StringDesc;
-	struct usb_string_descriptor *pStringDesc;
-
-	dbg("%s - USB String ID = %d", __func__, Id);
-
-	if (!usb_get_descriptor(dev, USB_DT_STRING, Id,
-					&StringDesc, sizeof(StringDesc)))
-		return 0;
-
-	pStringDesc = kmalloc(StringDesc.bLength, GFP_KERNEL);
-	if (!pStringDesc)
-		return 0;
-
-	if (!usb_get_descriptor(dev, USB_DT_STRING, Id,
-					pStringDesc, StringDesc.bLength)) {
-		kfree(pStringDesc);
-		return 0;
-	}
-
-	unicode_to_ascii(string, buflen,
-				pStringDesc->wData, pStringDesc->bLength/2);
-
-	kfree(pStringDesc);
-	dbg("%s - USB String %s", __func__, string);
-	return strlen(string);
-}
-
-
 #if 0
 /************************************************************************
  *
@@ -2997,10 +2961,12 @@ static int edge_startup(struct usb_seria
 	usb_set_serial_data(serial, edge_serial);
 
 	/* get the name for the device from the device */
-	i = get_string(dev, dev->descriptor.iManufacturer,
+	i = usb_string(dev, dev->descriptor.iManufacturer,
 	    &edge_serial->name[0], MAX_NAME_LEN+1);
+	if (i < 0)
+		i = 0;
 	edge_serial->name[i++] = ' ';
-	get_string(dev, dev->descriptor.iProduct,
+	usb_string(dev, dev->descriptor.iProduct,
 	    &edge_serial->name[i], MAX_NAME_LEN+2 - i);
 
 	dev_info(&serial->dev->dev, "%s detected\n", edge_serial->name);

Re: [patch] usb: io_edgeport: eliminate get_string()

[Johan Hovold]

On Mon, Jan 25, 2010 at 02:53:33PM +0300, Dan Carpenter wrote:
> Johan Hovold points out that get_string() is basically just a re-implimentation
> of usb_string().  It is also buggy.  It does DMA on the stack and it doesn't
> handle negative returns from usb_get_descriptor().  Plus unicode_to_ascii() is 
> a rubbish function and moving to usb_string() avoids using it.
> 
> Let's eliminate get_string() entirely.
> 
> Reported-by: Johan Hovold <jhovold@gmail.com>
> Signed-off-by: Dan Carpenter <error27@gmail.com>
> Cc: stable@kernel.org

Acked.

How do we generally deal with patches with multiple authors? This patch
is a logical and obvious extension of the patch I submitted yesterday,
which in turn was a fix (implemented from scratch) for two bugs pointed
out by Dan.

This particular change is trivial of course, but are there any general
guidelines for such cases?

Thanks,
Johan

Re: [patch] usb: io_edgeport: eliminate get_string()

[Greg KH]

On Mon, Jan 25, 2010 at 02:53:33PM +0300, Dan Carpenter wrote:
> Johan Hovold points out that get_string() is basically just a re-implimentation
> of usb_string().  It is also buggy.  It does DMA on the stack and it doesn't
> handle negative returns from usb_get_descriptor().  Plus unicode_to_ascii() is 
> a rubbish function and moving to usb_string() avoids using it.
> 
> Let's eliminate get_string() entirely.

Hm, for some reason I couldn't use usb_string() when I originally wrote
this code (way back in 1999).  I can't recall what it is, but have you
tested this with an edgeport device to verify that it doesn't break
anything?

thanks,

greg k-h

Re: [patch] usb: io_edgeport: eliminate get_string()

[Greg KH]

On Mon, Jan 25, 2010 at 05:57:13PM +0100, Johan Hovold wrote:
> On Mon, Jan 25, 2010 at 02:53:33PM +0300, Dan Carpenter wrote:
> > Johan Hovold points out that get_string() is basically just a re-implimentation
> > of usb_string().  It is also buggy.  It does DMA on the stack and it doesn't
> > handle negative returns from usb_get_descriptor().  Plus unicode_to_ascii() is 
> > a rubbish function and moving to usb_string() avoids using it.
> > 
> > Let's eliminate get_string() entirely.
> > 
> > Reported-by: Johan Hovold <jhovold@gmail.com>
> > Signed-off-by: Dan Carpenter <error27@gmail.com>
> > Cc: stable@kernel.org
> 
> Acked.
> 
> How do we generally deal with patches with multiple authors? This patch
> is a logical and obvious extension of the patch I submitted yesterday,
> which in turn was a fix (implemented from scratch) for two bugs pointed
> out by Dan.
> 
> This particular change is trivial of course, but are there any general
> guidelines for such cases?

No, not really.  If you can think of something that would work with git
as-is, I'm sure that there would be people interested.

thanks,

greg k-h

Re: [patch] usb: io_edgeport: eliminate get_string()

[Greg KH]

On Mon, Jan 25, 2010 at 02:53:33PM +0300, Dan Carpenter wrote:
> Johan Hovold points out that get_string() is basically just a re-implimentation
> of usb_string().  It is also buggy.  It does DMA on the stack and it doesn't
> handle negative returns from usb_get_descriptor().  Plus unicode_to_ascii() is 
> a rubbish function and moving to usb_string() avoids using it.
> 
> Let's eliminate get_string() entirely.
> 
> Reported-by: Johan Hovold <jhovold@gmail.com>
> Signed-off-by: Dan Carpenter <error27@gmail.com>
> Cc: stable@kernel.org

Why stable?  Does this fix an existing bug somewhere?

thanks,

greg k-h

Re: [patch] usb: io_edgeport: eliminate get_string()

[Johan Hovold]

On Mon, Jan 25, 2010 at 05:57:13PM +0100, Johan Hovold wrote:
> On Mon, Jan 25, 2010 at 02:53:33PM +0300, Dan Carpenter wrote:
> > Johan Hovold points out that get_string() is basically just a re-implimentation
> > of usb_string().  It is also buggy.  It does DMA on the stack and it doesn't
> > handle negative returns from usb_get_descriptor().  Plus unicode_to_ascii() is 
> > a rubbish function and moving to usb_string() avoids using it.
> > 
> > Let's eliminate get_string() entirely.
> > 
> > Reported-by: Johan Hovold <jhovold@gmail.com>
> > Signed-off-by: Dan Carpenter <error27@gmail.com>
> > Cc: stable@kernel.org
> 
> Acked.

Should qualify that ack:

I don't think this patch should go to stable as it does not fix anything
severe. Worst case is that in the unlikely event that usb_control_msg
fails _and_ you have debugging enabled one may end up with 64 non-sense
characters in you log. There's no risk for any memory corruption
AFAICT.

The patch do however change the way communication is made at probe time
quite a bit (asking for language codes available, resends, first tries
to fetch strings using the maximum length then falls back to getting the
length first, etc.).

So except for the stable bit: 

Acked-by: Johan Hovold <jhovold@gmail.com>

Re: [patch] usb: io_edgeport: eliminate get_string()

[Oliver Neukum]

Am Montag, 25. Januar 2010 21:54:16 schrieb Johan Hovold:
> I don't think this patch should go to stable as it does not fix anything
> severe. Worst case is that in the unlikely event that usb_control_msg
> fails and you have debugging enabled one may end up with 64 non-sense
> characters in you log. There's no risk for any memory corruption
> AFAICT.

It does fix DMA on the stack. Are you sure this is always harmless
on non-x86?

	Regards
		Oliver

Re: [patch] usb: io_edgeport: eliminate get_string()

[Johan Hovold]

On Mon, Jan 25, 2010 at 10:47:31PM +0100, Oliver Neukum wrote:
> Am Montag, 25. Januar 2010 21:54:16 schrieb Johan Hovold:
> > I don't think this patch should go to stable as it does not fix anything
> > severe. Worst case is that in the unlikely event that usb_control_msg
> > fails and you have debugging enabled one may end up with 64 non-sense
> > characters in you log. There's no risk for any memory corruption
> > AFAICT.
> 
> It does fix DMA on the stack. Are you sure this is always harmless
> on non-x86?

Good point. But if that is considered critical enough then Dan's
original patch should go to stable instead of this one. And so should
the other 10+ DMA-to-stack patches in Greg's tree?

/Johan

Re: [patch] usb: io_edgeport: eliminate get_string()

[Greg KH]

On Mon, Jan 25, 2010 at 11:13:29PM +0100, Johan Hovold wrote:
> On Mon, Jan 25, 2010 at 10:47:31PM +0100, Oliver Neukum wrote:
> > Am Montag, 25. Januar 2010 21:54:16 schrieb Johan Hovold:
> > > I don't think this patch should go to stable as it does not fix anything
> > > severe. Worst case is that in the unlikely event that usb_control_msg
> > > fails and you have debugging enabled one may end up with 64 non-sense
> > > characters in you log. There's no risk for any memory corruption
> > > AFAICT.
> > 
> > It does fix DMA on the stack. Are you sure this is always harmless
> > on non-x86?
> 
> Good point. But if that is considered critical enough then Dan's
> original patch should go to stable instead of this one. And so should
> the other 10+ DMA-to-stack patches in Greg's tree?

Given the age of this bug (1999) and the fact that the devices are no
longer made, I don't think this is -stable worthy.

thanks,

greg k-h

Re: [patch] usb: io_edgeport: eliminate get_string()

[Oliver Neukum]

Am Dienstag, 26. Januar 2010 05:27:31 schrieb Greg KH:
> > Good point. But if that is considered critical enough then Dan's
> > original patch should go to stable instead of this one. And so should
> > the other 10+ DMA-to-stack patches in Greg's tree?
> 
> Given the age of this bug (1999) and the fact that the devices are no
> longer made, I don't think this is -stable worthy.

That's an odd form of reasoning. As the patch touches just this driver
it makes no difference if you don't have the hardware. If you have
the hardware on an architecture where it matters, this is no longer
true.

	Regards
		Oliver

Re: [patch] usb: io_edgeport: eliminate get_string()

[Dan Carpenter]

On Mon, Jan 25, 2010 at 12:41:54PM -0800, Greg KH wrote:
> On Mon, Jan 25, 2010 at 02:53:33PM +0300, Dan Carpenter wrote:
> > Johan Hovold points out that get_string() is basically just a re-implimentation
> > of usb_string().  It is also buggy.  It does DMA on the stack and it doesn't
> > handle negative returns from usb_get_descriptor().  Plus unicode_to_ascii() is 
> > a rubbish function and moving to usb_string() avoids using it.
> > 
> > Let's eliminate get_string() entirely.
> > 
> > Reported-by: Johan Hovold <jhovold@gmail.com>
> > Signed-off-by: Dan Carpenter <error27@gmail.com>
> > Cc: stable@kernel.org
> 
> Why stable?  Does this fix an existing bug somewhere?
> 

No sorry.  My script added that automatically and I meant forgot to 
remove it.  This should go through -next I think.

I will be more careful in the future.

regards,
dan carpenter

> thanks,
> 
> greg k-h

Re: [patch] usb: io_edgeport: eliminate get_string()

[Dan Carpenter]

On Tue, Jan 26, 2010 at 07:50:22AM +0100, Oliver Neukum wrote:
> Am Dienstag, 26. Januar 2010 05:27:31 schrieb Greg KH:
> > > Good point. But if that is considered critical enough then Dan's
> > > original patch should go to stable instead of this one. And so should
> > > the other 10+ DMA-to-stack patches in Greg's tree?
> > 
> > Given the age of this bug (1999) and the fact that the devices are no
> > longer made, I don't think this is -stable worthy.
> 
> That's an odd form of reasoning. As the patch touches just this driver
> it makes no difference if you don't have the hardware. If you have
> the hardware on an architecture where it matters, this is no longer
> true.
> 
> 	Regards
> 		Oliver

There are 89 files that do dma on the stack in the -rc5 kernel.  No one
ever complains about it, so it can't be that serious.

regards,
dan carpenter

Re: [patch] usb: io_edgeport: eliminate get_string()

[Johan Hovold]

On Tue, Jan 26, 2010 at 12:21:12PM +0300, Dan Carpenter wrote:
> On Tue, Jan 26, 2010 at 07:50:22AM +0100, Oliver Neukum wrote:
> > Am Dienstag, 26. Januar 2010 05:27:31 schrieb Greg KH:
> > > > Good point. But if that is considered critical enough then Dan's
> > > > original patch should go to stable instead of this one. And so should
> > > > the other 10+ DMA-to-stack patches in Greg's tree?
> > > 
> > > Given the age of this bug (1999) and the fact that the devices are no
> > > longer made, I don't think this is -stable worthy.
> > 
> > That's an odd form of reasoning. As the patch touches just this driver
> > it makes no difference if you don't have the hardware. If you have
> > the hardware on an architecture where it matters, this is no longer
> > true.
> > 
> There are 89 files that do dma on the stack in the -rc5 kernel.  No one
> ever complains about it, so it can't be that serious.

DMA on stack can actually cause memory corruption on cache-incoherent
architectures as Oliver implies. However unlikely it may be, perhaps
this should be given higher priority to fix?

Thanks,
Johan