linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] USB: serial: fix invalid user-pointer checks
@ 2016-11-08 12:26 Johan Hovold
  2016-11-08 14:13 ` Oliver Neukum
  0 siblings, 1 reply; 5+ messages in thread
From: Johan Hovold @ 2016-11-08 12:26 UTC (permalink / raw)
  To: linux-usb; +Cc: linux-kernel, Johan Hovold

Drop invalid user-pointer checks from ioctl handlers.

A NULL-pointer can be valid in user space and copy_to_user() takes care
of sanity checking.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/ftdi_sio.c         | 5 -----
 drivers/usb/serial/io_edgeport.c      | 3 ---
 drivers/usb/serial/io_ti.c            | 3 ---
 drivers/usb/serial/mos7720.c          | 3 ---
 drivers/usb/serial/mos7840.c          | 3 ---
 drivers/usb/serial/opticon.c          | 3 ---
 drivers/usb/serial/quatech2.c         | 3 ---
 drivers/usb/serial/ssu100.c           | 3 ---
 drivers/usb/serial/ti_usb_3410_5052.c | 3 ---
 drivers/usb/serial/usb_wwan.c         | 3 ---
 10 files changed, 32 deletions(-)

diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
index 6e9fc8bcc285..23d14b98ae2a 100644
--- a/drivers/usb/serial/ftdi_sio.c
+++ b/drivers/usb/serial/ftdi_sio.c
@@ -1455,8 +1455,6 @@ static int get_serial_info(struct usb_serial_port *port,
 	struct ftdi_private *priv = usb_get_serial_port_data(port);
 	struct serial_struct tmp;
 
-	if (!retinfo)
-		return -EFAULT;
 	memset(&tmp, 0, sizeof(tmp));
 	tmp.flags = priv->flags;
 	tmp.baud_base = priv->baud_base;
@@ -1538,9 +1536,6 @@ static int get_lsr_info(struct usb_serial_port *port,
 	struct ftdi_private *priv = usb_get_serial_port_data(port);
 	unsigned int result = 0;
 
-	if (!retinfo)
-		return -EFAULT;
-
 	if (priv->transmit_empty)
 		result = TIOCSER_TEMT;
 
diff --git a/drivers/usb/serial/io_edgeport.c b/drivers/usb/serial/io_edgeport.c
index 11c05ce2f35f..dcc0c58aaad5 100644
--- a/drivers/usb/serial/io_edgeport.c
+++ b/drivers/usb/serial/io_edgeport.c
@@ -1554,9 +1554,6 @@ static int get_serial_info(struct edgeport_port *edge_port,
 {
 	struct serial_struct tmp;
 
-	if (!retinfo)
-		return -EFAULT;
-
 	memset(&tmp, 0, sizeof(tmp));
 
 	tmp.type		= PORT_16550A;
diff --git a/drivers/usb/serial/io_ti.c b/drivers/usb/serial/io_ti.c
index fce82fd79f77..c339163698eb 100644
--- a/drivers/usb/serial/io_ti.c
+++ b/drivers/usb/serial/io_ti.c
@@ -2459,9 +2459,6 @@ static int get_serial_info(struct edgeport_port *edge_port,
 	struct serial_struct tmp;
 	unsigned cwait;
 
-	if (!retinfo)
-		return -EFAULT;
-
 	cwait = edge_port->port->port.closing_wait;
 	if (cwait != ASYNC_CLOSING_WAIT_NONE)
 		cwait = jiffies_to_msecs(cwait) / 10;
diff --git a/drivers/usb/serial/mos7720.c b/drivers/usb/serial/mos7720.c
index de9992b492b0..d52caa03679c 100644
--- a/drivers/usb/serial/mos7720.c
+++ b/drivers/usb/serial/mos7720.c
@@ -1861,9 +1861,6 @@ static int get_serial_info(struct moschip_port *mos7720_port,
 {
 	struct serial_struct tmp;
 
-	if (!retinfo)
-		return -EFAULT;
-
 	memset(&tmp, 0, sizeof(tmp));
 
 	tmp.type		= PORT_16550A;
diff --git a/drivers/usb/serial/mos7840.c b/drivers/usb/serial/mos7840.c
index 57426d703a09..9a220b8e810f 100644
--- a/drivers/usb/serial/mos7840.c
+++ b/drivers/usb/serial/mos7840.c
@@ -1956,9 +1956,6 @@ static int mos7840_get_serial_info(struct moschip_port *mos7840_port,
 	if (mos7840_port == NULL)
 		return -1;
 
-	if (!retinfo)
-		return -EFAULT;
-
 	memset(&tmp, 0, sizeof(tmp));
 
 	tmp.type = PORT_16550A;
diff --git a/drivers/usb/serial/opticon.c b/drivers/usb/serial/opticon.c
index 4b7bfb394a32..5ded6f524d59 100644
--- a/drivers/usb/serial/opticon.c
+++ b/drivers/usb/serial/opticon.c
@@ -336,9 +336,6 @@ static int get_serial_info(struct usb_serial_port *port,
 {
 	struct serial_struct tmp;
 
-	if (!serial)
-		return -EFAULT;
-
 	memset(&tmp, 0x00, sizeof(tmp));
 
 	/* fake emulate a 16550 uart to make userspace code happy */
diff --git a/drivers/usb/serial/quatech2.c b/drivers/usb/serial/quatech2.c
index 85acb50a7ee2..659cb8606bd9 100644
--- a/drivers/usb/serial/quatech2.c
+++ b/drivers/usb/serial/quatech2.c
@@ -463,9 +463,6 @@ static int get_serial_info(struct usb_serial_port *port,
 {
 	struct serial_struct tmp;
 
-	if (!retinfo)
-		return -EFAULT;
-
 	memset(&tmp, 0, sizeof(tmp));
 	tmp.line		= port->minor;
 	tmp.port		= 0;
diff --git a/drivers/usb/serial/ssu100.c b/drivers/usb/serial/ssu100.c
index 70a098de429f..2a156144c76c 100644
--- a/drivers/usb/serial/ssu100.c
+++ b/drivers/usb/serial/ssu100.c
@@ -318,9 +318,6 @@ static int get_serial_info(struct usb_serial_port *port,
 {
 	struct serial_struct tmp;
 
-	if (!retinfo)
-		return -EFAULT;
-
 	memset(&tmp, 0, sizeof(tmp));
 	tmp.line		= port->minor;
 	tmp.port		= 0;
diff --git a/drivers/usb/serial/ti_usb_3410_5052.c b/drivers/usb/serial/ti_usb_3410_5052.c
index a8b9bdba314f..8db9d071d940 100644
--- a/drivers/usb/serial/ti_usb_3410_5052.c
+++ b/drivers/usb/serial/ti_usb_3410_5052.c
@@ -1426,9 +1426,6 @@ static int ti_get_serial_info(struct ti_port *tport,
 	struct serial_struct ret_serial;
 	unsigned cwait;
 
-	if (!ret_arg)
-		return -EFAULT;
-
 	cwait = port->port.closing_wait;
 	if (cwait != ASYNC_CLOSING_WAIT_NONE)
 		cwait = jiffies_to_msecs(cwait) / 10;
diff --git a/drivers/usb/serial/usb_wwan.c b/drivers/usb/serial/usb_wwan.c
index 3dfdfc81254b..59bfcb3da116 100644
--- a/drivers/usb/serial/usb_wwan.c
+++ b/drivers/usb/serial/usb_wwan.c
@@ -140,9 +140,6 @@ static int get_serial_info(struct usb_serial_port *port,
 {
 	struct serial_struct tmp;
 
-	if (!retinfo)
-		return -EFAULT;
-
 	memset(&tmp, 0, sizeof(tmp));
 	tmp.line            = port->minor;
 	tmp.port            = port->port_number;
-- 
2.7.3

^ permalink raw reply related	[flat|nested] 5+ messages in thread
* Re: [PATCH] USB: serial: fix invalid user-pointer checks
  2016-11-08 12:26 [PATCH] USB: serial: fix invalid user-pointer checks Johan Hovold
@ 2016-11-08 14:13 ` Oliver Neukum
  2016-11-08 15:41   ` Johan Hovold
  0 siblings, 1 reply; 5+ messages in thread
From: Oliver Neukum @ 2016-11-08 14:13 UTC (permalink / raw)
  To: Johan Hovold; +Cc: linux-usb, linux-kernel

On Tue, 2016-11-08 at 13:26 +0100, Johan Hovold wrote:
> Drop invalid user-pointer checks from ioctl handlers.
> 
> A NULL-pointer can be valid in user space and copy_to_user() takes
> care
> of sanity checking.

Shouldn't we bail out early in these cases?

	Regards
		Oliver

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] USB: serial: fix invalid user-pointer checks
  2016-11-08 14:13 ` Oliver Neukum
@ 2016-11-08 15:41   ` Johan Hovold
  2016-11-09 10:58     ` Oliver Neukum
  0 siblings, 1 reply; 5+ messages in thread
From: Johan Hovold @ 2016-11-08 15:41 UTC (permalink / raw)
  To: Oliver Neukum; +Cc: Johan Hovold, linux-usb, linux-kernel

On Tue, Nov 08, 2016 at 03:13:13PM +0100, Oliver Neukum wrote:
> On Tue, 2016-11-08 at 13:26 +0100, Johan Hovold wrote:
> > Drop invalid user-pointer checks from ioctl handlers.
> > 
> > A NULL-pointer can be valid in user space and copy_to_user() takes
> > care
> > of sanity checking.
> 
> Shouldn't we bail out early in these cases?

I don't think it's worth it, and this is also the general pattern for
such ioctls. The added overhead for an error case like this is really
negligible.

Thanks,
Johan

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] USB: serial: fix invalid user-pointer checks
  2016-11-08 15:41   ` Johan Hovold
@ 2016-11-09 10:58     ` Oliver Neukum
  2016-11-09 11:07       ` Johan Hovold
  0 siblings, 1 reply; 5+ messages in thread
From: Oliver Neukum @ 2016-11-09 10:58 UTC (permalink / raw)
  To: Johan Hovold; +Cc: linux-kernel, linux-usb

On Tue, 2016-11-08 at 16:41 +0100, Johan Hovold wrote:
> On Tue, Nov 08, 2016 at 03:13:13PM +0100, Oliver Neukum wrote:
> > On Tue, 2016-11-08 at 13:26 +0100, Johan Hovold wrote:
> > > Drop invalid user-pointer checks from ioctl handlers.
> > > 
> > > A NULL-pointer can be valid in user space and copy_to_user() takes
> > > care
> > > of sanity checking.
> > 
> > Shouldn't we bail out early in these cases?
> 
> I don't think it's worth it, and this is also the general pattern for
> such ioctls. The added overhead for an error case like this is really
> negligible.

OK, as you say.

	Regards
		Oliver

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] USB: serial: fix invalid user-pointer checks
  2016-11-09 10:58     ` Oliver Neukum
@ 2016-11-09 11:07       ` Johan Hovold
  0 siblings, 0 replies; 5+ messages in thread
From: Johan Hovold @ 2016-11-09 11:07 UTC (permalink / raw)
  To: Oliver Neukum; +Cc: Johan Hovold, linux-kernel, linux-usb

On Wed, Nov 09, 2016 at 11:58:31AM +0100, Oliver Neukum wrote:
> On Tue, 2016-11-08 at 16:41 +0100, Johan Hovold wrote:
> > On Tue, Nov 08, 2016 at 03:13:13PM +0100, Oliver Neukum wrote:
> > > On Tue, 2016-11-08 at 13:26 +0100, Johan Hovold wrote:
> > > > Drop invalid user-pointer checks from ioctl handlers.
> > > > 
> > > > A NULL-pointer can be valid in user space and copy_to_user() takes
> > > > care
> > > > of sanity checking.
> > > 
> > > Shouldn't we bail out early in these cases?
> > 
> > I don't think it's worth it, and this is also the general pattern for
> > such ioctls. The added overhead for an error case like this is really
> > negligible.
> 
> OK, as you say.

Also remember that access_ok() is not a sufficient sanity check, and
would specifically fail to catch the NULL-pointer case.

Thanks,
Johan

^ permalink raw reply	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-11-09 11:07 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-08 12:26 [PATCH] USB: serial: fix invalid user-pointer checks Johan Hovold
2016-11-08 14:13 ` Oliver Neukum
2016-11-08 15:41   ` Johan Hovold
2016-11-09 10:58     ` Oliver Neukum
2016-11-09 11:07       ` Johan Hovold

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).