On Tue, Jan 03, 2017 at 05:48:05PM +0100, Johan Hovold wrote:
> On Tue, Jan 03, 2017 at 05:27:07PM +0100, Greg Kroah-Hartman wrote:
> > On Tue, Jan 03, 2017 at 04:39:40PM +0100, Johan Hovold wrote:
> > > Fix NULL-pointer dereference when clearing halt at open should the device
> > > lack a bulk-out endpoint.
> > >
> > > Unable to handle kernel NULL pointer dereference at virtual address 00000030
> > > ...
> > > PC is at cyberjack_open+0x40/0x9c [cyberjack]
> > >
> > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > > Cc: stable <stable@xxxxxxxxxxxxxxx>
> > > Signed-off-by: Johan Hovold <johan@xxxxxxxxxx>
> > > ---
> > > drivers/usb/serial/cyberjack.c | 10 ++++++++++
> > > 1 file changed, 10 insertions(+)
> > >
> > > diff --git a/drivers/usb/serial/cyberjack.c b/drivers/usb/serial/cyberjack.c
> > > index 5f17a3b9916d..80260b08398b 100644
> > > --- a/drivers/usb/serial/cyberjack.c
> > > +++ b/drivers/usb/serial/cyberjack.c
> > > @@ -50,6 +50,7 @@
> > > #define CYBERJACK_PRODUCT_ID 0x0100
> > >
> > > /* Function prototypes */
> > > +static int cyberjack_attach(struct usb_serial *serial);
> > > static int cyberjack_port_probe(struct usb_serial_port *port);
> > > static int cyberjack_port_remove(struct usb_serial_port *port);
> > > static int cyberjack_open(struct tty_struct *tty,
> > > @@ -77,6 +78,7 @@ static struct usb_serial_driver cyberjack_device = {
> > > .description = "Reiner SCT Cyberjack USB card reader",
> > > .id_table = id_table,
> > > .num_ports = 1,
> > > + .attach = cyberjack_attach,
> > > .port_probe = cyberjack_port_probe,
> > > .port_remove = cyberjack_port_remove,
> > > .open = cyberjack_open,
> > > @@ -100,6 +102,14 @@ struct cyberjack_private {
> > > short wrsent; /* Data already sent */
> > > };
> > >
> > > +static int cyberjack_attach(struct usb_serial *serial)
> > > +{
> > > + if (serial->num_bulk_out < serial->num_ports)
> > > + return -ENODEV;
> > > +
> > > + return 0;
> > > +}
> >
> > You end up doing much the same thing for most of these drivers, is there
> > any way to do it in the usb-serial core instead?
> >
> > I've been playing with an idea to have a USB driver specify the number
> > and types of endpoints it requires and have the core just not even call
> > the probe function if that doesn't match up. That should solve lots of
> > these issues, can't you do much the same type of thing here instead of
> > requiring a callback to do this?
>
> I've been playing with that same idea, but I wanted minimal fixes that
> could be backported to the stable trees for this first. I also kept the
> checks as loose as possible to avoid any regressions.
>
> Note that there seems to have been a general mechanism for this that was
> removed in 2008 (see 07c3b1a10016 ("USB: remove broken usb-serial
> num_endpoints check")), possibly because the checks were too tight.
I have no idea why I wrote that patch, but I guess things were failing
to bind for devices that were valid.
> But since there appear to be exploits out there for this class of
> issues, we should probably consider reintroducing it in some form (e.g.
> in USB core or USB serial core).
As the usb-serial core has to take all types of devices, I think it will
have to go into both :(
Your patches are good, and you are right, they will work well for stable
kernels, so we should take them now. I'll work on the usb core option
in the next few weeks after I catch up from my vacation patch backlog,
so that's a nicer long-term option for where we can make the drivers do
less work and remove code when that happens.
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
