Re: [PATCH v6 03/12] media: microchip-isc: fix race condition on stream stop

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Use-after-destroy in `isc_async_unbind()` due to incorrect teardown ordering.
- [High] Missing histogram disable and workqueue cancellation in the `isc_start_streaming` error path leads to a kernel panic.
- [High] Missing IRQ synchronization in `isc_stop_streaming()` leads to a synchronous external abort.
--

commit 9cf229b57997c74f4db26f1539c0b73c428dd501
Author: Balakrishnan Sambath <balakrishnan.s@microchip.com>

media: microchip-isc: fix race condition on stream stop

Disable histogram and drain AWB work queue before releasing DMA
buffers to prevent use-after-free if histogram IRQ fires during
stream stop.

Cc: stable@vger.kernel.org

> diff --git a/drivers/media/platform/microchip/microchip-isc-base.c b/drivers/media/platform/microchip/microchip-isc-base.c
> index 45b94f1e89d86..b19c5a63b4bdc 100644
> --- a/drivers/media/platform/microchip/microchip-isc-base.c
> +++ b/drivers/media/platform/microchip/microchip-isc-base.c
> @@ -427,6 +427,14 @@ static void isc_stop_streaming(struct vb2_queue *vq)
>  
>  	mutex_unlock(&isc->awb_mutex);
>  
> +	/*
> +	 * Disable the histogram so the ISR stops firing HISREQ, then drain
> +	 * any work that was already queued before returning.  This must happen
> +	 * after releasing awb_mutex because isc_awb_work also takes it.
> +	 */
> +	isc_set_histogram(isc, false);
> +	cancel_work_sync(&isc->awb_work);
> +
>  	/* Disable DMA interrupt */
>  	regmap_write(isc->regmap, ISC_INTDIS, ISC_INT_DDONE);
>  
>  	pm_runtime_put_sync(isc->dev);

[Severity: High]
This isn't a bug introduced by this patch, but I noticed that device power is
synchronously disabled here without waiting for in-flight IRQs to finish
executing.

Because the interrupt is registered via devm_request_irq() without a call to
synchronize_irq() here, microchip_isc_interrupt() may already be executing on
another CPU when the clocks are disabled.

The ISR performs a regmap_read() on ISC_INTSR, which will crash the system
with a bus fault/external abort when attempting to read the unclocked
memory-mapped registers. Could we add a call to synchronize_irq() before
shutting down power?

[ ... ]

While reviewing this file, I noticed a couple of other pre-existing issues:

drivers/media/platform/microchip/microchip-isc-base.c:isc_start_streaming() {
...
	ret = isc_configure(isc);
	if (unlikely(ret))
		goto err_configure;
...
err_configure:
	pm_runtime_put_sync(isc->dev);
...
}

[Severity: High]
This is a pre-existing issue, but the error path fails to call
isc_set_histogram(isc, false) and cancel_work_sync(), leaving the hardware
enabled and the workqueue vulnerable to accessing unpowered registers.

If isc_configure() succeeds (enabling the histogram hardware and its
ISC_INT_HISDONE interrupt) but a subsequent step times out, the error path
calls pm_runtime_put_sync() to power down the device clocks. It fails to
disable the histogram interrupt or cancel isc->awb_work.

The hardware can trigger the interrupt before or during shutdown, scheduling
the work. When isc_awb_work() runs, it bypasses the enabled check and calls
isc_hist_count(), which performs a regmap_bulk_read() on unpowered registers.
This causes a synchronous external abort (bus fault) and crashes the system.
Should this error path be updated to clean up the histogram state and
workqueue?


drivers/media/platform/microchip/microchip-isc-base.c:isc_async_unbind() {
...
	mutex_destroy(&isc->awb_mutex);
	cancel_work_sync(&isc->awb_work);
...
}

[Severity: High]
This is a pre-existing issue, but the mutex is incorrectly destroyed before
the asynchronous work using it is definitively flushed and cancelled.

The isc_awb_work handler fundamentally relies on isc->awb_mutex to serialize
its execution. If the work is running or scheduled when unbind is triggered,
the worker thread will attempt to lock a destroyed/poisoned mutex. Would it be
safer to call cancel_work_sync() before mutex_destroy()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260603-microchip-isc-fixes-v6-0-8c3d7474a768@microchip.com?part=3