[PATCH 0/8] serdev: receive_buf and locking fixes

[PATCH 0/8] serdev: receive_buf and locking fixes

[Johan Hovold]

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

-- 
2.15.0

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

[Johan Hovold]

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 <stable@vger.kernel.org>	# 4.11
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/tty/serdev/serdev-ttyport.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c
index 5b09ce920117..b8bc60a251c6 100644
--- a/drivers/tty/serdev/serdev-ttyport.c
+++ b/drivers/tty/serdev/serdev-ttyport.c
@@ -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)
-- 
2.15.0

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

[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 <johan@kernel.org>
---
 include/linux/serdev.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/serdev.h b/include/linux/serdev.h
index e69402d4a8ae..d609e6dc5bad 100644
--- a/include/linux/serdev.h
+++ b/include/linux/serdev.h
@@ -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);
 }
-- 
2.15.0

[PATCH 3/8] serdev: document driver callbacks

[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 <johan@kernel.org>
---
 include/linux/serdev.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/include/linux/serdev.h b/include/linux/serdev.h
index d609e6dc5bad..98b5f7978dac 100644
--- a/include/linux/serdev.h
+++ b/include/linux/serdev.h
@@ -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);
-- 
2.15.0

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

[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 <stable@vger.kernel.org>     # 4.11
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/tty/serdev/serdev-ttyport.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c
index b8bc60a251c6..31c05665a0e6 100644
--- a/drivers/tty/serdev/serdev-ttyport.c
+++ b/drivers/tty/serdev/serdev-ttyport.c
@@ -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 = {
-- 
2.15.0

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

[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 <johan@kernel.org>
---
 drivers/tty/serdev/serdev-ttyport.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c
index 31c05665a0e6..334bce00364f 100644
--- a/drivers/tty/serdev/serdev-ttyport.c
+++ b/drivers/tty/serdev/serdev-ttyport.c
@@ -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);
 }
-- 
2.15.0

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

[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 <johan@kernel.org>
---
 drivers/tty/serdev/serdev-ttyport.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c
index 334bce00364f..a8ddcd2ac5ea 100644
--- a/drivers/tty/serdev/serdev-ttyport.c
+++ b/drivers/tty/serdev/serdev-ttyport.c
@@ -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:
-- 
2.15.0

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

[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 <johan@kernel.org>
---
 drivers/tty/serdev/serdev-ttyport.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c
index a8ddcd2ac5ea..727664ef456c 100644
--- a/drivers/tty/serdev/serdev-ttyport.c
+++ b/drivers/tty/serdev/serdev-ttyport.c
@@ -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);
-- 
2.15.0

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

[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 <johan@kernel.org>
---
 drivers/tty/serdev/serdev-ttyport.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c
index 727664ef456c..ec07b903b7e5 100644
--- a/drivers/tty/serdev/serdev-ttyport.c
+++ b/drivers/tty/serdev/serdev-ttyport.c
@@ -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);
 }
-- 
2.15.0

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

[Greg Kroah-Hartman]

On Fri, Nov 03, 2017 at 03:30:59PM +0100, Johan Hovold wrote:
> 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 <johan@kernel.org>
> ---
>  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

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

[Johan Hovold]

On Tue, Nov 28, 2017 at 04:04:18PM +0100, Greg Kroah-Hartman wrote:
> On Fri, Nov 03, 2017 at 03:30:59PM +0100, Johan Hovold wrote:
> > 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 <johan@kernel.org>
> > ---
> >  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

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

[Greg Kroah-Hartman]

On Tue, Nov 28, 2017 at 04:16:29PM +0100, Johan Hovold wrote:
> On Tue, Nov 28, 2017 at 04:04:18PM +0100, Greg Kroah-Hartman wrote:
> > On Fri, Nov 03, 2017 at 03:30:59PM +0100, Johan Hovold wrote:
> > > 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 <johan@kernel.org>
> > > ---
> > >  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?

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

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

[Johan Hovold]

On Tue, Nov 28, 2017 at 08:39:26PM +0100, Greg Kroah-Hartman wrote:
> On Tue, Nov 28, 2017 at 04:16:29PM +0100, Johan Hovold wrote:
> > On Tue, Nov 28, 2017 at 04:04:18PM +0100, Greg Kroah-Hartman wrote:
> > > On Fri, Nov 03, 2017 at 03:30:59PM +0100, Johan Hovold wrote:
> > > > 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 <johan@kernel.org>
> > > > ---
> > > >  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?
> 
> 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

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

[Greg Kroah-Hartman]

On Wed, Nov 29, 2017 at 10:48:02AM +0100, Johan Hovold wrote:
> On Tue, Nov 28, 2017 at 08:39:26PM +0100, Greg Kroah-Hartman wrote:
> > On Tue, Nov 28, 2017 at 04:16:29PM +0100, Johan Hovold wrote:
> > > On Tue, Nov 28, 2017 at 04:04:18PM +0100, Greg Kroah-Hartman wrote:
> > > > On Fri, Nov 03, 2017 at 03:30:59PM +0100, Johan Hovold wrote:
> > > > > 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 <johan@kernel.org>
> > > > > ---
> > > > >  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?
> > 
> > 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.

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

thanks,

greg k-h

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

[Johan Hovold]

On Fri, Dec 15, 2017 at 08:26:08PM +0100, Greg Kroah-Hartman wrote:
> On Wed, Nov 29, 2017 at 10:48:02AM +0100, Johan Hovold wrote:
> > On Tue, Nov 28, 2017 at 08:39:26PM +0100, Greg Kroah-Hartman wrote:
> > > On Tue, Nov 28, 2017 at 04:16:29PM +0100, Johan Hovold wrote:
> > > > On Tue, Nov 28, 2017 at 04:04:18PM +0100, Greg Kroah-Hartman wrote:
> > > > > On Fri, Nov 03, 2017 at 03:30:59PM +0100, Johan Hovold wrote:
> > > > > > 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 <johan@kernel.org>
> > > > > > ---
> > > > > >  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?
> > > 
> > > 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.
> 
> 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