[PATCH 0/5] USB: serial: handle unbound ports

[PATCH 0/5] USB: serial: handle unbound ports

[Johan Hovold]

Drivers must make sure that a port is bound to a driver before accessing
port data in non-port callbacks.

Note that this is needed even with the port-driver bind attributes
removed as an individual port may have failed to probe.

Johan


Johan Hovold (5):
  USB: ch341: handle unbound port at reset_resume
  USB: serial: io_edgeport: handle unbound ports on URB completion
  USB: serial: io_edgeport: add missing active-port sanity check
  USB: serial: keyspan: handle unbound ports
  USB: serial: quatech2: handle unbound ports

 drivers/usb/serial/ch341.c       |  6 +++++-
 drivers/usb/serial/io_edgeport.c | 16 +++++++++-------
 drivers/usb/serial/keyspan.c     |  4 ++++
 drivers/usb/serial/quatech2.c    | 20 ++++++++++++++++++++
 4 files changed, 38 insertions(+), 8 deletions(-)

-- 
2.24.1

[PATCH 1/5] USB: ch341: handle unbound port at reset_resume

[Johan Hovold]

Check for NULL port data in reset_resume() to avoid dereferencing a NULL
pointer in case the port device isn't bound to a driver (e.g. after a
failed control request at port probe).

Fixes: 1ded7ea47b88 ("USB: ch341 serial: fix port number changed after resume")
Cc: stable <stable@vger.kernel.org>     # 2.6.30
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/ch341.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
index df582fe855f0..d3f420f3a083 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -642,9 +642,13 @@ static int ch341_tiocmget(struct tty_struct *tty)
 static int ch341_reset_resume(struct usb_serial *serial)
 {
 	struct usb_serial_port *port = serial->port[0];
-	struct ch341_private *priv = usb_get_serial_port_data(port);
+	struct ch341_private *priv;
 	int ret;
 
+	priv = usb_get_serial_port_data(port);
+	if (!priv)
+		return 0;
+
 	/* reconfigure ch341 serial port after bus-reset */
 	ch341_configure(serial->dev, priv);
 
-- 
2.24.1

[PATCH 2/5] USB: serial: io_edgeport: handle unbound ports on URB completion

[Johan Hovold]

Check for NULL port data in the shared interrupt and bulk completion
callbacks to avoid dereferencing a NULL pointer in case a device sends
data for a port device which isn't bound to a driver (e.g. due to a
malicious device having unexpected endpoints or after an allocation
failure on port probe).

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable <stable@vger.kernel.org>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/io_edgeport.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/serial/io_edgeport.c b/drivers/usb/serial/io_edgeport.c
index 9690a5f4b9d6..0582d78bdb1d 100644
--- a/drivers/usb/serial/io_edgeport.c
+++ b/drivers/usb/serial/io_edgeport.c
@@ -716,7 +716,7 @@ static void edge_interrupt_callback(struct urb *urb)
 			if (txCredits) {
 				port = edge_serial->serial->port[portNumber];
 				edge_port = usb_get_serial_port_data(port);
-				if (edge_port->open) {
+				if (edge_port && edge_port->open) {
 					spin_lock_irqsave(&edge_port->ep_lock,
 							  flags);
 					edge_port->txCredits += txCredits;
@@ -1825,7 +1825,7 @@ static void process_rcvd_data(struct edgeport_serial *edge_serial,
 				port = edge_serial->serial->port[
 							edge_serial->rxPort];
 				edge_port = usb_get_serial_port_data(port);
-				if (edge_port->open) {
+				if (edge_port && edge_port->open) {
 					dev_dbg(dev, "%s - Sending %d bytes to TTY for port %d\n",
 						__func__, rxLen,
 						edge_serial->rxPort);
-- 
2.24.1

[PATCH 3/5] USB: serial: io_edgeport: add missing active-port sanity check

[Johan Hovold]

The driver receives the active port number from the device, but never
made sure that the port number was valid. This could lead to a
NULL-pointer dereference or memory corruption in case a device sends
data for an invalid port.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable <stable@vger.kernel.org>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/io_edgeport.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/serial/io_edgeport.c b/drivers/usb/serial/io_edgeport.c
index 0582d78bdb1d..5737add6a2a4 100644
--- a/drivers/usb/serial/io_edgeport.c
+++ b/drivers/usb/serial/io_edgeport.c
@@ -1725,7 +1725,8 @@ static void edge_break(struct tty_struct *tty, int break_state)
 static void process_rcvd_data(struct edgeport_serial *edge_serial,
 				unsigned char *buffer, __u16 bufferLength)
 {
-	struct device *dev = &edge_serial->serial->dev->dev;
+	struct usb_serial *serial = edge_serial->serial;
+	struct device *dev = &serial->dev->dev;
 	struct usb_serial_port *port;
 	struct edgeport_port *edge_port;
 	__u16 lastBufferLength;
@@ -1821,9 +1822,8 @@ static void process_rcvd_data(struct edgeport_serial *edge_serial,
 
 			/* spit this data back into the tty driver if this
 			   port is open */
-			if (rxLen) {
-				port = edge_serial->serial->port[
-							edge_serial->rxPort];
+			if (rxLen && edge_serial->rxPort < serial->num_ports) {
+				port = serial->port[edge_serial->rxPort];
 				edge_port = usb_get_serial_port_data(port);
 				if (edge_port && edge_port->open) {
 					dev_dbg(dev, "%s - Sending %d bytes to TTY for port %d\n",
@@ -1833,8 +1833,8 @@ static void process_rcvd_data(struct edgeport_serial *edge_serial,
 							rxLen);
 					edge_port->port->icount.rx += rxLen;
 				}
-				buffer += rxLen;
 			}
+			buffer += rxLen;
 			break;
 
 		case EXPECT_HDR3:	/* Expect 3rd byte of status header */
@@ -1869,6 +1869,8 @@ static void process_rcvd_status(struct edgeport_serial *edge_serial,
 	__u8 code = edge_serial->rxStatusCode;
 
 	/* switch the port pointer to the one being currently talked about */
+	if (edge_serial->rxPort >= edge_serial->serial->num_ports)
+		return;
 	port = edge_serial->serial->port[edge_serial->rxPort];
 	edge_port = usb_get_serial_port_data(port);
 	if (edge_port == NULL) {
-- 
2.24.1

[PATCH 4/5] USB: serial: keyspan: handle unbound ports

[Johan Hovold]

Check for NULL port data in the control URB completion 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: 0ca1268e109a ("USB Serial Keyspan: add support for USA-49WG & USA-28XG")
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable <stable@vger.kernel.org>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/keyspan.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/usb/serial/keyspan.c b/drivers/usb/serial/keyspan.c
index e66a59ef43a1..aa3dbce22cfb 100644
--- a/drivers/usb/serial/keyspan.c
+++ b/drivers/usb/serial/keyspan.c
@@ -1058,6 +1058,8 @@ static void	usa49_glocont_callback(struct urb *urb)
 	for (i = 0; i < serial->num_ports; ++i) {
 		port = serial->port[i];
 		p_priv = usb_get_serial_port_data(port);
+		if (!p_priv)
+			continue;
 
 		if (p_priv->resend_cont) {
 			dev_dbg(&port->dev, "%s - sending setup\n", __func__);
@@ -1459,6 +1461,8 @@ static void usa67_glocont_callback(struct urb *urb)
 	for (i = 0; i < serial->num_ports; ++i) {
 		port = serial->port[i];
 		p_priv = usb_get_serial_port_data(port);
+		if (!p_priv)
+			continue;
 
 		if (p_priv->resend_cont) {
 			dev_dbg(&port->dev, "%s - sending setup\n", __func__);
-- 
2.24.1

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

[Johan Hovold]

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;
+
 	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);
 }
 
 /* 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;
 }
 
-- 
2.24.1

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

[Greg KH]

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?

>  	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.

>  }
>  
>  /* 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?

thanks,

greg k-h

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

[Johan Hovold]

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

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

[Greg KH]

On Fri, Jan 17, 2020 at 10:50:21AM +0100, Johan Hovold wrote:
> Drivers must make sure that a port is bound to a driver before accessing
> port data in non-port callbacks.
> 
> Note that this is needed even with the port-driver bind attributes
> removed as an individual port may have failed to probe.
> 
> Johan
> 
> 
> Johan Hovold (5):
>   USB: ch341: handle unbound port at reset_resume
>   USB: serial: io_edgeport: handle unbound ports on URB completion
>   USB: serial: io_edgeport: add missing active-port sanity check
>   USB: serial: keyspan: handle unbound ports
>   USB: serial: quatech2: handle unbound ports

For all 5:

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

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

[Greg KH]

On Fri, Jan 17, 2020 at 11:53:17AM +0100, Johan Hovold wrote:
> 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.

Ah, ok.

> > >  	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.

Nah, that's fine, this is ok as-is, thanks.

greg k-h

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

[Johan Hovold]

On Fri, Jan 17, 2020 at 02:13:56PM +0100, Greg Kroah-Hartman wrote:
> On Fri, Jan 17, 2020 at 11:53:17AM +0100, Johan Hovold wrote:
> > 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>

> > I'll respin this last one in some way, thanks.
> 
> Nah, that's fine, this is ok as-is, thanks.

I wasn't too happy with this myself, so I reverted to my first version
of simply adding the checks the lsr/msr helper where the actual
dereference takes place.

The downside is that it's a bit disconnected from where the actual port
lookup takes place (qt2_process_read_urb()). But I thinks it's still
preferred over adding sanity checks to those event-handler stubs, which
admittedly looks quite weird.

I've applied the first four and will send a v2 of this one.

Johan