| Message ID | 20200129232448.84704-1-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/mst: Fix possible NULL pointer dereference in drm_dp_mst_process_up_req() | expand |
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>
On Thu, 2020-01-30 at 10:49 +0000, Lisovskiy, Stanislav wrote: > 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. Well it do not matters if it set mstb if on the next block it will only handle messages of DP_CONNECTION_STATUS_NOTIFY type but for sure we should handle this two other message types. > > Anyways, even wrong logic to me is better than crashing so, > > Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> Thanks > > >
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); > }
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); }
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(-)