[PATCH v3 01/15] media: i2c: ds90ub9x3: Fix extra fwnode_handle_put()

Tomi Valkeinen posted 15 patches 1 year ago
There is a newer version of this series
[PATCH v3 01/15] media: i2c: ds90ub9x3: Fix extra fwnode_handle_put()
Posted by Tomi Valkeinen 1 year ago
The ub913 and ub953 drivers call fwnode_handle_put(priv->sd.fwnode) as
part of their remove process, and if the driver is removed multiple
times, eventually leads to put "overflow", possibly causing memory
corruption or crash.

The fwnode_handle_put() is a leftover from commit 905f88ccebb1 ("media:
i2c: ds90ub9x3: Fix sub-device matching"), which changed the code
related to the sd.fwnode, but missed removing these fwnode_handle_put()
calls.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Cc: stable@vger.kernel.org
Fixes: 905f88ccebb1 ("media: i2c: ds90ub9x3: Fix sub-device matching")
---
 drivers/media/i2c/ds90ub913.c | 1 -
 drivers/media/i2c/ds90ub953.c | 1 -
 2 files changed, 2 deletions(-)

diff --git a/drivers/media/i2c/ds90ub913.c b/drivers/media/i2c/ds90ub913.c
index 8eed4a200fd8..b5375d736629 100644
--- a/drivers/media/i2c/ds90ub913.c
+++ b/drivers/media/i2c/ds90ub913.c
@@ -793,7 +793,6 @@ static void ub913_subdev_uninit(struct ub913_data *priv)
 	v4l2_async_unregister_subdev(&priv->sd);
 	ub913_v4l2_nf_unregister(priv);
 	v4l2_subdev_cleanup(&priv->sd);
-	fwnode_handle_put(priv->sd.fwnode);
 	media_entity_cleanup(&priv->sd.entity);
 }
 
diff --git a/drivers/media/i2c/ds90ub953.c b/drivers/media/i2c/ds90ub953.c
index 16f88db14981..10daecf6f457 100644
--- a/drivers/media/i2c/ds90ub953.c
+++ b/drivers/media/i2c/ds90ub953.c
@@ -1291,7 +1291,6 @@ static void ub953_subdev_uninit(struct ub953_data *priv)
 	v4l2_async_unregister_subdev(&priv->sd);
 	ub953_v4l2_notifier_unregister(priv);
 	v4l2_subdev_cleanup(&priv->sd);
-	fwnode_handle_put(priv->sd.fwnode);
 	media_entity_cleanup(&priv->sd.entity);
 }
 

-- 
2.43.0
Re: [PATCH v3 01/15] media: i2c: ds90ub9x3: Fix extra fwnode_handle_put()
Posted by Sakari Ailus 1 year ago
Huomenta,

On Wed, Dec 04, 2024 at 01:05:15PM +0200, Tomi Valkeinen wrote:
> The ub913 and ub953 drivers call fwnode_handle_put(priv->sd.fwnode) as
> part of their remove process, and if the driver is removed multiple
> times, eventually leads to put "overflow", possibly causing memory

This is, in fact, an extra put. It'll lead to underflow, not overflow.
I'd expect removing it once would be already too much.

> corruption or crash.
> 
> The fwnode_handle_put() is a leftover from commit 905f88ccebb1 ("media:
> i2c: ds90ub9x3: Fix sub-device matching"), which changed the code
> related to the sd.fwnode, but missed removing these fwnode_handle_put()
> calls.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> Cc: stable@vger.kernel.org
> Fixes: 905f88ccebb1 ("media: i2c: ds90ub9x3: Fix sub-device matching")
> ---
>  drivers/media/i2c/ds90ub913.c | 1 -
>  drivers/media/i2c/ds90ub953.c | 1 -
>  2 files changed, 2 deletions(-)
> 
> diff --git a/drivers/media/i2c/ds90ub913.c b/drivers/media/i2c/ds90ub913.c
> index 8eed4a200fd8..b5375d736629 100644
> --- a/drivers/media/i2c/ds90ub913.c
> +++ b/drivers/media/i2c/ds90ub913.c
> @@ -793,7 +793,6 @@ static void ub913_subdev_uninit(struct ub913_data *priv)
>  	v4l2_async_unregister_subdev(&priv->sd);
>  	ub913_v4l2_nf_unregister(priv);
>  	v4l2_subdev_cleanup(&priv->sd);
> -	fwnode_handle_put(priv->sd.fwnode);
>  	media_entity_cleanup(&priv->sd.entity);
>  }
>  
> diff --git a/drivers/media/i2c/ds90ub953.c b/drivers/media/i2c/ds90ub953.c
> index 16f88db14981..10daecf6f457 100644
> --- a/drivers/media/i2c/ds90ub953.c
> +++ b/drivers/media/i2c/ds90ub953.c
> @@ -1291,7 +1291,6 @@ static void ub953_subdev_uninit(struct ub953_data *priv)
>  	v4l2_async_unregister_subdev(&priv->sd);
>  	ub953_v4l2_notifier_unregister(priv);
>  	v4l2_subdev_cleanup(&priv->sd);
> -	fwnode_handle_put(priv->sd.fwnode);
>  	media_entity_cleanup(&priv->sd.entity);
>  }
>  
> 

-- 
Terveisin,

Sakari Ailus
Re: [PATCH v3 01/15] media: i2c: ds90ub9x3: Fix extra fwnode_handle_put()
Posted by Tomi Valkeinen 1 year ago
Hi,

On 09/12/2024 11:09, Sakari Ailus wrote:
> Huomenta,
> 
> On Wed, Dec 04, 2024 at 01:05:15PM +0200, Tomi Valkeinen wrote:
>> The ub913 and ub953 drivers call fwnode_handle_put(priv->sd.fwnode) as
>> part of their remove process, and if the driver is removed multiple
>> times, eventually leads to put "overflow", possibly causing memory
> 
> This is, in fact, an extra put. It'll lead to underflow, not overflow.

Well, there are too many puts, so "put overflow" =). I don't think 
underflow is the right word here either, but it's probably better than 
overflow. Maybe I'll reword it so that it doesn't have a flow at all.

> I'd expect removing it once would be already too much.

True, but there's something keeping some refs to the fwnode even without 
these drivers (otherwise they'd be freed when these drivers are not 
around), so the issue shows only when those refs are taken out by the 
puts in these drivers.

  Tomi

> 
>> corruption or crash.
>>
>> The fwnode_handle_put() is a leftover from commit 905f88ccebb1 ("media:
>> i2c: ds90ub9x3: Fix sub-device matching"), which changed the code
>> related to the sd.fwnode, but missed removing these fwnode_handle_put()
>> calls.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> Cc: stable@vger.kernel.org
>> Fixes: 905f88ccebb1 ("media: i2c: ds90ub9x3: Fix sub-device matching")
>> ---
>>   drivers/media/i2c/ds90ub913.c | 1 -
>>   drivers/media/i2c/ds90ub953.c | 1 -
>>   2 files changed, 2 deletions(-)
>>
>> diff --git a/drivers/media/i2c/ds90ub913.c b/drivers/media/i2c/ds90ub913.c
>> index 8eed4a200fd8..b5375d736629 100644
>> --- a/drivers/media/i2c/ds90ub913.c
>> +++ b/drivers/media/i2c/ds90ub913.c
>> @@ -793,7 +793,6 @@ static void ub913_subdev_uninit(struct ub913_data *priv)
>>   	v4l2_async_unregister_subdev(&priv->sd);
>>   	ub913_v4l2_nf_unregister(priv);
>>   	v4l2_subdev_cleanup(&priv->sd);
>> -	fwnode_handle_put(priv->sd.fwnode);
>>   	media_entity_cleanup(&priv->sd.entity);
>>   }
>>   
>> diff --git a/drivers/media/i2c/ds90ub953.c b/drivers/media/i2c/ds90ub953.c
>> index 16f88db14981..10daecf6f457 100644
>> --- a/drivers/media/i2c/ds90ub953.c
>> +++ b/drivers/media/i2c/ds90ub953.c
>> @@ -1291,7 +1291,6 @@ static void ub953_subdev_uninit(struct ub953_data *priv)
>>   	v4l2_async_unregister_subdev(&priv->sd);
>>   	ub953_v4l2_notifier_unregister(priv);
>>   	v4l2_subdev_cleanup(&priv->sd);
>> -	fwnode_handle_put(priv->sd.fwnode);
>>   	media_entity_cleanup(&priv->sd.entity);
>>   }
>>   
>>
>
Re: [PATCH v3 01/15] media: i2c: ds90ub9x3: Fix extra fwnode_handle_put()
Posted by Sakari Ailus 1 year ago
Moi,

On Mon, Dec 09, 2024 at 11:46:45AM +0200, Tomi Valkeinen wrote:
> Hi,
> 
> On 09/12/2024 11:09, Sakari Ailus wrote:
> > Huomenta,
> > 
> > On Wed, Dec 04, 2024 at 01:05:15PM +0200, Tomi Valkeinen wrote:
> > > The ub913 and ub953 drivers call fwnode_handle_put(priv->sd.fwnode) as
> > > part of their remove process, and if the driver is removed multiple
> > > times, eventually leads to put "overflow", possibly causing memory
> > 
> > This is, in fact, an extra put. It'll lead to underflow, not overflow.
> 
> Well, there are too many puts, so "put overflow" =). I don't think underflow
> is the right word here either, but it's probably better than overflow. Maybe
> I'll reword it so that it doesn't have a flow at all.

Sound good.

> 
> > I'd expect removing it once would be already too much.
> 
> True, but there's something keeping some refs to the fwnode even without
> these drivers (otherwise they'd be freed when these drivers are not around),
> so the issue shows only when those refs are taken out by the puts in these
> drivers.

Port nodes perhaps?

-- 
Terveisin,

Sakari Ailus