[PATCH] drm: bridge: cdns-mhdp8546: Fix missing mutex unlock on error path
In cdns_mhdp_atomic_enable(), there is an error return on failure of
drm_mode_duplicate() which leads to the mutex remaining locked.
Add a mutex unlock call.
Found by Linux Verification Center (linuxtesting.org) with SVACE.
Fixes: 935a92a1c400 ("drm: bridge: cdns-mhdp8546: Fix possible null pointer
dereference")
Signed-off-by: Aleksandr Mishin
---
This patch is against drm-misc-next branch of drm-misc repo.
drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
index 8a91ef0ae065..65a4bd09d9c6 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
@@ -2059,8 +2059,10 @@ static void cdns_mhdp_atomic_enable(struct drm_bridge
*bridge,
mhdp_state = to_cdns_mhdp_bridge_state(new_state);
mhdp_state->current_mode = drm_mode_duplicate(bridge->dev, mode);
- if (!mhdp_state->current_mode)
- return;
+ if (!mhdp_state->current_mode) {
+ ret = -EINVAL;
+ goto out;
+ }
drm_mode_set_name(mhdp_state->current_mode);
--
2.30.2
[PATCH v2] drm: bridge: cdns-mhdp8546: Fix possible null pointer dereference
In cdns_mhdp_atomic_enable(), the return value of drm_mode_duplicate() is
assigned to mhdp_state->current_mode, and there is a dereference of it in
drm_mode_set_name(), which will lead to a NULL pointer dereference on
failure of drm_mode_duplicate().
Fix this bug by adding a check of mhdp_state->current_mode.
Fixes: fb43aa0acdfd ("drm: bridge: Add support for Cadence MHDP8546 DPI/DP
bridge")
Signed-off-by: Aleksandr Mishin
---
v2: Fix a mistake where the mutex remained locked
drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
index e226acc5c15e..5b831d6d7764 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
@@ -2059,6 +2059,11 @@ static void cdns_mhdp_atomic_enable(struct drm_bridge
*bridge,
mhdp_state = to_cdns_mhdp_bridge_state(new_state);
mhdp_state->current_mode = drm_mode_duplicate(bridge->dev, mode);
+ if (!mhdp_state->current_mode) {
+ ret = -EINVAL;
+ goto out;
+ }
+
drm_mode_set_name(mhdp_state->current_mode);
dev_dbg(mhdp->dev, "%s: Enabling mode %s\n", __func__, mode->name);
--
2.30.2
Re: [bug report] drm: bridge: cdns-mhdp8546: Fix possible null pointer dereference
On 11.04.2024 16:39, Dan Carpenter wrote:
Hello Aleksandr Mishin,
Commit 935a92a1c400 ("drm: bridge: cdns-mhdp8546: Fix possible null
pointer dereference") from Apr 8, 2024 (linux-next), leads to the
following Smatch static checker warning:
drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c:2074
cdns_mhdp_atomic_enable()
warn: inconsistent returns '>link_mutex'.
drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
1986 static void cdns_mhdp_atomic_enable(struct drm_bridge *bridge,
1987 struct drm_bridge_state
*bridge_state)
1988 {
1989 struct cdns_mhdp_device *mhdp = bridge_to_mhdp(bridge);
1990 struct drm_atomic_state *state = bridge_state->base.state;
1991 struct cdns_mhdp_bridge_state *mhdp_state;
1992 struct drm_crtc_state *crtc_state;
1993 struct drm_connector *connector;
1994 struct drm_connector_state *conn_state;
1995 struct drm_bridge_state *new_state;
1996 const struct drm_display_mode *mode;
1997 u32 resp;
1998 int ret;
1999
2000 dev_dbg(mhdp->dev, "bridge enable\n");
2001
2002 mutex_lock(>link_mutex);
^^
Holding a lock
2003
2004 if (mhdp->plugged && !mhdp->link_up) {
2005 ret = cdns_mhdp_link_up(mhdp);
2006 if (ret < 0)
2007 goto out;
2008 }
2009
2010 if (mhdp->info && mhdp->info->ops && mhdp->info->ops->enable)
2011 mhdp->info->ops->enable(mhdp);
2012
2013 /* Enable VIF clock for stream 0 */
2014 ret = cdns_mhdp_reg_read(mhdp, CDNS_DPTX_CAR, );
2015 if (ret < 0) {
2016 dev_err(mhdp->dev, "Failed to read CDNS_DPTX_CAR
%d\n", ret);
2017 goto out;
2018 }
2019
2020 cdns_mhdp_reg_write(mhdp, CDNS_DPTX_CAR,
2021 resp | CDNS_VIF_CLK_EN |
CDNS_VIF_CLK_RSTN);
2022
2023 connector = drm_atomic_get_new_connector_for_encoder(state,
2024
bridge->encoder);
2025 if (WARN_ON(!connector))
2026 goto out;
2027
2028 conn_state = drm_atomic_get_new_connector_state(state,
connector);
2029 if (WARN_ON(!conn_state))
2030 goto out;
2031
2032 if (mhdp->hdcp_supported &&
2033 mhdp->hw_state == MHDP_HW_READY &&
2034 conn_state->content_protection ==
2035 DRM_MODE_CONTENT_PROTECTION_DESIRED) {
2036 mutex_unlock(>link_mutex);
2037 cdns_mhdp_hdcp_enable(mhdp,
conn_state->hdcp_content_type);
2038 mutex_lock(>link_mutex);
2039 }
2040
2041 crtc_state = drm_atomic_get_new_crtc_state(state,
conn_state->crtc);
2042 if (WARN_ON(!crtc_state))
2043 goto out;
2044
2045 mode = _state->adjusted_mode;
2046
2047 new_state = drm_atomic_get_new_bridge_state(state, bridge);
2048 if (WARN_ON(!new_state))
2049 goto out;
2050
2051 if (!cdns_mhdp_bandwidth_ok(mhdp, mode, mhdp->link.num_lanes,
2052 mhdp->link.rate)) {
2053 ret = -EINVAL;
2054 goto out;
2055 }
2056
2057 cdns_mhdp_sst_enable(mhdp, mode);
2058
2059 mhdp_state = to_cdns_mhdp_bridge_state(new_state);
2060
2061 mhdp_state->current_mode = drm_mode_duplicate(bridge->dev,
mode);
2062 if (!mhdp_state->current_mode)
2063 return;
^^^
Needs to unlock before returning.
Yes. Sorry. It's my mistake.
I'll replace 'return' with 'ret=-EINVAL; goto out;' and offer v2 patch.
2064
2065 drm_mode_set_name(mhdp_state->current_mode);
2066
2067 dev_dbg(mhdp->dev, "%s: Enabling mode %s\n", __func__,
mode->name);
2068
2069 mhdp->bridge_enabled = true;
2070
2071 out:
2072 mutex_unlock(>link_mutex);
2073 if (ret < 0)
--> 2074 schedule_work(>modeset_retry_work);
2075 }
regards,
dan carpenter
--
Kind regards
Aleksandr
Re: [PATCH] drm/msm/dpu: Add callback function pointer check before its call
On 08.04.2024 12:03, Dmitry Baryshkov wrote:
On Mon, 8 Apr 2024 at 11:57, Aleksandr Mishin wrote:
In dpu_core_irq_callback_handler() callback function pointer is compared to
NULL,
but then callback function is unconditionally called by this pointer.
Fix this bug by adding conditional return.
Found by Linux Verification Center (linuxtesting.org) with SVACE.
This should be converted to a proper Reported-by: trailer.
It is an established practice for our project, you can find 700+ applied
patches with similar line:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/?qt=grep=linuxtesting.org
Fixes: c929ac60b3ed ("drm/msm/dpu: allow just single IRQ callback")
Signed-off-by: Aleksandr Mishin
---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
index 946dd0135dff..03a16fbd4c99 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
@@ -223,9 +223,11 @@ static void dpu_core_irq_callback_handler(struct dpu_kms
*dpu_kms, unsigned int
VERB("IRQ=[%d, %d]\n", DPU_IRQ_REG(irq_idx), DPU_IRQ_BIT(irq_idx));
- if (!irq_entry->cb)
+ if (!irq_entry->cb) {
DRM_ERROR("no registered cb, IRQ=[%d, %d]\n",
DPU_IRQ_REG(irq_idx), DPU_IRQ_BIT(irq_idx));
+ return;
+ }
atomic_inc(_entry->count);
--
2.30.2
--
Kind regards
Aleksandr
Re: [PATCH] drm/msm/dpu: Add callback function pointer check before its call
On 08.04.2024 19:51, Abhinav Kumar wrote:
On 4/8/2024 1:55 AM, Aleksandr Mishin wrote:
In dpu_core_irq_callback_handler() callback function pointer is
compared to NULL,
but then callback function is unconditionally called by this pointer.
Fix this bug by adding conditional return.
Found by Linux Verification Center (linuxtesting.org) with SVACE.
Yes , as dmitry wrote, this should be Reported-by.
It is an established practice for our project, you can find 700+ applied
patches with similar line:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/?qt=grep=linuxtesting.org
But rest LGTM.
Fixes: c929ac60b3ed ("drm/msm/dpu: allow just single IRQ callback")
Signed-off-by: Aleksandr Mishin
---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
index 946dd0135dff..03a16fbd4c99 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
@@ -223,9 +223,11 @@ static void dpu_core_irq_callback_handler(struct
dpu_kms *dpu_kms, unsigned int
VERB("IRQ=[%d, %d]\n", DPU_IRQ_REG(irq_idx), DPU_IRQ_BIT(irq_idx));
- if (!irq_entry->cb)
+ if (!irq_entry->cb) {
DRM_ERROR("no registered cb, IRQ=[%d, %d]\n",
DPU_IRQ_REG(irq_idx), DPU_IRQ_BIT(irq_idx));
+ return;
+ }
atomic_inc(_entry->count);
--
Kind regards
Aleksandr
[PATCH] drm: vc4: Fix possible null pointer dereference
In vc4_hdmi_audio_init() of_get_address() may return
NULL which is later dereferenced. Fix this bug by adding NULL check.
Found by Linux Verification Center (linuxtesting.org) with SVACE.
Fixes: bb7d78568814 ("drm/vc4: Add HDMI audio support")
Signed-off-by: Aleksandr Mishin
---
drivers/gpu/drm/vc4/vc4_hdmi.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index d8751ea20303..5f8d51b29370 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -2740,6 +2740,8 @@ static int vc4_hdmi_audio_init(struct vc4_hdmi *vc4_hdmi)
index = 1;
addr = of_get_address(dev->of_node, index, NULL, NULL);
+ if (!addr)
+ return -EINVAL;
vc4_hdmi->audio.dma_data.addr = be32_to_cpup(addr) + mai_data->offset;
vc4_hdmi->audio.dma_data.addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
--
2.30.2
[PATCH] drm/msm/dpu: Add callback function pointer check before its call
In dpu_core_irq_callback_handler() callback function pointer is compared to
NULL,
but then callback function is unconditionally called by this pointer.
Fix this bug by adding conditional return.
Found by Linux Verification Center (linuxtesting.org) with SVACE.
Fixes: c929ac60b3ed ("drm/msm/dpu: allow just single IRQ callback")
Signed-off-by: Aleksandr Mishin
---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
index 946dd0135dff..03a16fbd4c99 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
@@ -223,9 +223,11 @@ static void dpu_core_irq_callback_handler(struct dpu_kms
*dpu_kms, unsigned int
VERB("IRQ=[%d, %d]\n", DPU_IRQ_REG(irq_idx), DPU_IRQ_BIT(irq_idx));
- if (!irq_entry->cb)
+ if (!irq_entry->cb) {
DRM_ERROR("no registered cb, IRQ=[%d, %d]\n",
DPU_IRQ_REG(irq_idx), DPU_IRQ_BIT(irq_idx));
+ return;
+ }
atomic_inc(_entry->count);
--
2.30.2
[PATCH] drm: bridge: cdns-mhdp8546: Fix possible null pointer dereference
In cdns_mhdp_atomic_enable(), the return value of drm_mode_duplicate() is
assigned to mhdp_state->current_mode, and there is a dereference of it in
drm_mode_set_name(), which will lead to a NULL pointer dereference on
failure of drm_mode_duplicate().
Fix this bug add a check of mhdp_state->current_mode.
Fixes: fb43aa0acdfd ("drm: bridge: Add support for Cadence MHDP8546 DPI/DP
bridge")
Signed-off-by: Aleksandr Mishin
---
drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
index e226acc5c15e..8a91ef0ae065 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
@@ -2059,6 +2059,9 @@ static void cdns_mhdp_atomic_enable(struct drm_bridge
*bridge,
mhdp_state = to_cdns_mhdp_bridge_state(new_state);
mhdp_state->current_mode = drm_mode_duplicate(bridge->dev, mode);
+ if (!mhdp_state->current_mode)
+ return;
+
drm_mode_set_name(mhdp_state->current_mode);
dev_dbg(mhdp->dev, "%s: Enabling mode %s\n", __func__, mode->name);
--
2.30.2
