Re: [PATCH v3] drm/dp_mst: Fix NULL deref in get_mst_branch_device_by_guid_helper()
On Fri, Sep 22, 2023 at 8:34 AM Lukasz Majczak wrote:
>
> As drm_dp_get_mst_branch_device_by_guid() is called from
> drm_dp_get_mst_branch_device_by_guid(), mstb parameter has to be checked,
> otherwise NULL dereference may occur in the call to
> the memcpy() and cause following:
>
> [12579.365869] BUG: kernel NULL pointer dereference, address: 0049
> [12579.365878] #PF: supervisor read access in kernel mode
> [12579.365880] #PF: error_code(0x) - not-present page
> [12579.365882] PGD 0 P4D 0
> [12579.365887] Oops: [#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
>
> As get_mst_branch_device_by_guid_helper() is recursive, moving condition
> to the first line allow to remove a similar one for step over of NULL elements
> inside a loop.
>
> Fixes: 5e93b8208d3c ("drm/dp/mst: move GUID storage from mgr, port to only
> mst branch")
> Cc: # 4.14+
> Signed-off-by: Lukasz Majczak
> ---
>
> v2->v3:
> * Fixed patch description.
>
> drivers/gpu/drm/display/drm_dp_mst_topology.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 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..8c929ef72c72 100644
> --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> @@ -2574,14 +2574,14 @@ static struct drm_dp_mst_branch
> *get_mst_branch_device_by_guid_helper(
> struct drm_dp_mst_branch *found_mstb;
> struct drm_dp_mst_port *port;
>
> + if (!mstb)
> + return NULL;
> +
> if (memcmp(mstb->guid, guid, 16) == 0)
> return mstb;
>
>
> list_for_each_entry(port, >ports, next) {
> - if (!port->mstb)
> - continue;
> -
> found_mstb = get_mst_branch_device_by_guid_helper(port->mstb,
> guid);
>
> if (found_mstb)
> --
> 2.42.0.515.g380fc7ccd1-goog
>
Reviewed-by: Radoslaw Biernacki
Re: [PATCH] drm/dp_mst: Fix NULL deref in get_mst_branch_device_by_guid_helper()
śr., 16 sie 2023 o 11:08 Lukasz Majczak napisał(a):
>
> czw., 3 sie 2023 o 11:23 Lukasz Majczak 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:
> > 0049
> > [12579.365878] #PF: supervisor read access in kernel mode
> > [12579.365880] #PF: error_code(0x) - not-present page
> > [12579.365882] PGD 0 P4D 0
> > [12579.365887] Oops: [#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: # 4.14+
> > Signed-off-by: Lukasz Majczak
> > ---
> > 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(>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(>lock);
> > return mstb;
> > }
> > --
> > 2.41.0.640.ga95def55d0-goog
> >
> Hi,
>
> Is there anything more I should do regarding these changes?
>
> Best regards,
> Lukasz
Re: [PATCH v1] drm/i915/bdb: Fix version check
- dropping stable
...
> > diff --git a/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> > b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> > index 330077c2e588..fff456bf8783 100644
> > --- a/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> > +++ b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> > @@ -814,6 +814,11 @@ struct lfp_brightness_level {
> > u16 reserved;
> > } __packed;
> >
> > +/*
> > + * Changing struct bdb_lfp_backlight_data might affect its
> > + * size comparation to the value hold in BDB.
> > + * (e.g. in parse_lfp_backlight())
> > + */
>
> This is true for all the blocks so I don't think we need this comment.
Lack of such comment was probable cause of this overlook.
As this is an example of the consequence (bricking platforms dependent
on mentioned conditions) IMO we need some comment here, or this will
probably happen again.
>
> > struct bdb_lfp_backlight_data {
> > u8 entry_size;
> > struct lfp_backlight_data_entry data[16];
>
