Date:	Wed, 04 Jun 2014 10:29:37 -0400
From:	Mike Remski <mremski@...ualink.net>
To:	Johan Hovold <jhovold@...il.com>
CC:	linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org
Subject: Re: ftdi_sio BUG: NULL pointer dereference


On 06/04/2014 10:19 AM, Johan Hovold wrote:
> On Tue, Jun 03, 2014 at 06:17:33AM -0400, Mike Remski wrote:
>> On 06/02/2014 01:46 PM, Johan Hovold wrote:
>>> On Mon, Jun 02, 2014 at 01:11:37PM -0400, Mike Remski wrote:
>>>> On 06/02/2014 12:49 PM, Johan Hovold wrote:
>>>>> On Mon, Jun 02, 2014 at 12:24:44PM -0400, Mike Remski wrote:
>>>>>> On 06/02/2014 12:20 PM, Johan Hovold wrote:
>>>>>>> On Mon, Jun 02, 2014 at 12:02:40PM -0400, Mike Remski wrote:
>>>>>>>> On 06/02/2014 11:40 AM, Johan Hovold wrote:
>>>>>>>>> [ Please avoid top-posting. ]
>>>>>>>>>
>>>>>>>>> On Mon, Jun 02, 2014 at 11:16:11AM -0400, Mike Remski wrote:
>>>>>>> The third interface lacks endpoints and crashes the ftdi_sio driver.
>>>>>>> This shouldn't happen (even if you're forcing the wrong driver to bind),
>>>>>>> so I'll fix it up if still broken in v3.15-rc.
>>>>>>>
>>>>>> Johan,
>>>>>> Thanks again.  Yes, the device does indeed have an FTDI embedded in it;
>>>>>> they've programmed in their own ids.  They supply a Windows driver for
>>>>>> it, but that doesn't do me any good.  :)
>>>>> Not just their own ID's it seems.
>>>>>
>>>>> Have you tried just using the cdc-acm driver? The ports should up as
>>>>> /dev/ttyACMx instead of ttyUSBx.
>>>>>
>>>> Not yet, next on the list.
>>> You really should try this before anything else. :)
> How did it go with cdc-acm?
>
> You probably need the following commit (depending how well-supported your
> old kernel is):
>
> 	5b239f0aebd4d ("USB: cdc-acm: Add pseudo modem without AT
> 	command capabilities")
>
> in order for the device to bind.
>
>>>> I'm suspecting that bNumEndpoints == 0 is causing endpoint[1].desc to
>>>> stay at NULL (line 1567 in 3.1.4.5 source), so by the time it gets used
>>>> later on, I'm hitting the NULL dereference.
>>> Yeah, the code is obviously broken (also in v3.15-rc). It should
>>> probably work to just return from ftdi_set_max_packet_size if
>>> num_endpoints is 0 if you want to try that (or you can use your ?:
>>> construct), but I should be able to fix this up properly on Wednesday.
>>>
>>> Thanks,
>>> Johan
>> Johan,
>> I had a chance to play around with code over in ftdi_sio.c;   adding this:
>>
>>           if (!num_endpoints) {
>>                   return;
>>           }
>>    after the "Number of endpoints" message gets rid of the crash,
>> everything looks to be working correctly.
> Good. Care to try the patch below so I can add a formal Tested-by tag as
> well?
>
> Thanks,
> Johan
>
>
> >From 4ddea3a573b8c15beefb67bc35c440850063d79d Mon Sep 17 00:00:00 2001
> From: Johan Hovold <johan@...nel.org>
> Date: Wed, 4 Jun 2014 14:09:43 +0200
> Subject: [PATCH 1/5] USB: ftdi_sio: fix null deref at port probe
>
> Fix NULL-pointer dereference when probing an interface with no
> endpoints.
>
> These devices have two bulk endpoints per interface, but this avoids
> crashing the kernel if a user forces a non-FTDI device to be probed.
>
> Note that the iterator variable was made unsigned in order to avoid
> a maybe-uninitialized compiler warning for ep_desc after the loop.
>
> Fixes: 895f28badce9 ("USB: ftdi_sio: fix hi-speed device packet size
> calculation")
>
> Reported-by: Mike Remski <mremski@...ualink.net>
> Cc: <stable@...r.kernel.org>	# 2.3.61
> Signed-off-by: Johan Hovold <johan@...nel.org>
> ---
>   drivers/usb/serial/ftdi_sio.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
> index 7c6e1dedeb06..3019141397eb 100644
> --- a/drivers/usb/serial/ftdi_sio.c
> +++ b/drivers/usb/serial/ftdi_sio.c
> @@ -1564,14 +1564,17 @@ static void ftdi_set_max_packet_size(struct usb_serial_port *port)
>   	struct usb_device *udev = serial->dev;
>   
>   	struct usb_interface *interface = serial->interface;
> -	struct usb_endpoint_descriptor *ep_desc = &interface->cur_altsetting->endpoint[1].desc;
> +	struct usb_endpoint_descriptor *ep_desc;
>   
>   	unsigned num_endpoints;
> -	int i;
> +	unsigned i;
>   
>   	num_endpoints = interface->cur_altsetting->desc.bNumEndpoints;
>   	dev_info(&udev->dev, "Number of endpoints %d\n", num_endpoints);
>   
> +	if (!num_endpoints)
> +		return;
> +
>   	/* NOTE: some customers have programmed FT232R/FT245R devices
>   	 * with an endpoint size of 0 - not good.  In this case, we
>   	 * want to override the endpoint descriptor setting and use a

Thanks Johan.  I tried to get the cdc_acm working;  did not have much 
luck/time (typical overcommit on workload) I will retry with the commit 
mentioned.
I will try the patch today and get back to you.  Nice on the ep_desc: 
looking at the code priv->max_packet_size is attached to the port, your 
change would use the last thing off of cur_altsetting->endpoint[], but 
I'm wondering if we should actually be setting priv->max_packet_size to 
whatever the max is of all endpoint[].desc->wMaxPacketSize?

Thoughts?

m

-- 
Office: (978)401-4032 (x123 internally)
Cell: (603) 759-6953

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists