[PATCH 0/3] drm/msm/dsi: improve pointer validation checks

This series improves a few pointer validation checks around the
drm/msm/dsi driver.

Lloyd Atkinson (3):
  drm/msm/dsi: check src_pll for null in dsi manager
  drm/msm/dsi: correct DSI id bounds check during registration
  drm/msm/dsi: check msm_dsi and dsi pointers before use

 drivers/gpu/drm/msm/dsi/dsi.c         | 22 ++++++++++------------
 drivers/gpu/drm/msm/dsi/dsi_manager.c |  6 +++++-
 2 files changed, 15 insertions(+), 13 deletions(-)

-- 
QUALCOMM Canada, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Add checks for failure after retrieving the src_pll, since it
may fail. This prevents an invalid pointer dereference later in
msm_dsi_pll_get_clk_provider.

Signed-off-by: Lloyd Atkinson <redacted>
---
 drivers/gpu/drm/msm/dsi/dsi_manager.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
index 8552481..d276358 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c

@@ -88,6 +88,8 @@ static int dsi_mgr_setup_components(int id)
 
 		msm_dsi_phy_set_usecase(msm_dsi->phy, MSM_DSI_PHY_STANDALONE);
 		src_pll = msm_dsi_phy_get_pll(msm_dsi->phy);
+		if (!src_pll)
+			return -EINVAL;
 		ret = msm_dsi_host_set_src_pll(msm_dsi->host, src_pll);
 	} else if (!other_dsi) {
 		ret = 0;

@@ -116,6 +118,8 @@ static int dsi_mgr_setup_components(int id)
 		msm_dsi_phy_set_usecase(clk_slave_dsi->phy,
 					MSM_DSI_PHY_SLAVE);
 		src_pll = msm_dsi_phy_get_pll(clk_master_dsi->phy);
+		if (!src_pll)
+			return -EINVAL;
 		ret = msm_dsi_host_set_src_pll(msm_dsi->host, src_pll);
 		if (ret)
 			return ret;

-- 
QUALCOMM Canada, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Check DSI instance id argument against the proper boundary size
to protect against invalid configuration of the DSI id.

Signed-off-by: Lloyd Atkinson <redacted>
---
 drivers/gpu/drm/msm/dsi/dsi_manager.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
index d276358..dec2f74 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c

@@ -862,7 +862,7 @@ int msm_dsi_manager_register(struct msm_dsi *msm_dsi)
 	int id = msm_dsi->id;
 	int ret;
 
-	if (id > DSI_MAX) {
+	if (id >= DSI_MAX) {
 		pr_err("%s: invalid id %d\n", __func__, id);
 		return -EINVAL;
 	}

-- 
QUALCOMM Canada, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Move null checks of pointer arguments to the beginning of the
modeset init function since they are referenced immediately
instead of after they have already been used.

Signed-off-by: Lloyd Atkinson <redacted>
---
 drivers/gpu/drm/msm/dsi/dsi.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c
index 98742d7..be855db 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.c
+++ b/drivers/gpu/drm/msm/dsi/dsi.c

@@ -196,7 +196,7 @@ int msm_dsi_modeset_init(struct msm_dsi *msm_dsi, struct drm_device *dev,
 	struct drm_bridge *ext_bridge;
 	int ret;
 
-	if (WARN_ON(!encoder))
+	if (!encoder || !msm_dsi || !dev)
 		return -EINVAL;
 
 	msm_dsi->dev = dev;

@@ -245,19 +245,17 @@ int msm_dsi_modeset_init(struct msm_dsi *msm_dsi, struct drm_device *dev,
 
 	return 0;
 fail:
-	if (msm_dsi) {
-		/* bridge/connector are normally destroyed by drm: */
-		if (msm_dsi->bridge) {
-			msm_dsi_manager_bridge_destroy(msm_dsi->bridge);
-			msm_dsi->bridge = NULL;
-		}
+	/* bridge/connector are normally destroyed by drm: */
+	if (msm_dsi->bridge) {
+		msm_dsi_manager_bridge_destroy(msm_dsi->bridge);
+		msm_dsi->bridge = NULL;
+	}
 
-		/* don't destroy connector if we didn't make it */
-		if (msm_dsi->connector && !msm_dsi->external_bridge)
-			msm_dsi->connector->funcs->destroy(msm_dsi->connector);
+	/* don't destroy connector if we didn't make it */
+	if (msm_dsi->connector && !msm_dsi->external_bridge)
+		msm_dsi->connector->funcs->destroy(msm_dsi->connector);
 
-		msm_dsi->connector = NULL;
-	}
+	msm_dsi->connector = NULL;
 
 	return ret;
 }

-- 
QUALCOMM Canada, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

On Fri, Jan 12, 2018 at 3:55 PM, Lloyd Atkinson [off-list ref] wrote:

hmm, this is a bit awkward, and probably something that we should have
noticed/fixed by now.. but in case CONFIG_DRM_MSM_DSI_PLL is not
enabled, msm_dsi_pll_init() returns ERR_PTR(-ENODEV).  But if it is
enabled, then error paths return NULL.

Probably we should fix the enabled case to propagate back
ERR_PTR(errno) and never return NULL, and then use IS_ERR() here.

(I guess also IS_ERR_OR_NULL() here would do the job.. but perhaps
best to fix the root issue)

BR,
-R

On Fri, Jan 12, 2018 at 3:55 PM, Lloyd Atkinson [off-list ref] wrote:

good catch, I've queued this up on msm-next

BR,
-R

                pr_err("%s: invalid id %d\n", __func__, id);
                return -EINVAL;
        }
--
QUALCOMM Canada, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Fri, Jan 12, 2018 at 3:55 PM, Lloyd Atkinson [off-list ref] wrote:

hmm, the checking if msm_dsi is null later in the fail: case is
certainly sketchy after we've already deref'd it..  so this looks like
the right thing.  But I'd like to keep the WARN_ON(), since this is a
case that shouldn't really happen.  The WARN_ON() nicely documents
that none of these parameters are expected to be NULL, and it gives a
big shouty message to anyone who inadvertently changes something that
breaks that assumption.  Other than that, it looks good.

BR,
-R

On 1/15/2018 9:48 AM, Rob Clark wrote:

On Fri, Jan 12, 2018 at 3:55 PM, Lloyd Atkinson [off-list ref] wrote:

quoted

Move null checks of pointer arguments to the beginning of the
modeset init function since they are referenced immediately
instead of after they have already been used.

Signed-off-by: Lloyd Atkinson <redacted>
---
 drivers/gpu/drm/msm/dsi/dsi.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c
index 98742d7..be855db 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.c
+++ b/drivers/gpu/drm/msm/dsi/dsi.c

@@ -196,7 +196,7 @@ int msm_dsi_modeset_init(struct msm_dsi *msm_dsi, struct drm_device *dev,
        struct drm_bridge *ext_bridge;
        int ret;

-       if (WARN_ON(!encoder))
+       if (!encoder || !msm_dsi || !dev)

hmm, the checking if msm_dsi is null later in the fail: case is
certainly sketchy after we've already deref'd it..  so this looks like
the right thing.  But I'd like to keep the WARN_ON(), since this is a
case that shouldn't really happen.  The WARN_ON() nicely documents
that none of these parameters are expected to be NULL, and it gives a
big shouty message to anyone who inadvertently changes something that
breaks that assumption.  Other than that, it looks good.

BR,
-R

Sure. Do you want to add WARN_ONs to msm_dsi and dev as well?

Thanks,
Lloyd

quoted

                return -EINVAL;

        msm_dsi->dev = dev;

@@ -245,19 +245,17 @@ int msm_dsi_modeset_init(struct msm_dsi *msm_dsi, struct drm_device *dev,

        return 0;
 fail:
-       if (msm_dsi) {
-               /* bridge/connector are normally destroyed by drm: */
-               if (msm_dsi->bridge) {
-                       msm_dsi_manager_bridge_destroy(msm_dsi->bridge);
-                       msm_dsi->bridge = NULL;
-               }
+       /* bridge/connector are normally destroyed by drm: */
+       if (msm_dsi->bridge) {
+               msm_dsi_manager_bridge_destroy(msm_dsi->bridge);
+               msm_dsi->bridge = NULL;
+       }

-               /* don't destroy connector if we didn't make it */
-               if (msm_dsi->connector && !msm_dsi->external_bridge)
-                       msm_dsi->connector->funcs->destroy(msm_dsi->connector);
+       /* don't destroy connector if we didn't make it */
+       if (msm_dsi->connector && !msm_dsi->external_bridge)
+               msm_dsi->connector->funcs->destroy(msm_dsi->connector);

-               msm_dsi->connector = NULL;
-       }
+       msm_dsi->connector = NULL;

        return ret;
 }
--

QUALCOMM Canada, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
QUALCOMM Canada, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

On Mon, Jan 15, 2018 at 10:01 AM, Lloyd Atkinson
[off-list ref] wrote:

On 1/15/2018 9:48 AM, Rob Clark wrote:

quoted

On Fri, Jan 12, 2018 at 3:55 PM, Lloyd Atkinson [off-list ref] wrote:

quoted

Move null checks of pointer arguments to the beginning of the
modeset init function since they are referenced immediately
instead of after they have already been used.

Signed-off-by: Lloyd Atkinson <redacted>
---
 drivers/gpu/drm/msm/dsi/dsi.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c
index 98742d7..be855db 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.c
+++ b/drivers/gpu/drm/msm/dsi/dsi.c

@@ -196,7 +196,7 @@ int msm_dsi_modeset_init(struct msm_dsi *msm_dsi, struct drm_device *dev,
        struct drm_bridge *ext_bridge;
        int ret;

-       if (WARN_ON(!encoder))
+       if (!encoder || !msm_dsi || !dev)

hmm, the checking if msm_dsi is null later in the fail: case is
certainly sketchy after we've already deref'd it..  so this looks like
the right thing.  But I'd like to keep the WARN_ON(), since this is a
case that shouldn't really happen.  The WARN_ON() nicely documents
that none of these parameters are expected to be NULL, and it gives a
big shouty message to anyone who inadvertently changes something that
breaks that assumption.  Other than that, it looks good.

BR,
-R

Sure. Do you want to add WARN_ONs to msm_dsi and dev as well?

yup, thanks

BR,
-R

Thanks,
Lloyd

quoted

quoted

                return -EINVAL;

        msm_dsi->dev = dev;

@@ -245,19 +245,17 @@ int msm_dsi_modeset_init(struct msm_dsi *msm_dsi, struct drm_device *dev,

        return 0;
 fail:
-       if (msm_dsi) {
-               /* bridge/connector are normally destroyed by drm: */
-               if (msm_dsi->bridge) {
-                       msm_dsi_manager_bridge_destroy(msm_dsi->bridge);
-                       msm_dsi->bridge = NULL;
-               }
+       /* bridge/connector are normally destroyed by drm: */
+       if (msm_dsi->bridge) {
+               msm_dsi_manager_bridge_destroy(msm_dsi->bridge);
+               msm_dsi->bridge = NULL;
+       }

-               /* don't destroy connector if we didn't make it */
-               if (msm_dsi->connector && !msm_dsi->external_bridge)
-                       msm_dsi->connector->funcs->destroy(msm_dsi->connector);
+       /* don't destroy connector if we didn't make it */
+       if (msm_dsi->connector && !msm_dsi->external_bridge)
+               msm_dsi->connector->funcs->destroy(msm_dsi->connector);

-               msm_dsi->connector = NULL;
-       }
+       msm_dsi->connector = NULL;

        return ret;
 }
--

QUALCOMM Canada, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
QUALCOMM Canada, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Check DSI instance id argument against the proper boundary size
to protect against invalid configuration of the DSI id.

Signed-off-by: Lloyd Atkinson <redacted>
---
 drivers/gpu/drm/msm/dsi/dsi_manager.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
index 1a54fd6..4cb1cb6 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c

@@ -862,7 +862,7 @@ int msm_dsi_manager_register(struct msm_dsi *msm_dsi)
 	int id = msm_dsi->id;
 	int ret;
 
-	if (id > DSI_MAX) {
+	if (id >= DSI_MAX) {
 		pr_err("%s: invalid id %d\n", __func__, id);
 		return -EINVAL;
 	}

-- 
QUALCOMM Canada, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Move null checks of pointer arguments to the beginning of the
modeset init function since they are referenced immediately
instead of after they have already been used.

Signed-off-by: Lloyd Atkinson <redacted>
---
 drivers/gpu/drm/msm/dsi/dsi.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c
index 98742d7..ee7e090 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.c
+++ b/drivers/gpu/drm/msm/dsi/dsi.c

@@ -196,7 +196,7 @@ int msm_dsi_modeset_init(struct msm_dsi *msm_dsi, struct drm_device *dev,
 	struct drm_bridge *ext_bridge;
 	int ret;
 
-	if (WARN_ON(!encoder))
+	if (WARN_ON(!encoder) || WARN_ON(!msm_dsi) || WARN_ON(!dev))
 		return -EINVAL;
 
 	msm_dsi->dev = dev;

@@ -245,19 +245,17 @@ int msm_dsi_modeset_init(struct msm_dsi *msm_dsi, struct drm_device *dev,
 
 	return 0;
 fail:
-	if (msm_dsi) {
-		/* bridge/connector are normally destroyed by drm: */
-		if (msm_dsi->bridge) {
-			msm_dsi_manager_bridge_destroy(msm_dsi->bridge);
-			msm_dsi->bridge = NULL;
-		}
+	/* bridge/connector are normally destroyed by drm: */
+	if (msm_dsi->bridge) {
+		msm_dsi_manager_bridge_destroy(msm_dsi->bridge);
+		msm_dsi->bridge = NULL;
+	}
 
-		/* don't destroy connector if we didn't make it */
-		if (msm_dsi->connector && !msm_dsi->external_bridge)
-			msm_dsi->connector->funcs->destroy(msm_dsi->connector);
+	/* don't destroy connector if we didn't make it */
+	if (msm_dsi->connector && !msm_dsi->external_bridge)
+		msm_dsi->connector->funcs->destroy(msm_dsi->connector);
 
-		msm_dsi->connector = NULL;
-	}
+	msm_dsi->connector = NULL;
 
 	return ret;
 }

-- 
QUALCOMM Canada, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

This series improves a few pointer validation checks around the
drm/msm/dsi driver.

v2 incoporates feedback on patch 1/3 and patch 3/3.

Lloyd Atkinson (3):
  drm/msm/dsi: check for failure on retrieving pll in dsi manager
  drm/msm/dsi: correct DSI id bounds check during registration
  drm/msm/dsi: check msm_dsi and dsi pointers before use

 drivers/gpu/drm/msm/dsi/dsi.c         | 22 ++++++++++------------
 drivers/gpu/drm/msm/dsi/dsi_manager.c |  6 +++++-
 drivers/gpu/drm/msm/dsi/phy/dsi_phy.c |  6 +++---
 drivers/gpu/drm/msm/dsi/pll/dsi_pll.c |  2 +-
 4 files changed, 19 insertions(+), 17 deletions(-)

-- 
QUALCOMM Canada, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Make msm_dsi_pll_init consistently return an error code instead
of NULL when pll initialization fails so that later pll
retrieval can check against an error code. Add checks for these
failures after retrieval of src_pll to avoid invalid pointer
dereferences later in msm_dsi_pll_get_clk_provider.

Signed-off-by: Lloyd Atkinson <redacted>
---
 drivers/gpu/drm/msm/dsi/dsi_manager.c | 4 ++++
 drivers/gpu/drm/msm/dsi/phy/dsi_phy.c | 6 +++---
 drivers/gpu/drm/msm/dsi/pll/dsi_pll.c | 2 +-
 3 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
index 8552481..1a54fd6 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c

@@ -88,6 +88,8 @@ static int dsi_mgr_setup_components(int id)
 
 		msm_dsi_phy_set_usecase(msm_dsi->phy, MSM_DSI_PHY_STANDALONE);
 		src_pll = msm_dsi_phy_get_pll(msm_dsi->phy);
+		if (IS_ERR(src_pll))
+			return PTR_ERR(src_pll);
 		ret = msm_dsi_host_set_src_pll(msm_dsi->host, src_pll);
 	} else if (!other_dsi) {
 		ret = 0;

@@ -116,6 +118,8 @@ static int dsi_mgr_setup_components(int id)
 		msm_dsi_phy_set_usecase(clk_slave_dsi->phy,
 					MSM_DSI_PHY_SLAVE);
 		src_pll = msm_dsi_phy_get_pll(clk_master_dsi->phy);
+		if (IS_ERR(src_pll))
+			return PTR_ERR(src_pll);
 		ret = msm_dsi_host_set_src_pll(msm_dsi->host, src_pll);
 		if (ret)
 			return ret;

diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
index 790ca28..c8bfaa7 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c

@@ -503,10 +503,10 @@ static int dsi_phy_driver_probe(struct platform_device *pdev)
 		goto fail;
 
 	phy->pll = msm_dsi_pll_init(pdev, phy->cfg->type, phy->id);
-	if (!phy->pll)
+	if (IS_ERR_OR_NULL(phy->pll))
 		dev_info(dev,
-			"%s: pll init failed, need separate pll clk driver\n",
-			__func__);
+			"%s: pll init failed: %ld, need separate pll clk driver\n",
+			__func__, PTR_ERR(phy->pll));
 
 	dsi_phy_disable_resource(phy);
 

diff --git a/drivers/gpu/drm/msm/dsi/pll/dsi_pll.c b/drivers/gpu/drm/msm/dsi/pll/dsi_pll.c
index bc289f5..491f08d 100644
--- a/drivers/gpu/drm/msm/dsi/pll/dsi_pll.c
+++ b/drivers/gpu/drm/msm/dsi/pll/dsi_pll.c

@@ -173,7 +173,7 @@ struct msm_dsi_pll *msm_dsi_pll_init(struct platform_device *pdev,
 
 	if (IS_ERR(pll)) {
 		dev_err(dev, "%s: failed to init DSI PLL\n", __func__);
-		return NULL;
+		return pll;
 	}
 
 	pll->type = type;

-- 
QUALCOMM Canada, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation