[Intel-gfx] [PATCH] drm/mst: Fix possible NULL pointer dereference in drm_dp_mst_process_up_req()

[Lisovskiy, Stanislav]

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 at redhat.com>
> Cc: Sean Paul <sean at poorly.run>
> Signed-off-by: José Roberto de Souza <jose.souza at 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 at intel.com>