Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <b441d8e53c9b5e95da447b3681bb9143448388fc.camel@redhat.com>
Subject: Re: [PATCH] drm/display/dp_mst: Fix missing check for return value
 of drm_atomic_get_mst_payload_state()
From:   Lyude Paul <lyude@redhat.com>
To:     Chenyuan Mi <cymi20@fudan.edu.cn>, airlied@gmail.com
Cc:     daniel@ffwll.ch, jani.nikula@intel.com, Wayne.Lin@amd.com,
        imre.deak@intel.com, alexander.deucher@amd.com,
        dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org
Date:   Tue, 20 Jun 2023 15:48:25 -0400
In-Reply-To: <20230614160911.121716-1-cymi20@fudan.edu.cn>
References: <20230614160911.121716-1-cymi20@fudan.edu.cn>
Organization: Red Hat Inc.
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.48.3 (3.48.3-1.fc38) 
MIME-Version: 1.0
Precedence: bulk

Hm, I suppose it's not -impossible- for this to return NULL, but if it does
return NULL that absolutely means something broke well before this point in
the code. More comments below:

On Wed, 2023-06-14 at 09:09 -0700, Chenyuan Mi wrote:
> The drm_atomic_get_mst_payload_state() function may
> return NULL, which may cause null pointer deference,
> and most other callsites of drm_atomic_get_mst_payload_state()
> do Null check. Add Null check for return value of
> drm_atomic_get_mst_payload_state().
>=20
> Found by our static analysis tool.
>=20
> Signed-off-by: Chenyuan Mi <cymi20@fudan.edu.cn>
> ---
>  drivers/gpu/drm/display/drm_dp_mst_topology.c | 2 ++
>  1 file changed, 2 insertions(+)
>=20
> diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/=
drm/display/drm_dp_mst_topology.c
> index 38dab76ae69e..27f4bcf409ea 100644
> --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> @@ -4434,6 +4434,8 @@ void drm_dp_mst_atomic_wait_for_dependencies(struct=
 drm_atomic_state *state)
> =20
>  			new_payload =3D drm_atomic_get_mst_payload_state(new_mst_state,
>  								       old_payload->port);
> +			if (!new_payload)
> +				continue;

Could you change this check to:

	if (drm_WARN_ON(state->dev, !new_payload))

>  			new_payload->vc_start_slot =3D old_payload->vc_start_slot;
>  		}
>  	}

--=20
Cheers,
 Lyude Paul (she/her)
 Software Engineer at Red Hat