Nouveau - Mar 2014 - [PATCH] nouveau: fix fence waiting logic in screen destroy

nouveau_fence_wait has the expectation that an external entity is
holding onto the fence being waited on, not that it is merely held onto
by the current pointer. Fixes a use-after-free in nouveau_fence_wait
when used on the screen's current fence.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=75279
Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
---

Should the waiting be predicated on the current fence having been emitted? I
removed that from nv30 (which would just go ahead and _leak_ that fence it it
hadn't been emitted...). I figure it doesn't really matter enough to
worry
about that. The bigger reason to do it, I guess, is to make sure that all the
fences on the list get processed. But perhaps it'd be OK to just nuke them
without regard for such details?

 src/gallium/drivers/nouveau/nv30/nv30_screen.c | 14 ++++++++++----
 src/gallium/drivers/nouveau/nv50/nv50_screen.c | 11 +++++++++--
 src/gallium/drivers/nouveau/nvc0/nvc0_screen.c |  9 ++++++++-
 3 files changed, 27 insertions(+), 7 deletions(-)

diff --git a/src/gallium/drivers/nouveau/nv30/nv30_screen.c
b/src/gallium/drivers/nouveau/nv30/nv30_screen.c
index 82f2c06..5378913 100644
--- a/src/gallium/drivers/nouveau/nv30/nv30_screen.c
+++ b/src/gallium/drivers/nouveau/nv30/nv30_screen.c
@@ -308,10 +308,16 @@ nv30_screen_destroy(struct pipe_screen *pscreen)
    if (!nouveau_drm_screen_unref(&screen->base))
       return;
 
-   if (screen->base.fence.current &&
-       screen->base.fence.current->state >=
NOUVEAU_FENCE_STATE_EMITTED) {
-      nouveau_fence_wait(screen->base.fence.current);
-      nouveau_fence_ref (NULL, &screen->base.fence.current);
+   if (screen->base.fence.current) {
+      struct nouveau_fence *current = NULL;
+
+      /* nouveau_fence_wait will create a new current fence, so wait on the
+       * _current_ one, and remove both.
+       */
+      nouveau_fence_ref(screen->base.fence.current, &current);
+      nouveau_fence_wait(current);
+      nouveau_fence_ref(NULL, &current);
+      nouveau_fence_ref(NULL, &screen->base.fence.current);
    }
 
    nouveau_object_del(&screen->query);
diff --git a/src/gallium/drivers/nouveau/nv50/nv50_screen.c
b/src/gallium/drivers/nouveau/nv50/nv50_screen.c
index ab0d63e..e8c7fe3 100644
--- a/src/gallium/drivers/nouveau/nv50/nv50_screen.c
+++ b/src/gallium/drivers/nouveau/nv50/nv50_screen.c
@@ -294,8 +294,15 @@ nv50_screen_destroy(struct pipe_screen *pscreen)
       return;
 
    if (screen->base.fence.current) {
-      nouveau_fence_wait(screen->base.fence.current);
-      nouveau_fence_ref (NULL, &screen->base.fence.current);
+      struct nouveau_fence *current = NULL;
+
+      /* nouveau_fence_wait will create a new current fence, so wait on the
+       * _current_ one, and remove both.
+       */
+      nouveau_fence_ref(screen->base.fence.current, &current);
+      nouveau_fence_wait(current);
+      nouveau_fence_ref(NULL, &current);
+      nouveau_fence_ref(NULL, &screen->base.fence.current);
    }
    if (screen->base.pushbuf)
       screen->base.pushbuf->user_priv = NULL;
diff --git a/src/gallium/drivers/nouveau/nvc0/nvc0_screen.c
b/src/gallium/drivers/nouveau/nvc0/nvc0_screen.c
index 044847d..04f3088 100644
--- a/src/gallium/drivers/nouveau/nvc0/nvc0_screen.c
+++ b/src/gallium/drivers/nouveau/nvc0/nvc0_screen.c
@@ -340,7 +340,14 @@ nvc0_screen_destroy(struct pipe_screen *pscreen)
       return;
 
    if (screen->base.fence.current) {
-      nouveau_fence_wait(screen->base.fence.current);
+      struct nouveau_fence *current = NULL;
+
+      /* nouveau_fence_wait will create a new current fence, so wait on the
+       * _current_ one, and remove both.
+       */
+      nouveau_fence_ref(screen->base.fence.current, &current);
+      nouveau_fence_wait(current);
+      nouveau_fence_ref(NULL, &current);
       nouveau_fence_ref(NULL, &screen->base.fence.current);
    }
    if (screen->base.pushbuf)
-- 
1.8.3.2

On 06/03/14 04:01, Ilia Mirkin wrote:
> nouveau_fence_wait has the expectation that an external entity is
> holding onto the fence being waited on, not that it is merely held onto
> by the current pointer. Fixes a use-after-free in nouveau_fence_wait
> when used on the screen's current fence.
> 
IMHO one should flatten all the fence handling a bit to greatly improve
readability and minimise chances of similar problems.

Also there is yet another reason why this might be a good idea -
nouveau_fence_signalled, and all it's callers.

I'll send a few patches of what I have in mind wrt "flattening",
later
on today.

-Emil
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=75279
> Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
> ---
> 
> Should the waiting be predicated on the current fence having been emitted?
I
> removed that from nv30 (which would just go ahead and _leak_ that fence it
it
> hadn't been emitted...). I figure it doesn't really matter enough
to worry
> about that. The bigger reason to do it, I guess, is to make sure that all
the
> fences on the list get processed. But perhaps it'd be OK to just nuke
them
> without regard for such details?
> 
>  src/gallium/drivers/nouveau/nv30/nv30_screen.c | 14 ++++++++++----
>  src/gallium/drivers/nouveau/nv50/nv50_screen.c | 11 +++++++++--
>  src/gallium/drivers/nouveau/nvc0/nvc0_screen.c |  9 ++++++++-
>  3 files changed, 27 insertions(+), 7 deletions(-)
> 
> diff --git a/src/gallium/drivers/nouveau/nv30/nv30_screen.c
b/src/gallium/drivers/nouveau/nv30/nv30_screen.c
> index 82f2c06..5378913 100644
> --- a/src/gallium/drivers/nouveau/nv30/nv30_screen.c
> +++ b/src/gallium/drivers/nouveau/nv30/nv30_screen.c
> @@ -308,10 +308,16 @@ nv30_screen_destroy(struct pipe_screen *pscreen)
>     if (!nouveau_drm_screen_unref(&screen->base))
>        return;
>  
> -   if (screen->base.fence.current &&
> -       screen->base.fence.current->state >=
NOUVEAU_FENCE_STATE_EMITTED) {
> -      nouveau_fence_wait(screen->base.fence.current);
> -      nouveau_fence_ref (NULL, &screen->base.fence.current);
> +   if (screen->base.fence.current) {
> +      struct nouveau_fence *current = NULL;
> +
> +      /* nouveau_fence_wait will create a new current fence, so wait on
the
> +       * _current_ one, and remove both.
> +       */
> +      nouveau_fence_ref(screen->base.fence.current, &current);
> +      nouveau_fence_wait(current);
> +      nouveau_fence_ref(NULL, &current);
> +      nouveau_fence_ref(NULL, &screen->base.fence.current);
>     }
>  
>     nouveau_object_del(&screen->query);
> diff --git a/src/gallium/drivers/nouveau/nv50/nv50_screen.c
b/src/gallium/drivers/nouveau/nv50/nv50_screen.c
> index ab0d63e..e8c7fe3 100644
> --- a/src/gallium/drivers/nouveau/nv50/nv50_screen.c
> +++ b/src/gallium/drivers/nouveau/nv50/nv50_screen.c
> @@ -294,8 +294,15 @@ nv50_screen_destroy(struct pipe_screen *pscreen)
>        return;
>  
>     if (screen->base.fence.current) {
> -      nouveau_fence_wait(screen->base.fence.current);
> -      nouveau_fence_ref (NULL, &screen->base.fence.current);
> +      struct nouveau_fence *current = NULL;
> +
> +      /* nouveau_fence_wait will create a new current fence, so wait on
the
> +       * _current_ one, and remove both.
> +       */
> +      nouveau_fence_ref(screen->base.fence.current, &current);
> +      nouveau_fence_wait(current);
> +      nouveau_fence_ref(NULL, &current);
> +      nouveau_fence_ref(NULL, &screen->base.fence.current);
>     }
>     if (screen->base.pushbuf)
>        screen->base.pushbuf->user_priv = NULL;
> diff --git a/src/gallium/drivers/nouveau/nvc0/nvc0_screen.c
b/src/gallium/drivers/nouveau/nvc0/nvc0_screen.c
> index 044847d..04f3088 100644
> --- a/src/gallium/drivers/nouveau/nvc0/nvc0_screen.c
> +++ b/src/gallium/drivers/nouveau/nvc0/nvc0_screen.c
> @@ -340,7 +340,14 @@ nvc0_screen_destroy(struct pipe_screen *pscreen)
>        return;
>  
>     if (screen->base.fence.current) {
> -      nouveau_fence_wait(screen->base.fence.current);
> +      struct nouveau_fence *current = NULL;
> +
> +      /* nouveau_fence_wait will create a new current fence, so wait on
the
> +       * _current_ one, and remove both.
> +       */
> +      nouveau_fence_ref(screen->base.fence.current, &current);
> +      nouveau_fence_wait(current);
> +      nouveau_fence_ref(NULL, &current);
>        nouveau_fence_ref(NULL, &screen->base.fence.current);
>     }
>     if (screen->base.pushbuf)
>

On Fri, Mar 7, 2014 at 9:11 AM, Emil Velikov <emil.l.velikov at gmail.com>
wrote:
> On 06/03/14 04:01, Ilia Mirkin wrote:
>> nouveau_fence_wait has the expectation that an external entity is
>> holding onto the fence being waited on, not that it is merely held onto
>> by the current pointer. Fixes a use-after-free in nouveau_fence_wait
>> when used on the screen's current fence.
>>
> IMHO one should flatten all the fence handling a bit to greatly improve
> readability and minimise chances of similar problems.
>
> Also there is yet another reason why this might be a good idea -
> nouveau_fence_signalled, and all it's callers.
>
> I'll send a few patches of what I have in mind wrt
"flattening", later
> on today.
I'm not sure what you're planning, but if it's an intrusive change,
could you base it on top of this one (assuming you think that it's
correct)? That way this one can still be cherry-picked to stable mesa
versions.
>
> -Emil
>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=75279
>> Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
>> ---
>>
>> Should the waiting be predicated on the current fence having been
emitted? I
>> removed that from nv30 (which would just go ahead and _leak_ that fence
it it
>> hadn't been emitted...). I figure it doesn't really matter
enough to worry
>> about that. The bigger reason to do it, I guess, is to make sure that
all the
>> fences on the list get processed. But perhaps it'd be OK to just
nuke them
>> without regard for such details?
>>
>>  src/gallium/drivers/nouveau/nv30/nv30_screen.c | 14 ++++++++++----
>>  src/gallium/drivers/nouveau/nv50/nv50_screen.c | 11 +++++++++--
>>  src/gallium/drivers/nouveau/nvc0/nvc0_screen.c |  9 ++++++++-
>>  3 files changed, 27 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/gallium/drivers/nouveau/nv30/nv30_screen.c
b/src/gallium/drivers/nouveau/nv30/nv30_screen.c
>> index 82f2c06..5378913 100644
>> --- a/src/gallium/drivers/nouveau/nv30/nv30_screen.c
>> +++ b/src/gallium/drivers/nouveau/nv30/nv30_screen.c
>> @@ -308,10 +308,16 @@ nv30_screen_destroy(struct pipe_screen *pscreen)
>>     if (!nouveau_drm_screen_unref(&screen->base))
>>        return;
>>
>> -   if (screen->base.fence.current &&
>> -       screen->base.fence.current->state >=
NOUVEAU_FENCE_STATE_EMITTED) {
>> -      nouveau_fence_wait(screen->base.fence.current);
>> -      nouveau_fence_ref (NULL, &screen->base.fence.current);
>> +   if (screen->base.fence.current) {
>> +      struct nouveau_fence *current = NULL;
>> +
>> +      /* nouveau_fence_wait will create a new current fence, so wait
on the
>> +       * _current_ one, and remove both.
>> +       */
>> +      nouveau_fence_ref(screen->base.fence.current, &current);
>> +      nouveau_fence_wait(current);
>> +      nouveau_fence_ref(NULL, &current);
>> +      nouveau_fence_ref(NULL, &screen->base.fence.current);
>>     }
>>
>>     nouveau_object_del(&screen->query);
>> diff --git a/src/gallium/drivers/nouveau/nv50/nv50_screen.c
b/src/gallium/drivers/nouveau/nv50/nv50_screen.c
>> index ab0d63e..e8c7fe3 100644
>> --- a/src/gallium/drivers/nouveau/nv50/nv50_screen.c
>> +++ b/src/gallium/drivers/nouveau/nv50/nv50_screen.c
>> @@ -294,8 +294,15 @@ nv50_screen_destroy(struct pipe_screen *pscreen)
>>        return;
>>
>>     if (screen->base.fence.current) {
>> -      nouveau_fence_wait(screen->base.fence.current);
>> -      nouveau_fence_ref (NULL, &screen->base.fence.current);
>> +      struct nouveau_fence *current = NULL;
>> +
>> +      /* nouveau_fence_wait will create a new current fence, so wait
on the
>> +       * _current_ one, and remove both.
>> +       */
>> +      nouveau_fence_ref(screen->base.fence.current, &current);
>> +      nouveau_fence_wait(current);
>> +      nouveau_fence_ref(NULL, &current);
>> +      nouveau_fence_ref(NULL, &screen->base.fence.current);
>>     }
>>     if (screen->base.pushbuf)
>>        screen->base.pushbuf->user_priv = NULL;
>> diff --git a/src/gallium/drivers/nouveau/nvc0/nvc0_screen.c
b/src/gallium/drivers/nouveau/nvc0/nvc0_screen.c
>> index 044847d..04f3088 100644
>> --- a/src/gallium/drivers/nouveau/nvc0/nvc0_screen.c
>> +++ b/src/gallium/drivers/nouveau/nvc0/nvc0_screen.c
>> @@ -340,7 +340,14 @@ nvc0_screen_destroy(struct pipe_screen *pscreen)
>>        return;
>>
>>     if (screen->base.fence.current) {
>> -      nouveau_fence_wait(screen->base.fence.current);
>> +      struct nouveau_fence *current = NULL;
>> +
>> +      /* nouveau_fence_wait will create a new current fence, so wait
on the
>> +       * _current_ one, and remove both.
>> +       */
>> +      nouveau_fence_ref(screen->base.fence.current, &current);
>> +      nouveau_fence_wait(current);
>> +      nouveau_fence_ref(NULL, &current);
>>        nouveau_fence_ref(NULL, &screen->base.fence.current);
>>     }
>>     if (screen->base.pushbuf)
>>
>