[PATCH v2] media: v4l2-fwnode: Fix subdev owner overwritten in v4l2_async_register_subdev_sensor()

[PATCH v2] media: v4l2-fwnode: Fix subdev owner overwritten in v4l2_async_register_subdev_sensor()

[Mirela Rabulea]

The v4l2 helper v4l2_async_register_subdev_sensor() calls
v4l2_async_register_subdev(), which is a macro that expands to
__v4l2_async_register_subdev(sd,THIS_MODULE). Since the macro is expanded
inside v4l2-fwnode.c, THIS_MODULE resolves to the v4l2-fwnode module
rather than the sensor driver module that originally set sd->owner. When
v4l2-fwnode is built-in, THIS_MODULE evaluates to NULL, which then
overwrites the sensor driver's owner with NULL.

This causes the problem that the sensor module's reference count is never
incremented during async registration, so the module can be removed while
the subdevice is still in use by a notifier (e.g., a CSI-2 receiver
bridge driver).

Fix this by renaming v4l2_async_register_subdev_sensor() to
__v4l2_async_register_subdev_sensor() with an added explicit module
argument and introducing a wrapper macro:
    #define v4l2_async_register_subdev_sensor(sd) \
        __v4l2_async_register_subdev_sensor(sd, THIS_MODULE)

This ensures the sensor driver module is properly referenced even when
the sensor driver does not init the owner field before calling
v4l2_async_register_subdev_sensor() and prevents premature module removal.

Fixes: aef69d54755d ("media: v4l: fwnode: Add a convenience function for registering sensors")
Suggested-by: Frank Li <Frank.Li@nxp.com>
Link: https://lore.kernel.org/linux-media/20240315073125.275501-2-sakari.ailus@linux.intel.com/
Signed-off-by: Mirela Rabulea <mirela.rabulea@nxp.com>
---

Changes in v2:
	Do not rely on sd->owner set by v4l2_i2c_subdev_init(), introduce v4l2_async_register_subdev_sensor wrapper macro	
	Added Suggested-by: Frank Li <Frank.Li@nxp.com>
	Added Link: to Sakari's similar commit for v4l2_async_register_subdev macro

The v1 patch is also valid, as from what I see, all sensor drivers that use
v4l2_async_register_subdev_sensor also use v4l2_i2c_subdev_init(), which
sets sd->owner


 drivers/media/v4l2-core/v4l2-fwnode.c | 6 +++---
 include/media/v4l2-async.h            | 4 +++-
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
index 77f3298821b5..62a3a452f788 100644
--- a/drivers/media/v4l2-core/v4l2-fwnode.c
+++ b/drivers/media/v4l2-core/v4l2-fwnode.c
@@ -1256,7 +1256,7 @@ v4l2_async_nf_parse_fwnode_sensor(struct device *dev,
 	return 0;
 }
 
-int v4l2_async_register_subdev_sensor(struct v4l2_subdev *sd)
+int __v4l2_async_register_subdev_sensor(struct v4l2_subdev *sd, struct module *module)
 {
 	struct v4l2_async_notifier *notifier;
 	int ret;
@@ -1282,7 +1282,7 @@ int v4l2_async_register_subdev_sensor(struct v4l2_subdev *sd)
 	if (ret < 0)
 		goto out_cleanup;
 
-	ret = v4l2_async_register_subdev(sd);
+	ret = __v4l2_async_register_subdev(sd, module);
 	if (ret < 0)
 		goto out_unregister;
 
@@ -1300,7 +1300,7 @@ int v4l2_async_register_subdev_sensor(struct v4l2_subdev *sd)
 
 	return ret;
 }
-EXPORT_SYMBOL_GPL(v4l2_async_register_subdev_sensor);
+EXPORT_SYMBOL_GPL(__v4l2_async_register_subdev_sensor);
 
 MODULE_DESCRIPTION("V4L2 fwnode binding parsing library");
 MODULE_LICENSE("GPL");
diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
index f26c323e9c96..54a2d9620ed5 100644
--- a/include/media/v4l2-async.h
+++ b/include/media/v4l2-async.h
@@ -333,8 +333,10 @@ int __v4l2_async_register_subdev(struct v4l2_subdev *sd, struct module *module);
  * An error is returned if the module is no longer loaded on any attempts
  * to register it.
  */
+#define v4l2_async_register_subdev_sensor(sd) \
+	__v4l2_async_register_subdev_sensor(sd, THIS_MODULE)
 int __must_check
-v4l2_async_register_subdev_sensor(struct v4l2_subdev *sd);
+__v4l2_async_register_subdev_sensor(struct v4l2_subdev *sd, struct module *module);
 
 /**
  * v4l2_async_unregister_subdev - unregisters a sub-device to the asynchronous
-- 
2.43.0

Re: [PATCH v2] media: v4l2-fwnode: Fix subdev owner overwritten in v4l2_async_register_subdev_sensor()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] The `v4l2_flash_init()` and `v4l2_flash_indicator_init()` helpers incorrectly attribute the flash subdevice owner to the `v4l2-flash-led-class` module instead of the calling LED driver, leading to a Use-After-Free vulnerability upon premature module unloading.
- [High] Error handling in `__v4l2_async_register_subdev()` fails to unbind previously matched connections within the `while` loop, leaking them and causing a Use-After-Free when the subdevice is freed.
--

commit 6f9889016345e2094c47d91f08c20902a0dee478
Author: Mirela Rabulea <mirela.rabulea@nxp.com>

media: v4l2-fwnode: Fix subdev owner overwritten in v4l2_async_register_subdev_sensor()

The v4l2 helper v4l2_async_register_subdev_sensor() calls
v4l2_async_register_subdev(), which expands to pass THIS_MODULE. Since
this happens inside v4l2-fwnode.c, it incorrectly assigns the v4l2-fwnode
module as the owner instead of the sensor driver module. This prevents
proper reference counting and allows premature module removal.

Link: https://lore.kernel.org/linux-media/20240315073125.275501-2-sakari.ailus@linux.intel.com/

> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> index 77f3298821b5c..62a3a452f7884 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c

[ ... ]

> @@ -1282,7 +1282,7 @@ int v4l2_async_register_subdev_sensor(struct v4l2_subdev *sd)
>  	if (ret < 0)
>  		goto out_cleanup;
>  
> -	ret = v4l2_async_register_subdev(sd);
> +	ret = __v4l2_async_register_subdev(sd, module);

[Severity: High]
This is a pre-existing issue, but does __v4l2_async_register_subdev() fail to
unbind previously matched connections within its match loop if a later
connection fails?

If a subdevice successfully binds its first connection and moves it to the
notifier's done_list, but encounters an error on a subsequent iteration of the
while loop:

drivers/media/v4l2-core/v4l2-async.c:__v4l2_async_register_subdev() {
    ...
		while ((asc = v4l2_async_find_match(notifier, sd))) {
			ret = v4l2_async_match_notify(notifier, v4l2_dev, sd,
						      asc);
			if (ret)
				goto err_unlock;
    ...
}

The error paths appear to only clean up the current iteration's connection or
perform no cleanup at all.

Would this leave the first connection dangling in the bridge notifier's
done_list with asc->sd still pointing to the subdevice? If the registration
returns an error, the sensor driver might free the subdevice, leading to a
use-after-free later when the bridge driver unregisters its notifier.

>  	if (ret < 0)
>  		goto out_unregister;
>  
> diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
> index f26c323e9c963..54a2d9620ed5b 100644
> --- a/include/media/v4l2-async.h
> +++ b/include/media/v4l2-async.h

[ ... ]

> @@ -333,8 +333,10 @@ int __v4l2_async_register_subdev(struct v4l2_subdev *sd, struct module *module);
>   * An error is returned if the module is no longer loaded on any attempts
>   * to register it.
>   */
> +#define v4l2_async_register_subdev_sensor(sd) \
> +	__v4l2_async_register_subdev_sensor(sd, THIS_MODULE)

[Severity: High]
This is a pre-existing issue, but does the exact same ownership problem fixed
here also exist for v4l2_flash_init() and v4l2_flash_indicator_init()?

Both functions are compiled into the v4l2-flash-led-class module and internally
call v4l2_async_register_subdev(sd). Since the macro evaluates to THIS_MODULE,
would they capture the v4l2-flash-led-class module as the owner instead of the
calling LED driver (e.g., leds-tps6131x)?

Could this allow a user to unload the LED driver module while the subdevice
file descriptor remains open, leading to a use-after-free when interacting
with or closing the freed subdevice?

>  int __must_check
> -__v4l2_async_register_subdev_sensor(struct v4l2_subdev *sd);
> +__v4l2_async_register_subdev_sensor(struct v4l2_subdev *sd, struct module *module);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260522143120.690330-1-mirela.rabulea@nxp.com?part=1

Re: [PATCH v2] media: v4l2-fwnode: Fix subdev owner overwritten in v4l2_async_register_subdev_sensor()

[Frank Li]

On Fri, May 22, 2026 at 05:31:20PM +0300, Mirela Rabulea wrote:
> The v4l2 helper v4l2_async_register_subdev_sensor() calls
> v4l2_async_register_subdev(), which is a macro that expands to
> __v4l2_async_register_subdev(sd,THIS_MODULE). Since the macro is expanded
> inside v4l2-fwnode.c, THIS_MODULE resolves to the v4l2-fwnode module
> rather than the sensor driver module that originally set sd->owner. When
> v4l2-fwnode is built-in, THIS_MODULE evaluates to NULL, which then
> overwrites the sensor driver's owner with NULL.
>
> This causes the problem that the sensor module's reference count is never
> incremented during async registration, so the module can be removed while
> the subdevice is still in use by a notifier (e.g., a CSI-2 receiver
> bridge driver).
>
> Fix this by renaming v4l2_async_register_subdev_sensor() to
> __v4l2_async_register_subdev_sensor() with an added explicit module
> argument and introducing a wrapper macro:
>     #define v4l2_async_register_subdev_sensor(sd) \
>         __v4l2_async_register_subdev_sensor(sd, THIS_MODULE)
>
> This ensures the sensor driver module is properly referenced even when
> the sensor driver does not init the owner field before calling
> v4l2_async_register_subdev_sensor() and prevents premature module removal.
>
> Fixes: aef69d54755d ("media: v4l: fwnode: Add a convenience function for registering sensors")
> Suggested-by: Frank Li <Frank.Li@nxp.com>
> Link: https://lore.kernel.org/linux-media/20240315073125.275501-2-sakari.ailus@linux.intel.com/
> Signed-off-by: Mirela Rabulea <mirela.rabulea@nxp.com>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>
> Changes in v2:
> 	Do not rely on sd->owner set by v4l2_i2c_subdev_init(), introduce v4l2_async_register_subdev_sensor wrapper macro
> 	Added Suggested-by: Frank Li <Frank.Li@nxp.com>
> 	Added Link: to Sakari's similar commit for v4l2_async_register_subdev macro
>
> The v1 patch is also valid, as from what I see, all sensor drivers that use
> v4l2_async_register_subdev_sensor also use v4l2_i2c_subdev_init(), which
> sets sd->owner
>
>
>  drivers/media/v4l2-core/v4l2-fwnode.c | 6 +++---
>  include/media/v4l2-async.h            | 4 +++-
>  2 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> index 77f3298821b5..62a3a452f788 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -1256,7 +1256,7 @@ v4l2_async_nf_parse_fwnode_sensor(struct device *dev,
>  	return 0;
>  }
>
> -int v4l2_async_register_subdev_sensor(struct v4l2_subdev *sd)
> +int __v4l2_async_register_subdev_sensor(struct v4l2_subdev *sd, struct module *module)
>  {
>  	struct v4l2_async_notifier *notifier;
>  	int ret;
> @@ -1282,7 +1282,7 @@ int v4l2_async_register_subdev_sensor(struct v4l2_subdev *sd)
>  	if (ret < 0)
>  		goto out_cleanup;
>
> -	ret = v4l2_async_register_subdev(sd);
> +	ret = __v4l2_async_register_subdev(sd, module);
>  	if (ret < 0)
>  		goto out_unregister;
>
> @@ -1300,7 +1300,7 @@ int v4l2_async_register_subdev_sensor(struct v4l2_subdev *sd)
>
>  	return ret;
>  }
> -EXPORT_SYMBOL_GPL(v4l2_async_register_subdev_sensor);
> +EXPORT_SYMBOL_GPL(__v4l2_async_register_subdev_sensor);
>
>  MODULE_DESCRIPTION("V4L2 fwnode binding parsing library");
>  MODULE_LICENSE("GPL");
> diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
> index f26c323e9c96..54a2d9620ed5 100644
> --- a/include/media/v4l2-async.h
> +++ b/include/media/v4l2-async.h
> @@ -333,8 +333,10 @@ int __v4l2_async_register_subdev(struct v4l2_subdev *sd, struct module *module);
>   * An error is returned if the module is no longer loaded on any attempts
>   * to register it.
>   */
> +#define v4l2_async_register_subdev_sensor(sd) \
> +	__v4l2_async_register_subdev_sensor(sd, THIS_MODULE)
>  int __must_check
> -v4l2_async_register_subdev_sensor(struct v4l2_subdev *sd);
> +__v4l2_async_register_subdev_sensor(struct v4l2_subdev *sd, struct module *module);
>
>  /**
>   * v4l2_async_unregister_subdev - unregisters a sub-device to the asynchronous
> --
> 2.43.0
>

Re: [PATCH v2] media: v4l2-fwnode: Fix subdev owner overwritten in v4l2_async_register_subdev_sensor()

[Laurent Pinchart]

Hi Mirela,

Thank you for the patch.

On Fri, May 22, 2026 at 05:31:20PM +0300, Mirela Rabulea wrote:
> The v4l2 helper v4l2_async_register_subdev_sensor() calls
> v4l2_async_register_subdev(), which is a macro that expands to
> __v4l2_async_register_subdev(sd,THIS_MODULE). Since the macro is expanded
> inside v4l2-fwnode.c, THIS_MODULE resolves to the v4l2-fwnode module
> rather than the sensor driver module that originally set sd->owner. When
> v4l2-fwnode is built-in, THIS_MODULE evaluates to NULL, which then
> overwrites the sensor driver's owner with NULL.
> 
> This causes the problem that the sensor module's reference count is never
> incremented during async registration, so the module can be removed while
> the subdevice is still in use by a notifier (e.g., a CSI-2 receiver
> bridge driver).
> 
> Fix this by renaming v4l2_async_register_subdev_sensor() to
> __v4l2_async_register_subdev_sensor() with an added explicit module
> argument and introducing a wrapper macro:
>     #define v4l2_async_register_subdev_sensor(sd) \
>         __v4l2_async_register_subdev_sensor(sd, THIS_MODULE)
> 
> This ensures the sensor driver module is properly referenced even when
> the sensor driver does not init the owner field before calling
> v4l2_async_register_subdev_sensor() and prevents premature module removal.
> 
> Fixes: aef69d54755d ("media: v4l: fwnode: Add a convenience function for registering sensors")
> Suggested-by: Frank Li <Frank.Li@nxp.com>
> Link: https://lore.kernel.org/linux-media/20240315073125.275501-2-sakari.ailus@linux.intel.com/
> Signed-off-by: Mirela Rabulea <mirela.rabulea@nxp.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

> ---
> 
> Changes in v2:
> 	Do not rely on sd->owner set by v4l2_i2c_subdev_init(), introduce v4l2_async_register_subdev_sensor wrapper macro	
> 	Added Suggested-by: Frank Li <Frank.Li@nxp.com>
> 	Added Link: to Sakari's similar commit for v4l2_async_register_subdev macro
> 
> The v1 patch is also valid, as from what I see, all sensor drivers that use
> v4l2_async_register_subdev_sensor also use v4l2_i2c_subdev_init(), which
> sets sd->owner
> 
> 
>  drivers/media/v4l2-core/v4l2-fwnode.c | 6 +++---
>  include/media/v4l2-async.h            | 4 +++-
>  2 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> index 77f3298821b5..62a3a452f788 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -1256,7 +1256,7 @@ v4l2_async_nf_parse_fwnode_sensor(struct device *dev,
>  	return 0;
>  }
>  
> -int v4l2_async_register_subdev_sensor(struct v4l2_subdev *sd)
> +int __v4l2_async_register_subdev_sensor(struct v4l2_subdev *sd, struct module *module)
>  {
>  	struct v4l2_async_notifier *notifier;
>  	int ret;
> @@ -1282,7 +1282,7 @@ int v4l2_async_register_subdev_sensor(struct v4l2_subdev *sd)
>  	if (ret < 0)
>  		goto out_cleanup;
>  
> -	ret = v4l2_async_register_subdev(sd);
> +	ret = __v4l2_async_register_subdev(sd, module);
>  	if (ret < 0)
>  		goto out_unregister;
>  
> @@ -1300,7 +1300,7 @@ int v4l2_async_register_subdev_sensor(struct v4l2_subdev *sd)
>  
>  	return ret;
>  }
> -EXPORT_SYMBOL_GPL(v4l2_async_register_subdev_sensor);
> +EXPORT_SYMBOL_GPL(__v4l2_async_register_subdev_sensor);
>  
>  MODULE_DESCRIPTION("V4L2 fwnode binding parsing library");
>  MODULE_LICENSE("GPL");
> diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
> index f26c323e9c96..54a2d9620ed5 100644
> --- a/include/media/v4l2-async.h
> +++ b/include/media/v4l2-async.h
> @@ -333,8 +333,10 @@ int __v4l2_async_register_subdev(struct v4l2_subdev *sd, struct module *module);
>   * An error is returned if the module is no longer loaded on any attempts
>   * to register it.
>   */
> +#define v4l2_async_register_subdev_sensor(sd) \
> +	__v4l2_async_register_subdev_sensor(sd, THIS_MODULE)
>  int __must_check
> -v4l2_async_register_subdev_sensor(struct v4l2_subdev *sd);
> +__v4l2_async_register_subdev_sensor(struct v4l2_subdev *sd, struct module *module);
>  
>  /**
>   * v4l2_async_unregister_subdev - unregisters a sub-device to the asynchronous

-- 
Regards,

Laurent Pinchart