[PATCH 0/3] tty, serial: a couple of unrelated fixes

[PATCH 0/3] tty, serial: a couple of unrelated fixes

[Jiri Slaby (SUSE)]

Three unrelated fixes which accumulated in my tree:
* 8250_exar: an error pointer could be stored into priv->gpio and later
  dereferenced on unbind, reported by Sashiko,
* 8250_hp300: a pure reorg to get rid of forward declarations and
  shrink the number of ifdefs,
* tty_io: compat_ptr() was applied to the wrong ioctl() argument since
  2013.

Jiri Slaby (SUSE) (3):
  serial: 8250_exar: check priv->gpio properly
  serial: 8250_hp300: reorder HPDCA code
  tty: fix misapplied compat_ptr() in tty_compat_ioctl()

 drivers/tty/serial/8250/8250_exar.c  |  2 +-
 drivers/tty/serial/8250/8250_hp300.c | 62 ++++++++++++----------------
 drivers/tty/tty_io.c                 |  3 +-
 3 files changed, 28 insertions(+), 39 deletions(-)

-- 
2.56.0

[PATCH 1/3] serial: 8250_exar: check priv->gpio properly

[Jiri Slaby (SUSE)]

priv->gpio is set by platform_device_register_full(), which returns
error-encoded pointers on failure, not NULL. Sashiko reported:

> Does __xr17v35x_register_gpio() return an ERR_PTR if
> platform_device_register_full() fails?
>
> If it does, we might be storing an error pointer directly into
> priv->gpio in xr17v35x_register_gpio() without validating it with
> IS_ERR(). Because the error is ignored, the device probe
> successfully continues.

Before commit 1145d1a0703f ("serial: 8250_exar: use
platform_device_register_full()"), __xr17v35x_register_gpio()
returned NULL on any failure (platform_device_alloc(),
device_add_software_node() or platform_device_add() failing were all
turned into a NULL return). The plain truthy check in
xr17v35x_unregister_gpio() matched that convention. That commit
switched registration to platform_device_register_full(), which
reports failure as an ERR_PTR() instead of NULL, but the unregister
check was never updated to match. As a result, a failed registration
left an error pointer in priv->gpio, and unbind would tear it down as
if it were a real platform_device, dereferencing the encoded error
value in device_remove_software_node() and
platform_device_unregister().

Use IS_ERR_OR_NULL() instead, so both the unset (non-EXAR vendor,
priv->gpio never assigned) and failed-registration (ERR_PTR) cases
are skipped.

Fixes: 1145d1a0703f ("serial: 8250_exar: use platform_device_register_full()")
Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
Cc: stable@vger.kernel.org
Assisted-by: LLM
Link: https://lore.kernel.org/all/sashiko-outbox-170895@kernel.org/
---
 drivers/tty/serial/8250/8250_exar.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
index 57847c5bd2fc..b1e20fd4cfd2 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -1181,7 +1181,7 @@ static int xr17v35x_register_gpio(struct exar8250 *priv, struct pci_dev *pcidev,
 
 static void xr17v35x_unregister_gpio(struct exar8250 *priv)
 {
-	if (priv->gpio) {
+	if (!IS_ERR_OR_NULL(priv->gpio)) {
 		__xr17v35x_unregister_gpio(priv->gpio);
 		priv->gpio = NULL;
 	}
-- 
2.56.0

Re: [PATCH 1/3] serial: 8250_exar: check priv->gpio properly

[sashiko-bot]

> priv->gpio is set by platform_device_register_full(), which returns
> error-encoded pointers on failure, not NULL. Sashiko reported:
> 
> > Does __xr17v35x_register_gpio() return an ERR_PTR if
> > platform_device_register_full() fails?
> >
> > If it does, we might be storing an error pointer directly into
> > priv->gpio in xr17v35x_register_gpio() without validating it with
> > IS_ERR(). Because the error is ignored, the device probe
> > successfully continues.
> 
> Before commit 1145d1a0703f ("serial: 8250_exar: use
> platform_device_register_full()"), __xr17v35x_register_gpio()
> returned NULL on any failure (platform_device_alloc(),
> device_add_software_node() or platform_device_add() failing were all
> [ ... ]
> priv->gpio never assigned) and failed-registration (ERR_PTR) cases
> are skipped.
> 
> Fixes: 1145d1a0703f ("serial: 8250_exar: use platform_device_register_full()")
> Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793704520.git.jirislaby@kernel.org?part=1

[PATCH 2/3] serial: 8250_hp300: reorder HPDCA code

[Jiri Slaby (SUSE)]

To have less ifdefs (code is now under one) and no forward declarations.

Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
---
 drivers/tty/serial/8250/8250_hp300.c | 62 ++++++++++++----------------
 1 file changed, 26 insertions(+), 36 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_hp300.c b/drivers/tty/serial/8250/8250_hp300.c
index e9599c5c2fea..68ce5972c296 100644
--- a/drivers/tty/serial/8250/8250_hp300.c
+++ b/drivers/tty/serial/8250/8250_hp300.c
@@ -34,29 +34,6 @@ struct hp300_port {
 static struct hp300_port *hp300_ports;
 #endif
 
-#ifdef CONFIG_HPDCA
-
-static int hpdca_init_one(struct dio_dev *d,
-					const struct dio_device_id *ent);
-static void hpdca_remove_one(struct dio_dev *d);
-
-static struct dio_device_id hpdca_dio_tbl[] = {
-	{ DIO_ID_DCA0 },
-	{ DIO_ID_DCA0REM },
-	{ DIO_ID_DCA1 },
-	{ DIO_ID_DCA1REM },
-	{ 0 }
-};
-
-static struct dio_driver hpdca_driver = {
-	.name      = "hpdca",
-	.id_table  = hpdca_dio_tbl,
-	.probe     = hpdca_init_one,
-	.remove    = hpdca_remove_one,
-};
-
-#endif
-
 static unsigned int num_ports;
 
 extern int hp300_uart_scode;
@@ -197,6 +174,32 @@ static int hpdca_init_one(struct dio_dev *d,
 
 	return 0;
 }
+
+static void hpdca_remove_one(struct dio_dev *d)
+{
+	struct uart_8250_port *uport = dio_get_drvdata(d);
+
+	if (d->resource.start) {
+		/* Disable board-interrupts */
+		out_8(d->resource.start + DIO_VIRADDRBASE + DCA_IC, 0);
+	}
+	serial8250_unregister_port(uport);
+}
+
+static struct dio_device_id hpdca_dio_tbl[] = {
+	{ DIO_ID_DCA0 },
+	{ DIO_ID_DCA0REM },
+	{ DIO_ID_DCA1 },
+	{ DIO_ID_DCA1REM },
+	{ 0 }
+};
+
+static struct dio_driver hpdca_driver = {
+	.name      = "hpdca",
+	.id_table  = hpdca_dio_tbl,
+	.probe     = hpdca_init_one,
+	.remove    = hpdca_remove_one,
+};
 #endif
 
 static int __init hp300_8250_init(void)
@@ -280,19 +283,6 @@ static int __init hp300_8250_init(void)
 	return 0;
 }
 
-#ifdef CONFIG_HPDCA
-static void hpdca_remove_one(struct dio_dev *d)
-{
-	struct uart_8250_port *uport = dio_get_drvdata(d);
-
-	if (d->resource.start) {
-		/* Disable board-interrupts */
-		out_8(d->resource.start + DIO_VIRADDRBASE + DCA_IC, 0);
-	}
-	serial8250_unregister_port(uport);
-}
-#endif
-
 static void __exit hp300_8250_exit(void)
 {
 #ifdef CONFIG_HPAPCI
-- 
2.56.0

Re: [PATCH 2/3] serial: 8250_hp300: reorder HPDCA code

[sashiko-bot]

> To have less ifdefs (code is now under one) and no forward declarations.
> 
> Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793704520.git.jirislaby@kernel.org?part=2

[PATCH 3/3] tty: fix misapplied compat_ptr() in tty_compat_ioctl()

[Jiri Slaby (SUSE)]

Since commit f0193d3ea73b below, `cmd` was converted with compat_ptr(),
while `arg` was passed to ld->ops->ioctl() unmodified. Fix it so `cmd`
stays `cmd` and `arg` gets converted through compat_ptr() as intended.

Notes:
2) compat_ptr() did nothing to `cmd`, provided it is uint,
1) compat_ptr() is no-op everywhere (was different on s390 before
   8e0b986c59c6 ("s390: Remove compat support")),
3) `arg` might not have been chopped correctly until 8e0b986c59c6. But I
   am not sure why noone noticed all those (7) years (from f0193d3ea73b
   to 8e0b986c59c6)?

Fixes: f0193d3ea73b ("change semantics of ldisc ->compat_ioctl()")
Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
Cc: stable@vger.kernel.org # applicable from 4.20 to 6.18 incl
Assisted-by: LLM
---
 drivers/tty/tty_io.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 1a6c8a1bc764..150052f6d9db 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -2969,8 +2969,7 @@ static long tty_compat_ioctl(struct file *file, unsigned int cmd,
 	if (ld->ops->compat_ioctl)
 		retval = ld->ops->compat_ioctl(tty, cmd, arg);
 	if (retval == -ENOIOCTLCMD && ld->ops->ioctl)
-		retval = ld->ops->ioctl(tty, (unsigned long)compat_ptr(cmd),
-				arg);
+		retval = ld->ops->ioctl(tty, cmd, (unsigned long)compat_ptr(arg));
 	tty_ldisc_deref(ld);
 
 	return retval;
-- 
2.56.0

Re: [PATCH 3/3] tty: fix misapplied compat_ptr() in tty_compat_ioctl()

[sashiko-bot]

> Since commit f0193d3ea73b below, `cmd` was converted with compat_ptr(),
> while `arg` was passed to ld->ops->ioctl() unmodified. Fix it so `cmd`
> stays `cmd` and `arg` gets converted through compat_ptr() as intended.
> 
> Notes:
> 2) compat_ptr() did nothing to `cmd`, provided it is uint,
> 1) compat_ptr() is no-op everywhere (was different on s390 before
>    8e0b986c59c6 ("s390: Remove compat support")),
> 3) `arg` might not have been chopped correctly until 8e0b986c59c6. But I
>    am not sure why noone noticed all those (7) years (from f0193d3ea73b
>    to 8e0b986c59c6)?
> 
> Fixes: f0193d3ea73b ("change semantics of ldisc ->compat_ioctl()")
> Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793704520.git.jirislaby@kernel.org?part=3