* [PATCH 1/6] ipack: ipoctal: fix stack information leak
2021-09-17 11:46 [PATCH 0/6] ipack: ipoctal: fix info leak and other bugs Johan Hovold
@ 2021-09-17 11:46 ` Johan Hovold
2021-09-17 11:46 ` [PATCH 2/6] ipack: ipoctal: fix tty registration race Johan Hovold
` (5 subsequent siblings)
6 siblings, 0 replies; 9+ messages in thread
From: Johan Hovold @ 2021-09-17 11:46 UTC (permalink / raw)
To: Samuel Iglesias Gonsalvez, Jens Taprogge, Greg Kroah-Hartman
Cc: industrypack-devel, linux-kernel, Johan Hovold, stable
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: stable@vger.kernel.org # 3.5
Signed-off-by: Johan Hovold <johan@kernel.org>
---
drivers/ipack/devices/ipoctal.c | 19 ++++++++++++++-----
1 file changed, 14 insertions(+), 5 deletions(-)
diff --git a/drivers/ipack/devices/ipoctal.c b/drivers/ipack/devices/ipoctal.c
index c14e65a5d38f..c62fec75987c 100644
--- a/drivers/ipack/devices/ipoctal.c
+++ b/drivers/ipack/devices/ipoctal.c
@@ -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);
}
--
2.32.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 2/6] ipack: ipoctal: fix tty registration race
2021-09-17 11:46 [PATCH 0/6] ipack: ipoctal: fix info leak and other bugs Johan Hovold
2021-09-17 11:46 ` [PATCH 1/6] ipack: ipoctal: fix stack information leak Johan Hovold
@ 2021-09-17 11:46 ` Johan Hovold
2021-09-17 11:46 ` [PATCH 3/6] ipack: ipoctal: fix tty-registration error handling Johan Hovold
` (4 subsequent siblings)
6 siblings, 0 replies; 9+ messages in thread
From: Johan Hovold @ 2021-09-17 11:46 UTC (permalink / raw)
To: Samuel Iglesias Gonsalvez, Jens Taprogge, Greg Kroah-Hartman
Cc: industrypack-devel, linux-kernel, Johan Hovold, stable
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: stable@vger.kernel.org # 3.7
Signed-off-by: Johan Hovold <johan@kernel.org>
---
drivers/ipack/devices/ipoctal.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/ipack/devices/ipoctal.c b/drivers/ipack/devices/ipoctal.c
index c62fec75987c..262451343127 100644
--- a/drivers/ipack/devices/ipoctal.c
+++ b/drivers/ipack/devices/ipoctal.c
@@ -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);
}
/*
--
2.32.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 3/6] ipack: ipoctal: fix tty-registration error handling
2021-09-17 11:46 [PATCH 0/6] ipack: ipoctal: fix info leak and other bugs Johan Hovold
2021-09-17 11:46 ` [PATCH 1/6] ipack: ipoctal: fix stack information leak Johan Hovold
2021-09-17 11:46 ` [PATCH 2/6] ipack: ipoctal: fix tty registration race Johan Hovold
@ 2021-09-17 11:46 ` Johan Hovold
2021-09-17 11:46 ` [PATCH 4/6] ipack: ipoctal: fix missing allocation-failure check Johan Hovold
` (3 subsequent siblings)
6 siblings, 0 replies; 9+ messages in thread
From: Johan Hovold @ 2021-09-17 11:46 UTC (permalink / raw)
To: Samuel Iglesias Gonsalvez, Jens Taprogge, Greg Kroah-Hartman
Cc: industrypack-devel, linux-kernel, Johan Hovold, stable
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: stable@vger.kernel.org # 3.7
Signed-off-by: Johan Hovold <johan@kernel.org>
---
drivers/ipack/devices/ipoctal.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/ipack/devices/ipoctal.c b/drivers/ipack/devices/ipoctal.c
index 262451343127..d6875aa6a295 100644
--- a/drivers/ipack/devices/ipoctal.c
+++ b/drivers/ipack/devices/ipoctal.c
@@ -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);
--
2.32.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 4/6] ipack: ipoctal: fix missing allocation-failure check
2021-09-17 11:46 [PATCH 0/6] ipack: ipoctal: fix info leak and other bugs Johan Hovold
` (2 preceding siblings ...)
2021-09-17 11:46 ` [PATCH 3/6] ipack: ipoctal: fix tty-registration error handling Johan Hovold
@ 2021-09-17 11:46 ` Johan Hovold
2021-09-17 11:46 ` [PATCH 5/6] ipack: ipoctal: fix module reference leak Johan Hovold
` (2 subsequent siblings)
6 siblings, 0 replies; 9+ messages in thread
From: Johan Hovold @ 2021-09-17 11:46 UTC (permalink / raw)
To: Samuel Iglesias Gonsalvez, Jens Taprogge, Greg Kroah-Hartman
Cc: industrypack-devel, linux-kernel, Johan Hovold, stable
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: stable@vger.kernel.org # 3.5
Signed-off-by: Johan Hovold <johan@kernel.org>
---
drivers/ipack/devices/ipoctal.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/ipack/devices/ipoctal.c b/drivers/ipack/devices/ipoctal.c
index d6875aa6a295..61c41f535510 100644
--- a/drivers/ipack/devices/ipoctal.c
+++ b/drivers/ipack/devices/ipoctal.c
@@ -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);
--
2.32.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 5/6] ipack: ipoctal: fix module reference leak
2021-09-17 11:46 [PATCH 0/6] ipack: ipoctal: fix info leak and other bugs Johan Hovold
` (3 preceding siblings ...)
2021-09-17 11:46 ` [PATCH 4/6] ipack: ipoctal: fix missing allocation-failure check Johan Hovold
@ 2021-09-17 11:46 ` Johan Hovold
2021-09-17 11:46 ` [PATCH 6/6] ipack: ipoctal: rename tty-driver pointer Johan Hovold
2021-09-22 5:49 ` [PATCH 0/6] ipack: ipoctal: fix info leak and other bugs Samuel Iglesias Gonsálvez
6 siblings, 0 replies; 9+ messages in thread
From: Johan Hovold @ 2021-09-17 11:46 UTC (permalink / raw)
To: Samuel Iglesias Gonsalvez, Jens Taprogge, Greg Kroah-Hartman
Cc: industrypack-devel, linux-kernel, Johan Hovold, stable, Federico Vaga
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: stable@vger.kernel.org # 3.18
Cc: Federico Vaga <federico.vaga@cern.ch>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
drivers/ipack/devices/ipoctal.c | 29 +++++++++++++++++++++--------
1 file changed, 21 insertions(+), 8 deletions(-)
diff --git a/drivers/ipack/devices/ipoctal.c b/drivers/ipack/devices/ipoctal.c
index 61c41f535510..c709861198e5 100644
--- a/drivers/ipack/devices/ipoctal.c
+++ b/drivers/ipack/devices/ipoctal.c
@@ -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,
--
2.32.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 6/6] ipack: ipoctal: rename tty-driver pointer
2021-09-17 11:46 [PATCH 0/6] ipack: ipoctal: fix info leak and other bugs Johan Hovold
` (4 preceding siblings ...)
2021-09-17 11:46 ` [PATCH 5/6] ipack: ipoctal: fix module reference leak Johan Hovold
@ 2021-09-17 11:46 ` Johan Hovold
2021-09-22 5:49 ` [PATCH 0/6] ipack: ipoctal: fix info leak and other bugs Samuel Iglesias Gonsálvez
6 siblings, 0 replies; 9+ messages in thread
From: Johan Hovold @ 2021-09-17 11:46 UTC (permalink / raw)
To: Samuel Iglesias Gonsalvez, Jens Taprogge, Greg Kroah-Hartman
Cc: industrypack-devel, linux-kernel, 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 <johan@kernel.org>
---
drivers/ipack/devices/ipoctal.c | 48 ++++++++++++++++-----------------
1 file changed, 24 insertions(+), 24 deletions(-)
diff --git a/drivers/ipack/devices/ipoctal.c b/drivers/ipack/devices/ipoctal.c
index c709861198e5..20d2b9ec1227 100644
--- a/drivers/ipack/devices/ipoctal.c
+++ b/drivers/ipack/devices/ipoctal.c
@@ -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;
}
--
2.32.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH 0/6] ipack: ipoctal: fix info leak and other bugs
2021-09-17 11:46 [PATCH 0/6] ipack: ipoctal: fix info leak and other bugs Johan Hovold
` (5 preceding siblings ...)
2021-09-17 11:46 ` [PATCH 6/6] ipack: ipoctal: rename tty-driver pointer Johan Hovold
@ 2021-09-22 5:49 ` Samuel Iglesias Gonsálvez
2021-09-27 15:40 ` Greg Kroah-Hartman
6 siblings, 1 reply; 9+ messages in thread
From: Samuel Iglesias Gonsálvez @ 2021-09-22 5:49 UTC (permalink / raw)
To: Johan Hovold, Jens Taprogge, Greg Kroah-Hartman
Cc: industrypack-devel, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 970 bytes --]
Hi Johan,
Thanks for the patch series!
Patch series is,
Acked-by: Samuel Iglesias Gonsalvez <siglesias@igalia.com>
Greg, Would you mind picking this patch series through your char-misc
tree?
Thanks!
Sam
On Fri, 2021-09-17 at 13:46 +0200, Johan Hovold wrote:
> 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(-)
>
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 0/6] ipack: ipoctal: fix info leak and other bugs
2021-09-22 5:49 ` [PATCH 0/6] ipack: ipoctal: fix info leak and other bugs Samuel Iglesias Gonsálvez
@ 2021-09-27 15:40 ` Greg Kroah-Hartman
0 siblings, 0 replies; 9+ messages in thread
From: Greg Kroah-Hartman @ 2021-09-27 15:40 UTC (permalink / raw)
To: Samuel Iglesias Gonsálvez
Cc: Johan Hovold, Jens Taprogge, industrypack-devel, linux-kernel
On Wed, Sep 22, 2021 at 07:49:45AM +0200, Samuel Iglesias Gonsálvez wrote:
> Hi Johan,
>
> Thanks for the patch series!
>
> Patch series is,
>
> Acked-by: Samuel Iglesias Gonsalvez <siglesias@igalia.com>
>
> 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
^ permalink raw reply [flat|nested] 9+ messages in thread