Re: [PATCH v2 03/13] media: stm32: dcmipp: bytecap: protect CMIER register access

[sashiko-bot@kernel.org]

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

New issues:
- [High] Kernel heap Use-After-Free on `vcap->vdev.v4l2_dev->lock` in `dcmipp_bytecap_stop_streaming()` due to lifetime mismatch.

Pre-existing issues:
- [High] Use-After-Free during device unbind because hardware interrupt remains active after devm-allocated memory is freed.
- [High] Missing synchronization with the IRQ thread in `stop_streaming` leads to Use-After-Free and unclocked register access (external abort).
--

commit def9653367321582625d53db751e4abad7f35c94
Author: Alain Volmat <alain.volmat@foss.st.com>

media: stm32: dcmipp: bytecap: protect CMIER register access

This patch adds a spinlock to protect the CMIER register from concurrent
access by utilizing the existing lock in the v4l2_device structure.

> 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 990bbaaf4bda1..dac7338dd9819 100644
> --- a/drivers/media/platform/st/stm32/stm32-dcmipp/dcmipp-bytecap.c
> +++ b/drivers/media/platform/st/stm32/stm32-dcmipp/dcmipp-bytecap.c
[ ... ]
> @@ -492,7 +494,9 @@ static void dcmipp_bytecap_stop_streaming(struct vb2_queue *vq)
>  	media_pipeline_stop(vcap->vdev.entity.pads);
>  
>  	/* Disable interruptions */
> +	spin_lock(&vcap->vdev.v4l2_dev->lock);

[Severity: High]
If a user closes the file descriptor after the device has been unbound,
does this dereference freed memory?

The parent dcmipp structure (which embeds v4l2_dev) is allocated with
devm_kzalloc() and freed by devres when the driver is unbound. However,
the vcap structure is refcounted by the V4L2 video device and remains
alive as long as the file descriptor is open.

When the file descriptor is eventually closed, dcmipp_bytecap_stop_streaming()
is invoked and attempts to acquire this lock inside the already-freed
v4l2_dev memory.

>  	reg_clear(vcap, DCMIPP_CMIER, DCMIPP_CMIER_P0ALL);
> +	spin_unlock(&vcap->vdev.v4l2_dev->lock);
>  
>  	/* Stop capture */
>  	reg_clear(vcap, DCMIPP_P0FCTCR, DCMIPP_P0FCTCR_CPTREQ);

[Severity: High]
This is a pre-existing issue, but is it possible for the IRQ thread to
race with dcmipp_bytecap_stop_streaming() since there is no call to
synchronize_irq() before tearing down the capture state?

If the threaded IRQ handler runs after this function sets
vcap->state = DCMIPP_STOPPED, it would observe the stopped state
and proceed to swap(vcap->active, vcap->next).

Because stop_streaming() doesn't clear vcap->next, a buffer that was
already returned to userspace could be swapped into vcap->active. The
IRQ thread would then call dcmipp_bytecap_set_next_frame_or_stop(),
which accesses the freed buffer's memory.

Additionally, since pm_runtime_put() is called during teardown, the
device clocks might be disabled, causing the subsequent register access
in the IRQ thread to trigger a synchronous external abort.

[Severity: High]
This isn't a bug introduced by this patch, but does the driver have an
allocation order issue during probe that leads to a use-after-free
during unbind?

The driver registers its interrupt handler via devm_request_threaded_irq()
before allocating the dcmipp->entity array via devm_kcalloc().

During device unbind, devres frees resources in reverse order. This means
dcmipp->entity is freed before the IRQ is disabled. If a hardware
interrupt fires during this window, dcmipp_irq_callback() will traverse
the freed dcmipp->entity array:

drivers/media/platform/st/stm32/stm32-dcmipp/dcmipp-core.c:dcmipp_irq_callback() {
    ...
    for (i = 0; i < dcmipp->num_ents; i++) {
        ved = dcmipp->entity[i];
        ...
    }
}

Could this reliably crash the system during hot-unplug or driver unbind?

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