Re: [PATCH v2 09/13] media: stm32: dcmipp: addition of a dcmipp-isp subdev

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Critical] A kernel panic can be triggered from userspace via a division by zero in dcmipp_isp_config_decimation() due to an improperly constrained minimum crop height.
- [High] Shifting `crop->top` by 12 bits without masking causes it to structurally overlap with `DCMIPP_P1SRCR_CROPEN` and adjacent fields, leading to hardware register corruption.
- [Medium] Missing cleanup of V4L2 subdev active state and media pads in dcmipp_isp_release().
- [Medium] Pad 2 format dimensions are out-of-sync after changing sink pad crop/compose rectangles.
- [Medium] Lockless data race and incorrect logic when checking `v4l2_subdev_is_streaming(sd)` for TRY formats and selections.
- [Low] Incorrect error log message when disabling streams.
--

commit af4dc8e19ff9f5d7a94a2442ffa7e909d7948584
Author: Alain Volmat <alain.volmat@foss.st.com>
media: stm32: dcmipp: addition of a dcmipp-isp subdev

The ISP subdev is the first element after input of the main pipeline.
Part (static configuration) of this block is done via this subdev while
other configuration done on a per-frame basis will be done via a output
metadata device attached to this subdev.

> diff --git a/drivers/media/platform/st/stm32/stm32-dcmipp/dcmipp-isp.c b/drivers/media/platform/st/stm32/stm32-dcmipp/dcmipp-isp.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/media/platform/st/stm32/stm32-dcmipp/dcmipp-isp.c
[ ... ]
> +static int dcmipp_isp_set_fmt(struct v4l2_subdev *sd,
> +			      struct v4l2_subdev_state *state,
> +			      struct v4l2_subdev_format *fmt)
> +{
> +	if (v4l2_subdev_is_streaming(sd))

[Severity: Medium]
Does this code locklessly reject TRY operations when streaming is active?

These format modifications are blocked if v4l2_subdev_is_streaming(sd)
is true, but the code fails to verify whether the format being changed is
V4L2_SUBDEV_FORMAT_TRY.  Rejecting TRY operations breaks userspace tooling
that expects to safely test formats while the active stream runs.

Also, calling v4l2_subdev_is_streaming() here reads sd->enabled_pads
locklessly when invoked for TRY formats, creating a race condition against
v4l2_subdev_enable_streams().

> +		return -EBUSY;
[ ... ]
> +static void dcmipp_isp_adjust_crop(struct v4l2_rect *r,
> +				   const struct v4l2_mbus_framefmt *fmt)
> +{
> +	struct v4l2_rect src_rect = {
> +		.top = 0,
> +		.left = 0,
> +		.width = fmt->width,
> +		.height = fmt->height,
> +	};
> +	struct v4l2_rect crop_min = {
> +		.top = 8,

[Severity: Critical]
Does this code inadvertently set the minimum crop height to 1 instead of 8?

A typo initializes crop_min with .top = 8 instead of .height = 8.
If userspace sets the crop height to 1 and subsequently requests a compose
height of 0 via V4L2_SEL_TGT_COMPOSE, dcmipp_isp_set_compose() scales size
down to 0, which sets compose->height to 0.

Later, during streaming initialization, dcmipp_isp_config_decimation()
would evaluate crop->height / compose->height, causing a fatal division
by zero kernel panic.

> +		.left = 0,
> +		.width = fmt->width,
> +		.height = 1,
> +	};
[ ... ]
> +static int dcmipp_isp_set_selection(struct v4l2_subdev *sd,
> +				    struct v4l2_subdev_state *state,
> +				    struct v4l2_subdev_selection *s)
> +{
> +	struct dcmipp_isp_device *isp = v4l2_get_subdevdata(sd);
> +	struct v4l2_mbus_framefmt *sink_fmt, *src_fmt;
> +	struct v4l2_rect *crop, *compose;
> +
> +	if (IS_SRC(s->pad))
> +		return -EINVAL;
> +
> +	if (v4l2_subdev_is_streaming(sd))

[Severity: Medium]
Does this code also locklessly reject TRY selections when streaming is active?

> +		return -EBUSY;
[ ... ]
> +	/* Update the source pad size */
> +	src_fmt = v4l2_subdev_state_get_format(state, 1);
> +	src_fmt->width = s->r.width;
> +	src_fmt->height = s->r.height;

[Severity: Medium]
Does this code leave pad 2 with out-of-sync format dimensions?

When a crop or compose operation alters the outgoing image dimensions,
only pad 1 is updated with the new dimensions.  Pad 2 is omitted entirely,
leaving it with stale format dimensions that no longer reflect the actual
composed image size.

> +
> +	return 0;
> +}
[ ... ]
> +static void dcmipp_isp_config_decimation(struct dcmipp_isp_device *isp,
> +					 struct v4l2_subdev_state *state)
> +{
> +	struct v4l2_rect *crop = v4l2_subdev_state_get_crop(state, 0);
> +	struct v4l2_rect *compose = v4l2_subdev_state_get_compose(state, 0);
> +	u32 decr;
> +
> +	decr = (fls(crop->width / compose->width) - 1) << DCMIPP_P1DECR_HDEC_SHIFT |
> +	       (fls(crop->height / compose->height) - 1) << DCMIPP_P1DECR_VDEC_SHIFT;

[Severity: Critical]
Does this cause a division by zero kernel panic if compose->height is 0?

Because dcmipp_isp_adjust_crop() mistakenly assigns .top = 8 instead of
.height = 8, userspace can set the compose height to 0, which leads to
a fatal division by zero here.

> +	if (decr)
> +		decr |= DCMIPP_P1DECR_ENABLE;
> +
> +	reg_write(isp, DCMIPP_P1DECR, decr);
> +}
[ ... ]
> +static int dcmipp_isp_enable_streams(struct v4l2_subdev *sd,
> +				     struct v4l2_subdev_state *state,
> +				     u32 pad, u64 streams_mask)
> +{
[ ... ]
> +	/* Configure Statistic Removal */
> +	crop = v4l2_subdev_state_get_crop(state, 0);
> +	reg_write(isp, DCMIPP_P1SRCR,
> +		  ((crop->top << DCMIPP_P1SRCR_FIRSTLINEDEL_SHIFT) |

[Severity: High]
Does shifting crop->top by 12 bits without masking structurally overlap with
DCMIPP_P1SRCR_CROPEN and clobber adjacent register fields?

DCMIPP_P1SRCR_FIRSTLINEDEL_SHIFT is defined as 12, while DCMIPP_P1SRCR_CROPEN
is BIT(15).  If crop->top specifies dozens or hundreds of lines to drop,
shifting a value like 100 by 12 places it across bits 12 through 23, creating
a structural overlap with CROPEN and potentially causing hardware state
corruption.

> +		   (crop->height << DCMIPP_P1SRCR_LASTLINE_SHIFT) |
> +		   DCMIPP_P1SRCR_CROPEN));
[ ... ]
> +static int dcmipp_isp_disable_streams(struct v4l2_subdev *sd,
> +				      struct v4l2_subdev_state *state,
> +				      u32 pad, u64 streams_mask)
> +{
[ ... ]
> +	ret = v4l2_subdev_disable_streams(s_subdev, s_pad->index, BIT_ULL(0));
> +	if (ret < 0) {
> +		dev_err(isp->dev,
> +			"failed to start source subdev streaming (%d)\n", ret);

[Severity: Low]
This isn't a bug, but is this error log message printing 'start' instead
of 'stop' or 'disable'?

> +		return ret;
> +	}
[ ... ]
> +static void dcmipp_isp_release(struct v4l2_subdev *sd)
> +{
> +	struct dcmipp_isp_device *isp = v4l2_get_subdevdata(sd);
> +
> +	kfree(isp);

[Severity: Medium]
Does this code leak the V4L2 subdev active state and media pads?

In dcmipp_isp_ent_init(), dcmipp_ent_sd_register() allocates media pads
(ved->pads) via dcmipp_pads_init() and the subdev's active state
(sd->active_state) via v4l2_subdev_init_finalize().  However, this release
callback completely omits the required dcmipp_pads_cleanup() and
v4l2_subdev_cleanup(), causing a memory leak upon device removal or unbind.

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260602-stm32-dcmipp-pixel-pipes-support-v2-0-3c76b5f93157@foss.st.com?part=9