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
