[PATCH] tty_port: Restore tty port default ops on unregistering

[PATCH] tty_port: Restore tty port default ops on unregistering

[Loic Poulain]

When a port is unregistered from serdev, its serdev specific client_ops
pointer is nullified, which can lead to future null pointer dereference.
Fix that by restoring default client_ops when port is unregistered from
serdev.

port registering/unregistering can happen several times, at least it
happens when statically registered 8250 ISA port are replaced at boot
time by e.g. device-tree defined 8250 ports.

Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
---
 drivers/tty/tty_port.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
index 044c3cb..bf893da 100644
--- a/drivers/tty/tty_port.c
+++ b/drivers/tty/tty_port.c
@@ -203,8 +203,10 @@ void tty_port_unregister_device(struct tty_port *port,
 	int ret;
 
 	ret = serdev_tty_port_unregister(port);
-	if (ret == 0)
+	if (ret == 0) {
+		port->client_ops = &default_client_ops;
 		return;
+	}
 
 	tty_unregister_device(driver, index);
 }
-- 
2.7.4

Re: [PATCH] tty_port: Restore tty port default ops on unregistering

[Johan Hovold]

On Wed, Jan 29, 2020 at 09:07:04AM +0100, Loic Poulain wrote:
> When a port is unregistered from serdev, its serdev specific client_ops
> pointer is nullified, which can lead to future null pointer dereference.
> Fix that by restoring default client_ops when port is unregistered from
> serdev.

Hmm, yeah, this seems like something we should fix as 8250 appears to
allow reregistering ports, but...

> port registering/unregistering can happen several times, at least it
> happens when statically registered 8250 ISA port are replaced at boot
> time by e.g. device-tree defined 8250 ports.

How did the serdev controller get registered in the first place here if
you're talking about statically registered 8250 ISA ports (i.e. where is
the client defined)?

Did you actually ever hit this one?

> Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
> ---
>  drivers/tty/tty_port.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
> index 044c3cb..bf893da 100644
> --- a/drivers/tty/tty_port.c
> +++ b/drivers/tty/tty_port.c
> @@ -203,8 +203,10 @@ void tty_port_unregister_device(struct tty_port *port,
>  	int ret;
>  
>  	ret = serdev_tty_port_unregister(port);
> -	if (ret == 0)
> +	if (ret == 0) {
> +		port->client_ops = &default_client_ops;
>  		return;
> +	}
>  
>  	tty_unregister_device(driver, index);
>  }

Johan

Re: [PATCH] tty_port: Restore tty port default ops on unregistering

[Johan Hovold]

On Tue, Feb 04, 2020 at 07:03:37PM +0100, Loic Poulain wrote:
> On Tue, 4 Feb 2020 at 10:47, Johan Hovold <johan@kernel.org> wrote:
> >
> > On Wed, Jan 29, 2020 at 09:07:04AM +0100, Loic Poulain wrote:
> > > When a port is unregistered from serdev, its serdev specific client_ops
> > > pointer is nullified, which can lead to future null pointer dereference.
> > > Fix that by restoring default client_ops when port is unregistered from
> > > serdev.
> >
> > Hmm, yeah, this seems like something we should fix as 8250 appears to
> > allow reregistering ports, but...
> >
> > > port registering/unregistering can happen several times, at least it
> > > happens when statically registered 8250 ISA port are replaced at boot
> > > time by e.g. device-tree defined 8250 ports.
> >
> > How did the serdev controller get registered in the first place here if
> > you're talking about statically registered 8250 ISA ports (i.e. where is
> > the client defined)?
> >
> > Did you actually ever hit this one?
> 
> Yes, but never in the mainline.

Ok, thanks for confirming.

> Actually a set of changes [1] [2] in the AOSP common kernel mades the tty
> ports bound to serdev by default, except for one(s) with console attached.
> So with some platforms the ISA ttyS0 is firstly registered with serdev and
> then replaced (unregistered/registered) later in the boot by the 8250 port
> enumerated from the fdt and used for console (expecting default tty ops).
> 
> TBH I disagree with the way it has been done in the AOSP, but it highlighted
> this possible null ops path, which can be fixed either by resetting the
> default ops on tty port unregister (this patch), or on register.

Yeah, those AOSP commits doesn't make much sense, especially not the
first one since it doesn't provide any means to define the client.

> [1]
> https://android.googlesource.com/kernel/common/+/c550a54f23026e92633c5145e8b919cf590a5624
> [2]
> https://android.googlesource.com/kernel/common/+/cc3b00864e3b316ff106f948834d7e0cc6244921

I spent some time looking into this today, and this doesn't seem to be
an issue for mainline unless we are reloading 8250 drivers.

Specifically, the statically defined ports that 8250 core would register
when an 8250 driver is being unbound could end up with a NULL client
ops.

This is seems unlikely to be an issue in practice, but I think we should
plug this nonetheless.

I already fixed the related issue of serdev not resetting the client_ops
on failed registration in aee5da783878 ("serdev: fix tty-port client
deregistration"), and I think the unregistration case should be handled
similarly by the serdev ttyport controller driver (rather than by tty
core).

I've prepared a patch with a commit message outlining how this may
affect mainline and included a stable tag. This should fix the issue in
AOSP as well, even that first commit [1] above really should be
reverted.

Johan

[PATCH] serdev: ttyport: restore client ops on deregistration

[Johan Hovold]

The serdev tty-port controller driver should reset the tty-port client
operations also on deregistration to avoid a NULL-pointer dereference in
case the port is later re-registered as a normal tty device.

Note that this can only happen with tty drivers such as 8250 which have
statically allocated port structures that can end up being reused and
where a later registration would not register a serdev controller (e.g.
due to registration errors or if the devicetree has been changed in
between).

Specifically, this can be an issue for any statically defined ports that
would be registered by 8250 core when an 8250 driver is being unbound.

Fixes: bed35c6dfa6a ("serdev: add a tty port controller driver")
Cc: stable <stable@vger.kernel.org>     # 4.11
Reported-by: Loic Poulain <loic.poulain@linaro.org>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/tty/serdev/serdev-ttyport.c | 6 ++----
 drivers/tty/tty_port.c              | 5 +++--
 include/linux/tty.h                 | 2 ++
 3 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c
index d1cdd2ab8b4c..d367803e2044 100644
--- a/drivers/tty/serdev/serdev-ttyport.c
+++ b/drivers/tty/serdev/serdev-ttyport.c
@@ -265,7 +265,6 @@ struct device *serdev_tty_port_register(struct tty_port *port,
 					struct device *parent,
 					struct tty_driver *drv, int idx)
 {
-	const struct tty_port_client_operations *old_ops;
 	struct serdev_controller *ctrl;
 	struct serport *serport;
 	int ret;
@@ -284,7 +283,6 @@ struct device *serdev_tty_port_register(struct tty_port *port,
 
 	ctrl->ops = &ctrl_ops;
 
-	old_ops = port->client_ops;
 	port->client_ops = &client_ops;
 	port->client_data = ctrl;
 
@@ -297,7 +295,7 @@ struct device *serdev_tty_port_register(struct tty_port *port,
 
 err_reset_data:
 	port->client_data = NULL;
-	port->client_ops = old_ops;
+	port->client_ops = &tty_port_default_client_ops;
 	serdev_controller_put(ctrl);
 
 	return ERR_PTR(ret);
@@ -312,8 +310,8 @@ int serdev_tty_port_unregister(struct tty_port *port)
 		return -ENODEV;
 
 	serdev_controller_remove(ctrl);
-	port->client_ops = NULL;
 	port->client_data = NULL;
+	port->client_ops = &tty_port_default_client_ops;
 	serdev_controller_put(ctrl);
 
 	return 0;
diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
index 044c3cbdcfa4..ea80bf872f54 100644
--- a/drivers/tty/tty_port.c
+++ b/drivers/tty/tty_port.c
@@ -52,10 +52,11 @@ static void tty_port_default_wakeup(struct tty_port *port)
 	}
 }
 
-static const struct tty_port_client_operations default_client_ops = {
+const struct tty_port_client_operations tty_port_default_client_ops = {
 	.receive_buf = tty_port_default_receive_buf,
 	.write_wakeup = tty_port_default_wakeup,
 };
+EXPORT_SYMBOL_GPL(tty_port_default_client_ops);
 
 void tty_port_init(struct tty_port *port)
 {
@@ -68,7 +69,7 @@ void tty_port_init(struct tty_port *port)
 	spin_lock_init(&port->lock);
 	port->close_delay = (50 * HZ) / 100;
 	port->closing_wait = (3000 * HZ) / 100;
-	port->client_ops = &default_client_ops;
+	port->client_ops = &tty_port_default_client_ops;
 	kref_init(&port->kref);
 }
 EXPORT_SYMBOL(tty_port_init);
diff --git a/include/linux/tty.h b/include/linux/tty.h
index bfa4e2ee94a9..bd5fe0e907e8 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -225,6 +225,8 @@ struct tty_port_client_operations {
 	void (*write_wakeup)(struct tty_port *port);
 };
 
+extern const struct tty_port_client_operations tty_port_default_client_ops;
+
 struct tty_port {
 	struct tty_bufhead	buf;		/* Locked internally */
 	struct tty_struct	*tty;		/* Back pointer */
-- 
2.24.1