| 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
| ||
|
Message-ID: <CANiDSCvS3gnrQ0sPrdhiQxY47rHHrvVMq_wDDBYa_L=Y-VZwAg@mail.gmail.com>
Date: Mon, 20 Oct 2025 21:58:47 +0200
From: Ricardo Ribalda <ribalda@...omium.org>
To: Sakari Ailus <sakari.ailus@....fi>
Cc: Dan Carpenter <dan.carpenter@...aro.org>, Ricardo Ribalda <ribalda@...nel.org>,
Hans Verkuil <hverkuil+cisco@...nel.org>, linux-media@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] media: i2c: imx214: Exit early on control init errors
Hi Sakai
On Mon, 20 Oct 2025 at 21:34, Sakari Ailus <sakari.ailus@....fi> wrote:
>
> Hi Ricardo,
>
> On Mon, Oct 20, 2025 at 08:51:44PM +0200, Ricardo Ribalda wrote:
> > Hi Sakari
> >
> > On Mon, 20 Oct 2025 at 20:28, Sakari Ailus <sakari.ailus@....fi> wrote:
> > >
> > > Hi Ricardo,
> > >
> > > On Tue, Oct 14, 2025 at 11:00:17AM +0000, Ricardo Ribalda wrote:
> > > > Now we try to initialize all the controls and at the very end check
> > > > ctrl_hdlr->error to check if one of them has failed.
> > > >
> > > > This confuses smatch, who do not know how to track the state of
> > > > imx214->link_freq.
> > > >
> > > > drivers/media/i2c/imx214.c:1109 imx214_ctrls_init() error: we previously assumed 'imx214->link_freq' could be null (see line 1017)
> > > >
> > > > Fix this by exiting early on control initialization errors.
> > > >
> > > > Signed-off-by: Ricardo Ribalda <ribalda@...omium.org>
> > > > ---
> > > > Right now we are handling this with a quirk in media-ci, if Dan cannot
> > > > fix smatch in a kernel cycle we should merge this patch.
> > > > ---
> > > > Changes in v2:
> > > > - Fix typo in commit message commit
> > > > - Move error tag where it belongs (Thanks Hans!)
> > > > - Link to v1: https://lore.kernel.org/r/20250829-imx214-smatch-v1-1-f3d1653b48e4@chromium.org
> > > > ---
> > > > drivers/media/i2c/imx214.c | 7 +++++--
> > > > 1 file changed, 5 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
> > > > index 94ebe625c9e6ee0fb67fe1d89b48b2f1bf58ffc6..c66f0e18726c3fc15df91c37888a797bcea82134 100644
> > > > --- a/drivers/media/i2c/imx214.c
> > > > +++ b/drivers/media/i2c/imx214.c
> > > > @@ -1014,8 +1014,10 @@ static int imx214_ctrls_init(struct imx214 *imx214)
> > > > V4L2_CID_LINK_FREQ,
> > > > imx214->bus_cfg.nr_of_link_frequencies - 1,
> > > > 0, imx214->bus_cfg.link_frequencies);
> > > > - if (imx214->link_freq)
> > > > - imx214->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > > > + if (!imx214->link_freq)
> > > > + goto err_init_ctrl;
> > > > +
> > > > + imx214->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > >
> > > You could do this cleaner by simply moving the assignment after the handler
> > > error check. Some drivers do that already.
> > >
> > > I wonder why this seems to be a problem for smatch in the imx214 driver as
> > > the pattern is widely used across the sensor drivers.
> >
> > Smatch thinks that there could be case where
> >
> > imx->link_freq = NULL, and imx214_pll_update returns 0.
> >
> > That is not solved by moving the assignment `imx214->link_freq->flags
> > |=` after if (ret)
>
> Did you test this? The smatch message suggests otherwise (but of course
> this could just turn into a different smatch error).
Actually smatch do not hate it :)
ribalda@...alda:~/work/linux$ make -i W=1 C=1
CHECK="../media-ci/third_party/smatch/smatch -p=kernel"
KCFLAGS="-Wmaybe-uninitialized" drivers/media/i2c/imx214.o
CC kernel/sched/rq-offsets.s
In file included from kernel/sched/rq-offsets.c:5:
kernel/sched/sched.h: In function ‘mm_cid_get’:
kernel/sched/sched.h:3743:25: error: variable ‘cpumask’ set but not
used [-Werror=unused-but-set-variable]
3743 | struct cpumask *cpumask;
| ^~~~~~~
cc1: all warnings being treated as errors
make[2]: [scripts/Makefile.build:182: kernel/sched/rq-offsets.s] Error
1 (ignored)
/bin/sh: line 1: kernel/sched/rq-offsets.s: No such file or directory
make[2]: [Kbuild:46: include/generated/rq-offsets.h] Error 1 (ignored)
CALL scripts/checksyscalls.sh
DESCEND objtool
INSTALL libsubcmd_headers
CC drivers/media/i2c/imx214.o
CHECK drivers/media/i2c/imx214.c
ribalda@...alda:~/work/linux$ git diff
diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
index 94ebe625c9e6..a21461b55923 100644
--- a/drivers/media/i2c/imx214.c
+++ b/drivers/media/i2c/imx214.c
@@ -1014,8 +1014,6 @@ static int imx214_ctrls_init(struct imx214 *imx214)
V4L2_CID_LINK_FREQ,
imx214->bus_cfg.nr_of_link_frequencies - 1,
0,
imx214->bus_cfg.link_frequencies);
- if (imx214->link_freq)
- imx214->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
/*
* WARNING!
@@ -1038,9 +1036,6 @@ static int imx214_ctrls_init(struct imx214 *imx214)
imx214->hblank = v4l2_ctrl_new_std(ctrl_hdlr, &imx214_ctrl_ops,
V4L2_CID_HBLANK, hblank, hblank,
1, hblank);
- if (imx214->hblank)
- imx214->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
-
exposure_max = mode->vts_def - IMX214_EXPOSURE_OFFSET;
exposure_def = min(exposure_max, IMX214_EXPOSURE_DEFAULT);
imx214->exposure = v4l2_ctrl_new_std(ctrl_hdlr, &imx214_ctrl_ops,
@@ -1060,13 +1055,9 @@ static int imx214_ctrls_init(struct imx214 *imx214)
imx214->hflip = v4l2_ctrl_new_std(ctrl_hdlr, &imx214_ctrl_ops,
V4L2_CID_HFLIP, 0, 1, 1, 0);
- if (imx214->hflip)
- imx214->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
imx214->vflip = v4l2_ctrl_new_std(ctrl_hdlr, &imx214_ctrl_ops,
V4L2_CID_VFLIP, 0, 1, 1, 0);
- if (imx214->vflip)
- imx214->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
v4l2_ctrl_cluster(2, &imx214->hflip);
@@ -1106,6 +1097,11 @@ static int imx214_ctrls_init(struct imx214 *imx214)
return ret;
}
+ imx214->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+ imx214->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+ imx214->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
+ imx214->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
+
ret = imx214_pll_update(imx214);
if (ret < 0) {
v4l2_ctrl_handler_free(ctrl_hdlr);
>
> >
> > I believe Dan is already flagged about this, but I do not think that
> > it will be super simple to fix in his code.
> >
> > If smatch can handle this case before rc5 I will delete this patch.
>
> There are other options, too, such as storing the link frequency index (the
> driver won't even support setting it) or the frequency itself.
There are plenty of options :) But I am still failing to see what is
wrong with this patch.
We exit early when there is an error instead of continuing doing work
that will be useless.
If you really prefer your way I can send a v3... but we have probably
more fun work to do :P
Regards!
>
> --
> Regards,
>
> Sakari Ailus
--
Ricardo Ribalda
Powered by blists - more mailing lists