[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <255a8604-5aa9-441a-a4d6-ebc592a00be9@stanley.mountain>
Date: Wed, 30 Apr 2025 12:59:21 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Tarang Raval <tarang.raval@...iconsignals.io>
Cc: Sakari Ailus <sakari.ailus@...ux.intel.com>,
Mauro Carvalho Chehab <mchehab@...nel.org>,
Hans Verkuil <hverkuil@...all.nl>,
Shravan Chippa <Shravan.Chippa@...rochip.com>,
"linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/3] media: i2c: imx334: uninitialized variable in
imx334_update_exp_gain()
On Wed, Apr 30, 2025 at 09:47:44AM +0000, Tarang Raval wrote:
> Hi Dan,
>
> > The "ret" variable is not initialized on the success path. Set it to
> > zero.
> >
> > Fixes: 7b19b0fc8ac8 ("media: i2c: imx334: Convert to CCI register access helpers")
> > Signed-off-by: Dan Carpenter <dan.carpenter@...aro.org>
> > ---
> > drivers/media/i2c/imx334.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
> > index fc875072f859..846b9928d4e8 100644
> > --- a/drivers/media/i2c/imx334.c
> > +++ b/drivers/media/i2c/imx334.c
> > @@ -536,7 +536,8 @@ static int imx334_update_controls(struct imx334 *imx334,
> > static int imx334_update_exp_gain(struct imx334 *imx334, u32 exposure, u32 gain)
> > {
> > u32 lpfr, shutter;
> > - int ret, ret_hold;
> > + int ret_hold;
> > + int ret = 0;
>
> I think this initialization may not really be necessary.
>
> If all of those cci_write are skipped, then yes, using ret uninitialized
> would be a problem.
>
> However, I don’t see any case where they would be skipped in the
> current implementation.
>
> So, is this initialization really needed?
>
> Best Regards,
> Tarang
This is a new bug that was introduced in linux-next...
drivers/media/i2c/imx334.c
536 static int imx334_update_exp_gain(struct imx334 *imx334, u32 exposure, u32 gain)
537 {
538 u32 lpfr, shutter;
539 int ret, ret_hold;
540
541 lpfr = imx334->vblank + imx334->cur_mode->height;
542 shutter = lpfr - exposure;
543
544 dev_dbg(imx334->dev, "Set long exp %u analog gain %u sh0 %u lpfr %u\n",
545 exposure, gain, shutter, lpfr);
546
547 cci_write(imx334->cci, IMX334_REG_HOLD, 1, &ret);
This first call will do an unitialized read of ret to check if it holds
an error code.
548 cci_write(imx334->cci, IMX334_REG_VMAX, lpfr, &ret);
549 cci_write(imx334->cci, IMX334_REG_SHUTTER, shutter, &ret);
550 cci_write(imx334->cci, IMX334_REG_AGAIN, gain, &ret);
cci_write() is designed to preserve the error codes from previous calls.
It will only write error codes to "ret", it will not write success. If
everything succeeds then "ret" is uninitialized.
551
552 ret_hold = cci_write(imx334->cci, IMX334_REG_HOLD, 0, NULL);
553 if (ret_hold)
554 return ret_hold;
555
556 return ret;
557 }
In production then everyone should run with INIT_STACK_ALL_ZERO. In that
case everything works fine. However some older distributions do not use
this option. Also in testing, I would encourage everyone to run with
INIT_STACK_ALL_PATTERN.
regards,
dan carpenter
Powered by blists - more mailing lists