Re: [PATCH 5/5] USB: serial: quatech2: handle unbound ports

[Johan Hovold <johan@kernel.org>]

On Fri, Jan 17, 2020 at 11:36:39AM +0100, Greg Kroah-Hartman wrote:
> On Fri, Jan 17, 2020 at 10:50:26AM +0100, Johan Hovold wrote:
> > Check for NULL port data in the event handlers to avoid dereferencing a
> > NULL pointer in the unlikely case where a port device isn't bound to a
> > driver (e.g. after an allocation failure on port probe).
> > 
> > Fixes: f7a33e608d9a ("USB: serial: add quatech2 usb to serial driver")
> > Cc: stable <stable@vger.kernel.org>     # 3.5
> > Signed-off-by: Johan Hovold <johan@kernel.org>
> > ---
> >  drivers/usb/serial/quatech2.c | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> > 
> > diff --git a/drivers/usb/serial/quatech2.c b/drivers/usb/serial/quatech2.c
> > index a62981ca7a73..c76a2c0c32ff 100644
> > --- a/drivers/usb/serial/quatech2.c
> > +++ b/drivers/usb/serial/quatech2.c
> > @@ -470,6 +470,13 @@ static int get_serial_info(struct tty_struct *tty,
> >  
> >  static void qt2_process_status(struct usb_serial_port *port, unsigned char *ch)
> >  {
> > +	struct qt2_port_private *port_priv;
> > +
> > +	/* May be called from qt2_process_read_urb() for an unbound port. */
> > +	port_priv = usb_get_serial_port_data(port);
> > +	if (!port_priv)
> > +		return;
> > +
> 
> Where is the null dereference here?  Will port be NULL somehow?

The NULL-dereference happens in qt2_update_lsr() and qt2_update_msr()
called below.

> >  	switch (*ch) {
> >  	case QT2_LINE_STATUS:
> >  		qt2_update_lsr(port, ch + 1);
> > @@ -484,14 +491,27 @@ static void qt2_process_status(struct usb_serial_port *port, unsigned char *ch)
> >  static void qt2_process_xmit_empty(struct usb_serial_port *port,
> >  				   unsigned char *ch)
> >  {
> > +	struct qt2_port_private *port_priv;
> >  	int bytes_written;
> >  
> > +	/* May be called from qt2_process_read_urb() for an unbound port. */
> > +	port_priv = usb_get_serial_port_data(port);
> > +	if (!port_priv)
> > +		return;
> > +
> >  	bytes_written = (int)(*ch) + (int)(*(ch + 1) << 4);
> 
> What's the harm in doing a pointless calculation here?  Nothing seems to
> happen in this function at all.

Right, none at all.

Both of these handler appear to be here for documentation purposes. In
case any one ever adds code here, they need to be aware that the port
data may be NULL.

I should have mentioned this in the commit message and perhaps split
the last two checks in a separate patch as they do not need to be
backported. 

The alternative would be a more intrusive change handling an unbound
port entirely in qt2_process_read_urb().

> >  }
> >  
> >  /* not needed, kept to document functionality */
> >  static void qt2_process_flush(struct usb_serial_port *port, unsigned char *ch)
> >  {
> > +	struct qt2_port_private *port_priv;
> > +
> > +	/* May be called from qt2_process_read_urb() for an unbound port. */
> > +	port_priv = usb_get_serial_port_data(port);
> > +	if (!port_priv)
> > +		return;
> > +
> >  	return;
> >  }
> 
> This whole function can just be removed, right?

Yep, just "kept to document functionality" the header says.

I'll respin this last one in some way, thanks.

Johan