Re: [PATCH v4 1/3] drm/nouveau: unsubscribe the channel-kill event before the fence context

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

Pre-existing issues:
- [High] Race condition in nouveau_fence_context_del between cancel_work_sync and nvif_event_dtor leads to use-after-free of fctx.
- [High] Missing cancel_work_sync for irq_work in nouveau_connector_destroy leads to use-after-free of the connector.
--

--- Patch [1]: [PATCH v4 1/3] drm/nouveau: unsubscribe the channel-kill event before the fence context ---
commit 4ae9b89defb4e27bc74c023fe090b6ae974e3378
Author: Marek Czernohous <[email protected]>

drm/nouveau: unsubscribe the channel-kill event before the fence context

nouveau_channel_del() tears the fence context down first and only drops
the channel-kill subscription later, which can cause a use-after-free
if a kill event is delivered in that window. This patch drops the
subscription before anything it depends on is torn down.

> diff --git a/drivers/gpu/drm/nouveau/nouveau_chan.c b/drivers/gpu/drm/nouveau/nouveau_chan.c
> index 598513f604493..f142f63105966 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_chan.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_chan.c
> @@ -90,6 +90,14 @@ nouveau_channel_del(struct nouveau_channel **pchan)
>  {
>  	struct nouveau_channel *chan = *pchan;
>  	if (chan) {
> +		/*
> +		 * Drop the kill-event subscription first.  Its handler
> +		 * dereferences chan->fence, which the fence context teardown
> +		 * below frees, so leaving it armed across the teardown leaves
> +		 * a window for a use-after-free.
> +		 */
> +		nvif_event_dtor(&chan->kill);
> +
>  		if (chan->fence)
>  			nouveau_fence(chan->cli->drm)->context_del(chan);
>  
[ ... ]
> @@ -100,7 +108,6 @@ nouveau_channel_del(struct nouveau_channel **pchan)
>  		nvif_object_dtor(&chan->nvsw);
>  		nvif_object_dtor(&chan->gart);
>  		nvif_object_dtor(&chan->vram);
> -		nvif_event_dtor(&chan->kill);
>  		nvif_object_dtor(&chan->user);
>  		nvif_mem_dtor(&chan->mem_userd);
>  		nouveau_vma_del(&chan->sema.vma);

[Severity: High]
This isn't a bug introduced by this patch, but looking at similar teardown
paths, is there a race condition in nouveau_fence_context_del() between
cancel_work_sync() and nvif_event_dtor()?

drivers/gpu/drm/nouveau/nouveau_fence.c:nouveau_fence_context_del() {
	...
	cancel_work_sync(&fctx->uevent_work);
	nouveau_fence_context_kill(fctx, 0);
	nvif_event_dtor(&fctx->event);
	...
}

If a hardware interrupt fires in the narrow window after cancel_work_sync()
but before nvif_event_dtor(), could the event handler schedule the work
again?

The work would remain pending on the workqueue while fctx is subsequently
freed, leading to a use-after-free when the work executes.


[Severity: High]
This is another pre-existing issue in a similar teardown path, but is there a
missing cancel_work_sync() for irq_work in nouveau_connector_destroy()?

drivers/gpu/drm/nouveau/nouveau_connector.c:nouveau_connector_destroy() {
	...
	nvif_event_dtor(&nv_connector->irq);
	nvif_event_dtor(&nv_connector->hpd);
	kfree(nv_connector->edid);
	drm_connector_unregister(connector);
	...
}

When a connector is destroyed, it calls nvif_event_dtor() to unregister the
hardware event, but does not appear to call cancel_work_sync() for
nv_connector->irq_work.

If an IRQ fires right before the event is destroyed, the handler schedules
irq_work:

drivers/gpu/drm/nouveau/nouveau_connector.c:nouveau_connector_irq() {
	...
	schedule_work(&nv_connector->irq_work);
	return NVIF_EVENT_KEEP;
}

Because the work is never canceled, will it eventually execute on freed
memory after the connector has been freed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1