Skip to contentSkip to conversation

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

PATCH In review Dan Carpenter 4 participants 6 messages Jan 21, 2014

Subsystems (4)
DRM drivers Kernel janitors Arm Samsung Exynos DRM drivers for Exynos
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

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

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 <[email protected]>
drivers/gpu/drm/exynos/exynos_drm_drv.c +2 -1
index 9d096a0c5f8d..3c845292845a 100644
@@ -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,
Reviewed-by on v1 1/1
#1 lore raw
Quoted 5 lines · 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 <[email protected]>
Reviewed-by Jingoo Han <[email protected]>

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

Best regards,
Jingoo Han

Quoted 22 lines
drivers/gpu/drm/exynos/exynos_drm_drv.c +2 -1
index 9d096a0c5f8d..3c845292845a 100644
@@ -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,
maintainer
#2 lore raw
In reply to Dan Carpenter

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 <[email protected]>:

Quoted 30 lines

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 <[email protected]>
drivers/gpu/drm/exynos/exynos_drm_drv.c +2 -1
index 9d096a0c5f8d..3c845292845a 100644
@@ -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
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

#3 lore raw
In reply to Dan Carpenter

Am 21.01.2014 07:57, schrieb Dan Carpenter:

Quoted 10 lines

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 <[email protected]>
drivers/gpu/drm/exynos/exynos_drm_drv.c +0 -0
index 9d096a0c5f8d..3c845292845a 100644

drivers/gpu/drm/exynos/exynos_drm_drv.c static int exynos_drm_open(struct drm_device *dev, struct drm_file *file) · line 178

Quoted 5 lines
drivers/gpu/drm/exynos/exynos_drm_drv.c +1 -0
@@ -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

Quoted 16 lines
drivers/gpu/drm/exynos/exynos_drm_drv.c +1 -2
 	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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#4 lore raw

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:

Quoted 51 lines

Am 21.01.2014 07:57, schrieb Dan Carpenter:

Quoted 10 lines

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 <[email protected]>
drivers/gpu/drm/exynos/exynos_drm_drv.c +0 -0
index 9d096a0c5f8d..3c845292845a 100644

drivers/gpu/drm/exynos/exynos_drm_drv.c static int exynos_drm_open(struct drm_device *dev, struct drm_file *file) · line 178

Quoted 5 lines
drivers/gpu/drm/exynos/exynos_drm_drv.c +1 -0
@@ -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

Quoted 12 lines
drivers/gpu/drm/exynos/exynos_drm_drv.c +1 -2
 	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,
--
Quoted 4 lines

To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#5 lore raw
Quoted 11 lines · walter harms

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