[PATCH] media: i2c: imx471: Fix uninitialized error value in imx471_set_ctrl()

[PATCH] media: i2c: imx471: Fix uninitialized error value in imx471_set_ctrl()

[David Carlier]

The exposure control write passes the address of the local ret variable
to cci_write() as its error pointer while ret is still uninitialized.
cci_write() returns early without touching the hardware when the error
value it is handed is already non-zero, so a non-zero garbage value on
the stack silently skips programming the exposure register and makes the
control write return a bogus, possibly positive, status.

This path runs on every stream start and on every exposure update from
userspace. Initialize ret to zero so the exposure register is always
written and a correct status is returned.

Fixes: be1589e567ae ("media: i2c: imx471: Add Sony IMX471 image sensor driver")
Signed-off-by: David Carlier <devnexen@gmail.com>
---
 drivers/media/i2c/imx471.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/imx471.c b/drivers/media/i2c/imx471.c
index 6d358b11e96d..5aca5f2355d6 100644
--- a/drivers/media/i2c/imx471.c
+++ b/drivers/media/i2c/imx471.c
@@ -306,7 +306,7 @@ static int imx471_set_ctrl(struct v4l2_ctrl *ctrl)
 			v4l2_subdev_get_locked_active_state(&sensor->sd);
 	const struct v4l2_mbus_framefmt *format =
 			v4l2_subdev_state_get_format(state, 0);
-	int ret;
+	int ret = 0;
 
 	if (ctrl->id == V4L2_CID_VBLANK) {
 		s64 exposure_max = format->height + ctrl->val -
-- 
2.53.0

Re: [PATCH] media: i2c: imx471: Fix uninitialized error value in imx471_set_ctrl()

[Kate Hsuan]

Hi David,

Thank you for the fix.

On Fri, Jul 24, 2026 at 12:14 PM David Carlier <devnexen@gmail.com> wrote:
>
> The exposure control write passes the address of the local ret variable
> to cci_write() as its error pointer while ret is still uninitialized.
> cci_write() returns early without touching the hardware when the error
> value it is handed is already non-zero, so a non-zero garbage value on
> the stack silently skips programming the exposure register and makes the
> control write return a bogus, possibly positive, status.
>
> This path runs on every stream start and on every exposure update from
> userspace. Initialize ret to zero so the exposure register is always
> written and a correct status is returned.
>
> Fixes: be1589e567ae ("media: i2c: imx471: Add Sony IMX471 image sensor driver")
> Signed-off-by: David Carlier <devnexen@gmail.com>
> ---
>  drivers/media/i2c/imx471.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/imx471.c b/drivers/media/i2c/imx471.c
> index 6d358b11e96d..5aca5f2355d6 100644
> --- a/drivers/media/i2c/imx471.c
> +++ b/drivers/media/i2c/imx471.c
> @@ -306,7 +306,7 @@ static int imx471_set_ctrl(struct v4l2_ctrl *ctrl)
>                         v4l2_subdev_get_locked_active_state(&sensor->sd);
>         const struct v4l2_mbus_framefmt *format =
>                         v4l2_subdev_state_get_format(state, 0);
> -       int ret;
> +       int ret = 0;

Setting ret to 0 can resolve the issue, but the root cause is at lines
337 and 342. Since we don't need the previous error state when calling
cci_write() in imx471_set_ctrl(), the fourth parameter of cci_write()
should be NULL.
So the fix for the V4L2_CID_EXPOSURE and V4L2_CID_VBLANK is

case V4L2_CID_EXPOSURE:
    ret = cci_write(sensor->regmap, IMX471_REG_EXPOSURE, ctrl->val, NULL);
    break;
case V4L2_CID_VBLANK:
    /* Update FLL that meets expected vertical blanking */
    ret = cci_write(sensor->regmap, IMX471_REG_FLL, format->height +
ctrl->val, NULL);
    break;

This ensures the cci_write() writes the data to the register without
being influenced by an uninitialized ret value and the ret can be set
properly.

>
>         if (ctrl->id == V4L2_CID_VBLANK) {
>                 s64 exposure_max = format->height + ctrl->val -
> --
> 2.53.0
>


-- 
BR,
Kate

Re: [PATCH] media: i2c: imx471: Fix uninitialized error value in imx471_set_ctrl()

[David CARLIER]

Hi,

On Mon, 27 Jul 2026 at 08:01, Kate Hsuan <hpa@redhat.com> wrote:
>
> Hi David,
>
> Thank you for the fix.
>
> On Fri, Jul 24, 2026 at 12:14 PM David Carlier <devnexen@gmail.com> wrote:
> >
> > The exposure control write passes the address of the local ret variable
> > to cci_write() as its error pointer while ret is still uninitialized.
> > cci_write() returns early without touching the hardware when the error
> > value it is handed is already non-zero, so a non-zero garbage value on
> > the stack silently skips programming the exposure register and makes the
> > control write return a bogus, possibly positive, status.
> >
> > This path runs on every stream start and on every exposure update from
> > userspace. Initialize ret to zero so the exposure register is always
> > written and a correct status is returned.
> >
> > Fixes: be1589e567ae ("media: i2c: imx471: Add Sony IMX471 image sensor driver")
> > Signed-off-by: David Carlier <devnexen@gmail.com>
> > ---
> >  drivers/media/i2c/imx471.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/i2c/imx471.c b/drivers/media/i2c/imx471.c
> > index 6d358b11e96d..5aca5f2355d6 100644
> > --- a/drivers/media/i2c/imx471.c
> > +++ b/drivers/media/i2c/imx471.c
> > @@ -306,7 +306,7 @@ static int imx471_set_ctrl(struct v4l2_ctrl *ctrl)
> >                         v4l2_subdev_get_locked_active_state(&sensor->sd);
> >         const struct v4l2_mbus_framefmt *format =
> >                         v4l2_subdev_state_get_format(state, 0);
> > -       int ret;
> > +       int ret = 0;
>
> Setting ret to 0 can resolve the issue, but the root cause is at lines
> 337 and 342. Since we don't need the previous error state when calling
> cci_write() in imx471_set_ctrl(), the fourth parameter of cci_write()
> should be NULL.
> So the fix for the V4L2_CID_EXPOSURE and V4L2_CID_VBLANK is
>
> case V4L2_CID_EXPOSURE:
>     ret = cci_write(sensor->regmap, IMX471_REG_EXPOSURE, ctrl->val, NULL);
>     break;
> case V4L2_CID_VBLANK:
>     /* Update FLL that meets expected vertical blanking */
>     ret = cci_write(sensor->regmap, IMX471_REG_FLL, format->height +
> ctrl->val, NULL);
>     break;
>
> This ensures the cci_write() writes the data to the register without
> being influenced by an uninitialized ret value and the ret can be set
> properly.
>
> >
> >         if (ctrl->id == V4L2_CID_VBLANK) {
> >                 s64 exposure_max = format->height + ctrl->val -
> > --
> > 2.53.0
> >
>
>
> --
> BR,
> Kate
>

Good suggestion, will send v2 tomorrow.

Cheers.

[PATCH v2] media: i2c: imx471: Fix uninitialized error value in imx471_set_ctrl()

[David Carlier]

The exposure and vertical blanking writes pass the address of the local
ret variable to cci_write() as its error pointer, but there is no earlier
error to propagate: each case is a single standalone write, like the other
controls in the same switch that already pass NULL. In the exposure case
ret is still uninitialized, so a non-zero stack value makes cci_write()
return early without programming the register, and the control write
reports a bogus status. The vertical blanking case is benign today because
ret is zero there, but the construct is equally wrong.

Pass NULL as the error pointer in both cases.

Fixes: be1589e567ae ("media: i2c: imx471: Add Sony IMX471 image sensor driver")
Suggested-by: Kate Hsuan <hpa@redhat.com>
Signed-off-by: David Carlier <devnexen@gmail.com>
---
Changes in v2:
- Fix the root cause by passing NULL as the cci_write() error pointer for
  the exposure and vertical blanking writes, instead of initializing ret
  to zero (Kate Hsuan).

v1: https://lore.kernel.org/linux-media/20260724041417.9921-1-devnexen@gmail.com/

 drivers/media/i2c/imx471.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/imx471.c b/drivers/media/i2c/imx471.c
index 6d358b11e96d..4053aed84340 100644
--- a/drivers/media/i2c/imx471.c
+++ b/drivers/media/i2c/imx471.c
@@ -334,12 +334,12 @@ static int imx471_set_ctrl(struct v4l2_ctrl *ctrl)
 		break;
 	case V4L2_CID_EXPOSURE:
 		ret = cci_write(sensor->regmap, IMX471_REG_EXPOSURE,
-				ctrl->val, &ret);
+				ctrl->val, NULL);
 		break;
 	case V4L2_CID_VBLANK:
 		/* Update FLL that meets expected vertical blanking */
 		ret = cci_write(sensor->regmap, IMX471_REG_FLL,
-				format->height + ctrl->val, &ret);
+				format->height + ctrl->val, NULL);
 		break;
 	case V4L2_CID_TEST_PATTERN:
 		ret = cci_write(sensor->regmap, IMX471_REG_TEST_PATTERN,
-- 
2.53.0

Re: [PATCH v2] media: i2c: imx471: Fix uninitialized error value in imx471_set_ctrl()

[Tarang Raval]

Hi David,

> The exposure and vertical blanking writes pass the address of the local
> ret variable to cci_write() as its error pointer, but there is no earlier
> error to propagate: each case is a single standalone write, like the other
> controls in the same switch that already pass NULL. In the exposure case
> ret is still uninitialized, so a non-zero stack value makes cci_write()
> return early without programming the register, and the control write
> reports a bogus status. The vertical blanking case is benign today because
> ret is zero there, but the construct is equally wrong.
>
> Pass NULL as the error pointer in both cases.

Nice catch! Thanks for spotting this.

Reviewed-by: Tarang Raval <tarang.raval@siliconsignals.io>

Best Regards,
Tarang

>
> Fixes: be1589e567ae ("media: i2c: imx471: Add Sony IMX471 image sensor driver")
> Suggested-by: Kate Hsuan <hpa@redhat.com>
> Signed-off-by: David Carlier <devnexen@gmail.com>
> ---
> Changes in v2:
> - Fix the root cause by passing NULL as the cci_write() error pointer for
>   the exposure and vertical blanking writes, instead of initializing ret
>   to zero (Kate Hsuan).
>
> v1: https://lore.kernel.org/linux-media/20260724041417.9921-1-devnexen@gmail.com/
>
>  drivers/media/i2c/imx471.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/i2c/imx471.c b/drivers/media/i2c/imx471.c
> index 6d358b11e96d..4053aed84340 100644
> --- a/drivers/media/i2c/imx471.c
> +++ b/drivers/media/i2c/imx471.c
> @@ -334,12 +334,12 @@ static int imx471_set_ctrl(struct v4l2_ctrl *ctrl)
>                 break;
>         case V4L2_CID_EXPOSURE:
>                 ret = cci_write(sensor->regmap, IMX471_REG_EXPOSURE,
> -                               ctrl->val, &ret);
> +                               ctrl->val, NULL);
>                 break;
>         case V4L2_CID_VBLANK:
>                 /* Update FLL that meets expected vertical blanking */
>                 ret = cci_write(sensor->regmap, IMX471_REG_FLL,
> -                               format->height + ctrl->val, &ret);
> +                               format->height + ctrl->val, NULL);
>                 break;
>         case V4L2_CID_TEST_PATTERN:
>                 ret = cci_write(sensor->regmap, IMX471_REG_TEST_PATTERN,
> --
> 2.53.0

Re: [PATCH v2] media: i2c: imx471: Fix uninitialized error value in imx471_set_ctrl()

[Kate Hsuan]

Hi David,

Thank you for your work.

On Tue, Jul 28, 2026 at 5:24 PM David Carlier <devnexen@gmail.com> wrote:
>
> The exposure and vertical blanking writes pass the address of the local
> ret variable to cci_write() as its error pointer, but there is no earlier
> error to propagate: each case is a single standalone write, like the other
> controls in the same switch that already pass NULL. In the exposure case
> ret is still uninitialized, so a non-zero stack value makes cci_write()
> return early without programming the register, and the control write
> reports a bogus status. The vertical blanking case is benign today because
> ret is zero there, but the construct is equally wrong.
>
> Pass NULL as the error pointer in both cases.
>
> Fixes: be1589e567ae ("media: i2c: imx471: Add Sony IMX471 image sensor driver")
> Suggested-by: Kate Hsuan <hpa@redhat.com>
> Signed-off-by: David Carlier <devnexen@gmail.com>
> ---
> Changes in v2:
> - Fix the root cause by passing NULL as the cci_write() error pointer for
>   the exposure and vertical blanking writes, instead of initializing ret
>   to zero (Kate Hsuan).
>
> v1: https://lore.kernel.org/linux-media/20260724041417.9921-1-devnexen@gmail.com/
>
>  drivers/media/i2c/imx471.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/i2c/imx471.c b/drivers/media/i2c/imx471.c
> index 6d358b11e96d..4053aed84340 100644
> --- a/drivers/media/i2c/imx471.c
> +++ b/drivers/media/i2c/imx471.c
> @@ -334,12 +334,12 @@ static int imx471_set_ctrl(struct v4l2_ctrl *ctrl)
>                 break;
>         case V4L2_CID_EXPOSURE:
>                 ret = cci_write(sensor->regmap, IMX471_REG_EXPOSURE,
> -                               ctrl->val, &ret);
> +                               ctrl->val, NULL);
>                 break;
>         case V4L2_CID_VBLANK:
>                 /* Update FLL that meets expected vertical blanking */
>                 ret = cci_write(sensor->regmap, IMX471_REG_FLL,
> -                               format->height + ctrl->val, &ret);
> +                               format->height + ctrl->val, NULL);
>                 break;
>         case V4L2_CID_TEST_PATTERN:
>                 ret = cci_write(sensor->regmap, IMX471_REG_TEST_PATTERN,
> --
> 2.53.0
>

It is good for me.

Reviewed-by: Kate Hsuan <hpa@redhat.com>

-- 
BR,
Kate