drm/msm: Fix NULL deref in adreno_load_gpu

PATCH Superseded Archit Taneja 2 participants 5 messages Dec 14, 2017 – Jan 2, 2018

Subsystems (4)

DRM drivers Arm Qualcomm mailing list DRM driver for Qualcomm display hardware DRM driver for Qualcomm Adreno GPUs

lore

Share & tools

lore

Archit Taneja

author

2017-12-14 05:41 UTC #0 lore raw

PATCH 1/1 drm/msm: Fix NULL deref in adreno_load_gpu

The msm/kms driver should work even if there is no GPU device specified in DT. Currently, we get a NULL dereference crash in adreno_load_gpu since the driver assumes that priv->gpu_pdev is non-NULL.

Perform an additional check on priv->gpu_pdev before trying to retrieve the msm_gpu pointer from it.

Fixes eec874ce5ff1 (drm/msm/adreno: load gpu at probe/bind time)

Signed-off-by Archit Taneja <[email protected]>

---
 drivers/gpu/drm/msm/adreno/adreno_device.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

drivers/gpu/drm/msm/adreno/adreno_device.c +9 -2

index 05022ea2a007..ac60cf3c794e 100644
@@ -124,10 +124,17 @@ const struct adreno_info *adreno_info(struct adreno_rev rev)
 struct msm_gpu *adreno_load_gpu(struct drm_device *dev)
 {
 	struct msm_drm_private *priv = dev->dev_private;
-	struct platform_device *pdev = priv->gpu_pdev;
-	struct msm_gpu *gpu = platform_get_drvdata(priv->gpu_pdev);
+	struct platform_device *pdev;
+	struct msm_gpu *gpu;
 	int ret;
 
+	pdev = priv->gpu_pdev;
+	if (!pdev) {
+		dev_dbg(dev->dev, "no adreno platform device found\n");
+		return NULL;
+	}
+
+	gpu = platform_get_drvdata(pdev);
 	if (!gpu) {
 		dev_err(dev->dev, "no adreno device\n");
 		return NULL;

Signature · 2 lines

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

Jordan Crouse

2017-12-15 15:33 UTC #1 lore raw

Quoted 28 lines · Archit Taneja

The msm/kms driver should work even if there is no GPU device specified in DT. Currently, we get a NULL dereference crash in adreno_load_gpu since the driver assumes that priv->gpu_pdev is non-NULL.
Perform an additional check on priv->gpu_pdev before trying to retrieve the msm_gpu pointer from it.
Fixes eec874ce5ff1 (drm/msm/adreno: load gpu at probe/bind time)
Signed-off-by Archit Taneja <[email protected]>
---
 drivers/gpu/drm/msm/adreno/adreno_device.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)
drivers/gpu/drm/msm/adreno/adreno_device.c +2 -2
index 05022ea2a007..ac60cf3c794e 100644
@@ -124,10 +124,17 @@ const struct adreno_info *adreno_info(struct adreno_rev rev)
 struct msm_gpu *adreno_load_gpu(struct drm_device *dev)
 {
 	struct msm_drm_private *priv = dev->dev_private;
-	struct platform_device *pdev = priv->gpu_pdev;
-	struct msm_gpu *gpu = platform_get_drvdata(priv->gpu_pdev);
+	struct platform_device *pdev;
+	struct msm_gpu *gpu;
 	int ret;
 

drivers/gpu/drm/msm/adreno/adreno_device.c const struct adreno_info *adreno_info(struct adreno_rev rev) · line 140

Quoted 10 lines

drivers/gpu/drm/msm/adreno/adreno_device.c +7 -0

+	pdev = priv->gpu_pdev;
+	if (!pdev) {
+		dev_dbg(dev->dev, "no adreno platform device found\n");
+		return NULL;
+	}
+
+	gpu = platform_get_drvdata(pdev);
 	if (!gpu) {
 		dev_err(dev->dev, "no adreno device\n");
 		return NULL;

Obviously correct fix but I can't help but think that we should share the same error message, so something like:

struct msm_gpu *gpu = NULL;

if (priv->gpu_pdev)
gpu = platform_get_drvdata(priv->gpu_pdev);

if (!gpu) {
	dev_err(dev->dev, "No GPU device was was found\n");

return NULL;
}

(also, I can't help but think maybe that dev_err should be a ONCE so you don't get a nasty message every time you open the file descriptor).

Jordan

Signature · 2 lines

The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Archit Taneja

author

2017-12-20 06:48 UTC #2 lore raw

Quoted 50 lines · Jordan Crouse

Quoted 28 lines · Archit Taneja
The msm/kms driver should work even if there is no GPU device specified in DT. Currently, we get a NULL dereference crash in adreno_load_gpu since the driver assumes that priv->gpu_pdev is non-NULL.
Perform an additional check on priv->gpu_pdev before trying to retrieve the msm_gpu pointer from it.
Fixes eec874ce5ff1 (drm/msm/adreno: load gpu at probe/bind time)
Signed-off-by Archit Taneja <[email protected]>
---
  drivers/gpu/drm/msm/adreno/adreno_device.c | 11 +++++++++--
  1 file changed, 9 insertions(+), 2 deletions(-)
drivers/gpu/drm/msm/adreno/adreno_device.c +2 -2
index 05022ea2a007..ac60cf3c794e 100644
@@ -124,10 +124,17 @@ const struct adreno_info *adreno_info(struct adreno_rev rev)
  struct msm_gpu *adreno_load_gpu(struct drm_device *dev)
  {
  	struct msm_drm_private *priv = dev->dev_private;
-	struct platform_device *pdev = priv->gpu_pdev;
-	struct msm_gpu *gpu = platform_get_drvdata(priv->gpu_pdev);
+	struct platform_device *pdev;
+	struct msm_gpu *gpu;
  	int ret;
 
drivers/gpu/drm/msm/adreno/adreno_device.c const struct adreno_info *adreno_info(struct adreno_rev rev) · line 140
Quoted 10 lines
drivers/gpu/drm/msm/adreno/adreno_device.c +7 -0
+	pdev = priv->gpu_pdev;
+	if (!pdev) {
+		dev_dbg(dev->dev, "no adreno platform device found\n");
+		return NULL;
+	}
+
+	gpu = platform_get_drvdata(pdev);
  	if (!gpu) {
  		dev_err(dev->dev, "no adreno device\n");
  		return NULL;
Obviously correct fix but I can't help but think that we should share the same error message, so something like:
struct msm_gpu *gpu = NULL;
..
if (priv->gpu_pdev)
    gpu = platform_get_drvdata(priv->gpu_pdev);

Quoted 7 lines

if (!gpu) {
	dev_err(dev->dev, "No GPU device was was found\n");
return NULL;
}
(also, I can't help but think maybe that dev_err should be a ONCE so you don't get a nasty message every time you open the file descriptor).

This approach looks better. I'll re-spin.

Thanks,
Archit

Quoted 3 lines

Jordan

Signature · 6 lines

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Archit Taneja

author

2017-12-22 10:11 UTC #3 lore raw

PATCH v2 1/1 drm/msm: Fix NULL deref in adreno_load_gpu

In reply to Archit Taneja

Perform an additional check on priv->gpu_pdev before trying to retrieve the msm_gpu pointer from it.

v2: Incorporate Jordan's comments:
- Simplify the check to share the same error message.
- Use dev_err_once() to avoid an error message every time we open the drm device fd.

Fixes eec874ce5ff1 (drm/msm/adreno: load gpu at probe/bind time)

Signed-off-by Archit Taneja <[email protected]>

---
 drivers/gpu/drm/msm/adreno/adreno_device.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

drivers/gpu/drm/msm/adreno/adreno_device.c +5 -2

index 05022ea2a007..bfb3d689f47d 100644
@@ -125,11 +125,14 @@ struct msm_gpu *adreno_load_gpu(struct drm_device *dev)
 {
 	struct msm_drm_private *priv = dev->dev_private;
 	struct platform_device *pdev = priv->gpu_pdev;
-	struct msm_gpu *gpu = platform_get_drvdata(priv->gpu_pdev);
+	struct msm_gpu *gpu = NULL;
 	int ret;
 
+	if (pdev)
+		gpu = platform_get_drvdata(pdev);
+
 	if (!gpu) {
-		dev_err(dev->dev, "no adreno device\n");
+		dev_err_once(dev->dev, "no GPU device was found\n");
 		return NULL;
 	}

Signature · 7 lines

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Jordan Crouse

Acked-by on v2 1/1

2018-01-02 18:32 UTC #4 lore raw

Quoted 7 lines · Archit Taneja

The msm/kms driver should work even if there is no GPU device specified in DT. Currently, we get a NULL dereference crash in adreno_load_gpu since the driver assumes that priv->gpu_pdev is non-NULL.
Perform an additional check on priv->gpu_pdev before trying to retrieve the msm_gpu pointer from it.

Quoted 8 lines

v2: Incorporate Jordan's comments:
- Simplify the check to share the same error message.
- Use dev_err_once() to avoid an error message every time we open the drm device fd.
Fixes eec874ce5ff1 (drm/msm/adreno: load gpu at probe/bind time)
Signed-off-by Archit Taneja <[email protected]>

Acked-by Jordan Crouse <[email protected]>

Quoted 24 lines

---
 drivers/gpu/drm/msm/adreno/adreno_device.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

drivers/gpu/drm/msm/adreno/adreno_device.c +5 -2

index 05022ea2a007..bfb3d689f47d 100644
@@ -125,11 +125,14 @@ struct msm_gpu *adreno_load_gpu(struct drm_device *dev)
 {
 	struct msm_drm_private *priv = dev->dev_private;
 	struct platform_device *pdev = priv->gpu_pdev;
-	struct msm_gpu *gpu = platform_get_drvdata(priv->gpu_pdev);
+	struct msm_gpu *gpu = NULL;
 	int ret;
 
+	if (pdev)
+		gpu = platform_get_drvdata(pdev);
+
 	if (!gpu) {
-		dev_err(dev->dev, "no adreno device\n");
+		dev_err_once(dev->dev, "no GPU device was found\n");
 		return NULL;
 	}

Signature · 6 lines

The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel