USB: serial: fix invalid user-pointer checks

PATCH In review Johan Hovold 2 participants 5 messages Nov 8, 2016 – Nov 9, 2016

Subsystems (2)

USB USB serial

lore

Share & tools

lore

Johan Hovold

author maintainer

2016-11-08 12:27 UTC #0 lore raw

PATCH 1/1 USB: serial: fix invalid user-pointer checks

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 <[email protected]>

---
 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(-)

drivers/usb/serial/ftdi_sio.c +0 -5

index 6e9fc8bcc285..23d14b98ae2a 100644
@@ -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;

drivers/usb/serial/io_edgeport.c +0 -3

index 11c05ce2f35f..dcc0c58aaad5 100644
@@ -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;

drivers/usb/serial/io_ti.c +0 -3

index fce82fd79f77..c339163698eb 100644
@@ -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;

drivers/usb/serial/mos7720.c +0 -3

index de9992b492b0..d52caa03679c 100644
@@ -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;

drivers/usb/serial/mos7840.c +0 -3

index 57426d703a09..9a220b8e810f 100644
@@ -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;

drivers/usb/serial/opticon.c +0 -3

index 4b7bfb394a32..5ded6f524d59 100644
@@ -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 */

drivers/usb/serial/quatech2.c +0 -3

index 85acb50a7ee2..659cb8606bd9 100644
@@ -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;

drivers/usb/serial/ssu100.c +0 -3

index 70a098de429f..2a156144c76c 100644
@@ -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;

drivers/usb/serial/ti_usb_3410_5052.c +0 -3

index a8b9bdba314f..8db9d071d940 100644
@@ -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;

drivers/usb/serial/usb_wwan.c +0 -3

index 3dfdfc81254b..59bfcb3da116 100644
@@ -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;

Signature · 1 line

2.7.3

Oliver Neukum

2016-11-08 14:13 UTC #1 lore raw

Quoted 5 lines · 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.

Shouldn't we bail out early in these cases?

Regards
Oliver

Johan Hovold

author maintainer

2016-11-08 15:41 UTC #2 lore raw

Quoted 8 lines · Oliver Neukum

Quoted 5 lines · 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.
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

Oliver Neukum

2016-11-09 11:04 UTC #3 lore raw

Quoted 8 lines · Johan Hovold

Quoted 7 lines · Oliver Neukum
Quoted 5 lines · 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.

Quoted 5 lines

Quoted 1 line
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

Johan Hovold

author maintainer

2016-11-09 11:07 UTC #4 lore raw

Quoted 9 lines · Oliver Neukum

Quoted 8 lines · Johan Hovold
Quoted 7 lines · Oliver Neukum
Quoted 5 lines · 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.

Quoted 7 lines

Quoted 5 lines
Quoted 1 line
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