| Message ID | 20200117095026.27655-6-johan@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | USB: serial: handle unbound ports | expand |
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
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
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
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
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; }
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(+)