Re: [PATCH] drm/dp_mst: Fix NULL deref in drm_dp_get_one_sb_msg()

[Sean Paul]

On Mon, Apr 6, 2020 at 3:34 PM Lyude Paul <lyude@xxxxxxxxxx> wrote:
>
> While we don't need this function to store an mstb anywhere for UP
> requests since we process them asynchronously, we do need to make sure
> that we don't try to write to **mstb for UP requests otherwise we'll
> cause a NULL pointer deref:
>
>     RIP: 0010:drm_dp_get_one_sb_msg+0x4b/0x460 [drm_kms_helper]
>     Call Trace:
>      ? vprintk_emit+0x16a/0x230
>      ? drm_dp_mst_hpd_irq+0x133/0x1010 [drm_kms_helper]
>      drm_dp_mst_hpd_irq+0x133/0x1010 [drm_kms_helper]
>      ? __drm_dbg+0x87/0x90 [drm]
>      ? intel_dp_hpd_pulse+0x24b/0x400 [i915]
>      intel_dp_hpd_pulse+0x24b/0x400 [i915]
>      i915_digport_work_func+0xd6/0x160 [i915]
>      process_one_work+0x1a9/0x370
>      worker_thread+0x4d/0x3a0
>      kthread+0xf9/0x130
>      ? process_one_work+0x370/0x370
>      ? kthread_park+0x90/0x90
>      ret_from_fork+0x35/0x40
>
> So, fix this.

Ugggh, what a fail! I found this in Feb and posted the patch in
20200218171522.GF253734@art_vandelay. I had to migrate my workstation
due to WFH order and didn't apply the patch before pushing. Messy
messy messy.

Thanks for fixing!

Reviewed-by: Sean Paul <sean@xxxxxxxxxx>

>
> Signed-off-by: Lyude Paul <lyude@xxxxxxxxxx>
> Fixes: fbc821c4a506 ("drm/mst: Support simultaneous down replies")
> Cc: Wayne Lin <Wayne.Lin@xxxxxxx>
> Cc: Lyude Paul <lyude@xxxxxxxxxx>
> Cc: Wayne Lin <waynelin@xxxxxxx>
> Cc: Sean Paul <seanpaul@xxxxxxxxxxxx>
> ---
>  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 1ff49547b2e8..8751278b3941 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -3703,7 +3703,8 @@ static bool drm_dp_get_one_sb_msg(struct drm_dp_mst_topology_mgr *mgr, bool up,
>         int basereg = up ? DP_SIDEBAND_MSG_UP_REQ_BASE :
>                            DP_SIDEBAND_MSG_DOWN_REP_BASE;
>
> -       *mstb = NULL;
> +       if (!up)
> +               *mstb = NULL;
>         *seqno = -1;
>
>         len = min(mgr->max_dpcd_transaction_bytes, 16);
> --
> 2.25.1
>