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

[Radosław Biernacki]

śr., 16 sie 2023 o 11:08 Lukasz Majczak <lma@xxxxxxxxxxxx> napisał(a):
>
> czw., 3 sie 2023 o 11:23 Lukasz Majczak <lma@xxxxxxxxxxxx> napisał(a):
> >
> > Check mgr->mst_primary, before passing it to
> > the get_mst_branch_device_by_guid_helper(), otherwise NULL dereference
> > may occur in the call to memcpy() and cause:
> >
> > [12579.365869] BUG: kernel NULL pointer dereference, address: 0000000000000049
> > [12579.365878] #PF: supervisor read access in kernel mode
> > [12579.365880] #PF: error_code(0x0000) - not-present page
> > [12579.365882] PGD 0 P4D 0
> > [12579.365887] Oops: 0000 [#1] PREEMPT SMP NOPTI
> > ...
> > [12579.365895] Workqueue: events_long drm_dp_mst_up_req_work
> > [12579.365899] RIP: 0010:memcmp+0xb/0x29
> > [12579.365921] Call Trace:
> > [12579.365927] get_mst_branch_device_by_guid_helper+0x22/0x64
> > [12579.365930] drm_dp_mst_up_req_work+0x137/0x416
> > [12579.365933] process_one_work+0x1d0/0x419
> > [12579.365935] worker_thread+0x11a/0x289
> > [12579.365938] kthread+0x13e/0x14f
> > [12579.365941] ? process_one_work+0x419/0x419
> > [12579.365943] ? kthread_blkcg+0x31/0x31
> > [12579.365946] ret_from_fork+0x1f/0x30
> >
> > Similar check is done in e.g: drm_dp_mst_topology_get_mstb_validated().
> >
> > Fixes: 5e93b8208d3c ("drm/dp/mst: move GUID storage from mgr, port to only mst branch")
> > Cc: <stable@xxxxxxxxxxxxxxx> # 4.14+
> > Signed-off-by: Lukasz Majczak <lma@xxxxxxxxxxxx>
> > ---
> >  drivers/gpu/drm/display/drm_dp_mst_topology.c | 16 ++++++++--------
> >  1 file changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > index ed96cfcfa304..703cd97b1d11 100644
> > --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > @@ -2595,19 +2595,19 @@ static struct drm_dp_mst_branch *
> >  drm_dp_get_mst_branch_device_by_guid(struct drm_dp_mst_topology_mgr *mgr,
> >                                      const uint8_t *guid)
> >  {
> > -       struct drm_dp_mst_branch *mstb;
> > +       struct drm_dp_mst_branch *mstb = NULL;
> >         int ret;
> >
> >         /* find the port by iterating down */
> >         mutex_lock(&mgr->lock);
> > -
> > -       mstb = get_mst_branch_device_by_guid_helper(mgr->mst_primary, guid);
> > -       if (mstb) {
> > -               ret = drm_dp_mst_topology_try_get_mstb(mstb);
> > -               if (!ret)
> > -                       mstb = NULL;
> > +       if (mgr->mst_primary) {

One suggestion which just came to my mind:
get_mst_branch_device_by_guid_helper() is a recursive function.
This condition might be moved to the inside of that function as the first line.
This way we would have a single condition, meaning remove a similar
one for step over of NULL elements inside a recursive call so NULL
would be an acceptable value as param and therefore no need to check
for this here.

> > +               mstb = get_mst_branch_device_by_guid_helper(mgr->mst_primary, guid);
> > +               if (mstb) {
> > +                       ret = drm_dp_mst_topology_try_get_mstb(mstb);
> > +                       if (!ret)
> > +                               mstb = NULL;
> > +               }
> >         }
> > -
> >         mutex_unlock(&mgr->lock);
> >         return mstb;
> >  }
> > --
> > 2.41.0.640.ga95def55d0-goog
> >
> Hi,
>
> Is there anything more I should do regarding these changes?
>
> Best regards,
> Lukasz