[PATCH] serial: core: don't kfree device managed data

[PATCH] serial: core: don't kfree device managed data

[Dan Carpenter]

The put_device() function will call serial_base_ctrl_release() or
serial_base_port_release() so these kfrees() are a double free bug.

Fixes: 84a9582fd203 ("serial: core: Start managing serial controllers to enable runtime PM")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
 drivers/tty/serial/serial_base_bus.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/serial/serial_base_bus.c b/drivers/tty/serial/serial_base_bus.c
index 1b37833b8f66..f125d16f3d16 100644
--- a/drivers/tty/serial/serial_base_bus.c
+++ b/drivers/tty/serial/serial_base_bus.c
@@ -98,7 +98,7 @@ struct serial_ctrl_device *serial_base_ctrl_add(struct uart_port *port,
 				      serial_base_ctrl_release,
 				      port->ctrl_id);
 	if (err)
-		goto err_free_ctrl_dev;
+		goto err_put_device;
 
 	err = device_add(&ctrl_dev->dev);
 	if (err)
@@ -108,8 +108,6 @@ struct serial_ctrl_device *serial_base_ctrl_add(struct uart_port *port,
 
 err_put_device:
 	put_device(&ctrl_dev->dev);
-err_free_ctrl_dev:
-	kfree(ctrl_dev);
 
 	return ERR_PTR(err);
 }
@@ -140,7 +138,7 @@ struct serial_port_device *serial_base_port_add(struct uart_port *port,
 				      serial_base_port_release,
 				      port->line);
 	if (err)
-		goto err_free_port_dev;
+		goto err_put_device;
 
 	port_dev->port = port;
 
@@ -152,8 +150,6 @@ struct serial_port_device *serial_base_port_add(struct uart_port *port,
 
 err_put_device:
 	put_device(&port_dev->dev);
-err_free_port_dev:
-	kfree(port_dev);
 
 	return ERR_PTR(err);
 }
-- 
2.39.2

Re: [PATCH] serial: core: don't kfree device managed data

[Tony Lindgren]

* Dan Carpenter <dan.carpenter@linaro.org> [230606 08:26]:
> The put_device() function will call serial_base_ctrl_release() or
> serial_base_port_release() so these kfrees() are a double free bug.
> 
> Fixes: 84a9582fd203 ("serial: core: Start managing serial controllers to enable runtime PM")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>

Thanks for catching it:

Reviewed-by: Tony Lindgren <tony@atomide.com>

Re: [PATCH] serial: core: don't kfree device managed data

[Andy Shevchenko]

On Tue, Jun 06, 2023 at 11:26:25AM +0300, Dan Carpenter wrote:
> The put_device() function will call serial_base_ctrl_release() or
> serial_base_port_release() so these kfrees() are a double free bug.

...

These labels are also called without device being even added.
So, this is not good enough as far as I can tell.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] serial: core: don't kfree device managed data

[Andy Shevchenko]

On Tue, Jun 06, 2023 at 03:28:43PM +0300, Tony Lindgren wrote:
> * Dan Carpenter <dan.carpenter@linaro.org> [230606 08:26]:
> > The put_device() function will call serial_base_ctrl_release() or
> > serial_base_port_release() so these kfrees() are a double free bug.
> > 
> > Fixes: 84a9582fd203 ("serial: core: Start managing serial controllers to enable runtime PM")
> > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> 
> Thanks for catching it:
> 
> Reviewed-by: Tony Lindgren <tony@atomide.com>

Be careful, we can now get memory leaks :-)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] serial: core: don't kfree device managed data

[Tony Lindgren]

* Andy Shevchenko <andriy.shevchenko@linux.intel.com> [230606 13:16]:
> On Tue, Jun 06, 2023 at 11:26:25AM +0300, Dan Carpenter wrote:
> > The put_device() function will call serial_base_ctrl_release() or
> > serial_base_port_release() so these kfrees() are a double free bug.
> 
> ...
> 
> These labels are also called without device being even added.
> So, this is not good enough as far as I can tell.

I guess you mean the possibe error returned from the call to
serial_base_device_init()?

If serial_base_device_init() fails, we return error and end up doing
the put_device().

We have serial_base_device_init() call device_initialize(), is that
not enough for put_device()?

Regards,

Tony

Re: [PATCH] serial: core: don't kfree device managed data

[Dan Carpenter]

On Tue, Jun 06, 2023 at 04:16:48PM +0300, Andy Shevchenko wrote:
> On Tue, Jun 06, 2023 at 11:26:25AM +0300, Dan Carpenter wrote:
> > The put_device() function will call serial_base_ctrl_release() or
> > serial_base_port_release() so these kfrees() are a double free bug.
> 
> ...
> 
> These labels are also called without device being even added.
> So, this is not good enough as far as I can tell.
> 

put_device() matches with the device_initialize() in
serial_base_device_init().  If we wanted to undo a device_add() the
function is device_del().

I originally wrote this patch last week but only resent it now because
of an issue with my mail set up.  I see that serial_base_device_init()
has actually changed and there is an issue now where the -EPROBE_DEFER
path leaks.

I think making callers handle -EPROBE_DEFER as a special case would be
an ongoing source of bugs.  So really I'd prefer something like this:

regards,
dan carpenter

diff --git a/drivers/tty/serial/serial_base_bus.c b/drivers/tty/serial/serial_base_bus.c
index 9354af7c11af..15fa0576d362 100644
--- a/drivers/tty/serial/serial_base_bus.c
+++ b/drivers/tty/serial/serial_base_bus.c
@@ -50,17 +50,17 @@ static int serial_base_device_init(struct uart_port *port,
 				   void (*release)(struct device *dev),
 				   int id)
 {
-	if (!serial_base_initialized) {
-		dev_err(port->dev, "uart_add_one_port() called before arch_initcall()?\n");
-		return -EPROBE_DEFER;
-	}
-
 	device_initialize(dev);
 	dev->type = type;
 	dev->parent = parent_dev;
 	dev->bus = &serial_base_bus_type;
 	dev->release = release;
 
+	if (!serial_base_initialized) {
+		dev_err(port->dev, "uart_add_one_port() called before arch_initcall()?\n");
+		return -EPROBE_DEFER;
+	}
+
 	return dev_set_name(dev, "%s.%s.%d", type->name, dev_name(port->dev), id);
 }
 

Re: [PATCH] serial: core: don't kfree device managed data

[Andy Shevchenko]

On Tue, Jun 06, 2023 at 04:37:49PM +0300, Tony Lindgren wrote:
> * Andy Shevchenko <andriy.shevchenko@linux.intel.com> [230606 13:16]:
> > On Tue, Jun 06, 2023 at 11:26:25AM +0300, Dan Carpenter wrote:
> > > The put_device() function will call serial_base_ctrl_release() or
> > > serial_base_port_release() so these kfrees() are a double free bug.

...

> > These labels are also called without device being even added.
> > So, this is not good enough as far as I can tell.
> 
> I guess you mean the possibe error returned from the call to
> serial_base_device_init()?
> 
> If serial_base_device_init() fails, we return error and end up doing
> the put_device().
> 
> We have serial_base_device_init() call device_initialize(), is that
> not enough for put_device()?

It's not. The error is returned when device release callback is not assigned
yet.

And also just noticed since we return deferred probe, the message there should
be ratelimited or given only _once().

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] serial: core: don't kfree device managed data

[Andy Shevchenko]

On Tue, Jun 06, 2023 at 04:51:13PM +0300, Dan Carpenter wrote:
> On Tue, Jun 06, 2023 at 04:16:48PM +0300, Andy Shevchenko wrote:
> > On Tue, Jun 06, 2023 at 11:26:25AM +0300, Dan Carpenter wrote:
> > > The put_device() function will call serial_base_ctrl_release() or
> > > serial_base_port_release() so these kfrees() are a double free bug.

...

> > These labels are also called without device being even added.
> > So, this is not good enough as far as I can tell.
> 
> put_device() matches with the device_initialize() in
> serial_base_device_init().  If we wanted to undo a device_add() the
> function is device_del().
> 
> I originally wrote this patch last week but only resent it now because
> of an issue with my mail set up.  I see that serial_base_device_init()
> has actually changed and there is an issue now where the -EPROBE_DEFER
> path leaks.
> 
> I think making callers handle -EPROBE_DEFER as a special case would be
> an ongoing source of bugs.  So really I'd prefer something like this:

I'm okay with the above, but it seems at the same time we need to limit the
messages:

	dev_err_once(port->dev, "uart_add_one_port() called before arch_initcall()?\n");


> regards,
> dan carpenter
> 
> diff --git a/drivers/tty/serial/serial_base_bus.c b/drivers/tty/serial/serial_base_bus.c
> index 9354af7c11af..15fa0576d362 100644
> --- a/drivers/tty/serial/serial_base_bus.c
> +++ b/drivers/tty/serial/serial_base_bus.c
> @@ -50,17 +50,17 @@ static int serial_base_device_init(struct uart_port *port,
>  				   void (*release)(struct device *dev),
>  				   int id)
>  {
> -	if (!serial_base_initialized) {
> -		dev_err(port->dev, "uart_add_one_port() called before arch_initcall()?\n");
> -		return -EPROBE_DEFER;
> -	}
> -
>  	device_initialize(dev);
>  	dev->type = type;
>  	dev->parent = parent_dev;
>  	dev->bus = &serial_base_bus_type;
>  	dev->release = release;
>  
> +	if (!serial_base_initialized) {
> +		dev_err(port->dev, "uart_add_one_port() called before arch_initcall()?\n");
> +		return -EPROBE_DEFER;
> +	}
> +
>  	return dev_set_name(dev, "%s.%s.%d", type->name, dev_name(port->dev), id);
>  }
>  
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] serial: core: don't kfree device managed data

[Dan Carpenter]

On Tue, Jun 06, 2023 at 05:55:16PM +0300, Andy Shevchenko wrote:
> 
> I'm okay with the above, but it seems at the same time we need to limit the
> messages:
> 
> 	dev_err_once(port->dev, "uart_add_one_port() called before arch_initcall()?\n");
> 

Yeah.  I would prefer if that was only printed as a debug message.
-EPROBE_DEFER is supposed to be a normal part of the process.

regards,
dan carpenter

Re: [PATCH] serial: core: don't kfree device managed data

[Tony Lindgren]

* Dan Carpenter <dan.carpenter@linaro.org> [230606 15:01]:
> On Tue, Jun 06, 2023 at 05:55:16PM +0300, Andy Shevchenko wrote:
> > 
> > I'm okay with the above, but it seems at the same time we need to limit the
> > messages:
> > 
> > 	dev_err_once(port->dev, "uart_add_one_port() called before arch_initcall()?\n");
> > 
> 
> Yeah.  I would prefer if that was only printed as a debug message.
> -EPROBE_DEFER is supposed to be a normal part of the process.

Debug here should do the trick yeah.

Regards,

Tony

Re: [PATCH] serial: core: don't kfree device managed data

[Tony Lindgren]

* Andy Shevchenko <andriy.shevchenko@linux.intel.com> [230606 14:54]:
> On Tue, Jun 06, 2023 at 04:37:49PM +0300, Tony Lindgren wrote:
> > * Andy Shevchenko <andriy.shevchenko@linux.intel.com> [230606 13:16]:
> > > On Tue, Jun 06, 2023 at 11:26:25AM +0300, Dan Carpenter wrote:
> > > > The put_device() function will call serial_base_ctrl_release() or
> > > > serial_base_port_release() so these kfrees() are a double free bug.
> 
> ...
> 
> > > These labels are also called without device being even added.
> > > So, this is not good enough as far as I can tell.
> > 
> > I guess you mean the possibe error returned from the call to
> > serial_base_device_init()?
> > 
> > If serial_base_device_init() fails, we return error and end up doing
> > the put_device().
> > 
> > We have serial_base_device_init() call device_initialize(), is that
> > not enough for put_device()?
> 
> It's not. The error is returned when device release callback is not assigned
> yet.

OK thanks for clarifying.

> And also just noticed since we return deferred probe, the message there should
> be ratelimited or given only _once().

We should be OK with debug here like Dan suggested.

Regards,

Tony

Re: [PATCH] serial: core: don't kfree device managed data

[Tony Lindgren]

* Andy Shevchenko <andriy.shevchenko@linux.intel.com> [230606 13:17]:
> On Tue, Jun 06, 2023 at 03:28:43PM +0300, Tony Lindgren wrote:
> > * Dan Carpenter <dan.carpenter@linaro.org> [230606 08:26]:
> > > The put_device() function will call serial_base_ctrl_release() or
> > > serial_base_port_release() so these kfrees() are a double free bug.
> > > 
> > > Fixes: 84a9582fd203 ("serial: core: Start managing serial controllers to enable runtime PM")
> > > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> > 
> > Thanks for catching it:
> > 
> > Reviewed-by: Tony Lindgren <tony@atomide.com>
> 
> Be careful, we can now get memory leaks :-)

I'll start doing Reviewed-by-my-hazy-understanding :)

Regards,

Tony

Re: [PATCH] serial: core: don't kfree device managed data

[Tony Lindgren]

Hi Dan,

* Tony Lindgren <tony@atomide.com> [230606 19:18]:
> * Dan Carpenter <dan.carpenter@linaro.org> [230606 15:01]:
> > On Tue, Jun 06, 2023 at 05:55:16PM +0300, Andy Shevchenko wrote:
> > > 
> > > I'm okay with the above, but it seems at the same time we need to limit the
> > > messages:
> > > 
> > > 	dev_err_once(port->dev, "uart_add_one_port() called before arch_initcall()?\n");
> > > 
> > 
> > Yeah.  I would prefer if that was only printed as a debug message.
> > -EPROBE_DEFER is supposed to be a normal part of the process.
> 
> Debug here should do the trick yeah.

Just wondering.. Are you going to send this fix or do you want me to type
it up?

Regards,

Tony

Re: [PATCH] serial: core: don't kfree device managed data

[Dan Carpenter]

On Wed, Jun 14, 2023 at 07:46:07AM +0300, Tony Lindgren wrote:
> Hi Dan,
> 
> * Tony Lindgren <tony@atomide.com> [230606 19:18]:
> > * Dan Carpenter <dan.carpenter@linaro.org> [230606 15:01]:
> > > On Tue, Jun 06, 2023 at 05:55:16PM +0300, Andy Shevchenko wrote:
> > > > 
> > > > I'm okay with the above, but it seems at the same time we need to limit the
> > > > messages:
> > > > 
> > > > 	dev_err_once(port->dev, "uart_add_one_port() called before arch_initcall()?\n");
> > > > 
> > > 
> > > Yeah.  I would prefer if that was only printed as a debug message.
> > > -EPROBE_DEFER is supposed to be a normal part of the process.
> > 
> > Debug here should do the trick yeah.
> 
> Just wondering.. Are you going to send this fix or do you want me to type
> it up?

I've sent it now.  Thanks!

regards,
dan carpenter