Re: [PATCH 1/3] media: i2c: imx334: uninitialized variable in imx334_update_exp_gain()

[Dan Carpenter <dan.carpenter@linaro.org>]

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@linaro.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