[v1] drm: bridge: cdns-mhdp8546: Fix possible null pointer dereference

[RFC PATCH] drm: bridge: cdns-mhdp8546: Fix possible null pointer dereference

Posted by Jayesh Choudhary 2 weeks, 6 days ago

For the cases we have DRM_BRIDGE_ATTACH_NO_CONNECTOR flag set,
the connector structure is not initialised in the bridge. That's done
by encoder. So in case of some failure in cdns_mhdp_atomic_enable,
when we schedule work for modeset_retry_work, we will use the mutex
of connector which will result in NULL pointer dereference.
Handle it by adding condition for the connector. Otherwise, since
the modeset_retry_work tries to set the connector status as bad,
set the mhdp->plugged as false which would give the connector
status as disconnected in detect hook.

Fixes: fb43aa0acdfd ("drm: bridge: Add support for Cadence MHDP8546 DPI/DP bridge")
Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
---

NOTE: Found this issue in one particular board where edid read failed.
Issue log: <https://gist.github.com/Jayesh2000/233f87f9becdf1e66f1da6fd53f77429>

Adding conditional fixes the null pointer issue but there is still
flooding of these logs (128 times):
"cdns-mhdp8546 a000000.bridge: Failed to read DPCD addr 0"

Sending RFC as I am still not sure about how to handle this flooding.
Is it okay to decrease the log level for DPCD read and DPCD write in
the cdns_mhdp_transfer to debug?

 .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 24 ++++++++++---------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
index d081850e3c03..6a121a2700d2 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
@@ -2363,18 +2363,20 @@ static void cdns_mhdp_modeset_retry_fn(struct work_struct *work)
 
 	mhdp = container_of(work, typeof(*mhdp), modeset_retry_work);
 
-	conn = &mhdp->connector;
-
-	/* Grab the locks before changing connector property */
-	mutex_lock(&conn->dev->mode_config.mutex);
-
-	/*
-	 * Set connector link status to BAD and send a Uevent to notify
-	 * userspace to do a modeset.
-	 */
-	drm_connector_set_link_status_property(conn, DRM_MODE_LINK_STATUS_BAD);
-	mutex_unlock(&conn->dev->mode_config.mutex);
+	if (mhdp->connector.dev) {
+		conn = &mhdp->connector;
+		/* Grab the locks before changing connector property */
+		mutex_lock(&conn->dev->mode_config.mutex);
 
+		/*
+		 * Set connector link status to BAD and send a Uevent to notify
+		 * userspace to do a modeset.
+		 */
+		drm_connector_set_link_status_property(conn, DRM_MODE_LINK_STATUS_BAD);
+		mutex_unlock(&conn->dev->mode_config.mutex);
+	} else {
+		mhdp->plugged = false;
+	}
 	/* Send Hotplug uevent so userspace can reprobe */
 	drm_kms_helper_hotplug_event(mhdp->bridge.dev);
 }
-- 
2.34.1

Re: [RFC PATCH] drm: bridge: cdns-mhdp8546: Fix possible null pointer dereference

Posted by Tomi Valkeinen 1 week, 5 days ago

Hi,

On 16/01/2025 13:16, Jayesh Choudhary wrote:
> For the cases we have DRM_BRIDGE_ATTACH_NO_CONNECTOR flag set,

Any idea if any other platform than K3 is using this driver? tidss 
supports DRM_BRIDGE_ATTACH_NO_CONNECTOR, so if K3 is the only user, we 
could drop the legacy !DRM_BRIDGE_ATTACH_NO_CONNECTOR case. Which would 
remove quite a bit of code, I think, and make the driver a bit more easy 
to understand (although I think it could use a major cleanup...).

> the connector structure is not initialised in the bridge. That's done
> by encoder. So in case of some failure in cdns_mhdp_atomic_enable,
> when we schedule work for modeset_retry_work, we will use the mutex
> of connector which will result in NULL pointer dereference.
> Handle it by adding condition for the connector. Otherwise, since
> the modeset_retry_work tries to set the connector status as bad,
> set the mhdp->plugged as false which would give the connector
> status as disconnected in detect hook.

I'm not quite sure if this whole system makes sense (no one else is 
doing it), but I think you can find the connector from the current 
state. Then setting the property could be done for 
DRM_BRIDGE_ATTACH_NO_CONNECTOR case too.

  Tomi

> Fixes: fb43aa0acdfd ("drm: bridge: Add support for Cadence MHDP8546 DPI/DP bridge")
> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
> ---
> 
> NOTE: Found this issue in one particular board where edid read failed.
> Issue log: <https://gist.github.com/Jayesh2000/233f87f9becdf1e66f1da6fd53f77429>
> 
> Adding conditional fixes the null pointer issue but there is still
> flooding of these logs (128 times):
> "cdns-mhdp8546 a000000.bridge: Failed to read DPCD addr 0"
> 
> Sending RFC as I am still not sure about how to handle this flooding.
> Is it okay to decrease the log level for DPCD read and DPCD write in
> the cdns_mhdp_transfer to debug?
> 
>   .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 24 ++++++++++---------
>   1 file changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> index d081850e3c03..6a121a2700d2 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> @@ -2363,18 +2363,20 @@ static void cdns_mhdp_modeset_retry_fn(struct work_struct *work)
>   
>   	mhdp = container_of(work, typeof(*mhdp), modeset_retry_work);
>   
> -	conn = &mhdp->connector;
> -
> -	/* Grab the locks before changing connector property */
> -	mutex_lock(&conn->dev->mode_config.mutex);
> -
> -	/*
> -	 * Set connector link status to BAD and send a Uevent to notify
> -	 * userspace to do a modeset.
> -	 */
> -	drm_connector_set_link_status_property(conn, DRM_MODE_LINK_STATUS_BAD);
> -	mutex_unlock(&conn->dev->mode_config.mutex);
> +	if (mhdp->connector.dev) {
> +		conn = &mhdp->connector;
> +		/* Grab the locks before changing connector property */
> +		mutex_lock(&conn->dev->mode_config.mutex);
>   
> +		/*
> +		 * Set connector link status to BAD and send a Uevent to notify
> +		 * userspace to do a modeset.
> +		 */
> +		drm_connector_set_link_status_property(conn, DRM_MODE_LINK_STATUS_BAD);
> +		mutex_unlock(&conn->dev->mode_config.mutex);
> +	} else {
> +		mhdp->plugged = false;
> +	}
>   	/* Send Hotplug uevent so userspace can reprobe */
>   	drm_kms_helper_hotplug_event(mhdp->bridge.dev);
>   }

Re: [RFC PATCH] drm: bridge: cdns-mhdp8546: Fix possible null pointer dereference

Posted by Alexander Stein 1 week, 5 days ago

Hi,

Am Donnerstag, 23. Januar 2025, 17:20:34 CET schrieb Tomi Valkeinen:
> Hi,
> 
> On 16/01/2025 13:16, Jayesh Choudhary wrote:
> > For the cases we have DRM_BRIDGE_ATTACH_NO_CONNECTOR flag set,
> 
> Any idea if any other platform than K3 is using this driver? tidss 
> supports DRM_BRIDGE_ATTACH_NO_CONNECTOR, so if K3 is the only user, we 
> could drop the legacy !DRM_BRIDGE_ATTACH_NO_CONNECTOR case. Which would 
> remove quite a bit of code, I think, and make the driver a bit more easy 
> to understand (although I think it could use a major cleanup...).

FYI: Not directly using it, but patch series [1] is at least touching
this file.

Best regards,
Alexander

[1] https://lore.kernel.org/all/cover.1734340233.git.Sandor.yu@nxp.com/

-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

Re: [RFC PATCH] drm: bridge: cdns-mhdp8546: Fix possible null pointer dereference

Posted by Jayesh Choudhary 1 week, 1 day ago

Hello Tomi, Alexander,

On 24/01/25 13:38, Alexander Stein wrote:
> Hi,
> 
> Am Donnerstag, 23. Januar 2025, 17:20:34 CET schrieb Tomi Valkeinen:
>> Hi,
>>
>> On 16/01/2025 13:16, Jayesh Choudhary wrote:
>>> For the cases we have DRM_BRIDGE_ATTACH_NO_CONNECTOR flag set,
>>
>> Any idea if any other platform than K3 is using this driver? tidss
>> supports DRM_BRIDGE_ATTACH_NO_CONNECTOR, so if K3 is the only user, we
>> could drop the legacy !DRM_BRIDGE_ATTACH_NO_CONNECTOR case. Which would
>> remove quite a bit of code, I think, and make the driver a bit more easy
>> to understand (although I think it could use a major cleanup...).
> 
> FYI: Not directly using it, but patch series [1] is at least touching
> this file.

I can't see any other platform using this.
No one uses compatible "cdns,mhdp8546" and only K3 devices uses the
wrapper compatible "ti,j721e-mhdp8546"
Let me post next RFC version with legacy !DRM_BRIDGE_ATTACH_NO_CONNECTOR
dropped.

And the mentioned series only touches mailbox access function to move
then to a common helper file. (So independent change)

> 
> Best regards,
> Alexander
> 
> [1] https://lore.kernel.org/all/cover.1734340233.git.Sandor.yu@nxp.com/
> 

Warm Regards,
Jayesh