serdev: receive_buf and locking fixes

PATCH In review Johan Hovold 2 participants 15 messages Nov 3, 2017 – Dec 18, 2017

Subsystems (3)

Stable branch TTY layer and serial drivers Serial device bus

lore

Share & tools

lore

Johan Hovold

author

2017-11-03 14:30 UTC #0 lore raw

cover serdev: receive_buf and locking fixes

This series addresses a few issues with the serdev code, including potential information leaks due to missing sanity checks in the receive path, a NULL-deref in write_wakeup() due to missing reference handling, and missing tty locking in close().

Johan

Johan Hovold (8):
serdev: ttyport: add missing receive_buf sanity checks
serdev: fix receive_buf return value when no callback
serdev: document driver callbacks
serdev: ttyport: fix NULL-deref on hangup
serdev: ttyport: fix tty locking in close
serdev: ttyport: release tty lock sooner on open
serdev: ttyport: ignore carrier detect to avoid hangups
serdev: ttyport: do not used keyed wakeup in write_wakeup

 drivers/tty/serdev/serdev-ttyport.c | 32 ++++++++++++++++++++++++++++----
 include/linux/serdev.h              |  8 +++++---
 2 files changed, 33 insertions(+), 7 deletions(-)

Signature · 1 line

2.15.0

Johan Hovold

author

2017-11-03 14:30 UTC #1 lore raw

PATCH 1/8 serdev: ttyport: add missing receive_buf sanity checks

The receive_buf tty-port callback should return the number of bytes accepted and must specifically never return a negative errno (or a value larger than the buffer size) to the tty layer.

A serdev driver not providing a receive_buf callback would currently cause the flush_to_ldisc() worker to spin in a tight loop when the tty buffer pointers are incremented with -EINVAL (-22) after data has been received.

A serdev driver occasionally returning a negative errno (or a too large byte count) could cause information leaks or crashes when accessing memory outside the tty buffers in consecutive callbacks.

Fixes cd6484e1830b ("serdev: Introduce new bus for serial attached devices")

Cc stable <[email protected]> # 4.11

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/tty/serdev/serdev-ttyport.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

drivers/tty/serdev/serdev-ttyport.c +12 -1

index 5b09ce920117..b8bc60a251c6 100644
@@ -35,11 +35,22 @@ static int ttyport_receive_buf(struct tty_port *port, const unsigned char *cp,
 {
 	struct serdev_controller *ctrl = port->client_data;
 	struct serport *serport = serdev_controller_get_drvdata(ctrl);
+	int ret;
 
 	if (!test_bit(SERPORT_ACTIVE, &serport->flags))
 		return 0;
 
-	return serdev_controller_receive_buf(ctrl, cp, count);
+	ret = serdev_controller_receive_buf(ctrl, cp, count);
+
+	dev_WARN_ONCE(&ctrl->dev, ret < 0 || ret > count,
+				"receive_buf returns %d (count = %zu)\n",
+				ret, count);
+	if (ret < 0)
+		return 0;
+	else if (ret > count)
+		return count;
+
+	return ret;
 }
 
 static void ttyport_write_wakeup(struct tty_port *port)

Signature · 1 line

2.15.0

Johan Hovold

author

2017-11-03 14:30 UTC #2 lore raw

PATCH 2/8 serdev: fix receive_buf return value when no callback

In reply to Johan Hovold

The receive_buf callback is supposed to return the number of bytes processed and should specifically not return a negative errno.

Due to missing sanity checks in the serdev tty-port controller, a driver not providing a receive_buf callback could cause the flush_to_ldisc() worker to spin in a tight loop when the tty buffer pointers are incremented with -EINVAL (-22).

The missing sanity checks have now been added to the tty-port controller, but let's fix up the serdev-controller helper as well.

Signed-off-by Johan Hovold <[email protected]>

---
 include/linux/serdev.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

include/linux/serdev.h +1 -1

index e69402d4a8ae..d609e6dc5bad 100644
@@ -184,7 +184,7 @@ static inline int serdev_controller_receive_buf(struct serdev_controller *ctrl,
 	struct serdev_device *serdev = ctrl->serdev;
 
 	if (!serdev || !serdev->ops->receive_buf)
-		return -EINVAL;
+		return 0;
 
 	return serdev->ops->receive_buf(serdev, data, count);
 }

Signature · 1 line

2.15.0

Johan Hovold

author

2017-11-03 14:30 UTC #3 lore raw

PATCH 3/8 serdev: document driver callbacks

In reply to Johan Hovold

Amend the driver-callback kerneldoc with calling context and expected return values.

Note that this is based on the requirements and characteristics of the tty-port controller implementation which receives data in workqueue context and whose write_wakeup callback must not sleep.

Also note that while the receive_buf callback returns an integer, the returned value is still expected to be non-negative (and no greater than the buffer-size argument).

Signed-off-by Johan Hovold <[email protected]>

---
 include/linux/serdev.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

include/linux/serdev.h +4 -2

index d609e6dc5bad..98b5f7978dac 100644
@@ -27,8 +27,10 @@ struct serdev_device;
 
 /**
  * struct serdev_device_ops - Callback operations for a serdev device
- * @receive_buf:	Function called with data received from device.
- * @write_wakeup:	Function called when ready to transmit more data.
+ * @receive_buf:	Function called with data received from device;
+ *			returns number of bytes accepted; may sleep.
+ * @write_wakeup:	Function called when ready to transmit more data; must
+ *			not sleep.
  */
 struct serdev_device_ops {
 	int (*receive_buf)(struct serdev_device *, const unsigned char *, size_t);

Signature · 1 line

2.15.0

Johan Hovold

author

2017-11-03 14:30 UTC #4 lore raw

PATCH 4/8 serdev: ttyport: fix NULL-deref on hangup

In reply to Johan Hovold

Make sure to use a properly refcounted tty_struct in write_wake up to avoid dereferencing a NULL-pointer when a port is being hung up.

Fixes bed35c6dfa6a ("serdev: add a tty port controller driver")

Cc stable <[email protected]> # 4.11

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/tty/serdev/serdev-ttyport.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

drivers/tty/serdev/serdev-ttyport.c +9 -2

index b8bc60a251c6..31c05665a0e6 100644
@@ -57,12 +57,19 @@ static void ttyport_write_wakeup(struct tty_port *port)
 {
 	struct serdev_controller *ctrl = port->client_data;
 	struct serport *serport = serdev_controller_get_drvdata(ctrl);
+	struct tty_struct *tty;
+
+	tty = tty_port_tty_get(port);
+	if (!tty)
+		return;
 
-	if (test_and_clear_bit(TTY_DO_WRITE_WAKEUP, &port->tty->flags) &&
+	if (test_and_clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags) &&
 	    test_bit(SERPORT_ACTIVE, &serport->flags))
 		serdev_controller_write_wakeup(ctrl);
 
-	wake_up_interruptible_poll(&port->tty->write_wait, POLLOUT);
+	wake_up_interruptible_poll(&tty->write_wait, POLLOUT);
+
+	tty_kref_put(tty);
 }
 
 static const struct tty_port_client_operations client_ops = {

Signature · 1 line

2.15.0

Johan Hovold

author

2017-11-03 14:30 UTC #5 lore raw

PATCH 5/8 serdev: ttyport: fix tty locking in close

In reply to Johan Hovold

Make sure to hold the tty lock as required when calling tty-driver close() (e.g. to avoid racing with hangup()).

Note that the serport active flag is currently set under the lock at controller open, but really isn't protected by it.

Fixes cd6484e1830b ("serdev: Introduce new bus for serial attached devices")

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/tty/serdev/serdev-ttyport.c | 2 ++
 1 file changed, 2 insertions(+)

drivers/tty/serdev/serdev-ttyport.c +2 -0

index 31c05665a0e6..334bce00364f 100644
@@ -162,8 +162,10 @@ static void ttyport_close(struct serdev_controller *ctrl)
 
 	clear_bit(SERPORT_ACTIVE, &serport->flags);
 
+	tty_lock(tty);
 	if (tty->ops->close)
 		tty->ops->close(tty, NULL);
+	tty_unlock(tty);
 
 	tty_release_struct(tty, serport->tty_idx);
 }

Signature · 1 line

2.15.0

Johan Hovold

author

2017-11-03 14:30 UTC #6 lore raw

PATCH 6/8 serdev: ttyport: release tty lock sooner on open

In reply to Johan Hovold

Release the tty lock once tty-driver open returns to make it clear that it does not protect neither tty->termios or the serport flags.

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/tty/serdev/serdev-ttyport.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

drivers/tty/serdev/serdev-ttyport.c +2 -1

index 334bce00364f..a8ddcd2ac5ea 100644
@@ -130,6 +130,8 @@ static int ttyport_open(struct serdev_controller *ctrl)
 	if (ret)
 		goto err_close;
 
+	tty_unlock(serport->tty);
+
 	/* Bring the UART into a known 8 bits no parity hw fc state */
 	ktermios = tty->termios;
 	ktermios.c_iflag &= ~(IGNBRK | BRKINT | PARMRK | ISTRIP |

@@ -143,7 +145,6 @@ static int ttyport_open(struct serdev_controller *ctrl)
 
 	set_bit(SERPORT_ACTIVE, &serport->flags);
 
-	tty_unlock(serport->tty);
 	return 0;
 
 err_close:

Signature · 1 line

2.15.0

Johan Hovold

author

2017-11-03 14:30 UTC #7 lore raw

PATCH 7/8 serdev: ttyport: ignore carrier detect to avoid hangups

In reply to Johan Hovold

Serdev currently does not support hangups so make sure to set CLOCAL to prevent loss of carrier from triggering one.

Note however that not all tty drivers honour CLOCAL.

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/tty/serdev/serdev-ttyport.c | 2 ++
 1 file changed, 2 insertions(+)

drivers/tty/serdev/serdev-ttyport.c +2 -0

index a8ddcd2ac5ea..727664ef456c 100644
@@ -141,6 +141,8 @@ static int ttyport_open(struct serdev_controller *ctrl)
 	ktermios.c_cflag &= ~(CSIZE | PARENB);
 	ktermios.c_cflag |= CS8;
 	ktermios.c_cflag |= CRTSCTS;
+	/* Hangups are not supported so make sure to ignore carrier detect. */
+	ktermios.c_cflag |= CLOCAL;
 	tty_set_termios(tty, &ktermios);
 
 	set_bit(SERPORT_ACTIVE, &serport->flags);

Signature · 1 line

2.15.0

Johan Hovold

author

2017-11-03 14:30 UTC #8 lore raw

PATCH 8/8 serdev: ttyport: do not used keyed wakeup in write_wakeup

In reply to Johan Hovold

Serdev does not use the file abstraction and specifically there will never be anyone polling a file descriptor for POLLOUT events.

Just use plain wake_up_interruptible() in the write_wakeup callback and document why it's there.

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/tty/serdev/serdev-ttyport.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

drivers/tty/serdev/serdev-ttyport.c +2 -1

index 727664ef456c..ec07b903b7e5 100644
@@ -67,7 +67,8 @@ static void ttyport_write_wakeup(struct tty_port *port)
 	    test_bit(SERPORT_ACTIVE, &serport->flags))
 		serdev_controller_write_wakeup(ctrl);
 
-	wake_up_interruptible_poll(&tty->write_wait, POLLOUT);
+	/* Wake up any tty_wait_until_sent() */
+	wake_up_interruptible(&tty->write_wait);
 
 	tty_kref_put(tty);
 }

Signature · 1 line

2.15.0

Greg Kroah-Hartman

maintainer

2017-11-28 15:04 UTC #9 lore raw

Re: [PATCH 8/8] serdev: ttyport: do not used keyed wakeup in write_wakeup

Quoted 10 lines · Johan Hovold

Serdev does not use the file abstraction and specifically there will never be anyone polling a file descriptor for POLLOUT events.
Just use plain wake_up_interruptible() in the write_wakeup callback and document why it's there.
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/tty/serdev/serdev-ttyport.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

This patch didn't apply, perhaps because I split this series across my "for-next" and "for-linus" branches?

thanks,

greg k-h

Johan Hovold

author

2017-11-28 15:16 UTC #10 lore raw

Re: [PATCH 8/8] serdev: ttyport: do not used keyed wakeup in write_wakeup

Quoted 7 lines · Greg Kroah-Hartman

Quoted 6 lines · Johan Hovold
Serdev does not use the file abstraction and specifically there will never be anyone polling a file descriptor for POLLOUT events.
Just use plain wake_up_interruptible() in the write_wakeup callback and document why it's there.

Quoted 7 lines

Quoted 4 lines
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/tty/serdev/serdev-ttyport.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
This patch didn't apply, perhaps because I split this series across my "for-next" and "for-linus" branches?

That's right, this one depends on patch 4/8.

Perhaps you can take also this one through tty-linus? Or even better, just take the whole series through tty-linus?

Johan

Greg Kroah-Hartman

maintainer

2017-11-28 19:39 UTC #11 lore raw

Re: [PATCH 8/8] serdev: ttyport: do not used keyed wakeup in write_wakeup

Quoted 13 lines · Johan Hovold

Quoted 12 lines · Greg Kroah-Hartman
Quoted 10 lines · Johan Hovold
Serdev does not use the file abstraction and specifically there will never be anyone polling a file descriptor for POLLOUT events.
Just use plain wake_up_interruptible() in the write_wakeup callback and document why it's there.
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/tty/serdev/serdev-ttyport.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Quoted 7 lines

Quoted 2 lines
This patch didn't apply, perhaps because I split this series across my "for-next" and "for-linus" branches?
That's right, this one depends on patch 4/8.
Perhaps you can take also this one through tty-linus? Or even better, just take the whole series through tty-linus?

They all didn't feel like patches to go in after -rc1, right? Documentation updates? Minor tweaks? Would you want to defend them? :)

thanks,

greg k-h

Johan Hovold

author

2017-11-29 09:48 UTC #12 lore raw

Re: [PATCH 8/8] serdev: ttyport: do not used keyed wakeup in write_wakeup

Quoted 17 lines · Greg Kroah-Hartman

Quoted 16 lines · Johan Hovold
Quoted 14 lines · Greg Kroah-Hartman
Quoted 10 lines · Johan Hovold
Serdev does not use the file abstraction and specifically there will never be anyone polling a file descriptor for POLLOUT events.
Just use plain wake_up_interruptible() in the write_wakeup callback and document why it's there.
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/tty/serdev/serdev-ttyport.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
This patch didn't apply, perhaps because I split this series across my "for-next" and "for-linus" branches?

Quoted 8 lines

Quoted 4 lines
That's right, this one depends on patch 4/8.
Perhaps you can take also this one through tty-linus? Or even better, just take the whole series through tty-linus?
They all didn't feel like patches to go in after -rc1, right? Documentation updates? Minor tweaks? Would you want to defend them? :)

I agree that it's borderline, but the documentation update (patch 3/8) is related to the first two bug fixes, where a negative return value from a serdev driver could have triggered those bugs, so in a sense we are fixing the docs.

Patch 6 and 8 are clean ups, but the open lock clean up in patch 6 is related to the close lock fix in patch 5.

Patch 7 avoids a potential crash, albeit something that would not affect any mainline drivers (as serial-core sets CLOCAL by default).

But I'm perfectly fine with holding them off for 4.16. Perhaps you can just merge back rc2 and I can resubmit the final patch which didn't apply after that.

Thanks,
Johan

Greg Kroah-Hartman

maintainer

2017-12-15 19:26 UTC #13 lore raw

Re: [PATCH 8/8] serdev: ttyport: do not used keyed wakeup in write_wakeup

Quoted 35 lines · Johan Hovold

Quoted 17 lines · Greg Kroah-Hartman
Quoted 16 lines · Johan Hovold
Quoted 14 lines · Greg Kroah-Hartman
Quoted 10 lines · Johan Hovold
Serdev does not use the file abstraction and specifically there will never be anyone polling a file descriptor for POLLOUT events.
Just use plain wake_up_interruptible() in the write_wakeup callback and document why it's there.
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/tty/serdev/serdev-ttyport.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
This patch didn't apply, perhaps because I split this series across my "for-next" and "for-linus" branches?
Quoted 8 lines
Quoted 4 lines
That's right, this one depends on patch 4/8.
Perhaps you can take also this one through tty-linus? Or even better, just take the whole series through tty-linus?
They all didn't feel like patches to go in after -rc1, right? Documentation updates? Minor tweaks? Would you want to defend them? :)
I agree that it's borderline, but the documentation update (patch 3/8) is related to the first two bug fixes, where a negative return value from a serdev driver could have triggered those bugs, so in a sense we are fixing the docs.
Patch 6 and 8 are clean ups, but the open lock clean up in patch 6 is related to the close lock fix in patch 5.

Quoted 6 lines

Patch 7 avoids a potential crash, albeit something that would not affect any mainline drivers (as serial-core sets CLOCAL by default).
But I'm perfectly fine with holding them off for 4.16. Perhaps you can just merge back rc2 and I can resubmit the final patch which didn't apply after that.

I've "merged back" now, care to resend the remaining patches?

thanks,

greg k-h

Johan Hovold

author

2017-12-18 11:03 UTC #14 lore raw

Re: [PATCH 8/8] serdev: ttyport: do not used keyed wakeup in write_wakeup

Quoted 36 lines · Greg Kroah-Hartman

Quoted 35 lines · Johan Hovold
Quoted 17 lines · Greg Kroah-Hartman
Quoted 16 lines · Johan Hovold
Quoted 14 lines · Greg Kroah-Hartman
Quoted 10 lines · Johan Hovold
Serdev does not use the file abstraction and specifically there will never be anyone polling a file descriptor for POLLOUT events.
Just use plain wake_up_interruptible() in the write_wakeup callback and document why it's there.
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/tty/serdev/serdev-ttyport.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
This patch didn't apply, perhaps because I split this series across my "for-next" and "for-linus" branches?
Quoted 8 lines
Quoted 4 lines
That's right, this one depends on patch 4/8.
Perhaps you can take also this one through tty-linus? Or even better, just take the whole series through tty-linus?
They all didn't feel like patches to go in after -rc1, right? Documentation updates? Minor tweaks? Would you want to defend them? :)
I agree that it's borderline, but the documentation update (patch 3/8) is related to the first two bug fixes, where a negative return value from a serdev driver could have triggered those bugs, so in a sense we are fixing the docs.
Patch 6 and 8 are clean ups, but the open lock clean up in patch 6 is related to the close lock fix in patch 5.

Quoted 8 lines

Quoted 6 lines
Patch 7 avoids a potential crash, albeit something that would not affect any mainline drivers (as serial-core sets CLOCAL by default).
But I'm perfectly fine with holding them off for 4.16. Perhaps you can just merge back rc2 and I can resubmit the final patch which didn't apply after that.
I've "merged back" now, care to resend the remaining patches?

Just did so. It was just this this single patch that had not yet been applied.

Thanks,
Johan