linux-hardening - Re: Coverity: imx290_ctrl_init(): Error handling issues

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 10 Nov 2022 16:41:28 +0000
From:   Dave Stevenson <dave.stevenson@...pberrypi.com>
To:     coverity-bot <keescook@...omium.org>
Cc:     Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        linux-kernel@...r.kernel.org,
        Alexander Stein <alexander.stein@...tq-group.com>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Manivannan Sadhasivam <mani@...nel.org>,
        linux-media@...r.kernel.org,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        "Gustavo A. R. Silva" <gustavo@...eddedor.com>,
        linux-next@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: Coverity: imx290_ctrl_init(): Error handling issues

On Thu, 10 Nov 2022 at 16:31, coverity-bot <keescook@...omium.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@...omium.org>
> Addresses-Coverity-ID: 1527251 ("Error handling issues")
> Fixes: 4c9c93cf8657 ("media: i2c: imx290: Create controls for fwnode properties")
>
> Thanks for your attention!
>
> --
> Coverity-bot