Re: [PATCH v3 1/4] 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] Canceling `uevent_work` before destroying `fctx->event` in `nouveau_fence_context_del` leaves a window for use-after-free.
- [High] `nouveau_connector_destroy()` frees the connector without cancelling `nv_connector->irq_work`, leading to a UAF.
--

--- Patch [1]: [PATCH v3 1/4] drm/nouveau: unsubscribe the channel-kill event before the fence context ---
commit a14b59dbc876448fa7fc08aa6ef35ba9137e4909
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. This leaves a race window where a kill
event can be delivered and attempt to access a freed fence context. This
patch drops the subscription before anything it depends on is torn down.

Fixes: ea13e5abf807 ("drm/nouveau: signal pending fences when channel has been killed")
Cc: [email protected]

> 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);

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a similar pattern
in nouveau_fence_context_del() inside 
drivers/gpu/drm/nouveau/nouveau_fence.c:

void
nouveau_fence_context_del(struct nouveau_fence_chan *fctx)
{
	cancel_work_sync(&fctx->uevent_work);
	nouveau_fence_context_kill(fctx, 0);
	nvif_event_dtor(&fctx->event);
        ...

If a hardware event triggers exactly in the window between cancel_work_sync()
and nvif_event_dtor(), nouveau_fence_wait_uevent_handler() unconditionally
reschedules the work via schedule_work(&fctx->uevent_work). 

Does this leave a window where the work remains on the queue after the fctx
structure is freed, resulting in a use-after-free?


[Severity: High]
This is also a pre-existing issue, but there is a similar teardown problem
in nouveau_connector_destroy() inside 
drivers/gpu/drm/nouveau/nouveau_connector.c:

static void
nouveau_connector_destroy(struct drm_connector *connector)
{
	struct nouveau_connector *nv_connector = nouveau_connector(connector);
	nvif_event_dtor(&nv_connector->irq);
	nvif_event_dtor(&nv_connector->hpd);
        ...
	kfree(connector);
}

The connector is freed without calling cancel_work_sync(&nv_connector->irq_work)
to cancel any previously scheduled work. 

If an interrupt fires just before or during nvif_event_dtor(), the work is
successfully scheduled on the system queue. When the queued work executes, it
will dereference the freed nv_connector pointer. 

Should this function cancel the irq_work before freeing the connector?

[ ... ]

> @@ -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);

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