Re: ftdi_sio BUG: NULL pointer dereference

[Johan Hovold <jhovold@gmail.com>]

On Wed, Jun 04, 2014 at 10:29:37AM -0400, Mike Remski wrote:
> On 06/04/2014 10:19 AM, Johan Hovold wrote:

> > >From 4ddea3a573b8c15beefb67bc35c440850063d79d Mon Sep 17 00:00:00 2001
> > From: Johan Hovold <johan@kernel.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@mutualink.net>
> > Cc: <stable@vger.kernel.org>	# 2.3.61
> > Signed-off-by: Johan Hovold <johan@kernel.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?

This is the exact same behaviour as the old code (minus the NULL-deref).

These device have two bulk endpoints per interface and they are supposed
to be using the same max packet size (64 or 512 depending on device and
host).

This value is also used during depacketisation of incoming data (and
packetisation of outgoing data for legacy devices). I'm pretty convinced
you're using the wrong driver, something which would lead to corruption
of incoming data when the (non-existing) status bytes are stripped from
the stream.

You really should try cdc-acm.

Johan