| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <6a696e33-b855-4fc7-8325-304a009ed001@ideasonboard.com>
Date: Fri, 21 Nov 2025 15:12:13 +0200
From: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
To: Harikrishna Shenoy <h-shenoy@...com>
Cc: Laurent.pinchart@...asonboard.com, airlied@...il.com,
andrzej.hajda@...el.com, andy.yan@...k-chips.com, aradhya.bhatia@...ux.dev,
devarsht@...com, dianders@...omium.org, dri-devel@...ts.freedesktop.org,
javierm@...hat.com, jernej.skrabec@...il.com, jonas@...boo.se,
linux-kernel@...r.kernel.org, linux@...blig.org, luca.ceresoli@...tlin.com,
lumag@...nel.org, lyude@...hat.com, maarten.lankhorst@...ux.intel.com,
mordan@...ras.ru, mripard@...nel.org, neil.armstrong@...aro.org,
rfoss@...nel.org, s-jain1@...com, simona@...ll.ch, tzimmermann@...e.de,
u-kumar1@...com
Subject: Re: [PATCH RESEND v9 1/6] drm/bridge: cadence: cdns-mhdp8546-core:
Set the mhdp connector earlier in atomic_enable()
On 20/11/2025 14:14, Harikrishna Shenoy wrote:
> From: Jayesh Choudhary <j-choudhary@...com>
>
> In case if we get errors in cdns_mhdp_link_up() or cdns_mhdp_reg_read()
> in atomic_enable, we will go to cdns_mhdp_modeset_retry_fn() and will hit
> NULL pointer while trying to access the mutex. We need the connector to
> be set before that. Unlike in legacy cases with flag
> !DRM_BRIDGE_ATTACH_NO_CONNECTOR, we do not have connector initialised
> in bridge_attach(), so add the mhdp->connector_ptr in device structure
> to handle both cases with DRM_BRIDGE_ATTACH_NO_CONNECTOR and
> !DRM_BRIDGE_ATTACH_NO_CONNECTOR, set it in atomic_enable() earlier to
> avoid possible NULL pointer dereference in recovery paths like
> modeset_retry_fn() with the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag set.
>
> Fixes: c932ced6b585 ("drm/tidss: Update encoder/bridge chain connect model")
> Signed-off-by: Jayesh Choudhary <j-choudhary@...com>
> Signed-off-by: Harikrishna Shenoy <h-shenoy@...com>
> ---
> .../drm/bridge/cadence/cdns-mhdp8546-core.c | 29 ++++++++++---------
> .../drm/bridge/cadence/cdns-mhdp8546-core.h | 1 +
> .../drm/bridge/cadence/cdns-mhdp8546-hdcp.c | 26 ++++++++++++-----
> 3 files changed, 34 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> index 38726ae1bf150..f3076e9cdabbe 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> @@ -740,7 +740,7 @@ static void cdns_mhdp_fw_cb(const struct firmware *fw, void *context)
> bridge_attached = mhdp->bridge_attached;
> spin_unlock(&mhdp->start_lock);
> if (bridge_attached) {
> - if (mhdp->connector.dev)
> + if (mhdp->connector_ptr && mhdp->connector_ptr->dev)
> drm_kms_helper_hotplug_event(mhdp->bridge.dev);
> else
> drm_bridge_hpd_notify(&mhdp->bridge, cdns_mhdp_detect(mhdp));
This code looks odd, and there are a few other similar if-elses too.
What does this do?
Correct me if I'm wrong but:
With DRM_BRIDGE_ATTACH_NO_CONNECTOR, connector_ptr is set at
cdns_mhdp_atomic_enable(). If connector_ptr is set, I assume we also
always have dev there.
Without DRM_BRIDGE_ATTACH_NO_CONNECTOR, connector_ptr is set at
cdns_mhdp_connector_init(). If it is set, we have called
drm_connector_init(), and dev is valid.
So... If we have connector_ptr, connector_ptr->dev is always set? So no
need to check for the dev.
But overall I'm confused about the 'else' block. Why do we have these
code blocks that (in the current upstream code) check for
'mhdp->connector.dev' (I think that's checking if the
cdns_mhdp_connector_init() has been called), and then, e.g. here, call
either drm_kms_helper_hotplug_event() or drm_bridge_hpd_notify(). I
don't understand what is the idea here.
Also, in the above case, the code checks for 'bridge_attached'. It is
set in cdns_mhdp_attach(), after calling cdns_mhdp_connector_init().
So... If bridge_attached is true, we always have a connector if
DRM_BRIDGE_ATTACH_NO_CONNECTOR is not set. And if
DRM_BRIDGE_ATTACH_NO_CONNECTOR is set, we never have a connector,
because it's set only in atomic enable.
So the 'if' is effectively checking for DRM_BRIDGE_ATTACH_NO_CONNECTOR
flag? It still doesn't explain why we need to check it here.
Tomi
> @@ -1636,6 +1636,7 @@ static int cdns_mhdp_connector_init(struct cdns_mhdp_device *mhdp)
> return ret;
> }
>
> + mhdp->connector_ptr = conn;
> drm_connector_helper_add(conn, &cdns_mhdp_conn_helper_funcs);
>
> ret = drm_display_info_set_bus_formats(&conn->display_info,
> @@ -1915,17 +1916,25 @@ static void cdns_mhdp_atomic_enable(struct drm_bridge *bridge,
> struct cdns_mhdp_device *mhdp = bridge_to_mhdp(bridge);
> struct cdns_mhdp_bridge_state *mhdp_state;
> struct drm_crtc_state *crtc_state;
> - struct drm_connector *connector;
> struct drm_connector_state *conn_state;
> struct drm_bridge_state *new_state;
> const struct drm_display_mode *mode;
> u32 resp;
> - int ret;
> + int ret = 0;
>
> dev_dbg(mhdp->dev, "bridge enable\n");
>
> mutex_lock(&mhdp->link_mutex);
>
> + mhdp->connector_ptr = drm_atomic_get_new_connector_for_encoder(state,
> + bridge->encoder);
> + if (WARN_ON(!mhdp->connector_ptr))
> + goto out;
> +
> + conn_state = drm_atomic_get_new_connector_state(state, mhdp->connector_ptr);
> + if (WARN_ON(!conn_state))
> + goto out;
> +
> if (mhdp->plugged && !mhdp->link_up) {
> ret = cdns_mhdp_link_up(mhdp);
> if (ret < 0)
> @@ -1945,15 +1954,6 @@ static void cdns_mhdp_atomic_enable(struct drm_bridge *bridge,
> cdns_mhdp_reg_write(mhdp, CDNS_DPTX_CAR,
> resp | CDNS_VIF_CLK_EN | CDNS_VIF_CLK_RSTN);
>
> - connector = drm_atomic_get_new_connector_for_encoder(state,
> - bridge->encoder);
> - if (WARN_ON(!connector))
> - goto out;
> -
> - conn_state = drm_atomic_get_new_connector_state(state, connector);
> - if (WARN_ON(!conn_state))
> - goto out;
> -
> if (mhdp->hdcp_supported &&
> mhdp->hw_state == MHDP_HW_READY &&
> conn_state->content_protection ==
> @@ -2030,6 +2030,7 @@ static void cdns_mhdp_atomic_disable(struct drm_bridge *bridge,
> if (mhdp->info && mhdp->info->ops && mhdp->info->ops->disable)
> mhdp->info->ops->disable(mhdp);
>
> + mhdp->connector_ptr = NULL;
> mutex_unlock(&mhdp->link_mutex);
> }
>
> @@ -2296,7 +2297,7 @@ static void cdns_mhdp_modeset_retry_fn(struct work_struct *work)
>
> mhdp = container_of(work, typeof(*mhdp), modeset_retry_work);
>
> - conn = &mhdp->connector;
> + conn = mhdp->connector_ptr;
>
> /* Grab the locks before changing connector property */
> mutex_lock(&conn->dev->mode_config.mutex);
> @@ -2373,7 +2374,7 @@ static void cdns_mhdp_hpd_work(struct work_struct *work)
> int ret;
>
> ret = cdns_mhdp_update_link_status(mhdp);
> - if (mhdp->connector.dev) {
> + if (mhdp->connector_ptr && mhdp->connector_ptr->dev) {
> if (ret < 0)
> schedule_work(&mhdp->modeset_retry_work);
> else
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
> index bad2fc0c73066..a76775c768956 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
> @@ -376,6 +376,7 @@ struct cdns_mhdp_device {
> struct mutex link_mutex;
>
> struct drm_connector connector;
> + struct drm_connector *connector_ptr;
> struct drm_bridge bridge;
>
> struct cdns_mhdp_link link;
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-hdcp.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-hdcp.c
> index 42248f179b69d..5ac2fad2f0078 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-hdcp.c
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-hdcp.c
> @@ -393,8 +393,10 @@ static int _cdns_mhdp_hdcp_disable(struct cdns_mhdp_device *mhdp)
> {
> int ret;
>
> - dev_dbg(mhdp->dev, "[%s:%d] HDCP is being disabled...\n",
> - mhdp->connector.name, mhdp->connector.base.id);
> + if (mhdp->connector_ptr) {
> + dev_dbg(mhdp->dev, "[%s:%d] HDCP is being disabled...\n",
> + mhdp->connector_ptr->name, mhdp->connector_ptr->base.id);
> + }
>
> ret = cdns_mhdp_hdcp_set_config(mhdp, 0, false);
>
> @@ -443,9 +445,11 @@ static int cdns_mhdp_hdcp_check_link(struct cdns_mhdp_device *mhdp)
> if (!ret && hdcp_port_status & HDCP_PORT_STS_AUTH)
> goto out;
>
> - dev_err(mhdp->dev,
> - "[%s:%d] HDCP link failed, retrying authentication\n",
> - mhdp->connector.name, mhdp->connector.base.id);
> + if (mhdp->connector_ptr) {
> + dev_err(mhdp->dev,
> + "[%s:%d] HDCP link failed, retrying authentication\n",
> + mhdp->connector_ptr->name, mhdp->connector_ptr->base.id);
> + }
>
> ret = _cdns_mhdp_hdcp_disable(mhdp);
> if (ret) {
> @@ -487,13 +491,19 @@ static void cdns_mhdp_hdcp_prop_work(struct work_struct *work)
> struct cdns_mhdp_device *mhdp = container_of(hdcp,
> struct cdns_mhdp_device,
> hdcp);
> - struct drm_device *dev = mhdp->connector.dev;
> + struct drm_device *dev = NULL;
> struct drm_connector_state *state;
>
> + if (mhdp->connector_ptr)
> + dev = mhdp->connector_ptr->dev;
> +
> + if (!dev)
> + return;
> +
> drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> mutex_lock(&mhdp->hdcp.mutex);
> - if (mhdp->hdcp.value != DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
> - state = mhdp->connector.state;
> + if (mhdp->connector_ptr && mhdp->hdcp.value != DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
> + state = mhdp->connector_ptr->state;
> state->content_protection = mhdp->hdcp.value;
> }
> mutex_unlock(&mhdp->hdcp.mutex);
Powered by blists - more mailing lists