serial: liteuart: fix compile testing and driver unbind
Subsystems (3)
cover serial: liteuart: fix compile testing and driver unbind
Ilia Sergachev noted that the liteuart remove() function would trigger a NULL-pointer dereference if it was ever called since the driver data pointer was never initialised.
Turns out there are more bugs in this part of the driver which clearly has never been tested.
Also fix up the Kconfig dependencies so that the driver can actually be compile tested.
Note that this series depends on the fix by Ilia:
https://lore.kernel.org/r/20211115031808.7ab632ef@dtkw
Johan
Johan Hovold (3):
serial: liteuart: fix compile testing
serial: liteuart: fix use-after-free and memleak on unbind
serial: liteuart: fix minor-number leak on probe errors
drivers/tty/serial/Kconfig | 4 ++-- drivers/tty/serial/liteuart.c | 18 +++++++++++++++--- 2 files changed, 17 insertions(+), 5 deletions(-)
Signature · 1 line
2.32.0
PATCH 1/3 serial: liteuart: fix compile testing
Allow the liteuart driver to be compile tested by fixing the broken Kconfig dependencies.
--- drivers/tty/serial/Kconfig | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/tty/serial/Kconfig +2 -2
@@ -1531,9 +1531,9 @@ config SERIAL_MILBEAUT_USIO_CONSOLE config SERIAL_LITEUART tristate "LiteUART serial port support" + depends on LITEX || COMPILE_TEST depends on HAS_IOMEM - depends on OF || COMPILE_TEST - depends on LITEX + depends on OF select SERIAL_CORE help This driver is for the FPGA-based LiteUART serial controller from LiteX
Signature · 1 line
2.32.0
Re: [PATCH 1/3] serial: liteuart: fix compile testing
Quoted 9 lines · Johan Hovold
Allow the liteuart driver to be compile tested by fixing the broken
Kconfigdependencies.Fixes 1da81e5562fa ("drivers/tty/serial: add LiteUART driver")Cc [email protected] # 5.11Cc Filip Kokosinski <[email protected]>Cc Mateusz Holenko <[email protected]>Cc Stafford Horne <[email protected]>Signed-off-by Johan Hovold <[email protected]>
Quoted 23 lines
--- drivers/tty/serial/Kconfig | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)drivers/tty/serial/Kconfig +2
-2@@ -1531,9 +1531,9 @@ config SERIAL_MILBEAUT_USIO_CONSOLE config SERIAL_LITEUART tristate "LiteUART serial port support" + depends on LITEX || COMPILE_TEST depends on HAS_IOMEM - depends on OF || COMPILE_TEST - depends on LITEX + depends on OF select SERIAL_CORE help This driver is for the FPGA-based LiteUART serial controller from LiteXSignature · 1 line
2.32.0
Re: [PATCH 1/3] serial: liteuart: fix compile testing
In reply to Johan HovoldQuoted 3 lines · Johan Hovold
Allow the liteuart driver to be compile tested by fixing the broken
Kconfigdependencies.
...
drivers/tty/serial/Kconfig config SERIAL_MILBEAUT_USIO_CONSOLE · line 1536
Quoted 8 lines
config
SERIAL_LITEUART
tristate "LiteUART serial port support"drivers/tty/serial/Kconfig +2
-2+ depends on LITEX || COMPILE_TEST depends on HAS_IOMEM - depends on OF || COMPILE_TEST - depends on LITEX + depends on OF
AFAICS this is optional and prevents compile testing in some cases.
Quoted 3 lines
select
SERIAL_CORE
help
This driver is for the FPGA-based LiteUART serial controller from LiteX
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH 1/3] serial: liteuart: fix compile testing
Quoted 14 lines · Andy Shevchenko
Quoted 3 lines · Johan Hovold
Allow the liteuart driver to be compile tested by fixing the broken
Kconfigdependencies....
Quoted 6 lines
config
SERIAL_LITEUART
tristate "LiteUART serial port support"drivers/tty/serial/Kconfig +1
-2+ depends on LITEX || COMPILE_TEST depends on HAS_IOMEM - depends on OF || COMPILE_TEST - depends on LITEX
Quoted 3 lines
Quoted 1 line
+ depends on OF
AFAICS this is optional and prevents compile testing in some cases.
Yeah, you're right; that clause should stay. I'll send a v2. Thanks.
Quoted 3 lines
Quoted 3 lines
select
SERIAL_CORE
help
This driver is for the FPGA-based LiteUART serial controller from LiteX
Johan
PATCH 2/3 serial: liteuart: fix use-after-free and memleak on unbind
In reply to Johan HovoldDeregister the port when unbinding the driver to prevent it from being used after releasing the driver data and leaking memory allocated by serial core.
--- drivers/tty/serial/liteuart.c | 1 + 1 file changed, 1 insertion(+)
drivers/tty/serial/liteuart.c +1 -0
@@ -295,6 +295,7 @@ static int liteuart_remove(struct platform_device *pdev)struct uart_port *port =platform_get_drvdata (pdev);struct liteuart_port *uart =to_liteuart_port (port); +uart_remove_one_port (&liteuart_driver, port);xa_erase (&liteuart_array, uart->id);return 0 ;
Signature · 1 line
2.32.0
Re: [PATCH 2/3] serial: liteuart: fix use-after-free and memleak on unbind
Quoted 3 lines · Johan Hovold
Deregister the port when unbinding the driver to prevent it from being used after releasing the driver data and leaking memory allocated by serial core.
This looks good to me. Just curious did you test this on a Litex SoC/FPGA?
Quoted 6 lines
Fixes 1da81e5562fa ("drivers/tty/serial: add LiteUART driver")Cc [email protected] # 5.11Cc Filip Kokosinski <[email protected]>Cc Mateusz Holenko <[email protected]>Cc Stafford Horne <[email protected]>Signed-off-by Johan Hovold <[email protected]>
Quoted 19 lines
--- drivers/tty/serial/liteuart.c | 1 + 1 file changed, 1 insertion(+)drivers/tty/serial/liteuart.c +1
-0@@ -295,6 +295,7 @@ static int liteuart_remove(struct platform_device *pdev)struct uart_port *port =platform_get_drvdata (pdev);struct liteuart_port *uart =to_liteuart_port (port); +uart_remove_one_port (&liteuart_driver, port);xa_erase (&liteuart_array, uart->id);return 0 ;Signature · 1 line
2.32.0
Re: [PATCH 2/3] serial: liteuart: fix use-after-free and memleak on unbind
Quoted 7 lines · Stafford Horne
Quoted 3 lines · Johan Hovold
Deregister the port when unbinding the driver to prevent it from being used after releasing the driver data and leaking memory allocated by serial core.
This looks good to me. Just curious did you test this on a Litex SoC/FPGA?
No, this series has only been compile tested.
Quoted 8 lines
Quoted 6 lines
Fixes 1da81e5562fa ("drivers/tty/serial: add LiteUART driver")Cc [email protected] # 5.11Cc Filip Kokosinski <[email protected]>Cc Mateusz Holenko <[email protected]>Cc Stafford Horne <[email protected]>Signed-off-by Johan Hovold <[email protected]>Reviewed-by Stafford Horne <[email protected]>
Thanks for reviewing.
Johan
PATCH 3/3 serial: liteuart: fix minor-number leak on probe errors
In reply to Johan HovoldMake sure to release the allocated minor number before returning on probe errors.
--- drivers/tty/serial/liteuart.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-)
drivers/tty/serial/liteuart.c +14 -3
@@ -270,8 +270,10 @@ static int liteuart_probe(struct platform_device *pdev)/* get membase */ port->membase =devm_platform_get_and_ioremap_resource (pdev,0 ,NULL ); -if (IS_ERR (port->membase)) -return PTR_ERR (port->membase); +if (IS_ERR (port->membase)) { + ret =PTR_ERR (port->membase); + goto err_erase_id; + }/* values not from device tree */ port->dev = &pdev->dev;
@@ -287,7 +289,16 @@ static int liteuart_probe(struct platform_device *pdev)platform_set_drvdata (pdev, port); -return uart_add_one_port (&liteuart_driver, &uart->port); + ret =uart_add_one_port (&liteuart_driver, &uart->port); +if (ret) + goto err_erase_id; + +return 0 ; + +err_erase_id: +xa_erase (&liteuart_array, uart->id); + +return ret; }static int liteuart_remove (struct platform_device *pdev)
Signature · 1 line
2.32.0
Re: [PATCH 3/3] serial: liteuart: fix minor-number leak on probe errors
Quoted 9 lines · Johan Hovold
Make sure to release the allocated minor number before returning on probe errors.
Fixes 1da81e5562fa ("drivers/tty/serial: add LiteUART driver")Cc [email protected] # 5.11Cc Filip Kokosinski <[email protected]>Cc Mateusz Holenko <[email protected]>Cc Stafford Horne <[email protected]>Signed-off-by Johan Hovold <[email protected]>
Quoted 42 lines
--- drivers/tty/serial/liteuart.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-)drivers/tty/serial/liteuart.c +14
-3@@ -270,8 +270,10 @@ static int liteuart_probe(struct platform_device *pdev)/* get membase */ port->membase =devm_platform_get_and_ioremap_resource (pdev,0 ,NULL ); -if (IS_ERR (port->membase)) -return PTR_ERR (port->membase); +if (IS_ERR (port->membase)) { + ret =PTR_ERR (port->membase); + goto err_erase_id; + }/* values not from device tree */ port->dev = &pdev->dev;@@ -287,7 +289,16 @@ static int liteuart_probe(struct platform_device *pdev)platform_set_drvdata (pdev, port); -return uart_add_one_port (&liteuart_driver, &uart->port); + ret =uart_add_one_port (&liteuart_driver, &uart->port); +if (ret) + goto err_erase_id; + +return 0 ; + +err_erase_id: +xa_erase (&liteuart_array, uart->id); + +return ret; }static int liteuart_remove (struct platform_device *pdev)Signature · 1 line
2.32.0