media: em28xx: return error on media_device_register() failure

  In an unlikely case of failure in media_device_register(), release
resources and return the erroneous value. Otherwise, possible issues
with registering the device will continue to be ignored.

Found by Linux Verification Center (linuxtesting.org) with static
analysis tool SVACE.

Fixes: 37ecc7b1278f ("[media] em28xx: add media controller support")
Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru>
---
 drivers/media/usb/em28xx/em28xx-cards.c | 2 ++
 1 file changed, 2 insertions(+)
  

Le 10/01/2024 à 18:39, Nikita Zhandarovich a écrit :
> In an unlikely case of failure in media_device_register(), release
> resources and return the erroneous value. Otherwise, possible issues
> with registering the device will continue to be ignored.
> 
> Found by Linux Verification Center (linuxtesting.org) with static
> analysis tool SVACE.
> 
> Fixes: 37ecc7b1278f ("[media] em28xx: add media controller support")
> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru>
> ---
>   drivers/media/usb/em28xx/em28xx-cards.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
> index 4d037c92af7c..dae731dfc569 100644
> --- a/drivers/media/usb/em28xx/em28xx-cards.c
> +++ b/drivers/media/usb/em28xx/em28xx-cards.c
> @@ -4095,6 +4095,8 @@ static int em28xx_usb_probe(struct usb_interface *intf,
>   	 */
>   #ifdef CONFIG_MEDIA_CONTROLLER
>   	retval = media_device_register(dev->media_dev);
> +	if (retval)
> +		goto err_free;
>   #endif
>   
>   	return 0;
> 
> 
Hi,

I think that some resources allocated in em28xx_init_dev() should also 
be freed if media_device_register() fails. (see the error handling path 
at the end of em28xx_init_dev())

Just my 2c.

CJ
  

Em Wed, 10 Jan 2024 09:39:58 -0800
Nikita Zhandarovich <n.zhandarovich@fintech.ru> escreveu:

> In an unlikely case of failure in media_device_register(), release
> resources and return the erroneous value. Otherwise, possible issues
> with registering the device will continue to be ignored.
> 
> Found by Linux Verification Center (linuxtesting.org) with static
> analysis tool SVACE.
> 
> Fixes: 37ecc7b1278f ("[media] em28xx: add media controller support")
> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru>
> ---
>  drivers/media/usb/em28xx/em28xx-cards.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
> index 4d037c92af7c..dae731dfc569 100644
> --- a/drivers/media/usb/em28xx/em28xx-cards.c
> +++ b/drivers/media/usb/em28xx/em28xx-cards.c
> @@ -4095,6 +4095,8 @@ static int em28xx_usb_probe(struct usb_interface *intf,
>  	 */
>  #ifdef CONFIG_MEDIA_CONTROLLER
>  	retval = media_device_register(dev->media_dev);
> +	if (retval)
> +		goto err_free;

Not freeing resources here is intentional. See, the media controller 
API is optional on this driver. It will just provide a way to identify
the device's topology, but the device is completely usable without
it.

Perhaps we need, instead, a patch documenting it, and preventing
static analysis tools to point it as an issue.

Thanks,
Mauro
  

On 1/10/24 22:49, Mauro Carvalho Chehab wrote:
> Em Wed, 10 Jan 2024 09:39:58 -0800
> Nikita Zhandarovich <n.zhandarovich@fintech.ru> escreveu:
> 
>> In an unlikely case of failure in media_device_register(), release
>> resources and return the erroneous value. Otherwise, possible issues
>> with registering the device will continue to be ignored.
>>
>> Found by Linux Verification Center (linuxtesting.org) with static
>> analysis tool SVACE.
>>
>> Fixes: 37ecc7b1278f ("[media] em28xx: add media controller support")
>> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru>
>> ---
>>  drivers/media/usb/em28xx/em28xx-cards.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
>> index 4d037c92af7c..dae731dfc569 100644
>> --- a/drivers/media/usb/em28xx/em28xx-cards.c
>> +++ b/drivers/media/usb/em28xx/em28xx-cards.c
>> @@ -4095,6 +4095,8 @@ static int em28xx_usb_probe(struct usb_interface *intf,
>>  	 */
>>  #ifdef CONFIG_MEDIA_CONTROLLER
>>  	retval = media_device_register(dev->media_dev);
>> +	if (retval)
>> +		goto err_free;
> 
> Not freeing resources here is intentional. See, the media controller 
> API is optional on this driver. It will just provide a way to identify
> the device's topology, but the device is completely usable without
> it.
> 
> Perhaps we need, instead, a patch documenting it, and preventing
> static analysis tools to point it as an issue.
> 
> Thanks,
> Mauro

Thank you for your feedback, however I had a few questions...

While I understand what you mean about optional nature of media
controller registration in this case, a quick glance into other calls to
media_device_register() across the source code shows that usually
failure with registering is handled as a proper error regardless of
whether the device is still usable. But if you think that we can make an
exception here, I'll happily oblige.

Then if I am to continue on this path, would the following comment above
the call to media_device_register() suffice?

 #ifdef CONFIG_MEDIA_CONTROLLER
+	/*
+	 * No need to check the return value, the device will still be 	     +
 * usable without media controller API.
+	 */
 	retval = media_device_register(dev->media_dev);

Thanks,
Nikita
  

Em Thu, 11 Jan 2024 07:10:10 -0800
Nikita Zhandarovich <n.zhandarovich@fintech.ru> escreveu:

> On 1/10/24 22:49, Mauro Carvalho Chehab wrote:
> > Em Wed, 10 Jan 2024 09:39:58 -0800
> > Nikita Zhandarovich <n.zhandarovich@fintech.ru> escreveu:
> >   
> >> In an unlikely case of failure in media_device_register(), release
> >> resources and return the erroneous value. Otherwise, possible issues
> >> with registering the device will continue to be ignored.
> >>
> >> Found by Linux Verification Center (linuxtesting.org) with static
> >> analysis tool SVACE.
> >>
> >> Fixes: 37ecc7b1278f ("[media] em28xx: add media controller support")
> >> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru>
> >> ---
> >>  drivers/media/usb/em28xx/em28xx-cards.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
> >> index 4d037c92af7c..dae731dfc569 100644
> >> --- a/drivers/media/usb/em28xx/em28xx-cards.c
> >> +++ b/drivers/media/usb/em28xx/em28xx-cards.c
> >> @@ -4095,6 +4095,8 @@ static int em28xx_usb_probe(struct usb_interface *intf,
> >>  	 */
> >>  #ifdef CONFIG_MEDIA_CONTROLLER
> >>  	retval = media_device_register(dev->media_dev);
> >> +	if (retval)
> >> +		goto err_free;  
> > 
> > Not freeing resources here is intentional. See, the media controller 
> > API is optional on this driver. It will just provide a way to identify
> > the device's topology, but the device is completely usable without
> > it.
> > 
> > Perhaps we need, instead, a patch documenting it, and preventing
> > static analysis tools to point it as an issue.
> > 
> > Thanks,
> > Mauro  
> 
> Thank you for your feedback, however I had a few questions...
> 
> While I understand what you mean about optional nature of media
> controller registration in this case, a quick glance into other calls to
> media_device_register() across the source code shows that usually
> failure with registering is handled as a proper error regardless of
> whether the device is still usable. But if you think that we can make an
> exception here, I'll happily oblige.

It depends on how the actual device is controlled. "Normal"
media devices are fully controlled via v4l2 API. On those, the
media controller API is there just to let userspace to query
about the internal settings, but the actual pipelines are
created via V4L2 API. Almost all normal applications will
just ignore the media controller API.

Embedded hardware, however, require setting pipelines via media 
controller for they to actually work. Almost all drivers
implementing the media controller API fall on this category.

> 
> Then if I am to continue on this path, would the following comment above
> the call to media_device_register() suffice?
> 
>  #ifdef CONFIG_MEDIA_CONTROLLER
> +	/*
> +	 * No need to check the return value, the device will still be 	     +
>  * usable without media controller API.
> +	 */
>  	retval = media_device_register(dev->media_dev);

That works for me. It would still produce alerts at static
analyzers, as they'll notice that we're storing retval there
without actually using it.

Thanks,
Mauro