On Wed, 2020-01-29 at 15:24 -0800, José Roberto de Souza wrote:

According to DP specification, DP_SINK_EVENT_NOTIFY is also a broadcast message but as this function only handles DP_CONNECTION_STATUS_NOTIFY I will only make the static analyzer that caught this issue happy by not calling drm_dp_get_mst_branch_device_by_guid() with a NULL guid, causing drm_dp_mst_process_up_req() to return in the "if (!mstb)" right bellow.

Fixes: 9408cc94eb04 ("drm/dp_mst: Handle UP requests asynchronously") Cc: Lyude Paul lyude@redhat.com Cc: Sean Paul sean@poorly.run Signed-off-by: José Roberto de Souza jose.souza@intel.com

---

drivers/gpu/drm/drm_dp_mst_topology.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 23cf46bfef74..a811247cecfe 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -3829,7 +3829,8 @@ drm_dp_mst_process_up_req(struct drm_dp_mst_topology_mgr *mgr, else if (msg->req_type == DP_RESOURCE_STATUS_NOTIFY) guid = msg->u.resource_stat.guid;

• mstb = drm_dp_get_mst_branch_device_by_guid(mgr, guid);
  

• if (guid)
  

• 	mstb =
  

drm_dp_get_mst_branch_device_by_guid(mgr, guid); } else { mstb = drm_dp_get_mst_branch_device(mgr, hdr->lct, hdr-

...

rad);

}

Been fixing something similar in dp mst topology a while ago, was also similar NULL pointer dereference. Fix seems to be correct, however I would still have a look at least, how it affects overall logic then. I mean now we don't call drm_dp_get_mst_branch_device_by_guid if guid is NULL - that's ok, however it means that mstb will not get initialized and how this is going to affect the code flow then?

SHould we may be still try to initialize mstb somehow and check guid actually inside of this drm_dp_get_mst_branch_device_by_guid function? Or call drm_dp_get_mst_branch_device?

I'm not stating anything here, just asking question to make the overall picture bit more clear.

Anyways, even wrong logic to me is better than crashing so,

Reviewed-by: Stanislav Lisovskiy stanislav.lisovskiy@intel.com

Reviewed-by: Lyude Paul lyude@redhat.com

I'll go ahead and push this now, thanks!

On Wed, 2020-01-29 at 15:24 -0800, José Roberto de Souza wrote:

According to DP specification, DP_SINK_EVENT_NOTIFY is also a broadcast message but as this function only handles DP_CONNECTION_STATUS_NOTIFY I will only make the static analyzer that caught this issue happy by not calling drm_dp_get_mst_branch_device_by_guid() with a NULL guid, causing drm_dp_mst_process_up_req() to return in the "if (!mstb)" right bellow.

Fixes: 9408cc94eb04 ("drm/dp_mst: Handle UP requests asynchronously") Cc: Lyude Paul lyude@redhat.com Cc: Sean Paul sean@poorly.run Signed-off-by: José Roberto de Souza jose.souza@intel.com

---

drivers/gpu/drm/drm_dp_mst_topology.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 23cf46bfef74..a811247cecfe 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -3829,7 +3829,8 @@ drm_dp_mst_process_up_req(struct drm_dp_mst_topology_mgr *mgr, else if (msg->req_type == DP_RESOURCE_STATUS_NOTIFY) guid = msg->u.resource_stat.guid;

• mstb = drm_dp_get_mst_branch_device_by_guid(mgr, guid);
  

• if (guid)
  

• 	mstb = drm_dp_get_mst_branch_device_by_guid(mgr,
  

guid); } else { mstb = drm_dp_get_mst_branch_device(mgr, hdr->lct, hdr->rad); }