Skip to contentSkip to conversation

ipack: ipoctal: fix info leak and other bugs

PATCH In review Johan Hovold 3 participants 9 messages Sep 17, 2021 – Sep 27, 2021

Subsystems (2)
Stable branch Industry pack subsystem (ipack)
lore
Share & tools
lore

Johan Hovold

author
#0 lore raw

cover ipack: ipoctal: fix info leak and other bugs

This series fixes a stack info leak and a number of other bugs all found through inspection. Included is also a related cleanup.

Note that the series has only been compile tested.

Johan

Johan Hovold (6):
ipack: ipoctal: fix stack information leak
ipack: ipoctal: fix tty registration race
ipack: ipoctal: fix tty-registration error handling
ipack: ipoctal: fix missing allocation-failure check
ipack: ipoctal: fix module reference leak
ipack: ipoctal: rename tty-driver pointer

 drivers/ipack/devices/ipoctal.c | 101 +++++++++++++++++++++-----------
 1 file changed, 66 insertions(+), 35 deletions(-)
Signature · 1 line
2.32.0
author
#1 lore raw

PATCH 1/6 ipack: ipoctal: fix stack information leak

The tty driver name is used also after registering the driver and must specifically not be allocated on the stack to avoid leaking information to user space (or triggering an oops).

Drivers should not try to encode topology information in the tty device name but this one snuck in through staging without anyone noticing and another driver has since copied this malpractice.

Fixing the ABI is a separate issue, but this at least plugs the security hole.

Fixes ba4dc61fe8c5 ("Staging: ipack: add support for IP-OCTAL mezzanine board")
Cc [email protected] # 3.5
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/ipack/devices/ipoctal.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)
drivers/ipack/devices/ipoctal.c +14 -5
index c14e65a5d38f..c62fec75987c 100644
@@ -264,7 +264,6 @@ static int ipoctal_inst_slot(struct ipoctal *ipoctal, unsigned int bus_nr,
 	int res;
 	int i;
 	struct tty_driver *tty;
-	char name[20];
 	struct ipoctal_channel *channel;
 	struct ipack_region *region;
 	void __iomem *addr;

@@ -355,8 +354,11 @@ static int ipoctal_inst_slot(struct ipoctal *ipoctal, unsigned int bus_nr,
 	/* Fill struct tty_driver with ipoctal data */
 	tty->owner = THIS_MODULE;
 	tty->driver_name = KBUILD_MODNAME;
-	sprintf(name, KBUILD_MODNAME ".%d.%d.", bus_nr, slot);
-	tty->name = name;
+	tty->name = kasprintf(GFP_KERNEL, KBUILD_MODNAME ".%d.%d.", bus_nr, slot);
+	if (!tty->name) {
+		res = -ENOMEM;
+		goto err_put_driver;
+	}
 	tty->major = 0;
 
 	tty->minor_start = 0;

@@ -371,8 +373,7 @@ static int ipoctal_inst_slot(struct ipoctal *ipoctal, unsigned int bus_nr,
 	res = tty_register_driver(tty);
 	if (res) {
 		dev_err(&ipoctal->dev->dev, "Can't register tty driver.\n");
-		tty_driver_kref_put(tty);
-		return res;
+		goto err_free_name;
 	}
 
 	/* Save struct tty_driver for use it when uninstalling the device */

@@ -409,6 +410,13 @@ static int ipoctal_inst_slot(struct ipoctal *ipoctal, unsigned int bus_nr,
 				       ipoctal_irq_handler, ipoctal);
 
 	return 0;
+
+err_free_name:
+	kfree(tty->name);
+err_put_driver:
+	tty_driver_kref_put(tty);
+
+	return res;
 }
 
 static inline int ipoctal_copy_write_buffer(struct ipoctal_channel *channel,

@@ -696,6 +704,7 @@ static void __ipoctal_remove(struct ipoctal *ipoctal)
 	}
 
 	tty_unregister_driver(ipoctal->tty_drv);
+	kfree(ipoctal->tty_drv->name);
 	tty_driver_kref_put(ipoctal->tty_drv);
 	kfree(ipoctal);
 }
Signature · 1 line
2.32.0
author
#2 lore raw

PATCH 2/6 ipack: ipoctal: fix tty registration race

In reply to Johan Hovold

Make sure to set the tty class-device driver data before registering the tty to avoid having a racing open() dereference a NULL pointer.

Fixes 9c1d784afc6f ("Staging: ipack/devices/ipoctal: Get rid of ipoctal_list.")
Cc [email protected] # 3.7
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/ipack/devices/ipoctal.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/ipack/devices/ipoctal.c +2 -2
index c62fec75987c..262451343127 100644
@@ -392,13 +392,13 @@ static int ipoctal_inst_slot(struct ipoctal *ipoctal, unsigned int bus_nr,
 		spin_lock_init(&channel->lock);
 		channel->pointer_read = 0;
 		channel->pointer_write = 0;
-		tty_dev = tty_port_register_device(&channel->tty_port, tty, i, NULL);
+		tty_dev = tty_port_register_device_attr(&channel->tty_port, tty,
+							i, NULL, channel, NULL);
 		if (IS_ERR(tty_dev)) {
 			dev_err(&ipoctal->dev->dev, "Failed to register tty device.\n");
 			tty_port_destroy(&channel->tty_port);
 			continue;
 		}
-		dev_set_drvdata(tty_dev, channel);
 	}
 
 	/*
Signature · 1 line
2.32.0
author
#3 lore raw

PATCH 3/6 ipack: ipoctal: fix tty-registration error handling

In reply to Johan Hovold

Registration of the ipoctal tty devices is unlikely to fail, but if it ever does, make sure not to deregister a never registered tty device (and dereference a NULL pointer) when the driver is later unbound.

Fixes 2afb41d9d30d ("Staging: ipack/devices/ipoctal: Check tty_register_device return value.")
Cc [email protected] # 3.7
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/ipack/devices/ipoctal.c | 7 +++++++
 1 file changed, 7 insertions(+)
drivers/ipack/devices/ipoctal.c +7 -0
index 262451343127..d6875aa6a295 100644
@@ -33,6 +33,7 @@ struct ipoctal_channel {
 	unsigned int			pointer_read;
 	unsigned int			pointer_write;
 	struct tty_port			tty_port;
+	bool				tty_registered;
 	union scc2698_channel __iomem	*regs;
 	union scc2698_block __iomem	*block_regs;
 	unsigned int			board_id;

@@ -396,9 +397,11 @@ static int ipoctal_inst_slot(struct ipoctal *ipoctal, unsigned int bus_nr,
 							i, NULL, channel, NULL);
 		if (IS_ERR(tty_dev)) {
 			dev_err(&ipoctal->dev->dev, "Failed to register tty device.\n");
+			tty_port_free_xmit_buf(&channel->tty_port);
 			tty_port_destroy(&channel->tty_port);
 			continue;
 		}
+		channel->tty_registered = true;
 	}
 
 	/*

@@ -698,6 +701,10 @@ static void __ipoctal_remove(struct ipoctal *ipoctal)
 
 	for (i = 0; i < NR_CHANNELS; i++) {
 		struct ipoctal_channel *channel = &ipoctal->channel[i];
+
+		if (!channel->tty_registered)
+			continue;
+
 		tty_unregister_device(ipoctal->tty_drv, i);
 		tty_port_free_xmit_buf(&channel->tty_port);
 		tty_port_destroy(&channel->tty_port);
Signature · 1 line
2.32.0
author
#4 lore raw

PATCH 4/6 ipack: ipoctal: fix missing allocation-failure check

In reply to Johan Hovold

Add the missing error handling when allocating the transmit buffer to avoid dereferencing a NULL pointer in write() should the allocation ever fail.

Fixes ba4dc61fe8c5 ("Staging: ipack: add support for IP-OCTAL mezzanine board")
Cc [email protected] # 3.5
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/ipack/devices/ipoctal.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/ipack/devices/ipoctal.c +3 -1
index d6875aa6a295..61c41f535510 100644
@@ -385,7 +385,9 @@ static int ipoctal_inst_slot(struct ipoctal *ipoctal, unsigned int bus_nr,
 
 		channel = &ipoctal->channel[i];
 		tty_port_init(&channel->tty_port);
-		tty_port_alloc_xmit_buf(&channel->tty_port);
+		res = tty_port_alloc_xmit_buf(&channel->tty_port);
+		if (res)
+			continue;
 		channel->tty_port.ops = &ipoctal_tty_port_ops;
 
 		ipoctal_reset_stats(&channel->stats);
Signature · 1 line
2.32.0
author
#5 lore raw

PATCH 5/6 ipack: ipoctal: fix module reference leak

In reply to Johan Hovold

A reference to the carrier module was taken on every open but was only released once when the final reference to the tty struct was dropped.

Fix this by taking the module reference and initialising the tty driver data when installing the tty.

Fixes 82a82340bab6 ("ipoctal: get carrier driver to avoid rmmod")
Cc [email protected] # 3.18
Cc Federico Vaga <[email protected]>
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/ipack/devices/ipoctal.c | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)
drivers/ipack/devices/ipoctal.c +21 -8
index 61c41f535510..c709861198e5 100644
@@ -82,22 +82,34 @@ static int ipoctal_port_activate(struct tty_port *port, struct tty_struct *tty)
 	return 0;
 }
 
-static int ipoctal_open(struct tty_struct *tty, struct file *file)
+static int ipoctal_install(struct tty_driver *driver, struct tty_struct *tty)
 {
 	struct ipoctal_channel *channel = dev_get_drvdata(tty->dev);
 	struct ipoctal *ipoctal = chan_to_ipoctal(channel, tty->index);
-	int err;
-
-	tty->driver_data = channel;
+	int res;
 
 	if (!ipack_get_carrier(ipoctal->dev))
 		return -EBUSY;
 
-	err = tty_port_open(&channel->tty_port, tty, file);
-	if (err)
-		ipack_put_carrier(ipoctal->dev);
+	res = tty_standard_install(driver, tty);
+	if (res)
+		goto err_put_carrier;
+
+	tty->driver_data = channel;
+
+	return 0;
+
+err_put_carrier:
+	ipack_put_carrier(ipoctal->dev);
+
+	return res;
+}
+
+static int ipoctal_open(struct tty_struct *tty, struct file *file)
+{
+	struct ipoctal_channel *channel = tty->driver_data;
 
-	return err;
+	return tty_port_open(&channel->tty_port, tty, file);
 }
 
 static void ipoctal_reset_stats(struct ipoctal_stats *stats)

@@ -661,6 +673,7 @@ static void ipoctal_cleanup(struct tty_struct *tty)
 
 static const struct tty_operations ipoctal_fops = {
 	.ioctl =		NULL,
+	.install =		ipoctal_install,
 	.open =			ipoctal_open,
 	.close =		ipoctal_close,
 	.write =		ipoctal_write_tty,
Signature · 1 line
2.32.0
author
#6 lore raw

PATCH 6/6 ipack: ipoctal: rename tty-driver pointer

In reply to Johan Hovold

The name "tty" is typically used for pointers to struct tty_struct.

Rename the tty-driver pointer used during registration to something more apt to improve readability.

Signed-off-by Johan Hovold <[email protected]>
---
 drivers/ipack/devices/ipoctal.c | 48 ++++++++++++++++-----------------
 1 file changed, 24 insertions(+), 24 deletions(-)
drivers/ipack/devices/ipoctal.c +24 -24
index c709861198e5..20d2b9ec1227 100644
@@ -276,7 +276,7 @@ static int ipoctal_inst_slot(struct ipoctal *ipoctal, unsigned int bus_nr,
 {
 	int res;
 	int i;
-	struct tty_driver *tty;
+	struct tty_driver *drv;
 	struct ipoctal_channel *channel;
 	struct ipack_region *region;
 	void __iomem *addr;

@@ -359,38 +359,38 @@ static int ipoctal_inst_slot(struct ipoctal *ipoctal, unsigned int bus_nr,
 	/* Register the TTY device */
 
 	/* Each IP-OCTAL channel is a TTY port */
-	tty = tty_alloc_driver(NR_CHANNELS, TTY_DRIVER_REAL_RAW |
+	drv = tty_alloc_driver(NR_CHANNELS, TTY_DRIVER_REAL_RAW |
 			TTY_DRIVER_DYNAMIC_DEV);
-	if (IS_ERR(tty))
-		return PTR_ERR(tty);
+	if (IS_ERR(drv))
+		return PTR_ERR(drv);
 
 	/* Fill struct tty_driver with ipoctal data */
-	tty->owner = THIS_MODULE;
-	tty->driver_name = KBUILD_MODNAME;
-	tty->name = kasprintf(GFP_KERNEL, KBUILD_MODNAME ".%d.%d.", bus_nr, slot);
-	if (!tty->name) {
+	drv->owner = THIS_MODULE;
+	drv->driver_name = KBUILD_MODNAME;
+	drv->name = kasprintf(GFP_KERNEL, KBUILD_MODNAME ".%d.%d.", bus_nr, slot);
+	if (!drv->name) {
 		res = -ENOMEM;
 		goto err_put_driver;
 	}
-	tty->major = 0;
-
-	tty->minor_start = 0;
-	tty->type = TTY_DRIVER_TYPE_SERIAL;
-	tty->subtype = SERIAL_TYPE_NORMAL;
-	tty->init_termios = tty_std_termios;
-	tty->init_termios.c_cflag = B9600 | CS8 | CREAD | HUPCL | CLOCAL;
-	tty->init_termios.c_ispeed = 9600;
-	tty->init_termios.c_ospeed = 9600;
-
-	tty_set_operations(tty, &ipoctal_fops);
-	res = tty_register_driver(tty);
+	drv->major = 0;
+
+	drv->minor_start = 0;
+	drv->type = TTY_DRIVER_TYPE_SERIAL;
+	drv->subtype = SERIAL_TYPE_NORMAL;
+	drv->init_termios = tty_std_termios;
+	drv->init_termios.c_cflag = B9600 | CS8 | CREAD | HUPCL | CLOCAL;
+	drv->init_termios.c_ispeed = 9600;
+	drv->init_termios.c_ospeed = 9600;
+
+	tty_set_operations(drv, &ipoctal_fops);
+	res = tty_register_driver(drv);
 	if (res) {
 		dev_err(&ipoctal->dev->dev, "Can't register tty driver.\n");
 		goto err_free_name;
 	}
 
 	/* Save struct tty_driver for use it when uninstalling the device */
-	ipoctal->tty_drv = tty;
+	ipoctal->tty_drv = drv;
 
 	for (i = 0; i < NR_CHANNELS; i++) {
 		struct device *tty_dev;

@@ -407,7 +407,7 @@ static int ipoctal_inst_slot(struct ipoctal *ipoctal, unsigned int bus_nr,
 		spin_lock_init(&channel->lock);
 		channel->pointer_read = 0;
 		channel->pointer_write = 0;
-		tty_dev = tty_port_register_device_attr(&channel->tty_port, tty,
+		tty_dev = tty_port_register_device_attr(&channel->tty_port, drv,
 							i, NULL, channel, NULL);
 		if (IS_ERR(tty_dev)) {
 			dev_err(&ipoctal->dev->dev, "Failed to register tty device.\n");

@@ -429,9 +429,9 @@ static int ipoctal_inst_slot(struct ipoctal *ipoctal, unsigned int bus_nr,
 	return 0;
 
 err_free_name:
-	kfree(tty->name);
+	kfree(drv->name);
 err_put_driver:
-	tty_driver_kref_put(tty);
+	tty_driver_kref_put(drv);
 
 	return res;
 }
Signature · 1 line
2.32.0
Acked-by on the v1 series
#7 lore raw
In reply to Johan Hovold

Hi Johan,

Thanks for the patch series!

Patch series is,

Acked-by Samuel Iglesias Gonsalvez <[email protected]>

Greg, Would you mind picking this patch series through your char-misc tree?

Thanks!

Sam

Quoted 20 lines · Johan Hovold

This series fixes a stack info leak and a number of other bugs all found
through inspection. Included is also a related cleanup.

Note that the series has only been compile tested.

Johan

Johan Hovold (6):
  ipack: ipoctal: fix stack information leak
  ipack: ipoctal: fix tty registration race
  ipack: ipoctal: fix tty-registration error handling
  ipack: ipoctal: fix missing allocation-failure check
  ipack: ipoctal: fix module reference leak
  ipack: ipoctal: rename tty-driver pointer

 drivers/ipack/devices/ipoctal.c | 101 +++++++++++++++++++++-----------

 1 file changed, 66 insertions(+), 35 deletions(-)
maintainer
#8 lore raw
Quoted 10 lines · Samuel Iglesias Gonsálvez

Hi Johan,

Thanks for the patch series!

Patch series is,

Acked-by Samuel Iglesias Gonsalvez <[email protected]>

Greg, Would you mind picking this patch series through your char-misc tree?

Now done.

I'll wait to take patch 6/6 after the first 5 go into Linus's tree as that one is not needed for 5.15-final.

thanks,

greg k-h