[PATCH] drm/core: Fix ordering in drm_mode_config_cleanup.

[PATCH] drm/core: Fix ordering in drm_mode_config_cleanup.

[Maarten Lankhorst]

__drm_atomic_helper_plane_destroy_state calls
drm_framebuffer_unreference, which means that if drm_framebuffer_free
is called before plane->destroy freed memory will be accessed.

A similar case happens for the blob list, which was freed before the
crtc state was, resulting in the unreference_blob from crtc_destroy_state
pointing to garbage memory causing another opportunity for a GPF.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/drm_crtc.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 51c5a00ffdff..5a13b1afccbe 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -5958,6 +5958,15 @@ void drm_mode_config_cleanup(struct drm_device *dev)
 		drm_property_destroy(dev, property);
 	}
 
+	list_for_each_entry_safe(plane, plt, &dev->mode_config.plane_list,
+				 head) {
+		plane->funcs->destroy(plane);
+	}
+
+	list_for_each_entry_safe(crtc, ct, &dev->mode_config.crtc_list, head) {
+		crtc->funcs->destroy(crtc);
+	}
+
 	list_for_each_entry_safe(blob, bt, &dev->mode_config.property_blob_list,
 				 head_global) {
 		drm_property_unreference_blob(blob);
@@ -5976,15 +5985,6 @@ void drm_mode_config_cleanup(struct drm_device *dev)
 		drm_framebuffer_free(&fb->refcount);
 	}
 
-	list_for_each_entry_safe(plane, plt, &dev->mode_config.plane_list,
-				 head) {
-		plane->funcs->destroy(plane);
-	}
-
-	list_for_each_entry_safe(crtc, ct, &dev->mode_config.crtc_list, head) {
-		crtc->funcs->destroy(crtc);
-	}
-
 	ida_destroy(&dev->mode_config.connector_ida);
 	idr_destroy(&dev->mode_config.tile_idr);
 	idr_destroy(&dev->mode_config.crtc_idr);
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/core: Fix ordering in drm_mode_config_cleanup.

[Ville Syrjälä]

On Tue, Mar 22, 2016 at 03:42:14PM +0100, Maarten Lankhorst wrote:
> __drm_atomic_helper_plane_destroy_state calls
> drm_framebuffer_unreference, which means that if drm_framebuffer_free
> is called before plane->destroy freed memory will be accessed.
> 
> A similar case happens for the blob list, which was freed before the
> crtc state was, resulting in the unreference_blob from crtc_destroy_state
> pointing to garbage memory causing another opportunity for a GPF.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_crtc.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 51c5a00ffdff..5a13b1afccbe 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -5958,6 +5958,15 @@ void drm_mode_config_cleanup(struct drm_device *dev)
>  		drm_property_destroy(dev, property);
>  	}

And what about props? Any chance of explosion due to those being gone?

>  
> +	list_for_each_entry_safe(plane, plt, &dev->mode_config.plane_list,
> +				 head) {
> +		plane->funcs->destroy(plane);
> +	}
> +
> +	list_for_each_entry_safe(crtc, ct, &dev->mode_config.crtc_list, head) {
> +		crtc->funcs->destroy(crtc);
> +	}
> +
>  	list_for_each_entry_safe(blob, bt, &dev->mode_config.property_blob_list,
>  				 head_global) {
>  		drm_property_unreference_blob(blob);
> @@ -5976,15 +5985,6 @@ void drm_mode_config_cleanup(struct drm_device *dev)
>  		drm_framebuffer_free(&fb->refcount);
>  	}
>  
> -	list_for_each_entry_safe(plane, plt, &dev->mode_config.plane_list,
> -				 head) {
> -		plane->funcs->destroy(plane);
> -	}
> -
> -	list_for_each_entry_safe(crtc, ct, &dev->mode_config.crtc_list, head) {
> -		crtc->funcs->destroy(crtc);
> -	}
> -
>  	ida_destroy(&dev->mode_config.connector_ida);
>  	idr_destroy(&dev->mode_config.tile_idr);
>  	idr_destroy(&dev->mode_config.crtc_idr);
> -- 
> 2.1.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/core: Fix ordering in drm_mode_config_cleanup.

[Maarten Lankhorst]

Op 22-03-16 om 15:58 schreef Ville Syrjälä:
> On Tue, Mar 22, 2016 at 03:42:14PM +0100, Maarten Lankhorst wrote:
>> __drm_atomic_helper_plane_destroy_state calls
>> drm_framebuffer_unreference, which means that if drm_framebuffer_free
>> is called before plane->destroy freed memory will be accessed.
>>
>> A similar case happens for the blob list, which was freed before the
>> crtc state was, resulting in the unreference_blob from crtc_destroy_state
>> pointing to garbage memory causing another opportunity for a GPF.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/drm_crtc.c | 18 +++++++++---------
>>  1 file changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>> index 51c5a00ffdff..5a13b1afccbe 100644
>> --- a/drivers/gpu/drm/drm_crtc.c
>> +++ b/drivers/gpu/drm/drm_crtc.c
>> @@ -5958,6 +5958,15 @@ void drm_mode_config_cleanup(struct drm_device *dev)
>>  		drm_property_destroy(dev, property);
>>  	}
> And what about props? Any chance of explosion due to those being gone?
>
Not as far as I'm aware of.

If you use something like a crtc_x property, only the value gets written to crtc_state, the value is an int that would still be valid.

~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/core: Fix ordering in drm_mode_config_cleanup.

[Ville Syrjälä]

On Tue, Mar 22, 2016 at 04:08:39PM +0100, Maarten Lankhorst wrote:
> Op 22-03-16 om 15:58 schreef Ville Syrjälä:
> > On Tue, Mar 22, 2016 at 03:42:14PM +0100, Maarten Lankhorst wrote:
> >> __drm_atomic_helper_plane_destroy_state calls
> >> drm_framebuffer_unreference, which means that if drm_framebuffer_free
> >> is called before plane->destroy freed memory will be accessed.
> >>
> >> A similar case happens for the blob list, which was freed before the
> >> crtc state was, resulting in the unreference_blob from crtc_destroy_state
> >> pointing to garbage memory causing another opportunity for a GPF.
> >>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> ---
> >>  drivers/gpu/drm/drm_crtc.c | 18 +++++++++---------
> >>  1 file changed, 9 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> >> index 51c5a00ffdff..5a13b1afccbe 100644
> >> --- a/drivers/gpu/drm/drm_crtc.c
> >> +++ b/drivers/gpu/drm/drm_crtc.c
> >> @@ -5958,6 +5958,15 @@ void drm_mode_config_cleanup(struct drm_device *dev)
> >>  		drm_property_destroy(dev, property);
> >>  	}
> > And what about props? Any chance of explosion due to those being gone?
> >
> Not as far as I'm aware of.
> 
> If you use something like a crtc_x property, only the value gets written to crtc_state, the value is an int that would still be valid.

I was too lazy to confirm this for all drivers. But at least i915 looked
clean on that front. So

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 
> ~Maarten

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/core: Fix ordering in drm_mode_config_cleanup.

[Daniel Vetter]

On Fri, Apr 01, 2016 at 11:04:10PM +0300, Ville Syrjälä wrote:
> On Tue, Mar 22, 2016 at 04:08:39PM +0100, Maarten Lankhorst wrote:
> > Op 22-03-16 om 15:58 schreef Ville Syrjälä:
> > > On Tue, Mar 22, 2016 at 03:42:14PM +0100, Maarten Lankhorst wrote:
> > >> __drm_atomic_helper_plane_destroy_state calls
> > >> drm_framebuffer_unreference, which means that if drm_framebuffer_free
> > >> is called before plane->destroy freed memory will be accessed.
> > >>
> > >> A similar case happens for the blob list, which was freed before the
> > >> crtc state was, resulting in the unreference_blob from crtc_destroy_state
> > >> pointing to garbage memory causing another opportunity for a GPF.
> > >>
> > >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > >> ---
> > >>  drivers/gpu/drm/drm_crtc.c | 18 +++++++++---------
> > >>  1 file changed, 9 insertions(+), 9 deletions(-)
> > >>
> > >> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > >> index 51c5a00ffdff..5a13b1afccbe 100644
> > >> --- a/drivers/gpu/drm/drm_crtc.c
> > >> +++ b/drivers/gpu/drm/drm_crtc.c
> > >> @@ -5958,6 +5958,15 @@ void drm_mode_config_cleanup(struct drm_device *dev)
> > >>  		drm_property_destroy(dev, property);
> > >>  	}
> > > And what about props? Any chance of explosion due to those being gone?
> > >
> > Not as far as I'm aware of.
> > 
> > If you use something like a crtc_x property, only the value gets written to crtc_state, the value is an int that would still be valid.
> 
> I was too lazy to confirm this for all drivers. But at least i915 looked
> clean on that front. So
> 
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Applied to drm-misc, thanks.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx