media: v4l: async: Fix NULL pointer when v4l2 flash subdev binding

Commit Message

ChiYuan Huang May 8, 2024, 2:51 a.m. UTC

From: ChiYuan Huang <[e-mail]>

In v4l2_async_create_ancillary_links(), if v4l2 async notifier is
created from v4l2 device, the v4l2 flash subdev async binding will enter
the logic to create media link. Due to the subdev of notifier is NULL,
this will cause NULL pointer to access the subdev entity. Therefore, add
the check to bypass it.

Fixes: aa4faf6eb271 ("media: v4l2-async: Create links during v4l2_async_match_notify()")
Signed-off-by: ChiYuan Huang <[e-mail]>
---
Hi,

  I'm trying to bind the v4l2 subdev for flashlight testing. It seems
some logic in v4l2 asynd binding is incorrect.

From the change, I modified vim2m as the test driver to bind mt6370 flashlight.

Here's the backtrace log.

 vim2m soc:vim2m: bound [white:flash-2]
 Unable to handle kernel NULL pointer dereference at virtual address 0000000000000058
 ......skipping
 Call trace:
  media_create_ancillary_link+0x48/0xd8 [mc]
  v4l2_async_match_notify+0x17c/0x208 [v4l2_async]
  v4l2_async_register_subdev+0xb8/0x1d0 [v4l2_async]
  __v4l2_flash_init.part.0+0x3b4/0x4b0 [v4l2_flash_led_class]
  v4l2_flash_init+0x28/0x48 [v4l2_flash_led_class]
  mt6370_led_probe+0x348/0x690 [leds_mt6370_flash]

After tracing the code, it will let the subdev labeled as F_LENS or
F_FLASH function to create media link. To prevent the NULL pointer
issue, the simplest way is add a check when 'n->sd' is NULL and bypass
the later media link creataion.
---
 drivers/media/v4l2-core/v4l2-async.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Sakari Ailus May 16, 2024, 10:42 a.m. UTC | #1

Hi Chi Yuan,

On Wed, May 08, 2024 at 10:51:49AM +0800, [e-mail] wrote:
> From: ChiYuan Huang <[e-mail]>
> 
> In v4l2_async_create_ancillary_links(), if v4l2 async notifier is
> created from v4l2 device, the v4l2 flash subdev async binding will enter
> the logic to create media link. Due to the subdev of notifier is NULL,
> this will cause NULL pointer to access the subdev entity. Therefore, add
> the check to bypass it.
> 
> Fixes: aa4faf6eb271 ("media: v4l2-async: Create links during v4l2_async_match_notify()")
> Signed-off-by: ChiYuan Huang <[e-mail]>
> ---
> Hi,
> 
>   I'm trying to bind the v4l2 subdev for flashlight testing. It seems
> some logic in v4l2 asynd binding is incorrect.
> 
> From the change, I modified vim2m as the test driver to bind mt6370 flashlight.
> 
> Here's the backtrace log.
> 
>  vim2m soc:vim2m: bound [white:flash-2]
>  Unable to handle kernel NULL pointer dereference at virtual address 0000000000000058
>  ......skipping
>  Call trace:
>   media_create_ancillary_link+0x48/0xd8 [mc]
>   v4l2_async_match_notify+0x17c/0x208 [v4l2_async]
>   v4l2_async_register_subdev+0xb8/0x1d0 [v4l2_async]

There's something wrong obviously somewhere but where?

A sub-notifier does have a sub-device after the notifier initialisation.
Maybe the initialisation does not happen in the right order?

>   __v4l2_flash_init.part.0+0x3b4/0x4b0 [v4l2_flash_led_class]
>   v4l2_flash_init+0x28/0x48 [v4l2_flash_led_class]
>   mt6370_led_probe+0x348/0x690 [leds_mt6370_flash]
> 
> After tracing the code, it will let the subdev labeled as F_LENS or
> F_FLASH function to create media link. To prevent the NULL pointer
> issue, the simplest way is add a check when 'n->sd' is NULL and bypass
> the later media link creataion.
> ---
>  drivers/media/v4l2-core/v4l2-async.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index 3ec323bd528b..9d3161c51954 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -324,6 +324,9 @@ static int v4l2_async_create_ancillary_links(struct v4l2_async_notifier *n,
>  	    sd->entity.function != MEDIA_ENT_F_FLASH)
>  		return 0;
>  
> +	if (!n->sd)
> +		return 0;

This isn't the right fix: the ancillary link won't be created as a result.

> +
>  	link = media_create_ancillary_link(&n->sd->entity, &sd->entity);
>  
>  #endif

ChiYuan Huang May 17, 2024, 6:31 a.m. UTC | #2

Hi, Sakari:

	Thanks for your reply.
If any misunderstanding, please correct me.

On Thu, May 16, 2024 at 10:42:05AM +0000, Sakari Ailus wrote:
> Hi Chi Yuan,
> 
> On Wed, May 08, 2024 at 10:51:49AM +0800, [e-mail] wrote:
> > From: ChiYuan Huang <[e-mail]>
> > 
> > In v4l2_async_create_ancillary_links(), if v4l2 async notifier is
> > created from v4l2 device, the v4l2 flash subdev async binding will enter
> > the logic to create media link. Due to the subdev of notifier is NULL,
> > this will cause NULL pointer to access the subdev entity. Therefore, add
> > the check to bypass it.
> > 
> > Fixes: aa4faf6eb271 ("media: v4l2-async: Create links during v4l2_async_match_notify()")
> > Signed-off-by: ChiYuan Huang <[e-mail]>
> > ---
> > Hi,
> > 
> >   I'm trying to bind the v4l2 subdev for flashlight testing. It seems
> > some logic in v4l2 asynd binding is incorrect.
> > 
> > From the change, I modified vim2m as the test driver to bind mt6370 flashlight.
> > 
> > Here's the backtrace log.
> > 
> >  vim2m soc:vim2m: bound [white:flash-2]
> >  Unable to handle kernel NULL pointer dereference at virtual address 0000000000000058
> >  ......skipping
> >  Call trace:
> >   media_create_ancillary_link+0x48/0xd8 [mc]
> >   v4l2_async_match_notify+0x17c/0x208 [v4l2_async]
> >   v4l2_async_register_subdev+0xb8/0x1d0 [v4l2_async]
> 
> There's something wrong obviously somewhere but wherea?
> 
In vim2m driver, I added v4l2_async_nf_init -> v4l2_async_nf_add_fwnode_remote ->
v4l2_async_nf_register.

From the async flow, in notifier complete ops to create v4l-subdevX node for the 
specified subdev.
> A sub-notifier does have a sub-device after the notifier initialisation.

Why? Are you saying to the notifier can only be used for subdev and subdev binding, 
not v4l2 and subdev binding?

But to create v4l-subdevX, the key is only v4l2 device and its needed subdev.

> Maybe the initialisation does not happen in the right order?
AFAIK, Async flow can solve the probe order and makes the user no need to care
the probe order.

From the stacktrace, I'm pretty sure it's not the probe order issue.
> 
> >   __v4l2_flash_init.part.0+0x3b4/0x4b0 [v4l2_flash_led_class]
> >   v4l2_flash_init+0x28/0x48 [v4l2_flash_led_class]
> >   mt6370_led_probe+0x348/0x690 [leds_mt6370_flash]
> > 
> > After tracing the code, it will let the subdev labeled as F_LENS or
> > F_FLASH function to create media link. To prevent the NULL pointer
> > issue, the simplest way is add a check when 'n->sd' is NULL and bypass
> > the later media link creataion.
> > ---
> >  drivers/media/v4l2-core/v4l2-async.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> > index 3ec323bd528b..9d3161c51954 100644
> > --- a/drivers/media/v4l2-core/v4l2-async.c
> > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > @@ -324,6 +324,9 @@ static int v4l2_async_create_ancillary_links(struct v4l2_async_notifier *n,
> >  	    sd->entity.function != MEDIA_ENT_F_FLASH)
> >  		return 0;
> >  
> > +	if (!n->sd)
> > +		return 0;
> 
> This isn't the right fix: the ancillary link won't be created as a result.
> 
Due to the notifier is created by v4l2 device not subdev, this 'n->sd' is NULL.
The NULL 'n->sd' will be referenced by the next flow 'media_create_ancillary_link'.

Or is it caused by the wrong usage? 

> > +
> >  	link = media_create_ancillary_link(&n->sd->entity, &sd->entity);
> >  
> >  #endif
> 
> -- 
> Regards,
> 
> Sakari Ailus

Sakari Ailus May 17, 2024, 8 a.m. UTC | #3

Hi Chi Yuan,

On Fri, May 17, 2024 at 02:31:50PM +0800, ChiYuan Huang wrote:
> Hi, Sakari:
> 
> 	Thanks for your reply.
> If any misunderstanding, please correct me.
> 
> On Thu, May 16, 2024 at 10:42:05AM +0000, Sakari Ailus wrote:
> > Hi Chi Yuan,
> > 
> > On Wed, May 08, 2024 at 10:51:49AM +0800, [e-mail] wrote:
> > > From: ChiYuan Huang <[e-mail]>
> > > 
> > > In v4l2_async_create_ancillary_links(), if v4l2 async notifier is
> > > created from v4l2 device, the v4l2 flash subdev async binding will enter
> > > the logic to create media link. Due to the subdev of notifier is NULL,
> > > this will cause NULL pointer to access the subdev entity. Therefore, add
> > > the check to bypass it.
> > > 
> > > Fixes: aa4faf6eb271 ("media: v4l2-async: Create links during v4l2_async_match_notify()")
> > > Signed-off-by: ChiYuan Huang <[e-mail]>
> > > ---
> > > Hi,
> > > 
> > >   I'm trying to bind the v4l2 subdev for flashlight testing. It seems
> > > some logic in v4l2 asynd binding is incorrect.
> > > 
> > > From the change, I modified vim2m as the test driver to bind mt6370 flashlight.
> > > 
> > > Here's the backtrace log.
> > > 
> > >  vim2m soc:vim2m: bound [white:flash-2]
> > >  Unable to handle kernel NULL pointer dereference at virtual address 0000000000000058
> > >  ......skipping
> > >  Call trace:
> > >   media_create_ancillary_link+0x48/0xd8 [mc]
> > >   v4l2_async_match_notify+0x17c/0x208 [v4l2_async]
> > >   v4l2_async_register_subdev+0xb8/0x1d0 [v4l2_async]
> > 
> > There's something wrong obviously somewhere but wherea?
> > 
> In vim2m driver, I added v4l2_async_nf_init -> v4l2_async_nf_add_fwnode_remote ->
> v4l2_async_nf_register.
> 
> From the async flow, in notifier complete ops to create v4l-subdevX node for the 
> specified subdev.
> > A sub-notifier does have a sub-device after the notifier initialisation.
> 
> Why? Are you saying to the notifier can only be used for subdev and subdev binding, 
> not v4l2 and subdev binding?
> 
> But to create v4l-subdevX, the key is only v4l2 device and its needed subdev.
> 
> > Maybe the initialisation does not happen in the right order?
> AFAIK, Async flow can solve the probe order and makes the user no need to care
> the probe order.
> 
> From the stacktrace, I'm pretty sure it's not the probe order issue.
> > 
> > >   __v4l2_flash_init.part.0+0x3b4/0x4b0 [v4l2_flash_led_class]
> > >   v4l2_flash_init+0x28/0x48 [v4l2_flash_led_class]
> > >   mt6370_led_probe+0x348/0x690 [leds_mt6370_flash]
> > > 
> > > After tracing the code, it will let the subdev labeled as F_LENS or
> > > F_FLASH function to create media link. To prevent the NULL pointer
> > > issue, the simplest way is add a check when 'n->sd' is NULL and bypass
> > > the later media link creataion.
> > > ---
> > >  drivers/media/v4l2-core/v4l2-async.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> > > index 3ec323bd528b..9d3161c51954 100644
> > > --- a/drivers/media/v4l2-core/v4l2-async.c
> > > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > > @@ -324,6 +324,9 @@ static int v4l2_async_create_ancillary_links(struct v4l2_async_notifier *n,
> > >  	    sd->entity.function != MEDIA_ENT_F_FLASH)
> > >  		return 0;
> > >  
> > > +	if (!n->sd)
> > > +		return 0;
> > 
> > This isn't the right fix: the ancillary link won't be created as a result.
> > 
> Due to the notifier is created by v4l2 device not subdev, this 'n->sd' is NULL.
> The NULL 'n->sd' will be referenced by the next flow 'media_create_ancillary_link'.

Ah, right. I took a new look into the code and agree this is a problem.
This probably hasn't been hit previously as the root notifier driver tends
not to have any lens or flash devices.

I'd change the commit message slightly:

--------8<-------------
In v4l2_async_create_ancillary_links(), ancillary links are created for
lens and flash sub-devices. These are sub-device to sub-device links and if
the async notifier is related to a V4L2 device, the source sub-device of
the ancillary link is NULL, leading to a NULL pointer dereference. Check
the notifier's sd field is non-NULL in v4l2_async_create_ancillary_links().
--------8<-------------

> 
> Or is it caused by the wrong usage? 
> 
> > > +
> > >  	link = media_create_ancillary_link(&n->sd->entity, &sd->entity);
> > >  
> > >  #endif
> >

Sakari Ailus May 17, 2024, 8:03 a.m. UTC | #4

On Fri, May 17, 2024 at 08:00:32AM +0000, Sakari Ailus wrote:
> --------8<-------------
> In v4l2_async_create_ancillary_links(), ancillary links are created for
> lens and flash sub-devices. These are sub-device to sub-device links and if
> the async notifier is related to a V4L2 device, the source sub-device of
> the ancillary link is NULL, leading to a NULL pointer dereference. Check
> the notifier's sd field is non-NULL in v4l2_async_create_ancillary_links().
> --------8<-------------

And a slightly different subject, too: "media: v4l: async: Fix NULL pointer
dereference in adding ancillary links".

No need to send v2.

Laurent Pinchart May 17, 2024, 11:19 a.m. UTC | #5

On Fri, May 17, 2024 at 08:00:32AM +0000, Sakari Ailus wrote:
> Hi Chi Yuan,
> 
> On Fri, May 17, 2024 at 02:31:50PM +0800, ChiYuan Huang wrote:
> > Hi, Sakari:
> > 
> > 	Thanks for your reply.
> > If any misunderstanding, please correct me.
> > 
> > On Thu, May 16, 2024 at 10:42:05AM +0000, Sakari Ailus wrote:
> > > Hi Chi Yuan,
> > > 
> > > On Wed, May 08, 2024 at 10:51:49AM +0800, [e-mail] wrote:
> > > > From: ChiYuan Huang <[e-mail]>
> > > > 
> > > > In v4l2_async_create_ancillary_links(), if v4l2 async notifier is
> > > > created from v4l2 device, the v4l2 flash subdev async binding will enter
> > > > the logic to create media link. Due to the subdev of notifier is NULL,
> > > > this will cause NULL pointer to access the subdev entity. Therefore, add
> > > > the check to bypass it.
> > > > 
> > > > Fixes: aa4faf6eb271 ("media: v4l2-async: Create links during v4l2_async_match_notify()")
> > > > Signed-off-by: ChiYuan Huang <[e-mail]>
> > > > ---
> > > > Hi,
> > > > 
> > > >   I'm trying to bind the v4l2 subdev for flashlight testing. It seems
> > > > some logic in v4l2 asynd binding is incorrect.
> > > > 
> > > > From the change, I modified vim2m as the test driver to bind mt6370 flashlight.
> > > > 
> > > > Here's the backtrace log.
> > > > 
> > > >  vim2m soc:vim2m: bound [white:flash-2]
> > > >  Unable to handle kernel NULL pointer dereference at virtual address 0000000000000058
> > > >  ......skipping
> > > >  Call trace:
> > > >   media_create_ancillary_link+0x48/0xd8 [mc]
> > > >   v4l2_async_match_notify+0x17c/0x208 [v4l2_async]
> > > >   v4l2_async_register_subdev+0xb8/0x1d0 [v4l2_async]
> > > 
> > > There's something wrong obviously somewhere but wherea?
> >
> > In vim2m driver, I added v4l2_async_nf_init -> v4l2_async_nf_add_fwnode_remote ->
> > v4l2_async_nf_register.
> > 
> > From the async flow, in notifier complete ops to create v4l-subdevX node for the 
> > specified subdev.
> >
> > > A sub-notifier does have a sub-device after the notifier initialisation.
> > 
> > Why? Are you saying to the notifier can only be used for subdev and subdev binding, 
> > not v4l2 and subdev binding?
> > 
> > But to create v4l-subdevX, the key is only v4l2 device and its needed subdev.
> > 
> > > Maybe the initialisation does not happen in the right order?
> >
> > AFAIK, Async flow can solve the probe order and makes the user no need to care
> > the probe order.
> > 
> > From the stacktrace, I'm pretty sure it's not the probe order issue.
> >
> > > >   __v4l2_flash_init.part.0+0x3b4/0x4b0 [v4l2_flash_led_class]
> > > >   v4l2_flash_init+0x28/0x48 [v4l2_flash_led_class]
> > > >   mt6370_led_probe+0x348/0x690 [leds_mt6370_flash]
> > > > 
> > > > After tracing the code, it will let the subdev labeled as F_LENS or
> > > > F_FLASH function to create media link. To prevent the NULL pointer
> > > > issue, the simplest way is add a check when 'n->sd' is NULL and bypass
> > > > the later media link creataion.
> > > > ---
> > > >  drivers/media/v4l2-core/v4l2-async.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > > 
> > > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> > > > index 3ec323bd528b..9d3161c51954 100644
> > > > --- a/drivers/media/v4l2-core/v4l2-async.c
> > > > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > > > @@ -324,6 +324,9 @@ static int v4l2_async_create_ancillary_links(struct v4l2_async_notifier *n,
> > > >  	    sd->entity.function != MEDIA_ENT_F_FLASH)
> > > >  		return 0;
> > > >  
> > > > +	if (!n->sd)
> > > > +		return 0;
> > > 
> > > This isn't the right fix: the ancillary link won't be created as a result.
> >
> > Due to the notifier is created by v4l2 device not subdev, this 'n->sd' is NULL.
> > The NULL 'n->sd' will be referenced by the next flow 'media_create_ancillary_link'.
> 
> Ah, right. I took a new look into the code and agree this is a problem.
> This probably hasn't been hit previously as the root notifier driver tends
> not to have any lens or flash devices.
> 
> I'd change the commit message slightly:
> 
> --------8<-------------
> In v4l2_async_create_ancillary_links(), ancillary links are created for
> lens and flash sub-devices. These are sub-device to sub-device links and if
> the async notifier is related to a V4L2 device, the source sub-device of
> the ancillary link is NULL, leading to a NULL pointer dereference. Check
> the notifier's sd field is non-NULL in v4l2_async_create_ancillary_links().
> --------8<-------------

What's the use case for including lens or flash devices in the root
notifier ? Shouldn't lens and flash subdevices always be linked to
something ? We should of course not crash, but it seems that simply
ignoring the subdevs and not linking them isn't a great idea either.

> > Or is it caused by the wrong usage? 
> > 
> > > > +
> > > >  	link = media_create_ancillary_link(&n->sd->entity, &sd->entity);
> > > >  
> > > >  #endif

Sakari Ailus May 17, 2024, 11:27 a.m. UTC | #6

Hi Laurent,

On Fri, May 17, 2024 at 02:19:44PM +0300, Laurent Pinchart wrote:
> On Fri, May 17, 2024 at 08:00:32AM +0000, Sakari Ailus wrote:
> > Hi Chi Yuan,
> > 
> > On Fri, May 17, 2024 at 02:31:50PM +0800, ChiYuan Huang wrote:
> > > Hi, Sakari:
> > > 
> > > 	Thanks for your reply.
> > > If any misunderstanding, please correct me.
> > > 
> > > On Thu, May 16, 2024 at 10:42:05AM +0000, Sakari Ailus wrote:
> > > > Hi Chi Yuan,
> > > > 
> > > > On Wed, May 08, 2024 at 10:51:49AM +0800, [e-mail] wrote:
> > > > > From: ChiYuan Huang <[e-mail]>
> > > > > 
> > > > > In v4l2_async_create_ancillary_links(), if v4l2 async notifier is
> > > > > created from v4l2 device, the v4l2 flash subdev async binding will enter
> > > > > the logic to create media link. Due to the subdev of notifier is NULL,
> > > > > this will cause NULL pointer to access the subdev entity. Therefore, add
> > > > > the check to bypass it.
> > > > > 
> > > > > Fixes: aa4faf6eb271 ("media: v4l2-async: Create links during v4l2_async_match_notify()")
> > > > > Signed-off-by: ChiYuan Huang <[e-mail]>
> > > > > ---
> > > > > Hi,
> > > > > 
> > > > >   I'm trying to bind the v4l2 subdev for flashlight testing. It seems
> > > > > some logic in v4l2 asynd binding is incorrect.
> > > > > 
> > > > > From the change, I modified vim2m as the test driver to bind mt6370 flashlight.
> > > > > 
> > > > > Here's the backtrace log.
> > > > > 
> > > > >  vim2m soc:vim2m: bound [white:flash-2]
> > > > >  Unable to handle kernel NULL pointer dereference at virtual address 0000000000000058
> > > > >  ......skipping
> > > > >  Call trace:
> > > > >   media_create_ancillary_link+0x48/0xd8 [mc]
> > > > >   v4l2_async_match_notify+0x17c/0x208 [v4l2_async]
> > > > >   v4l2_async_register_subdev+0xb8/0x1d0 [v4l2_async]
> > > > 
> > > > There's something wrong obviously somewhere but wherea?
> > >
> > > In vim2m driver, I added v4l2_async_nf_init -> v4l2_async_nf_add_fwnode_remote ->
> > > v4l2_async_nf_register.
> > > 
> > > From the async flow, in notifier complete ops to create v4l-subdevX node for the 
> > > specified subdev.
> > >
> > > > A sub-notifier does have a sub-device after the notifier initialisation.
> > > 
> > > Why? Are you saying to the notifier can only be used for subdev and subdev binding, 
> > > not v4l2 and subdev binding?
> > > 
> > > But to create v4l-subdevX, the key is only v4l2 device and its needed subdev.
> > > 
> > > > Maybe the initialisation does not happen in the right order?
> > >
> > > AFAIK, Async flow can solve the probe order and makes the user no need to care
> > > the probe order.
> > > 
> > > From the stacktrace, I'm pretty sure it's not the probe order issue.
> > >
> > > > >   __v4l2_flash_init.part.0+0x3b4/0x4b0 [v4l2_flash_led_class]
> > > > >   v4l2_flash_init+0x28/0x48 [v4l2_flash_led_class]
> > > > >   mt6370_led_probe+0x348/0x690 [leds_mt6370_flash]
> > > > > 
> > > > > After tracing the code, it will let the subdev labeled as F_LENS or
> > > > > F_FLASH function to create media link. To prevent the NULL pointer
> > > > > issue, the simplest way is add a check when 'n->sd' is NULL and bypass
> > > > > the later media link creataion.
> > > > > ---
> > > > >  drivers/media/v4l2-core/v4l2-async.c | 3 +++
> > > > >  1 file changed, 3 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> > > > > index 3ec323bd528b..9d3161c51954 100644
> > > > > --- a/drivers/media/v4l2-core/v4l2-async.c
> > > > > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > > > > @@ -324,6 +324,9 @@ static int v4l2_async_create_ancillary_links(struct v4l2_async_notifier *n,
> > > > >  	    sd->entity.function != MEDIA_ENT_F_FLASH)
> > > > >  		return 0;
> > > > >  
> > > > > +	if (!n->sd)
> > > > > +		return 0;
> > > > 
> > > > This isn't the right fix: the ancillary link won't be created as a result.
> > >
> > > Due to the notifier is created by v4l2 device not subdev, this 'n->sd' is NULL.
> > > The NULL 'n->sd' will be referenced by the next flow 'media_create_ancillary_link'.
> > 
> > Ah, right. I took a new look into the code and agree this is a problem.
> > This probably hasn't been hit previously as the root notifier driver tends
> > not to have any lens or flash devices.
> > 
> > I'd change the commit message slightly:
> > 
> > --------8<-------------
> > In v4l2_async_create_ancillary_links(), ancillary links are created for
> > lens and flash sub-devices. These are sub-device to sub-device links and if
> > the async notifier is related to a V4L2 device, the source sub-device of
> > the ancillary link is NULL, leading to a NULL pointer dereference. Check
> > the notifier's sd field is non-NULL in v4l2_async_create_ancillary_links().
> > --------8<-------------
> 
> What's the use case for including lens or flash devices in the root
> notifier ? Shouldn't lens and flash subdevices always be linked to
> something ? We should of course not crash, but it seems that simply
> ignoring the subdevs and not linking them isn't a great idea either.

Yes, I think triggering this does require a very peculiar setup if not a
driver bug. We could also print a warning if this happens.

Also using the notifier's sub-device to create ancillary links is somewhat
opportunistic. We seem to rely on it currently but it would only seem
meaningful for a sensor in practice.

> 
> > > Or is it caused by the wrong usage? 
> > > 
> > > > > +
> > > > >  	link = media_create_ancillary_link(&n->sd->entity, &sd->entity);
> > > > >  
> > > > >  #endif
>

Laurent Pinchart May 17, 2024, 11:37 a.m. UTC | #7

On Fri, May 17, 2024 at 11:27:12AM +0000, Sakari Ailus wrote:
> Hi Laurent,
> 
> On Fri, May 17, 2024 at 02:19:44PM +0300, Laurent Pinchart wrote:
> > On Fri, May 17, 2024 at 08:00:32AM +0000, Sakari Ailus wrote:
> > > Hi Chi Yuan,
> > > 
> > > On Fri, May 17, 2024 at 02:31:50PM +0800, ChiYuan Huang wrote:
> > > > Hi, Sakari:
> > > > 
> > > > 	Thanks for your reply.
> > > > If any misunderstanding, please correct me.
> > > > 
> > > > On Thu, May 16, 2024 at 10:42:05AM +0000, Sakari Ailus wrote:
> > > > > Hi Chi Yuan,
> > > > > 
> > > > > On Wed, May 08, 2024 at 10:51:49AM +0800, [e-mail] wrote:
> > > > > > From: ChiYuan Huang <[e-mail]>
> > > > > > 
> > > > > > In v4l2_async_create_ancillary_links(), if v4l2 async notifier is
> > > > > > created from v4l2 device, the v4l2 flash subdev async binding will enter
> > > > > > the logic to create media link. Due to the subdev of notifier is NULL,
> > > > > > this will cause NULL pointer to access the subdev entity. Therefore, add
> > > > > > the check to bypass it.
> > > > > > 
> > > > > > Fixes: aa4faf6eb271 ("media: v4l2-async: Create links during v4l2_async_match_notify()")
> > > > > > Signed-off-by: ChiYuan Huang <[e-mail]>
> > > > > > ---
> > > > > > Hi,
> > > > > > 
> > > > > >   I'm trying to bind the v4l2 subdev for flashlight testing. It seems
> > > > > > some logic in v4l2 asynd binding is incorrect.
> > > > > > 
> > > > > > From the change, I modified vim2m as the test driver to bind mt6370 flashlight.
> > > > > > 
> > > > > > Here's the backtrace log.
> > > > > > 
> > > > > >  vim2m soc:vim2m: bound [white:flash-2]
> > > > > >  Unable to handle kernel NULL pointer dereference at virtual address 0000000000000058
> > > > > >  ......skipping
> > > > > >  Call trace:
> > > > > >   media_create_ancillary_link+0x48/0xd8 [mc]
> > > > > >   v4l2_async_match_notify+0x17c/0x208 [v4l2_async]
> > > > > >   v4l2_async_register_subdev+0xb8/0x1d0 [v4l2_async]
> > > > > 
> > > > > There's something wrong obviously somewhere but wherea?
> > > >
> > > > In vim2m driver, I added v4l2_async_nf_init -> v4l2_async_nf_add_fwnode_remote ->
> > > > v4l2_async_nf_register.
> > > > 
> > > > From the async flow, in notifier complete ops to create v4l-subdevX node for the 
> > > > specified subdev.
> > > >
> > > > > A sub-notifier does have a sub-device after the notifier initialisation.
> > > > 
> > > > Why? Are you saying to the notifier can only be used for subdev and subdev binding, 
> > > > not v4l2 and subdev binding?
> > > > 
> > > > But to create v4l-subdevX, the key is only v4l2 device and its needed subdev.
> > > > 
> > > > > Maybe the initialisation does not happen in the right order?
> > > >
> > > > AFAIK, Async flow can solve the probe order and makes the user no need to care
> > > > the probe order.
> > > > 
> > > > From the stacktrace, I'm pretty sure it's not the probe order issue.
> > > >
> > > > > >   __v4l2_flash_init.part.0+0x3b4/0x4b0 [v4l2_flash_led_class]
> > > > > >   v4l2_flash_init+0x28/0x48 [v4l2_flash_led_class]
> > > > > >   mt6370_led_probe+0x348/0x690 [leds_mt6370_flash]
> > > > > > 
> > > > > > After tracing the code, it will let the subdev labeled as F_LENS or
> > > > > > F_FLASH function to create media link. To prevent the NULL pointer
> > > > > > issue, the simplest way is add a check when 'n->sd' is NULL and bypass
> > > > > > the later media link creataion.
> > > > > > ---
> > > > > >  drivers/media/v4l2-core/v4l2-async.c | 3 +++
> > > > > >  1 file changed, 3 insertions(+)
> > > > > > 
> > > > > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> > > > > > index 3ec323bd528b..9d3161c51954 100644
> > > > > > --- a/drivers/media/v4l2-core/v4l2-async.c
> > > > > > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > > > > > @@ -324,6 +324,9 @@ static int v4l2_async_create_ancillary_links(struct v4l2_async_notifier *n,
> > > > > >  	    sd->entity.function != MEDIA_ENT_F_FLASH)
> > > > > >  		return 0;
> > > > > >  
> > > > > > +	if (!n->sd)
> > > > > > +		return 0;
> > > > > 
> > > > > This isn't the right fix: the ancillary link won't be created as a result.
> > > >
> > > > Due to the notifier is created by v4l2 device not subdev, this 'n->sd' is NULL.
> > > > The NULL 'n->sd' will be referenced by the next flow 'media_create_ancillary_link'.
> > > 
> > > Ah, right. I took a new look into the code and agree this is a problem.
> > > This probably hasn't been hit previously as the root notifier driver tends
> > > not to have any lens or flash devices.
> > > 
> > > I'd change the commit message slightly:
> > > 
> > > --------8<-------------
> > > In v4l2_async_create_ancillary_links(), ancillary links are created for
> > > lens and flash sub-devices. These are sub-device to sub-device links and if
> > > the async notifier is related to a V4L2 device, the source sub-device of
> > > the ancillary link is NULL, leading to a NULL pointer dereference. Check
> > > the notifier's sd field is non-NULL in v4l2_async_create_ancillary_links().
> > > --------8<-------------
> > 
> > What's the use case for including lens or flash devices in the root
> > notifier ? Shouldn't lens and flash subdevices always be linked to
> > something ? We should of course not crash, but it seems that simply
> > ignoring the subdevs and not linking them isn't a great idea either.
> 
> Yes, I think triggering this does require a very peculiar setup if not a
> driver bug. We could also print a warning if this happens.

I think a warning would be good.

> Also using the notifier's sub-device to create ancillary links is somewhat
> opportunistic. We seem to rely on it currently but it would only seem
> meaningful for a sensor in practice.

This should be improved indeed.

> > > > Or is it caused by the wrong usage? 
> > > > 
> > > > > > +
> > > > > >  	link = media_create_ancillary_link(&n->sd->entity, &sd->entity);
> > > > > >  
> > > > > >  #endif

Sakari Ailus May 20, 2024, 7:26 a.m. UTC | #8

Hi Laurent,

On Fri, May 17, 2024 at 02:37:30PM +0300, Laurent Pinchart wrote:
> On Fri, May 17, 2024 at 11:27:12AM +0000, Sakari Ailus wrote:
> > Hi Laurent,
> > 
> > On Fri, May 17, 2024 at 02:19:44PM +0300, Laurent Pinchart wrote:
> > > On Fri, May 17, 2024 at 08:00:32AM +0000, Sakari Ailus wrote:
> > > > Hi Chi Yuan,
> > > > 
> > > > On Fri, May 17, 2024 at 02:31:50PM +0800, ChiYuan Huang wrote:
> > > > > Hi, Sakari:
> > > > > 
> > > > > 	Thanks for your reply.
> > > > > If any misunderstanding, please correct me.
> > > > > 
> > > > > On Thu, May 16, 2024 at 10:42:05AM +0000, Sakari Ailus wrote:
> > > > > > Hi Chi Yuan,
> > > > > > 
> > > > > > On Wed, May 08, 2024 at 10:51:49AM +0800, [e-mail] wrote:
> > > > > > > From: ChiYuan Huang <[e-mail]>
> > > > > > > 
> > > > > > > In v4l2_async_create_ancillary_links(), if v4l2 async notifier is
> > > > > > > created from v4l2 device, the v4l2 flash subdev async binding will enter
> > > > > > > the logic to create media link. Due to the subdev of notifier is NULL,
> > > > > > > this will cause NULL pointer to access the subdev entity. Therefore, add
> > > > > > > the check to bypass it.
> > > > > > > 
> > > > > > > Fixes: aa4faf6eb271 ("media: v4l2-async: Create links during v4l2_async_match_notify()")
> > > > > > > Signed-off-by: ChiYuan Huang <[e-mail]>
> > > > > > > ---
> > > > > > > Hi,
> > > > > > > 
> > > > > > >   I'm trying to bind the v4l2 subdev for flashlight testing. It seems
> > > > > > > some logic in v4l2 asynd binding is incorrect.
> > > > > > > 
> > > > > > > From the change, I modified vim2m as the test driver to bind mt6370 flashlight.
> > > > > > > 
> > > > > > > Here's the backtrace log.
> > > > > > > 
> > > > > > >  vim2m soc:vim2m: bound [white:flash-2]
> > > > > > >  Unable to handle kernel NULL pointer dereference at virtual address 0000000000000058
> > > > > > >  ......skipping
> > > > > > >  Call trace:
> > > > > > >   media_create_ancillary_link+0x48/0xd8 [mc]
> > > > > > >   v4l2_async_match_notify+0x17c/0x208 [v4l2_async]
> > > > > > >   v4l2_async_register_subdev+0xb8/0x1d0 [v4l2_async]
> > > > > > 
> > > > > > There's something wrong obviously somewhere but wherea?
> > > > >
> > > > > In vim2m driver, I added v4l2_async_nf_init -> v4l2_async_nf_add_fwnode_remote ->
> > > > > v4l2_async_nf_register.
> > > > > 
> > > > > From the async flow, in notifier complete ops to create v4l-subdevX node for the 
> > > > > specified subdev.
> > > > >
> > > > > > A sub-notifier does have a sub-device after the notifier initialisation.
> > > > > 
> > > > > Why? Are you saying to the notifier can only be used for subdev and subdev binding, 
> > > > > not v4l2 and subdev binding?
> > > > > 
> > > > > But to create v4l-subdevX, the key is only v4l2 device and its needed subdev.
> > > > > 
> > > > > > Maybe the initialisation does not happen in the right order?
> > > > >
> > > > > AFAIK, Async flow can solve the probe order and makes the user no need to care
> > > > > the probe order.
> > > > > 
> > > > > From the stacktrace, I'm pretty sure it's not the probe order issue.
> > > > >
> > > > > > >   __v4l2_flash_init.part.0+0x3b4/0x4b0 [v4l2_flash_led_class]
> > > > > > >   v4l2_flash_init+0x28/0x48 [v4l2_flash_led_class]
> > > > > > >   mt6370_led_probe+0x348/0x690 [leds_mt6370_flash]
> > > > > > > 
> > > > > > > After tracing the code, it will let the subdev labeled as F_LENS or
> > > > > > > F_FLASH function to create media link. To prevent the NULL pointer
> > > > > > > issue, the simplest way is add a check when 'n->sd' is NULL and bypass
> > > > > > > the later media link creataion.
> > > > > > > ---
> > > > > > >  drivers/media/v4l2-core/v4l2-async.c | 3 +++
> > > > > > >  1 file changed, 3 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> > > > > > > index 3ec323bd528b..9d3161c51954 100644
> > > > > > > --- a/drivers/media/v4l2-core/v4l2-async.c
> > > > > > > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > > > > > > @@ -324,6 +324,9 @@ static int v4l2_async_create_ancillary_links(struct v4l2_async_notifier *n,
> > > > > > >  	    sd->entity.function != MEDIA_ENT_F_FLASH)
> > > > > > >  		return 0;
> > > > > > >  
> > > > > > > +	if (!n->sd)
> > > > > > > +		return 0;
> > > > > > 
> > > > > > This isn't the right fix: the ancillary link won't be created as a result.
> > > > >
> > > > > Due to the notifier is created by v4l2 device not subdev, this 'n->sd' is NULL.
> > > > > The NULL 'n->sd' will be referenced by the next flow 'media_create_ancillary_link'.
> > > > 
> > > > Ah, right. I took a new look into the code and agree this is a problem.
> > > > This probably hasn't been hit previously as the root notifier driver tends
> > > > not to have any lens or flash devices.
> > > > 
> > > > I'd change the commit message slightly:
> > > > 
> > > > --------8<-------------
> > > > In v4l2_async_create_ancillary_links(), ancillary links are created for
> > > > lens and flash sub-devices. These are sub-device to sub-device links and if
> > > > the async notifier is related to a V4L2 device, the source sub-device of
> > > > the ancillary link is NULL, leading to a NULL pointer dereference. Check
> > > > the notifier's sd field is non-NULL in v4l2_async_create_ancillary_links().
> > > > --------8<-------------
> > > 
> > > What's the use case for including lens or flash devices in the root
> > > notifier ? Shouldn't lens and flash subdevices always be linked to
> > > something ? We should of course not crash, but it seems that simply
> > > ignoring the subdevs and not linking them isn't a great idea either.
> > 
> > Yes, I think triggering this does require a very peculiar setup if not a
> > driver bug. We could also print a warning if this happens.
> 
> I think a warning would be good.
> 
> > Also using the notifier's sub-device to create ancillary links is somewhat
> > opportunistic. We seem to rely on it currently but it would only seem
> > meaningful for a sensor in practice.
> 
> This should be improved indeed.

This requires a little more work actually. The CCS driver should probably
have the lens and flash bound to its pixel array rather than the source
(binner or scaler) sub-device.

I'll post a patch to add a warning for non-subdev use of this now, the rest
will need to wait a little.

> 
> > > > > Or is it caused by the wrong usage? 
> > > > > 
> > > > > > > +
> > > > > > >  	link = media_create_ancillary_link(&n->sd->entity, &sd->entity);
> > > > > > >  
> > > > > > >  #endif
>

Patch

diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index 3ec323bd528b..9d3161c51954 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -324,6 +324,9 @@  static int v4l2_async_create_ancillary_links(struct v4l2_async_notifier *n,
 	    sd->entity.function != MEDIA_ENT_F_FLASH)
 		return 0;
 
+	if (!n->sd)
+		return 0;
+
 	link = media_create_ancillary_link(&n->sd->entity, &sd->entity);
 
 #endif