Re: [PATCH 1/3] drm/nouveau: destroy the fence event before cancelling its work

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Moving `nvif_event_dtor()` before `nouveau_fence_context_kill()` introduces an unprotected Time-of-Check to Time-of-Use (TOCTOU) race condition that results in a NULL pointer dereference in `nvif_object_ioctl`.
--

--- Patch [1]: [PATCH 1/3] drm/nouveau: destroy the fence event before cancelling its work ---
commit 248f9b5f2479dc65d3572babf739eb3883d7b9f7
Author: Marek Czernohous <[email protected]>

drm/nouveau: destroy the fence event before cancelling its work

This commit reorders the teardown sequence in nouveau_fence_context_del() by
moving nvif_event_dtor() before cancel_work_sync() and
nouveau_fence_context_kill(). It aims to prevent a newly arriving interrupt
from re-arming the work that was just cancelled.

Link: https://sashiko.dev/#/patchset/[email protected]?part=1

> diff --git a/drivers/gpu/drm/nouveau/nouveau_fence.c b/drivers/gpu/drm/nouveau/nouveau_fence.c
> index edbe9e08ba0fd..4a3698dc2cd14 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_fence.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_fence.c
> @@ -96,9 +96,9 @@ nouveau_fence_context_kill(struct nouveau_fence_chan *fctx, int error)
>  void
>  nouveau_fence_context_del(struct nouveau_fence_chan *fctx)
>  {
> +	nvif_event_dtor(&fctx->event);
>  	cancel_work_sync(&fctx->uevent_work);
>  	nouveau_fence_context_kill(fctx, 0);

[Severity: Critical]
Does moving nvif_event_dtor() before nouveau_fence_context_kill() introduce a
Time-of-Check to Time-of-Use (TOCTOU) race condition?

Since nouveau_fence_context_kill() hasn't run yet, the fences are still
active and unsignaled. If Thread A is polling a fence and calls
nouveau_fence_enable_signaling() which reaches nvif_event_allow(), it might
evaluate nvif_event_constructed() right before Thread B destroys the event:

Thread A:
drivers/gpu/drm/nouveau/nvif/event.c:nvif_event_allow() {
	if (nvif_event_constructed(event)) {

Thread B concurrently cleans up the channel:
drivers/gpu/drm/nouveau/nouveau_fence.c:nouveau_fence_context_del() {
	nvif_event_dtor(&fctx->event);
	...
drivers/gpu/drm/nouveau/nvif/object.c:nvif_object_dtor() {
	nvif_object_ioctl(object, &args, sizeof(args), NULL);
	object->client = NULL;

Thread A then continues and calls into nvif_mthd():
drivers/gpu/drm/nouveau/nvif/event.c:nvif_event_allow() {
		int ret = nvif_mthd(&event->object, NVIF_EVENT_V0_ALLOW, ...);
		...
drivers/gpu/drm/nouveau/nvif/object.c:nvif_object_ioctl() {
	struct nvif_client *client = object->client;
	/* ... */
	return client->driver->ioctl(client->object.priv, data, size, hack);

Could Thread A read the stale object->client as NULL and fatally dereference
it here, since both polling on an exported sync_file fd and channel cleanup
are accessible to unprivileged userspace?

> -	nvif_event_dtor(&fctx->event);
>  	fctx->dead = 1;
>  
>  	/*

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