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@vger.kernel.org # 4.14+ Signed-off-by: Lukasz Majczak lma@semihalf.com --- 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) { + 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; }
czw., 3 sie 2023 o 11:23 Lukasz Majczak lma@semihalf.com 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@vger.kernel.org # 4.14+ Signed-off-by: Lukasz Majczak lma@semihalf.com
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) {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
śr., 16 sie 2023 o 11:08 Lukasz Majczak lma@semihalf.com napisał(a):
czw., 3 sie 2023 o 11:23 Lukasz Majczak lma@semihalf.com 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@vger.kernel.org # 4.14+ Signed-off-by: Lukasz Majczak lma@semihalf.com
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
linux-stable-mirror@lists.linaro.org
-
Lukasz Majczak -
Radosław Biernacki