[PATCH 0/3] serial: liteuart: fix compile testing and driver unbind

[PATCH 0/3] serial: liteuart: fix compile testing and driver unbind

[Johan Hovold]

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(-)

-- 
2.32.0

[PATCH 1/3] serial: liteuart: fix compile testing

[Johan Hovold]

Allow the liteuart driver to be compile tested by fixing the broken
Kconfig dependencies.

Fixes: 1da81e5562fa ("drivers/tty/serial: add LiteUART driver")
Cc: stable@vger.kernel.org	# 5.11
Cc: Filip Kokosinski <fkokosinski@antmicro.com>
Cc: Mateusz Holenko <mholenko@antmicro.com>
Cc: Stafford Horne <shorne@gmail.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/tty/serial/Kconfig | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
index 6ff94cfcd9db..67de892e0947 100644
--- a/drivers/tty/serial/Kconfig
+++ b/drivers/tty/serial/Kconfig
@@ -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
-- 
2.32.0

Re: [PATCH 1/3] serial: liteuart: fix compile testing

[Stafford Horne]

On Mon, Nov 15, 2021 at 02:37:43PM +0100, Johan Hovold wrote:
> Allow the liteuart driver to be compile tested by fixing the broken
> Kconfig dependencies.
> 
> Fixes: 1da81e5562fa ("drivers/tty/serial: add LiteUART driver")
> Cc: stable@vger.kernel.org	# 5.11
> Cc: Filip Kokosinski <fkokosinski@antmicro.com>
> Cc: Mateusz Holenko <mholenko@antmicro.com>
> Cc: Stafford Horne <shorne@gmail.com>
> Signed-off-by: Johan Hovold <johan@kernel.org>

Reviewed-by: Stafford Horne <shorne@gmail.com>

> ---
>  drivers/tty/serial/Kconfig | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
> index 6ff94cfcd9db..67de892e0947 100644
> --- a/drivers/tty/serial/Kconfig
> +++ b/drivers/tty/serial/Kconfig
> @@ -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
> -- 
> 2.32.0
> 

Re: [PATCH 1/3] serial: liteuart: fix compile testing

[Andy Shevchenko]

On Mon, Nov 15, 2021 at 3:44 PM Johan Hovold <johan@kernel.org> wrote:
>
> Allow the liteuart driver to be compile tested by fixing the broken
> Kconfig dependencies.

...

>  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

AFAICS this is optional and prevents compile testing in some cases.

>         select SERIAL_CORE
>         help
>           This driver is for the FPGA-based LiteUART serial controller from LiteX


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/3] serial: liteuart: fix compile testing

[Johan Hovold]

On Tue, Nov 16, 2021 at 05:44:14PM +0200, Andy Shevchenko wrote:
> On Mon, Nov 15, 2021 at 3:44 PM Johan Hovold <johan@kernel.org> wrote:
> >
> > Allow the liteuart driver to be compile tested by fixing the broken
> > Kconfig dependencies.
> 
> ...
> 
> >  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
> 
> 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.

> >         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

[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.

Fixes: 1da81e5562fa ("drivers/tty/serial: add LiteUART driver")
Cc: stable@vger.kernel.org      # 5.11
Cc: Filip Kokosinski <fkokosinski@antmicro.com>
Cc: Mateusz Holenko <mholenko@antmicro.com>
Cc: Stafford Horne <shorne@gmail.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/tty/serial/liteuart.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
index f075f4ff5fcf..da792d0df790 100644
--- a/drivers/tty/serial/liteuart.c
+++ b/drivers/tty/serial/liteuart.c
@@ -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;
-- 
2.32.0

Re: [PATCH 2/3] serial: liteuart: fix use-after-free and memleak on unbind

[Stafford Horne]

On Mon, Nov 15, 2021 at 02:37:44PM +0100, Johan Hovold wrote:
> 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?

> Fixes: 1da81e5562fa ("drivers/tty/serial: add LiteUART driver")
> Cc: stable@vger.kernel.org      # 5.11
> Cc: Filip Kokosinski <fkokosinski@antmicro.com>
> Cc: Mateusz Holenko <mholenko@antmicro.com>
> Cc: Stafford Horne <shorne@gmail.com>
> Signed-off-by: Johan Hovold <johan@kernel.org>

Reviewed-by: Stafford Horne <shorne@gmail.com>

> ---
>  drivers/tty/serial/liteuart.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
> index f075f4ff5fcf..da792d0df790 100644
> --- a/drivers/tty/serial/liteuart.c
> +++ b/drivers/tty/serial/liteuart.c
> @@ -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;
> -- 
> 2.32.0
> 

Re: [PATCH 2/3] serial: liteuart: fix use-after-free and memleak on unbind

[Johan Hovold]

On Tue, Nov 16, 2021 at 06:15:05AM +0900, Stafford Horne wrote:
> On Mon, Nov 15, 2021 at 02:37:44PM +0100, Johan Hovold wrote:
> > 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.

> > Fixes: 1da81e5562fa ("drivers/tty/serial: add LiteUART driver")
> > Cc: stable@vger.kernel.org      # 5.11
> > Cc: Filip Kokosinski <fkokosinski@antmicro.com>
> > Cc: Mateusz Holenko <mholenko@antmicro.com>
> > Cc: Stafford Horne <shorne@gmail.com>
> > Signed-off-by: Johan Hovold <johan@kernel.org>
> 
> Reviewed-by: Stafford Horne <shorne@gmail.com>

Thanks for reviewing.

Johan

[PATCH 3/3] serial: liteuart: fix minor-number leak on probe errors

[Johan Hovold]

Make sure to release the allocated minor number before returning on
probe errors.

Fixes: 1da81e5562fa ("drivers/tty/serial: add LiteUART driver")
Cc: stable@vger.kernel.org      # 5.11
Cc: Filip Kokosinski <fkokosinski@antmicro.com>
Cc: Mateusz Holenko <mholenko@antmicro.com>
Cc: Stafford Horne <shorne@gmail.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/tty/serial/liteuart.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
index da792d0df790..2941659e5274 100644
--- a/drivers/tty/serial/liteuart.c
+++ b/drivers/tty/serial/liteuart.c
@@ -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)
-- 
2.32.0

Re: [PATCH 3/3] serial: liteuart: fix minor-number leak on probe errors

[Stafford Horne]

On Mon, Nov 15, 2021 at 02:37:45PM +0100, Johan Hovold wrote:
> Make sure to release the allocated minor number before returning on
> probe errors.
> 
> Fixes: 1da81e5562fa ("drivers/tty/serial: add LiteUART driver")
> Cc: stable@vger.kernel.org      # 5.11
> Cc: Filip Kokosinski <fkokosinski@antmicro.com>
> Cc: Mateusz Holenko <mholenko@antmicro.com>
> Cc: Stafford Horne <shorne@gmail.com>
> Signed-off-by: Johan Hovold <johan@kernel.org>

Reviewed-by: Stafford Horne <shorne@gmail.com>

> ---
>  drivers/tty/serial/liteuart.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
> index da792d0df790..2941659e5274 100644
> --- a/drivers/tty/serial/liteuart.c
> +++ b/drivers/tty/serial/liteuart.c
> @@ -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)
> -- 
> 2.32.0
>