Coverity: imx290_ctrl_init(): Error handling issues

Coverity: imx290_ctrl_init(): Error handling issues

[coverity-bot]

Hello!

This is an experimental semi-automated report about issues detected by
Coverity from a scan of next-20221110 as part of the linux-next scan project:
https://scan.coverity.com/projects/linux-next-weekly-scan

You're getting this email because you were associated with the identified
lines of code (noted below) that were touched by commits:

  Thu Oct 27 14:38:02 2022 +0300
    4c9c93cf8657 ("media: i2c: imx290: Create controls for fwnode properties")

Coverity reported the following:

*** CID 1527251:  Error handling issues  (CHECKED_RETURN)
drivers/media/i2c/imx290.c:1056 in imx290_ctrl_init()
1050     	imx290->vblank = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
1051     					   V4L2_CID_VBLANK, blank, blank, 1,
1052     					   blank);
1053     	if (imx290->vblank)
1054     		imx290->vblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
1055
vvv     CID 1527251:  Error handling issues  (CHECKED_RETURN)
vvv     Calling "v4l2_ctrl_new_fwnode_properties" without checking return value (as is done elsewhere 9 out of 10 times).
1056     	v4l2_ctrl_new_fwnode_properties(&imx290->ctrls, &imx290_ctrl_ops,
1057     					&props);
1058
1059     	imx290->sd.ctrl_handler = &imx290->ctrls;
1060
1061     	if (imx290->ctrls.error) {

If this is a false positive, please let us know so we can mark it as
such, or teach the Coverity rules to be smarter. If not, please make
sure fixes get into linux-next. :) For patches fixing this, please
include these lines (but double-check the "Fixes" first):

Reported-by: coverity-bot <keescook+coverity-bot@chromium.org>
Addresses-Coverity-ID: 1527251 ("Error handling issues")
Fixes: 4c9c93cf8657 ("media: i2c: imx290: Create controls for fwnode properties")

Thanks for your attention!

-- 
Coverity-bot

Re: Coverity: imx290_ctrl_init(): Error handling issues

[Dave Stevenson]

On Thu, 10 Nov 2022 at 16:31, coverity-bot <keescook@chromium.org> wrote:
>
> Hello!
>
> This is an experimental semi-automated report about issues detected by
> Coverity from a scan of next-20221110 as part of the linux-next scan project:
> https://scan.coverity.com/projects/linux-next-weekly-scan
>
> You're getting this email because you were associated with the identified
> lines of code (noted below) that were touched by commits:
>
>   Thu Oct 27 14:38:02 2022 +0300
>     4c9c93cf8657 ("media: i2c: imx290: Create controls for fwnode properties")
>
> Coverity reported the following:
>
> *** CID 1527251:  Error handling issues  (CHECKED_RETURN)
> drivers/media/i2c/imx290.c:1056 in imx290_ctrl_init()
> 1050            imx290->vblank = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
> 1051                                               V4L2_CID_VBLANK, blank, blank, 1,
> 1052                                               blank);
> 1053            if (imx290->vblank)
> 1054                    imx290->vblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> 1055
> vvv     CID 1527251:  Error handling issues  (CHECKED_RETURN)
> vvv     Calling "v4l2_ctrl_new_fwnode_properties" without checking return value (as is done elsewhere 9 out of 10 times).
> 1056            v4l2_ctrl_new_fwnode_properties(&imx290->ctrls, &imx290_ctrl_ops,
> 1057                                            &props);
> 1058
> 1059            imx290->sd.ctrl_handler = &imx290->ctrls;
> 1060
> 1061            if (imx290->ctrls.error) {
>
> If this is a false positive, please let us know so we can mark it as
> such, or teach the Coverity rules to be smarter. If not, please make
> sure fixes get into linux-next. :) For patches fixing this, please
> include these lines (but double-check the "Fixes" first):

I looked at this one when the patches were sent to the list.

On failure, v4l2_ctrl_new_fwnode_properties will have set the error
flag in struct v4l2_ctrl_handler. This is also what it returns.

In most of the existing drivers the error flag has already been
checked before calling v4l2_ctrl_new_fwnode_properties, therefore the
return value has to be checked explicitly. In this case it is checked
at line 1061 which is after v4l2_ctrl_new_fwnode_properties has been
called, and therefore there is no need to check the return value of
the call.

IMHO Neither is particularly right or wrong, just slightly different
approaches. In some regards this new code pattern is nicer as it
removes a number of error handling paths.

  Dave

> Reported-by: coverity-bot <keescook+coverity-bot@chromium.org>
> Addresses-Coverity-ID: 1527251 ("Error handling issues")
> Fixes: 4c9c93cf8657 ("media: i2c: imx290: Create controls for fwnode properties")
>
> Thanks for your attention!
>
> --
> Coverity-bot

Re: Coverity: imx290_ctrl_init(): Error handling issues

[Kees Cook]

On Thu, Nov 10, 2022 at 04:41:28PM +0000, Dave Stevenson wrote:
> On Thu, 10 Nov 2022 at 16:31, coverity-bot <keescook@chromium.org> wrote:
> >
> > Hello!
> >
> > This is an experimental semi-automated report about issues detected by
> > Coverity from a scan of next-20221110 as part of the linux-next scan project:
> > https://scan.coverity.com/projects/linux-next-weekly-scan
> >
> > You're getting this email because you were associated with the identified
> > lines of code (noted below) that were touched by commits:
> >
> >   Thu Oct 27 14:38:02 2022 +0300
> >     4c9c93cf8657 ("media: i2c: imx290: Create controls for fwnode properties")
> >
> > Coverity reported the following:
> >
> > *** CID 1527251:  Error handling issues  (CHECKED_RETURN)
> > drivers/media/i2c/imx290.c:1056 in imx290_ctrl_init()
> > 1050            imx290->vblank = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
> > 1051                                               V4L2_CID_VBLANK, blank, blank, 1,
> > 1052                                               blank);
> > 1053            if (imx290->vblank)
> > 1054                    imx290->vblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > 1055
> > vvv     CID 1527251:  Error handling issues  (CHECKED_RETURN)
> > vvv     Calling "v4l2_ctrl_new_fwnode_properties" without checking return value (as is done elsewhere 9 out of 10 times).
> > 1056            v4l2_ctrl_new_fwnode_properties(&imx290->ctrls, &imx290_ctrl_ops,
> > 1057                                            &props);
> > 1058
> > 1059            imx290->sd.ctrl_handler = &imx290->ctrls;
> > 1060
> > 1061            if (imx290->ctrls.error) {
> >
> > If this is a false positive, please let us know so we can mark it as
> > such, or teach the Coverity rules to be smarter. If not, please make
> > sure fixes get into linux-next. :) For patches fixing this, please
> > include these lines (but double-check the "Fixes" first):
> 
> I looked at this one when the patches were sent to the list.
> 
> On failure, v4l2_ctrl_new_fwnode_properties will have set the error
> flag in struct v4l2_ctrl_handler. This is also what it returns.
> 
> In most of the existing drivers the error flag has already been
> checked before calling v4l2_ctrl_new_fwnode_properties, therefore the
> return value has to be checked explicitly. In this case it is checked
> at line 1061 which is after v4l2_ctrl_new_fwnode_properties has been
> called, and therefore there is no need to check the return value of
> the call.
> 
> IMHO Neither is particularly right or wrong, just slightly different
> approaches. In some regards this new code pattern is nicer as it
> removes a number of error handling paths.

Great! Thanks for double-checking it. :)

-- 
Kees Cook