[PATCH v3 0/6] media: Fix new smatch warnings

[PATCH v3 0/6] media: Fix new smatch warnings

[Ricardo Ribalda]

Current version of smatch triggers some warnings for the media tree.
Most of them are inoffensive, but we would like to have zero smatch
warnings.

drivers/media/platform/amlogic/c3/isp/c3-isp-params.c:111 c3_isp_params_awb_wt() error: buffer overflow 'cfg->zone_weight' 768 <= u32max
drivers/media/platform/amlogic/c3/isp/c3-isp-params.c:111 c3_isp_params_awb_wt() error: buffer overflow 'cfg->zone_weight' 768 <= u32max
drivers/media/platform/amlogic/c3/isp/c3-isp-params.c:227 c3_isp_params_ae_wt() error: buffer overflow 'cfg->zone_weight' 255 <= u32max
drivers/media/platform/amlogic/c3/isp/c3-isp-params.c:227 c3_isp_params_ae_wt() error: buffer overflow 'cfg->zone_weight' 255 <= u32max
drivers/media/v4l2-core/v4l2-dev.c:1036 __video_register_device() error: buffer overflow 'video_devices' 256 <= 288
drivers/media/v4l2-core/v4l2-dev.c:1043 __video_register_device() error: buffer overflow 'video_devices' 256 <= 288
drivers/media/v4l2-core/v4l2-dev.c:1101 __video_register_device() error: buffer overflow 'video_devices' 256 <= 288
drivers/media/platform/chips-media/wave5/wave5-vpuapi.c:588 wave5_vpu_dec_get_output_info() error: buffer overflow 'inst->frame_buf' 64 <= 127
drivers/staging/media/ipu3/ipu3-css-params.c:1792 imgu_css_cfg_acc_stripe() warn: 'acc->stripe.bds_out_stripes[0]->width - 2 * f' 4294967168 can't fit into 65535 'acc->stripe.bds_out_stripes[1]->offset'
drivers/media/i2c/adv7604.c:3672 adv76xx_probe() error: buffer overflow 'state->pads' 7 <= 4294967294
drivers/media/i2c/adv7604.c:3673 adv76xx_probe() error: buffer overflow 'state->pads' 7 <= u32max
drivers/media/i2c/mt9p031.c:799 mt9p031_s_ctrl() warn: assigning (-1952) to unsigned variable 'data'

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
Changes in v3:
- Rewrite mt9p031's bitmask to keep ~7
- Link to v2: https://lore.kernel.org/r/20260501-smatch-7-1-v2-0-a2fcfb2531ac@chromium.org

Changes in v2:
- Remove WARN_ON() in user triggerable checks.
- Add fixes for user triggerable errors.
- Remove pr_err in v4l-dev
- Link to v1: https://lore.kernel.org/r/20260428-smatch-7-1-v1-0-46890dffb611@chromium.org

---
Ricardo Ribalda (6):
      media: v4l2-dev: Add range check for vdev->minor
      media: i2c: mt9p031: Rewrite assignment to make smatch happy
      media: i2c: adv7604: Add range checks for chip info
      media: chips-media: wave5: Add range checks for dec_output_info
      media: staging: ipu3-imgu: Add range check for imgu_css_cfg_acc_stripe
      media: amlogic-c3: Add validations for ae and awb config

 drivers/media/i2c/adv7604.c                             |  6 ++++++
 drivers/media/i2c/mt9p031.c                             |  3 ++-
 drivers/media/platform/amlogic/c3/isp/c3-isp-params.c   |  4 ++++
 drivers/media/platform/chips-media/wave5/wave5-vpuapi.c | 11 +++++++++--
 drivers/media/v4l2-core/v4l2-dev.c                      |  5 +++++
 drivers/staging/media/ipu3/ipu3-css-params.c            |  8 ++++++--
 6 files changed, 32 insertions(+), 5 deletions(-)
---
base-commit: 254f49634ee16a731174d2ae34bc50bd5f45e731
change-id: 20260428-smatch-7-1-d969299dd3cf

Best regards,
-- 
Ricardo Ribalda <ribalda@chromium.org>

[PATCH v3 1/6] media: v4l2-dev: Add range check for vdev->minor

[Ricardo Ribalda]

If the fixed minor ranges are not properly set we could end up in a
situation where the calculated minor is invalid. Add a check for this in
the code to make it more robust.

This check also fixes the following false positive smatch warning:

drivers/media/v4l2-core/v4l2-dev.c:1036 __video_register_device() error: buffer overflow 'video_devices' 256 <= 288
drivers/media/v4l2-core/v4l2-dev.c:1043 __video_register_device() error: buffer overflow 'video_devices' 256 <= 288
drivers/media/v4l2-core/v4l2-dev.c:1101 __video_register_device() error: buffer overflow 'video_devices' 256 <= 288

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/v4l2-core/v4l2-dev.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
index 6ce623a1245a..5516b2bbb08f 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -1032,6 +1032,11 @@ int __video_register_device(struct video_device *vdev,
 	vdev->minor = i + minor_offset;
 	vdev->num = nr;
 
+	if (WARN_ON(vdev->minor >= VIDEO_NUM_DEVICES)) {
+		mutex_unlock(&videodev_lock);
+		return -EINVAL;
+	}
+
 	/* Should not happen since we thought this minor was free */
 	if (WARN_ON(video_devices[vdev->minor])) {
 		mutex_unlock(&videodev_lock);

-- 
2.54.0.545.g6539524ca2-goog

[PATCH v3 2/6] media: i2c: mt9p031: Rewrite assignment to make smatch happy

[Ricardo Ribalda]

The current code makes smatch a bit uncomfortable:
drivers/media/i2c/mt9p031.c:799 mt9p031_s_ctrl() warn: assigning (-1952) to unsigned variable 'data'

Probably because smatch is not clever enough (yet). Do a simple rewrite
to make sure that smatch understands what we are doing here.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/i2c/mt9p031.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
index ea5d43d925ff..8dc57eeba606 100644
--- a/drivers/media/i2c/mt9p031.c
+++ b/drivers/media/i2c/mt9p031.c
@@ -796,7 +796,8 @@ static int mt9p031_s_ctrl(struct v4l2_ctrl *ctrl)
 			data = (1 << 6) | (ctrl->val >> 1);
 		} else {
 			ctrl->val &= ~7;
-			data = ((ctrl->val - 64) << 5) | (1 << 6) | 32;
+			data = ((ctrl->val - 64) >> 3) & 0x7f;
+			data = (data << 8) | (1 << 6) | 32;
 		}
 
 		return mt9p031_write(client, MT9P031_GLOBAL_GAIN, data);

-- 
2.54.0.545.g6539524ca2-goog

[PATCH v3 3/6] media: i2c: adv7604: Add range checks for chip info

[Ricardo Ribalda]

If the driver's chip information is invalid we can end up accessing an
invalid memory region.

This fixes the following false positive smatch errors:
drivers/media/i2c/adv7604.c:3672 adv76xx_probe() error: buffer overflow 'state->pads' 7 <= 4294967294
drivers/media/i2c/adv7604.c:3673 adv76xx_probe() error: buffer overflow 'state->pads' 7 <= u32max

Reviewed-by: Hans Verkuil <hverkuil+cisco@kernel.org>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/i2c/adv7604.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
index 67116a4ef134..ae75982fb514 100644
--- a/drivers/media/i2c/adv7604.c
+++ b/drivers/media/i2c/adv7604.c
@@ -3668,6 +3668,12 @@ static int adv76xx_probe(struct i2c_client *client)
 
 	state->source_pad = state->info->num_dv_ports
 			  + (state->info->has_afe ? 2 : 0);
+	if (WARN_ON(state->source_pad >= ADV76XX_PAD_MAX)) {
+		err = -EINVAL;
+		v4l2_err(sd, "invalid chip info\n");
+		goto err_i2c;
+	}
+
 	for (i = 0; i < state->source_pad; ++i)
 		state->pads[i].flags = MEDIA_PAD_FL_SINK;
 	state->pads[state->source_pad].flags = MEDIA_PAD_FL_SOURCE;

-- 
2.54.0.545.g6539524ca2-goog

[PATCH v3 4/6] media: chips-media: wave5: Add range checks for dec_output_info

[Ricardo Ribalda]

If the driver's dec_output_info contains invalid data the driver can
write in invalid memory. Add a range check for that.

This fixes this smatch error:
drivers/media/platform/chips-media/wave5/wave5-vpuapi.c:588 wave5_vpu_dec_get_output_info() error: buffer overflow 'inst->frame_buf' 64 <= 127

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/platform/chips-media/wave5/wave5-vpuapi.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.c b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.c
index d26ffc942219..f77abd5e122a 100644
--- a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.c
+++ b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.c
@@ -584,8 +584,15 @@ int wave5_vpu_dec_get_output_info(struct vpu_instance *inst, struct dec_output_i
 		p_dec_info->num_of_decoding_fbs : p_dec_info->num_of_display_fbs;
 
 	if (info->index_frame_display >= 0 &&
-	    info->index_frame_display < (int)max_dec_index)
-		info->disp_frame = inst->frame_buf[val + info->index_frame_display];
+	    info->index_frame_display < (int)max_dec_index) {
+		u32 idx = val + info->index_frame_display;
+
+		if (WARN_ON(idx >= MAX_REG_FRAME)) {
+			ret = -EINVAL;
+			goto err_out;
+		}
+		info->disp_frame = inst->frame_buf[idx];
+	}
 
 	info->rd_ptr = p_dec_info->stream_rd_ptr;
 	info->wr_ptr = p_dec_info->stream_wr_ptr;

-- 
2.54.0.545.g6539524ca2-goog

[PATCH v3 5/6] media: staging: ipu3-imgu: Add range check for imgu_css_cfg_acc_stripe

[Ricardo Ribalda]

If the driver's stripe information is invalid it can result in an integer
overflow. Add a range check with a WARN_ON to expose this kind of
error.

This patch fixes the following smatch error:
drivers/staging/media/ipu3/ipu3-css-params.c:1792 imgu_css_cfg_acc_stripe() warn: 'acc->stripe.bds_out_stripes[0]->width - 2 * f' 4294967168 can't fit into 65535 'acc->stripe.bds_out_stripes[1]->offset'

Cc: stable@vger.kernel.org
Fixes: e11110a5b744 ("media: staging/intel-ipu3: css: Compute and program ccs")
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/staging/media/ipu3/ipu3-css-params.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/ipu3/ipu3-css-params.c b/drivers/staging/media/ipu3/ipu3-css-params.c
index 2c48d57a3180..92cce31e35c5 100644
--- a/drivers/staging/media/ipu3/ipu3-css-params.c
+++ b/drivers/staging/media/ipu3/ipu3-css-params.c
@@ -1770,6 +1770,8 @@ static int imgu_css_cfg_acc_stripe(struct imgu_css *css, unsigned int pipe,
 		acc->stripe.bds_out_stripes[0].width =
 			ALIGN(css_pipe->rect[IPU3_CSS_RECT_BDS].width, f);
 	} else {
+		u32 offset;
+
 		/* Image processing is divided into two stripes */
 		acc->stripe.bds_out_stripes[0].width =
 			acc->stripe.bds_out_stripes[1].width =
@@ -1788,8 +1790,10 @@ static int imgu_css_cfg_acc_stripe(struct imgu_css *css, unsigned int pipe,
 			acc->stripe.bds_out_stripes[1].width += f;
 		}
 		/* Overlap between stripes is IPU3_UAPI_ISP_VEC_ELEMS * 4 */
-		acc->stripe.bds_out_stripes[1].offset =
-			acc->stripe.bds_out_stripes[0].width - 2 * f;
+		offset = acc->stripe.bds_out_stripes[0].width - 2 * f;
+		if (offset > 65535)
+			return -EINVAL;
+		acc->stripe.bds_out_stripes[1].offset = offset;
 	}
 
 	acc->stripe.effective_stripes[0].height =

-- 
2.54.0.545.g6539524ca2-goog

[PATCH v3 6/6] media: amlogic-c3: Add validations for ae and awb config

[Ricardo Ribalda]

Avoid invalid memory access if the zones_num is bigger than
zone_weight.

This patch fixes the following smatch errors:
drivers/media/platform/amlogic/c3/isp/c3-isp-params.c:111 c3_isp_params_awb_wt() error: buffer overflow 'cfg->zone_weight' 768 <= u32max
drivers/media/platform/amlogic/c3/isp/c3-isp-params.c:111 c3_isp_params_awb_wt() error: buffer overflow 'cfg->zone_weight' 768 <= u32max
drivers/media/platform/amlogic/c3/isp/c3-isp-params.c:227 c3_isp_params_ae_wt() error: buffer overflow 'cfg->zone_weight' 255 <= u32max
drivers/media/platform/amlogic/c3/isp/c3-isp-params.c:227 c3_isp_params_ae_wt() error: buffer overflow 'cfg->zone_weight' 255 <= u32max

Cc: stable@vger.kernel.org
Fixes: fb2e135208f3 ("media: platform: Add C3 ISP driver")
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/platform/amlogic/c3/isp/c3-isp-params.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/media/platform/amlogic/c3/isp/c3-isp-params.c b/drivers/media/platform/amlogic/c3/isp/c3-isp-params.c
index 6f9ca7a7dd88..aec3eed0e443 100644
--- a/drivers/media/platform/amlogic/c3/isp/c3-isp-params.c
+++ b/drivers/media/platform/amlogic/c3/isp/c3-isp-params.c
@@ -104,6 +104,8 @@ static void c3_isp_params_awb_wt(struct c3_isp_device *isp,
 	c3_isp_write(isp, ISP_AWB_BLK_WT_ADDR, 0);
 
 	zones_num = cfg->horiz_zones_num * cfg->vert_zones_num;
+	if (zones_num > C3_ISP_AWB_MAX_ZONES)
+		zones_num = C3_ISP_AWB_MAX_ZONES;
 
 	/* Need to write 8 weights at once */
 	for (i = 0; i < zones_num / 8; i++) {
@@ -220,6 +222,8 @@ static void c3_isp_params_ae_wt(struct c3_isp_device *isp,
 	c3_isp_write(isp, ISP_AE_BLK_WT_ADDR, 0);
 
 	zones_num = cfg->horiz_zones_num * cfg->vert_zones_num;
+	if (zones_num > C3_ISP_AE_MAX_ZONES)
+		zones_num = C3_ISP_AE_MAX_ZONES;
 
 	/* Need to write 8 weights at once */
 	for (i = 0; i < zones_num / 8; i++) {

-- 
2.54.0.545.g6539524ca2-goog

Re: [PATCH v3 6/6] media: amlogic-c3: Add validations for ae and awb config

[Jacopo Mondi]

Hi Ricardo

On Mon, May 04, 2026 at 06:54:09AM +0000, Ricardo Ribalda wrote:
> Avoid invalid memory access if the zones_num is bigger than
> zone_weight.
>
> This patch fixes the following smatch errors:
> drivers/media/platform/amlogic/c3/isp/c3-isp-params.c:111 c3_isp_params_awb_wt() error: buffer overflow 'cfg->zone_weight' 768 <= u32max
> drivers/media/platform/amlogic/c3/isp/c3-isp-params.c:111 c3_isp_params_awb_wt() error: buffer overflow 'cfg->zone_weight' 768 <= u32max
> drivers/media/platform/amlogic/c3/isp/c3-isp-params.c:227 c3_isp_params_ae_wt() error: buffer overflow 'cfg->zone_weight' 255 <= u32max
> drivers/media/platform/amlogic/c3/isp/c3-isp-params.c:227 c3_isp_params_ae_wt() error: buffer overflow 'cfg->zone_weight' 255 <= u32max
>
> Cc: stable@vger.kernel.org
> Fixes: fb2e135208f3 ("media: platform: Add C3 ISP driver")
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/platform/amlogic/c3/isp/c3-isp-params.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/media/platform/amlogic/c3/isp/c3-isp-params.c b/drivers/media/platform/amlogic/c3/isp/c3-isp-params.c
> index 6f9ca7a7dd88..aec3eed0e443 100644
> --- a/drivers/media/platform/amlogic/c3/isp/c3-isp-params.c
> +++ b/drivers/media/platform/amlogic/c3/isp/c3-isp-params.c
> @@ -104,6 +104,8 @@ static void c3_isp_params_awb_wt(struct c3_isp_device *isp,
>  	c3_isp_write(isp, ISP_AWB_BLK_WT_ADDR, 0);
>
>  	zones_num = cfg->horiz_zones_num * cfg->vert_zones_num;
> +	if (zones_num > C3_ISP_AWB_MAX_ZONES)
> +		zones_num = C3_ISP_AWB_MAX_ZONES;

Or
        zones_num = min(cfg->horiz_zones_num * cfg->vert_zones_num,
                        C3_ISP_AWB_MAX_ZONES);

Whatever you prefer:
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

>
>  	/* Need to write 8 weights at once */
>  	for (i = 0; i < zones_num / 8; i++) {
> @@ -220,6 +222,8 @@ static void c3_isp_params_ae_wt(struct c3_isp_device *isp,
>  	c3_isp_write(isp, ISP_AE_BLK_WT_ADDR, 0);
>
>  	zones_num = cfg->horiz_zones_num * cfg->vert_zones_num;
> +	if (zones_num > C3_ISP_AE_MAX_ZONES)
> +		zones_num = C3_ISP_AE_MAX_ZONES;
>
>  	/* Need to write 8 weights at once */
>  	for (i = 0; i < zones_num / 8; i++) {
>
> --
> 2.54.0.545.g6539524ca2-goog
>

Re: [PATCH v3 2/6] media: i2c: mt9p031: Rewrite assignment to make smatch happy

[Laurent Pinchart]

On Mon, May 04, 2026 at 06:54:05AM +0000, Ricardo Ribalda wrote:
> The current code makes smatch a bit uncomfortable:
> drivers/media/i2c/mt9p031.c:799 mt9p031_s_ctrl() warn: assigning (-1952) to unsigned variable 'data'
> 
> Probably because smatch is not clever enough (yet). Do a simple rewrite
> to make sure that smatch understands what we are doing here.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/i2c/mt9p031.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
> index ea5d43d925ff..8dc57eeba606 100644
> --- a/drivers/media/i2c/mt9p031.c
> +++ b/drivers/media/i2c/mt9p031.c
> @@ -796,7 +796,8 @@ static int mt9p031_s_ctrl(struct v4l2_ctrl *ctrl)
>  			data = (1 << 6) | (ctrl->val >> 1);
>  		} else {
>  			ctrl->val &= ~7;
> -			data = ((ctrl->val - 64) << 5) | (1 << 6) | 32;
> +			data = ((ctrl->val - 64) >> 3) & 0x7f;
> +			data = (data << 8) | (1 << 6) | 32;
>  		}
>  
>  		return mt9p031_write(client, MT9P031_GLOBAL_GAIN, data);

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 1/6] media: v4l2-dev: Add range check for vdev->minor

[Laurent Pinchart]

On Mon, May 04, 2026 at 06:54:04AM +0000, Ricardo Ribalda wrote:
> If the fixed minor ranges are not properly set we could end up in a
> situation where the calculated minor is invalid. Add a check for this in
> the code to make it more robust.

If it was just for that, we could define the ranges in a way that could
not lead to future programmatic errors.

> This check also fixes the following false positive smatch warning:
> 
> drivers/media/v4l2-core/v4l2-dev.c:1036 __video_register_device() error: buffer overflow 'video_devices' 256 <= 288
> drivers/media/v4l2-core/v4l2-dev.c:1043 __video_register_device() error: buffer overflow 'video_devices' 256 <= 288
> drivers/media/v4l2-core/v4l2-dev.c:1101 __video_register_device() error: buffer overflow 'video_devices' 256 <= 288
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/v4l2-core/v4l2-dev.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> index 6ce623a1245a..5516b2bbb08f 100644
> --- a/drivers/media/v4l2-core/v4l2-dev.c
> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> @@ -1032,6 +1032,11 @@ int __video_register_device(struct video_device *vdev,
>  	vdev->minor = i + minor_offset;
>  	vdev->num = nr;
>  
> +	if (WARN_ON(vdev->minor >= VIDEO_NUM_DEVICES)) {
> +		mutex_unlock(&videodev_lock);

I may get tempted to convert code to using scoped guards at some point.

> +		return -EINVAL;
> +	}
> +

I'm annoyed by the proliferation of workarounds for smatch false
positives that generate useless code :-/ This is in particular is not a
big deal though, so

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

but I don't want to continue in this direction with every new kernel
release. We need a way to tell smatch that this is safe with incurring
an runtime cost.

>  	/* Should not happen since we thought this minor was free */
>  	if (WARN_ON(video_devices[vdev->minor])) {
>  		mutex_unlock(&videodev_lock);
> 

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 6/6] media: amlogic-c3: Add validations for ae and awb config

[Laurent Pinchart]

On Mon, May 04, 2026 at 09:19:30AM +0200, Jacopo Mondi wrote:
> On Mon, May 04, 2026 at 06:54:09AM +0000, Ricardo Ribalda wrote:
> > Avoid invalid memory access if the zones_num is bigger than
> > zone_weight.
> >
> > This patch fixes the following smatch errors:
> > drivers/media/platform/amlogic/c3/isp/c3-isp-params.c:111 c3_isp_params_awb_wt() error: buffer overflow 'cfg->zone_weight' 768 <= u32max
> > drivers/media/platform/amlogic/c3/isp/c3-isp-params.c:111 c3_isp_params_awb_wt() error: buffer overflow 'cfg->zone_weight' 768 <= u32max
> > drivers/media/platform/amlogic/c3/isp/c3-isp-params.c:227 c3_isp_params_ae_wt() error: buffer overflow 'cfg->zone_weight' 255 <= u32max
> > drivers/media/platform/amlogic/c3/isp/c3-isp-params.c:227 c3_isp_params_ae_wt() error: buffer overflow 'cfg->zone_weight' 255 <= u32max
> >
> > Cc: stable@vger.kernel.org
> > Fixes: fb2e135208f3 ("media: platform: Add C3 ISP driver")
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> >  drivers/media/platform/amlogic/c3/isp/c3-isp-params.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/media/platform/amlogic/c3/isp/c3-isp-params.c b/drivers/media/platform/amlogic/c3/isp/c3-isp-params.c
> > index 6f9ca7a7dd88..aec3eed0e443 100644
> > --- a/drivers/media/platform/amlogic/c3/isp/c3-isp-params.c
> > +++ b/drivers/media/platform/amlogic/c3/isp/c3-isp-params.c
> > @@ -104,6 +104,8 @@ static void c3_isp_params_awb_wt(struct c3_isp_device *isp,
> >  	c3_isp_write(isp, ISP_AWB_BLK_WT_ADDR, 0);
> >
> >  	zones_num = cfg->horiz_zones_num * cfg->vert_zones_num;
> > +	if (zones_num > C3_ISP_AWB_MAX_ZONES)
> > +		zones_num = C3_ISP_AWB_MAX_ZONES;
> 
> Or
>         zones_num = min(cfg->horiz_zones_num * cfg->vert_zones_num,
>                         C3_ISP_AWB_MAX_ZONES);

I have a slight preference for that, but both options work for me.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> Whatever you prefer:
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> 
> >
> >  	/* Need to write 8 weights at once */
> >  	for (i = 0; i < zones_num / 8; i++) {
> > @@ -220,6 +222,8 @@ static void c3_isp_params_ae_wt(struct c3_isp_device *isp,
> >  	c3_isp_write(isp, ISP_AE_BLK_WT_ADDR, 0);
> >
> >  	zones_num = cfg->horiz_zones_num * cfg->vert_zones_num;
> > +	if (zones_num > C3_ISP_AE_MAX_ZONES)
> > +		zones_num = C3_ISP_AE_MAX_ZONES;
> >
> >  	/* Need to write 8 weights at once */
> >  	for (i = 0; i < zones_num / 8; i++) {

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 1/6] media: v4l2-dev: Add range check for vdev->minor

[Ricardo Ribalda]

Hi Laurent

On Wed, 6 May 2026 at 01:12, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Mon, May 04, 2026 at 06:54:04AM +0000, Ricardo Ribalda wrote:
> > If the fixed minor ranges are not properly set we could end up in a
> > situation where the calculated minor is invalid. Add a check for this in
> > the code to make it more robust.
>
> If it was just for that, we could define the ranges in a way that could
> not lead to future programmatic errors.
>
> > This check also fixes the following false positive smatch warning:
> >
> > drivers/media/v4l2-core/v4l2-dev.c:1036 __video_register_device() error: buffer overflow 'video_devices' 256 <= 288
> > drivers/media/v4l2-core/v4l2-dev.c:1043 __video_register_device() error: buffer overflow 'video_devices' 256 <= 288
> > drivers/media/v4l2-core/v4l2-dev.c:1101 __video_register_device() error: buffer overflow 'video_devices' 256 <= 288
> >
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> >  drivers/media/v4l2-core/v4l2-dev.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> > index 6ce623a1245a..5516b2bbb08f 100644
> > --- a/drivers/media/v4l2-core/v4l2-dev.c
> > +++ b/drivers/media/v4l2-core/v4l2-dev.c
> > @@ -1032,6 +1032,11 @@ int __video_register_device(struct video_device *vdev,
> >       vdev->minor = i + minor_offset;
> >       vdev->num = nr;
> >
> > +     if (WARN_ON(vdev->minor >= VIDEO_NUM_DEVICES)) {
> > +             mutex_unlock(&videodev_lock);
>
> I may get tempted to convert code to using scoped guards at some point.
>
> > +             return -EINVAL;
> > +     }
> > +
>
> I'm annoyed by the proliferation of workarounds for smatch false
> positives that generate useless code :-/ This is in particular is not a
> big deal though, so
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>
> but I don't want to continue in this direction with every new kernel
> release. We need a way to tell smatch that this is safe with incurring
> an runtime cost.

To provide some context, in this release alone we have found two "Fixes:".

Do you have any data regarding the runtime cost of these new checks?
Most of the time, the compiler simply optimizes them out, so the
impact is either zero or minimal.

The added checks make the code more robust for future refactoring and
serve as useful documentation. Furthermore, when false positives occur
in new code, they force the author to write more idiomatic code, which
improves maintainability. We have a deficit of maintainers, not
authors.

So, I DO want to continue in this direction. I am very grateful for
these static analysis tools; they truly make our code more
maintainable.

Regards!


>
> >       /* Should not happen since we thought this minor was free */
> >       if (WARN_ON(video_devices[vdev->minor])) {
> >               mutex_unlock(&videodev_lock);
> >
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Ricardo Ribalda

Re: [PATCH v3 1/6] media: v4l2-dev: Add range check for vdev->minor

[Laurent Pinchart]

On Wed, May 06, 2026 at 08:48:06AM +0200, Ricardo Ribalda wrote:
> On Wed, 6 May 2026 at 01:12, Laurent Pinchart wrote:
> > On Mon, May 04, 2026 at 06:54:04AM +0000, Ricardo Ribalda wrote:
> > > If the fixed minor ranges are not properly set we could end up in a
> > > situation where the calculated minor is invalid. Add a check for this in
> > > the code to make it more robust.
> >
> > If it was just for that, we could define the ranges in a way that could
> > not lead to future programmatic errors.
> >
> > > This check also fixes the following false positive smatch warning:
> > >
> > > drivers/media/v4l2-core/v4l2-dev.c:1036 __video_register_device() error: buffer overflow 'video_devices' 256 <= 288
> > > drivers/media/v4l2-core/v4l2-dev.c:1043 __video_register_device() error: buffer overflow 'video_devices' 256 <= 288
> > > drivers/media/v4l2-core/v4l2-dev.c:1101 __video_register_device() error: buffer overflow 'video_devices' 256 <= 288
> > >
> > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > ---
> > >  drivers/media/v4l2-core/v4l2-dev.c | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > >
> > > diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> > > index 6ce623a1245a..5516b2bbb08f 100644
> > > --- a/drivers/media/v4l2-core/v4l2-dev.c
> > > +++ b/drivers/media/v4l2-core/v4l2-dev.c
> > > @@ -1032,6 +1032,11 @@ int __video_register_device(struct video_device *vdev,
> > >       vdev->minor = i + minor_offset;
> > >       vdev->num = nr;
> > >
> > > +     if (WARN_ON(vdev->minor >= VIDEO_NUM_DEVICES)) {
> > > +             mutex_unlock(&videodev_lock);
> >
> > I may get tempted to convert code to using scoped guards at some point.
> >
> > > +             return -EINVAL;
> > > +     }
> > > +
> >
> > I'm annoyed by the proliferation of workarounds for smatch false
> > positives that generate useless code :-/ This is in particular is not a
> > big deal though, so
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> >
> > but I don't want to continue in this direction with every new kernel
> > release. We need a way to tell smatch that this is safe with incurring
> > an runtime cost.
> 
> To provide some context, in this release alone we have found two "Fixes:".
> 
> Do you have any data regarding the runtime cost of these new checks?
> Most of the time, the compiler simply optimizes them out, so the
> impact is either zero or minimal.

If the compiler can optimize them out, then I expect the static analysis
tools to also get the ability to avoid those false positives.

> The added checks make the code more robust for future refactoring and
> serve as useful documentation.

In this specific case, if we want to make the code more robust, I think
we should have an array of minor offsets and calculate the minor ranges
based on that. It would avoid offsets and ranges getting out of sync. I
don't think it's worth it though, because I don't foresee we will ever
change ranges for the fixed minors case.

> Furthermore, when false positives occur
> in new code, they force the author to write more idiomatic code, which
> improves maintainability.

Up to a point only. Sometimes guarantees are provided in a very remote
place. For instance, control values have ranges that are set when
creating the control, and enforced by the control framework. Adding
range checks to .s_ctrl() is redundant, but I don't expect static
analysis tools to be able to understand that the value is guaranteed to
be within the range (especially given that we support modifying ranges
at runtime). How can we avoid those redundant checks (or mask
operations, as in the mt9p031 patch in this series) ?

> We have a deficit of maintainers, not
> authors.
> 
> So, I DO want to continue in this direction. I am very grateful for
> these static analysis tools; they truly make our code more
> maintainable.

I do like smatch a lot, it points out real issues. My concern is that we
have code that offers guarantees in ways that are not visible to static
analysis tools (or to compilers), and having to add more runtime checks
everywhere to silence warning is not nice. Those checks can over time
become redundant (because tools improve, or because we remove the code),
and will likely not be removed because nobody notices.

There's a broader question here of how to connect pieces of code where
one piece offers a guarantee that the other piece depends on, in a way
that can be exposed to tools.

> > >       /* Should not happen since we thought this minor was free */
> > >       if (WARN_ON(video_devices[vdev->minor])) {
> > >               mutex_unlock(&videodev_lock);
> > >

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 1/6] media: v4l2-dev: Add range check for vdev->minor

[Ricardo Ribalda]

Hi Laurent

On Wed, 6 May 2026 at 13:18, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Wed, May 06, 2026 at 08:48:06AM +0200, Ricardo Ribalda wrote:
> > On Wed, 6 May 2026 at 01:12, Laurent Pinchart wrote:
> > > On Mon, May 04, 2026 at 06:54:04AM +0000, Ricardo Ribalda wrote:
> > > > If the fixed minor ranges are not properly set we could end up in a
> > > > situation where the calculated minor is invalid. Add a check for this in
> > > > the code to make it more robust.
> > >
> > > If it was just for that, we could define the ranges in a way that could
> > > not lead to future programmatic errors.
> > >
> > > > This check also fixes the following false positive smatch warning:
> > > >
> > > > drivers/media/v4l2-core/v4l2-dev.c:1036 __video_register_device() error: buffer overflow 'video_devices' 256 <= 288
> > > > drivers/media/v4l2-core/v4l2-dev.c:1043 __video_register_device() error: buffer overflow 'video_devices' 256 <= 288
> > > > drivers/media/v4l2-core/v4l2-dev.c:1101 __video_register_device() error: buffer overflow 'video_devices' 256 <= 288
> > > >
> > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > > ---
> > > >  drivers/media/v4l2-core/v4l2-dev.c | 5 +++++
> > > >  1 file changed, 5 insertions(+)
> > > >
> > > > diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> > > > index 6ce623a1245a..5516b2bbb08f 100644
> > > > --- a/drivers/media/v4l2-core/v4l2-dev.c
> > > > +++ b/drivers/media/v4l2-core/v4l2-dev.c
> > > > @@ -1032,6 +1032,11 @@ int __video_register_device(struct video_device *vdev,
> > > >       vdev->minor = i + minor_offset;
> > > >       vdev->num = nr;
> > > >
> > > > +     if (WARN_ON(vdev->minor >= VIDEO_NUM_DEVICES)) {
> > > > +             mutex_unlock(&videodev_lock);
> > >
> > > I may get tempted to convert code to using scoped guards at some point.
> > >
> > > > +             return -EINVAL;
> > > > +     }
> > > > +
> > >
> > > I'm annoyed by the proliferation of workarounds for smatch false
> > > positives that generate useless code :-/ This is in particular is not a
> > > big deal though, so
> > >
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > >
> > > but I don't want to continue in this direction with every new kernel
> > > release. We need a way to tell smatch that this is safe with incurring
> > > an runtime cost.
> >
> > To provide some context, in this release alone we have found two "Fixes:".
> >
> > Do you have any data regarding the runtime cost of these new checks?
> > Most of the time, the compiler simply optimizes them out, so the
> > impact is either zero or minimal.
>
> If the compiler can optimize them out, then I expect the static analysis
> tools to also get the ability to avoid those false positives.

Ideally yes, but LLVM/gcc has hundreds (maybe thousands?) of
contributors and sparse/smatch are maintained by a handful group of
heroic warriors.

>
> > The added checks make the code more robust for future refactoring and
> > serve as useful documentation.
>
> In this specific case, if we want to make the code more robust, I think
> we should have an array of minor offsets and calculate the minor ranges
> based on that. It would avoid offsets and ranges getting out of sync. I
> don't think it's worth it though, because I don't foresee we will ever
> change ranges for the fixed minors case.
>
> > Furthermore, when false positives occur
> > in new code, they force the author to write more idiomatic code, which
> > improves maintainability.
>
> Up to a point only. Sometimes guarantees are provided in a very remote
> place. For instance, control values have ranges that are set when
> creating the control, and enforced by the control framework. Adding
> range checks to .s_ctrl() is redundant, but I don't expect static
> analysis tools to be able to understand that the value is guaranteed to
> be within the range (especially given that we support modifying ranges
> at runtime). How can we avoid those redundant checks (or mask
> operations, as in the mt9p031 patch in this series) ?

It depends on the nature of the range check and the consequences of
failing it. We can decide case by case and worst case scenario we can
ignore specific errors.


The mt9p031 patch in this series is special, the issue is not the
range check, but that smatch did not properly handle a mask operation.
Dan is working to fix it.



>
> > We have a deficit of maintainers, not
> > authors.
> >
> > So, I DO want to continue in this direction. I am very grateful for
> > these static analysis tools; they truly make our code more
> > maintainable.
>
> I do like smatch a lot, it points out real issues. My concern is that we
> have code that offers guarantees in ways that are not visible to static
> analysis tools (or to compilers), and having to add more runtime checks
> everywhere to silence warning is not nice. Those checks can over time
> become redundant (because tools improve, or because we remove the code),
> and will likely not be removed because nobody notices.
>
> There's a broader question here of how to connect pieces of code where
> one piece offers a guarantee that the other piece depends on, in a way
> that can be exposed to tools.

I agree with the idea, but this might not the right forum. It would be
a great topic for the Kernel Summit, though.

The "rust people" would love to have some annotations about the
guarantees of the C code.

But until we have those annotations the best we can do is runtime
checks where they make sense and keep track of where they do not make
sense (allow lists). That will make future discussions more
productive.

>
> > > >       /* Should not happen since we thought this minor was free */
> > > >       if (WARN_ON(video_devices[vdev->minor])) {
> > > >               mutex_unlock(&videodev_lock);
> > > >
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Ricardo Ribalda