diff mbox

drm/exynos: potential use after free in exynos_drm_open()

Message ID 20140121065748.GC31535@elgon.mountain (mailing list archive)
State New, archived
Headers show

Commit Message

Dan Carpenter Jan. 21, 2014, 6:57 a.m. UTC 
If exynos_drm_subdrv_open() fails then we re-use "file_priv".

Fixes: 96f5421523df ('drm/exynos: use a new anon file for exynos gem mmaper')
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Comments

Jingoo Han Jan. 21, 2014, 7:12 a.m. UTC | #1 
On Tuesday, January 21, 2014 3:58 PM, Dan Carpenter wrote:
> 
> If exynos_drm_subdrv_open() fails then we re-use "file_priv".
> 
> Fixes: 96f5421523df ('drm/exynos: use a new anon file for exynos gem mmaper')
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Reviewed-by: Jingoo Han <jg1.han@samsung.com>

Yes, right.
The freed 'file_priv' should not be re-used.

Best regards,
Jingoo Han

> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> index 9d096a0c5f8d..3c845292845a 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> @@ -174,6 +174,7 @@ static int exynos_drm_open(struct drm_device *dev, struct drm_file *file)
>  	if (ret) {
>  		kfree(file_priv);
>  		file->driver_priv = NULL;
> +		return ret;
>  	}
> 
>  	anon_filp = anon_inode_getfile("exynos_gem", &exynos_drm_gem_fops,
> @@ -186,7 +187,7 @@ static int exynos_drm_open(struct drm_device *dev, struct drm_file *file)
>  	anon_filp->f_mode = FMODE_READ | FMODE_WRITE;
>  	file_priv->anon_filp = anon_filp;
> 
> -	return ret;
> +	return 0;
>  }
> 
>  static void exynos_drm_preclose(struct drm_device *dev,
Inki Dae Jan. 21, 2014, 7:12 a.m. UTC | #2 
Hi,

This issue had already been reported, and fixed by Sachin. And also
that patch has been merged to exynos-drm-next. Please see below link,
        http://www.spinics.net/lists/dri-devel/msg51889.html


Thanks,
Inki Dae

2014/1/21 Dan Carpenter <dan.carpenter@oracle.com>:
> If exynos_drm_subdrv_open() fails then we re-use "file_priv".
>
> Fixes: 96f5421523df ('drm/exynos: use a new anon file for exynos gem mmaper')
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> index 9d096a0c5f8d..3c845292845a 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> @@ -174,6 +174,7 @@ static int exynos_drm_open(struct drm_device *dev, struct drm_file *file)
>         if (ret) {
>                 kfree(file_priv);
>                 file->driver_priv = NULL;
> +               return ret;
>         }
>
>         anon_filp = anon_inode_getfile("exynos_gem", &exynos_drm_gem_fops,
> @@ -186,7 +187,7 @@ static int exynos_drm_open(struct drm_device *dev, struct drm_file *file)
>         anon_filp->f_mode = FMODE_READ | FMODE_WRITE;
>         file_priv->anon_filp = anon_filp;
>
> -       return ret;
> +       return 0;
>  }
>
>  static void exynos_drm_preclose(struct drm_device *dev,
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
Walter Harms Jan. 21, 2014, 12:37 p.m. UTC | #3 
Am 21.01.2014 07:57, schrieb Dan Carpenter:
> If exynos_drm_subdrv_open() fails then we re-use "file_priv".
> 
> Fixes: 96f5421523df ('drm/exynos: use a new anon file for exynos gem mmaper')
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> index 9d096a0c5f8d..3c845292845a 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> @@ -174,6 +174,7 @@ static int exynos_drm_open(struct drm_device *dev, struct drm_file *file)
>  	if (ret) {
>  		kfree(file_priv);
>  		file->driver_priv = NULL;
> +		return ret;
>  	}

using
	kfree(	file->driver_priv );
	file->driver_priv = NULL;

would be less confusing to read, and give checkers a better chance to spot mistakes.
(btw: file_priv could be removed from this function completely).

just my 2 cents,
re,
 wh

>  
>  	anon_filp = anon_inode_getfile("exynos_gem", &exynos_drm_gem_fops,
> @@ -186,7 +187,7 @@ static int exynos_drm_open(struct drm_device *dev, struct drm_file *file)
>  	anon_filp->f_mode = FMODE_READ | FMODE_WRITE;
>  	file_priv->anon_filp = anon_filp;
>  
> -	return ret;
> +	return 0;
>  }
>  
>  static void exynos_drm_preclose(struct drm_device *dev,
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
Walter Harms Jan. 21, 2014, 12:43 p.m. UTC | #4 
i have just noticed: The function already exits

194 static void exynos_drm_postclose(struct drm_device *dev, struct drm_file *file)
195 {
196         if (!file->driver_priv)
197                 return;
198
199         kfree(file->driver_priv);
200         file->driver_priv = NULL;
201 }


Am 21.01.2014 13:37, schrieb walter harms:
> 
> 
> Am 21.01.2014 07:57, schrieb Dan Carpenter:
>> If exynos_drm_subdrv_open() fails then we re-use "file_priv".
>>
>> Fixes: 96f5421523df ('drm/exynos: use a new anon file for exynos gem mmaper')
>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
>> index 9d096a0c5f8d..3c845292845a 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
>> @@ -174,6 +174,7 @@ static int exynos_drm_open(struct drm_device *dev, struct drm_file *file)
>>  	if (ret) {
>>  		kfree(file_priv);
>>  		file->driver_priv = NULL;
>> +		return ret;
>>  	}
> 
> using
> 	kfree(	file->driver_priv );
> 	file->driver_priv = NULL;
> 
> would be less confusing to read, and give checkers a better chance to spot mistakes.
> (btw: file_priv could be removed from this function completely).
> 
> just my 2 cents,
> re,
>  wh
> 
>>  
>>  	anon_filp = anon_inode_getfile("exynos_gem", &exynos_drm_gem_fops,
>> @@ -186,7 +187,7 @@ static int exynos_drm_open(struct drm_device *dev, struct drm_file *file)
>>  	anon_filp->f_mode = FMODE_READ | FMODE_WRITE;
>>  	file_priv->anon_filp = anon_filp;
>>  
>> -	return ret;
>> +	return 0;
>>  }
>>  
>>  static void exynos_drm_preclose(struct drm_device *dev,
>> --
>> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
Dan Carpenter Jan. 21, 2014, 1:35 p.m. UTC | #5 
On Tue, Jan 21, 2014 at 01:43:55PM +0100, walter harms wrote:
> 
> i have just noticed: The function already exits
> 
> 194 static void exynos_drm_postclose(struct drm_device *dev, struct drm_file *file)
> 195 {
> 196         if (!file->driver_priv)
> 197                 return;
> 198
> 199         kfree(file->driver_priv);
> 200         file->driver_priv = NULL;
> 201 }

The function is different in the current code.  I glanced through
drm_open_helper() and I don't see that file->driver_priv to NULL is
needed anyway...

regards,
dan carpenter
diff mbox

Patch

diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
index 9d096a0c5f8d..3c845292845a 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
@@ -174,6 +174,7 @@  static int exynos_drm_open(struct drm_device *dev, struct drm_file *file)
 	if (ret) {
 		kfree(file_priv);
 		file->driver_priv = NULL;
+		return ret;
 	}
 
 	anon_filp = anon_inode_getfile("exynos_gem", &exynos_drm_gem_fops,
@@ -186,7 +187,7 @@  static int exynos_drm_open(struct drm_device *dev, struct drm_file *file)
 	anon_filp->f_mode = FMODE_READ | FMODE_WRITE;
 	file_priv->anon_filp = anon_filp;
 
-	return ret;
+	return 0;
 }
 
 static void exynos_drm_preclose(struct drm_device *dev,