Re: [PATCH v2 11/13] media: stm32: dcmipp: add pixel-pipe support in bytecap

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:

New issues:
- [High] Unprivileged userspace can trigger kernel panics (`WARN_ON`) by requesting an unaligned width for the dump pipe.
- [High] Inadequate width and height alignment constraints for planar formats like YUV420 result in misaligned chroma strides and plane addresses.
- [High] Missing `else` block in `dcmipp_bytecap_link_validate` causes link validation to unconditionally fail for pixel pipes when the source width is unaligned.
- [High] Error handling for `pipe_id` initialization is defeated by a signedness error, leading to out-of-bounds memory accesses.

Pre-existing issues:
- [Critical] NULL pointer dereference when restarting capture after queue starvation.
- [High] DMA addresses are permanently cached in `buf_prepare`, breaking DMABUF and USERPTR memory models when backing memory changes.
- [High] A race condition between `stop_streaming` and the IRQ thread leads to a Use-After-Free of V4L2 buffers.
--

commit fc3be1eca23ef149d9562a27aaccdb4c295d85ad
Author: Alain Volmat <alain.volmat@foss.st.com>
Subject: media: stm32: dcmipp: add pixel-pipe support in bytecap

This commit adds pixel pipe capture support to the existing bytecap driver to
avoid code duplication, since buffer handling and format manipulation are
similar. It differentiates dump and pixel pipes by introducing a pipe_id and
adds support for configuring the pixel packer on pixel pipes.

> diff --git a/drivers/media/platform/st/stm32/stm32-dcmipp/dcmipp-bytecap.c b/drivers/media/platform/st/stm32/stm32-dcmipp/dcmipp-bytecap.c
> index bd6f2d68df80c..d4432a9fa0e24 100644
> --- a/drivers/media/platform/st/stm32/stm32-dcmipp/dcmipp-bytecap.c
> +++ b/drivers/media/platform/st/stm32/stm32-dcmipp/dcmipp-bytecap.c
> @@ -168,6 +274,73 @@ static const struct v4l2_pix_format fmt_default = {
>  	.xfer_func = DCMIPP_XFER_FUNC_DEFAULT,
>  };
>  
> +static inline int hdw_pixel_alignment(u32 format)
> +{
> +	/* 16 bytes alignment required by hardware */
> +	switch (format) {
> +	case V4L2_PIX_FMT_NV12:
> +	case V4L2_PIX_FMT_NV21:
> +	case V4L2_PIX_FMT_YUV420:
> +	case V4L2_PIX_FMT_YVU420:
> +	case V4L2_PIX_FMT_NV16:
> +	case V4L2_PIX_FMT_NV61:
> +	case V4L2_PIX_FMT_GREY:
> +		return 4;/* 2^4 = 16 pixels = 16 bytes */

[Severity: High]
Does this provide an adequate alignment constraint for planar formats like 
YUV420? Since the chroma plane stride is width / 2, a width of 16 pixels 
yields an 8-byte chroma stride. Since buf_prepare() asserts 16-byte alignment 
across all planes, will this trigger the WARN_ON() checks for unaligned 
strides and plane addresses?

[ ... ]
> @@ -192,19 +365,20 @@ static int dcmipp_bytecap_try_fmt_vid_cap(struct file *file, void *priv,
>  {
>  	struct dcmipp_bytecap_device *vcap = video_drvdata(file);
>  	struct v4l2_pix_format *format = &f->fmt.pix;
> -	const struct dcmipp_bytecap_pix_map *vpix;
> +	/* Take into consideration the width constraint of the pixel packer */
> +	unsigned int walign = (vcap->pipe_id != 0 ?
> +			       hdw_pixel_alignment(format->pixelformat) : 0);
>  	u32 in_w, in_h;

[Severity: High]
If walign is 0 for the dump pipe, unaligned widths can be negotiated. Will
this cause the unconditional WARN_ON() checks in dcmipp_bytecap_buf_prepare()
to trigger on user-controlled unaligned geometry?

[ ... ]
> @@ -494,25 +701,26 @@ static void dcmipp_bytecap_stop_streaming(struct vb2_queue *vq)
>  
>  	/* Disable pipe */
> -	reg_clear(vcap, DCMIPP_P0FSCR, DCMIPP_P0FSCR_PIPEN);
> +	reg_clear(vcap, DCMIPP_PxFSCR(vcap->pipe_id), DCMIPP_PxFSCR_PIPEN);
>  
>  	/* Clear any pending interrupts */
> -	reg_write(vcap, DCMIPP_CMFCR, DCMIPP_CMIER_P0ALL);
> +	reg_write(vcap, DCMIPP_CMFCR, DCMIPP_CMIER_PxALL(vcap->pipe_id));
>  
>  	spin_lock_irq(&vcap->irqlock);
>  
> @@ -525,7 +733,8 @@ static void dcmipp_bytecap_stop_streaming(struct vb2_queue *vq)
>  
>  	spin_unlock_irq(&vcap->irqlock);

[Severity: High]
This is a pre-existing issue, but does dcmipp_bytecap_stop_streaming() lack
synchronization with the IRQ thread? The buffers are freed and vcap->active is
cleared, but vcap->next remains intact. If the IRQ thread executes concurrently,
can it swap the dangling next pointer into active and use it, leading to a
Use-After-Free?

[ ... ]
> @@ -541,7 +750,9 @@ static int dcmipp_bytecap_buf_prepare(struct vb2_buffer *vb)
>  	struct dcmipp_bytecap_device *vcap =  vb2_get_drv_priv(vb->vb2_queue);
>  	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
>  	struct dcmipp_buf *buf = container_of(vbuf, struct dcmipp_buf, vb);
> +	struct v4l2_pix_format *format = &vcap->format;
>  	unsigned long size;
> +	int ret;
>  
>  	size = vcap->format.sizeimage;
>  
> @@ -557,6 +768,24 @@ static int dcmipp_bytecap_buf_prepare(struct vb2_buffer *vb)
>  		/* Get memory addresses */
>  		buf->addr = vb2_dma_contig_plane_dma_addr(&buf->vb.vb2_buf, 0);
>  		buf->size = vb2_plane_size(&buf->vb.vb2_buf, 0);
> +
> +		ret = frame_planes(buf->addr,
> +				   buf->addrs, buf->strides, buf->sizes,
> +				   format->width, format->height,
> +				   format->pixelformat);
> +		if (ret) {
> +			dev_err(vcap->dev, "%s: Unsupported pixel format (%x)\n",
> +				__func__, format->pixelformat);
> +			return ret;
> +		}
> +
> +		/* Check for 16 bytes alignment required by hardware */
> +		WARN_ON(buf->addrs[0] & 15);
> +		WARN_ON(buf->strides[0] & 15);
> +		WARN_ON(buf->addrs[1] & 15);
> +		WARN_ON(buf->strides[1] & 15);
> +		WARN_ON(buf->addrs[2] & 15);
> +
>  		buf->prepared = true;

[Severity: High]
This is a pre-existing issue, but does setting buf->prepared = true here
permanently cache the DMA addresses? If userspace uses V4L2_MEMORY_DMABUF and
queues a different file descriptor on the same buffer index, it seems the
driver will skip updating the hardware addresses and DMA into the old memory.

[ ... ]
> @@ -690,7 +919,7 @@ dcmipp_bytecap_set_next_frame_or_stop(struct dcmipp_bytecap_device *vcap)
>  		 * for next frame). On-going frame capture will continue until
>  		 * FRAME END but no further capture will be done.
>  		 */
> -		reg_clear(vcap, DCMIPP_P0FCTCR, DCMIPP_P0FCTCR_CPTREQ);
> +		reg_clear(vcap, DCMIPP_PxFCTCR(vcap->pipe_id), DCMIPP_PxFCTCR_CPTREQ);
>  
>  		dev_dbg(vcap->dev, "Capture restart is deferred to next buffer queueing\n");
>  		vcap->next = NULL;
> @@ -707,7 +936,13 @@ dcmipp_bytecap_set_next_frame_or_stop(struct dcmipp_bytecap_device *vcap)
>  	 * This register is shadowed and will be taken into
>  	 * account on next VSYNC (start of next frame)
>  	 */
> -	reg_write(vcap, DCMIPP_P0PPM0AR1, vcap->next->addr);
> +	reg_write(vcap, DCMIPP_PxPPM0AR1(vcap->pipe_id), vcap->next->addrs[0]);

[Severity: Critical]
This is a pre-existing issue, but is there a risk of a NULL pointer dereference
here when restarting capture after queue starvation? If the queue runs empty,
dcmipp_bytecap_process_frame() completes the last buffer and sets vcap->active
to NULL. If userspace resumes capture by queuing two new buffers, it seems
list_next_entry() will dereference vcap->active while it is NULL.

[ ... ]
> @@ -743,29 +978,39 @@ static irqreturn_t dcmipp_bytecap_irq_thread(int irq, void *arg)
>  {
>  	struct dcmipp_bytecap_device *vcap =
>  			container_of(arg, struct dcmipp_bytecap_device, ved);
> +	u32 cmsr2_pxframef;
> +	u32 cmsr2_pxvsyncf;
> +	u32 cmsr2_pxovrf;
>  	size_t bytesused = 0;
>  
>  	spin_lock_irq(&vcap->irqlock);
>  
> +	cmsr2_pxovrf = DCMIPP_CMSR2_PxOVRF(vcap->pipe_id);
> +	cmsr2_pxvsyncf = DCMIPP_CMSR2_PxVSYNCF(vcap->pipe_id);
> +	cmsr2_pxframef = DCMIPP_CMSR2_PxFRAMEF(vcap->pipe_id);
> +
>  	/*
>  	 * If we have an overrun, a frame-end will probably not be generated,
>  	 * in that case the active buffer will be recycled as next buffer by
>  	 * the VSYNC handler
>  	 */
> -	if (vcap->cmsr2 & DCMIPP_CMSR2_P0OVRF) {
> +	if (vcap->cmsr2 & cmsr2_pxovrf) {
>  		vcap->count.errors++;
>  		vcap->count.overrun++;
>  	}
>  
> -	if (vcap->cmsr2 & DCMIPP_CMSR2_P0FRAMEF) {
> +	if (vcap->cmsr2 & cmsr2_pxframef) {
>  		vcap->count.frame++;
>  
>  		/* Read captured buffer size */
> -		bytesused = reg_read(vcap, DCMIPP_P0DCCNTR);
> +		if (vcap->pipe_id == 0)
> +			bytesused = reg_read(vcap, DCMIPP_P0DCCNTR);
> +		else
> +			bytesused = vcap->format.sizeimage;
>  		dcmipp_bytecap_process_frame(vcap, bytesused);
>  	}
>  
> -	if (vcap->cmsr2 & DCMIPP_CMSR2_P0VSYNCF) {
> +	if (vcap->cmsr2 & cmsr2_pxvsyncf) {
>  		vcap->count.vsync++;
>  		if (vcap->state == DCMIPP_WAIT_FOR_BUFFER) {
>  			vcap->count.underrun++;

[Severity: High]
This is a pre-existing issue, but as mentioned above regarding
dcmipp_bytecap_stop_streaming(), does the IRQ thread miss checking for the
DCMIPP_STOPPED state? If so, it looks like it can swap the dangling next pointer
into active and use it.

[ ... ]
> @@ -826,6 +1071,26 @@ static int dcmipp_bytecap_link_validate(struct media_link *link)
>  	if (ret < 0)
>  		return 0;
>  
> +	/* On pixel pipes there can be alignment constraints */
> +	if (vcap->pipe_id != 0) {
> +		u32 width_aligned;
> +		/*
> +		 * Depending on the format & pixelpacker constraints, vcap width is
> +		 * different from mbus width.  Compute expected vcap width based on
> +		 * mbus width
> +		 */
> +		width_aligned = round_up(source_fmt.format.width,
> +					 1 << hdw_pixel_alignment(vcap->format.pixelformat));
> +
> +		if (width_aligned != vcap->format.width ||
> +		    source_fmt.format.height != vcap->format.height) {
> +			dev_err(vcap->dev, "Wrong width or height %ux%u (%ux%u expected)\n",
> +				vcap->format.width, vcap->format.height,
> +				width_aligned, source_fmt.format.height);
> +			return -EINVAL;
> +		}
> +	}
> +
>  	if (source_fmt.format.width != vcap->format.width ||
>  	    source_fmt.format.height != vcap->format.height) {

[Severity: High]
Are we missing an else block here? If the alignment block above successfully
matches the aligned width, this unconditional exact-match check will run
afterward. If the source width was unaligned, they won't match exactly, causing
link validation to unconditionally fail for pixel pipes.

[ ... ]
> @@ -870,6 +1145,22 @@ struct dcmipp_ent_device *dcmipp_bytecap_ent_init(const char *entity_name,
>  	if (!vcap)
>  		return ERR_PTR(-ENOMEM);
>  
> +	/* Retrieve the pipe_id */
> +	vcap->pipe_id = dcmipp_name_to_pipe_id(entity_name);
> +	if (vcap->pipe_id < 0) {
> +		dev_err(dev, "failed to retrieve pipe_id\n");
> +		goto err_free_vcap;
> +	}

[Severity: High]
Since vcap->pipe_id is a u32, this check (vcap->pipe_id < 0) evaluates to false.
If dcmipp_name_to_pipe_id() returns a negative error code like -EINVAL, will it
be treated as a massive positive integer, leading to out-of-bounds I/O memory
accesses when used in register offset calculations?

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