Re: [PATCH] drm: fix use of freed memory in drm_mode_setcrtc

[Tomi Valkeinen <tomi.valkeinen@ti.com>]

On 21/09/18 11:55, Daniel Vetter wrote:
> On Mon, Sep 17, 2018 at 02:00:54PM +0300, Tomi Valkeinen wrote:
>> drm_mode_setcrtc() retries modesetting in case one of the functions it
>> calls returns -EDEADLK. connector_set, mode and fb are freed before
>> retrying, but they are not set to NULL. This can cause
>> drm_mode_setcrtc() to use those variables.
>>
>> For example: On the first try __drm_mode_set_config_internal() returns
>> -EDEADLK. connector_set, mode and fb are freed. Next retry starts, and
>> drm_modeset_lock_all_ctx() returns -EDEADLK, and we jump to 'out'. The
>> code will happily try to release all three again.
>>
>> This leads to crashes of different kinds, depending on the sequence the
>> EDEADLKs happen.
>>
>> Fix this by setting the three variables to NULL at the start of the
>> retry loop.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> Cc: stable@vger.kernel.org
>> ---
>>  drivers/gpu/drm/drm_crtc.c | 10 +++++++---
>>  1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>> index 2f6c877299e4..2ad14593fb23 100644
>> --- a/drivers/gpu/drm/drm_crtc.c
>> +++ b/drivers/gpu/drm/drm_crtc.c
>> @@ -570,9 +570,9 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
>>  	struct drm_mode_crtc *crtc_req = data;
>>  	struct drm_crtc *crtc;
>>  	struct drm_plane *plane;
>> -	struct drm_connector **connector_set = NULL, *connector;
>> -	struct drm_framebuffer *fb = NULL;
>> -	struct drm_display_mode *mode = NULL;
>> +	struct drm_connector **connector_set, *connector;
>> +	struct drm_framebuffer *fb;
>> +	struct drm_display_mode *mode;
>>  	struct drm_mode_set set;
>>  	uint32_t __user *set_connectors_ptr;
>>  	struct drm_modeset_acquire_ctx ctx;
>> @@ -601,6 +601,10 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
>>  	mutex_lock(&crtc->dev->mode_config.mutex);
>>  	drm_modeset_acquire_init(&ctx, DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
>>  retry:
>> +	connector_set = NULL;
>> +	fb = NULL;
>> +	mode = NULL;
> 
> Bit a bikeshed, but I'd reset the pointers right after we release/free
> them, in the out: block. Avoids accidental leaking. But it's fine either
> way.

I did that first, but then changed it to this version as this seemed (to
me) to make it more clear what's going on. But I don't feel strongly
either way, so if anyone else chimes in before I push, I can change it =).

> And I agree with Ville, I don't think we need a cc: stable here.

Ok, I'll drop the stable, and push to drm-misc-fixes.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki