[PATCH] media: mgb4: cancel err_work before freeing the vin device

[PATCH] media: mgb4: cancel err_work before freeing the vin device

[hyunjungg]

From: Hyunjung Ko <hj351016@gmail.com>

The input (vin) device schedules err_work (signal_change) from its error
interrupt handler err_handler(). On device removal, mgb4_vin_free() masks
and frees the error IRQ, but never drains err_work before kfree(vindev):

	xdma_disable_user_irq(vindev->mgbdev->xdev, err_irq);
	free_irq(vin_irq, vindev);
	free_irq(err_irq, vindev);
	...
	kfree(vindev);

free_irq() only synchronises against the IRQ handler itself; it does not
wait for a work item the handler has already queued on the system
workqueue. If an error interrupt fires shortly before removal, the queued
signal_change() work can run after vindev has been freed and dereferences
it (vindev->mgbdev->video, vindev->vdev, vindev->queue, vindev->timings),
causing a use-after-free.

Note the dma_work counterpart is already drained via cancel_work_sync() in
stop_streaming(); err_work has no corresponding cancel on any path. Add a
cancel_work_sync() for err_work in mgb4_vin_free(), placed after both IRQs
are freed so the handler can no longer re-queue the work.

Found by code review; not reproduced (requires the Digiteq MGB4 frame
grabber and an error interrupt racing device removal).

Signed-off-by: Hyunjung Ko <hj351016@gmail.com>
---
 drivers/media/pci/mgb4/mgb4_vin.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/media/pci/mgb4/mgb4_vin.c b/drivers/media/pci/mgb4/mgb4_vin.c
--- a/drivers/media/pci/mgb4/mgb4_vin.c
+++ b/drivers/media/pci/mgb4/mgb4_vin.c
@@ -1068,6 +1068,8 @@ void mgb4_vin_free(struct mgb4_vin_dev *vindev)
 	free_irq(vin_irq, vindev);
 	free_irq(err_irq, vindev);

+	cancel_work_sync(&vindev->err_work);
+
 	device_remove_groups(&vindev->vdev.dev, module_groups(vindev->mgbdev));

 	mgb4_i2c_free(&vindev->deser);

Re: [PATCH] media: mgb4: cancel err_work before freeing the vin device

[Martin Tůma]

On 11/6/26 5:35 AM, hyunjungg wrote:
> From: Hyunjung Ko <hj351016@gmail.com>
> 
> The input (vin) device schedules err_work (signal_change) from its error
> interrupt handler err_handler(). On device removal, mgb4_vin_free() masks
> and frees the error IRQ, but never drains err_work before kfree(vindev):
> 
> 	xdma_disable_user_irq(vindev->mgbdev->xdev, err_irq);
> 	free_irq(vin_irq, vindev);
> 	free_irq(err_irq, vindev);
> 	...
> 	kfree(vindev);
> 
> free_irq() only synchronises against the IRQ handler itself; it does not
> wait for a work item the handler has already queued on the system
> workqueue. If an error interrupt fires shortly before removal, the queued
> signal_change() work can run after vindev has been freed and dereferences
> it (vindev->mgbdev->video, vindev->vdev, vindev->queue, vindev->timings),
> causing a use-after-free.
> 
> Note the dma_work counterpart is already drained via cancel_work_sync() in
> stop_streaming(); err_work has no corresponding cancel on any path. Add a
> cancel_work_sync() for err_work in mgb4_vin_free(), placed after both IRQs
> are freed so the handler can no longer re-queue the work.
> 
> Found by code review; not reproduced (requires the Digiteq MGB4 frame
> grabber and an error interrupt racing device removal).
> 
> Signed-off-by: Hyunjung Ko <hj351016@gmail.com>
> ---
>   drivers/media/pci/mgb4/mgb4_vin.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/media/pci/mgb4/mgb4_vin.c b/drivers/media/pci/mgb4/mgb4_vin.c
> --- a/drivers/media/pci/mgb4/mgb4_vin.c
> +++ b/drivers/media/pci/mgb4/mgb4_vin.c
> @@ -1068,6 +1068,8 @@ void mgb4_vin_free(struct mgb4_vin_dev *vindev)
>   	free_irq(vin_irq, vindev);
>   	free_irq(err_irq, vindev);
> 
> +	cancel_work_sync(&vindev->err_work);
> +
>   	device_remove_groups(&vindev->vdev.dev, module_groups(vindev->mgbdev));
> 
>   	mgb4_i2c_free(&vindev->deser);
> 

Reviewed-by: Martin Tůma <martin.tuma@digiteqautomotive.com>