[PATCH] drm/amdkfd: Fix UBSAN shift-out-of-bounds warning

[Lazar, Lijo]

[AMD Public Use]

Hi Anson,

Please ignore the earlier comment. 

Thanks,
Lijo

-----Original Message-----
From: Lazar, Lijo 
Sent: Friday, March 5, 2021 11:05 AM
To: Anson Jacob <Anson.Jacob at amd.com>; amd-gfx at lists.freedesktop.org
Cc: Jacob, Anson <Anson.Jacob at amd.com>; Deucher, Alexander <Alexander.Deucher at amd.com>; Kuehling, Felix <Felix.Kuehling at amd.com>
Subject: RE: [PATCH] drm/amdkfd: Fix UBSAN shift-out-of-bounds warning

[AMD Public Use]



-----Original Message-----
From: amd-gfx <amd-gfx-bounces at lists.freedesktop.org> On Behalf Of Anson Jacob
Sent: Friday, March 5, 2021 1:39 AM
To: amd-gfx at lists.freedesktop.org
Cc: Jacob, Anson <Anson.Jacob at amd.com>; Deucher, Alexander <Alexander.Deucher at amd.com>; Kuehling, Felix <Felix.Kuehling at amd.com>
Subject: [PATCH] drm/amdkfd: Fix UBSAN shift-out-of-bounds warning

If get_num_sdma_queues or get_num_xgmi_sdma_queues is 0, we end up doing a shift operation where the number of bits shifted equals number of bits in the operand. This behaviour is undefined.

Set num_sdma_queues or num_xgmi_sdma_queues to ULLONG_MAX, if the count is >= number of bits in the operand.

Bug: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.freedesktop.org%2Fdrm%2Famd%2F-%2Fissues%2F1472&data=04%7C01%7Clijo.lazar%40amd.com%7Cc731ee10b10b4728138808d8df496648%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637504853648181515%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=iNLxyPbJfbuUeKFA6ygwcBGDSRJcfgOGjMFNHwGzun0%3D&reserved=0
Reported-by: Lyude Paul <lyude at redhat.com>
Signed-off-by: Anson Jacob <Anson.Jacob at amd.com>
Reviewed-by: Alex Deucher <alexander.deucher at amd.com>
Reviewed-by: Felix Kuehling <Felix.Kuehling at amd.com>
---
 .../drm/amd/amdkfd/kfd_device_queue_manager.c   | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
index c37e9c4b1fb4..e7a3c496237f 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
@@ -1128,6 +1128,9 @@ static int set_sched_resources(struct device_queue_manager *dqm)
 
 static int initialize_cpsch(struct device_queue_manager *dqm)  {
+	uint64_t num_sdma_queues;
+	uint64_t num_xgmi_sdma_queues;
+
 	pr_debug("num of pipes: %d\n", get_pipes_per_mec(dqm));
 
 	mutex_init(&dqm->lock_hidden);
@@ -1136,8 +1139,18 @@ static int initialize_cpsch(struct device_queue_manager *dqm)
 	dqm->active_cp_queue_count = 0;
 	dqm->gws_queue_count = 0;
 	dqm->active_runlist = false;
-	dqm->sdma_bitmap = ~0ULL >> (64 - get_num_sdma_queues(dqm));
-	dqm->xgmi_sdma_bitmap = ~0ULL >> (64 - get_num_xgmi_sdma_queues(dqm));
+
+	num_sdma_queues = get_num_sdma_queues(dqm);
+	if (num_sdma_queues >= BITS_PER_TYPE(dqm->sdma_bitmap))

< > No assumption about bitmap size here

+		dqm->sdma_bitmap = ULLONG_MAX;

<> This assumes/fixes the max size. In that case why not make the earlier check also consistent?

+	else
+		dqm->sdma_bitmap = (BIT_ULL(num_sdma_queues) - 1);
+
+	num_xgmi_sdma_queues = get_num_xgmi_sdma_queues(dqm);
+	if (num_xgmi_sdma_queues >= BITS_PER_TYPE(dqm->xgmi_sdma_bitmap))
+		dqm->xgmi_sdma_bitmap = ULLONG_MAX;
+	else
+		dqm->xgmi_sdma_bitmap = (BIT_ULL(num_xgmi_sdma_queues) - 1);
 
 	INIT_WORK(&dqm->hw_exception_work, kfd_process_hw_exception);
 
--
2.25.1

_______________________________________________
amd-gfx mailing list
amd-gfx at lists.freedesktop.org
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=04%7C01%7Clijo.lazar%40amd.com%7Cc731ee10b10b4728138808d8df496648%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637504853648191471%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=EpuPBBf03EMa0S7rOqI8JieOmcT3GvsnsQMaYujGgeY%3D&reserved=0