The address watch clear code receives watch_id as an unsigned value
(u32), but some helper functions were using a signed int and checked
bits by shifting with watch_id.
If a very large watch_id is passed from userspace, it can be converted
to a negative value. This can cause invalid shifts and may access
memory outside the watch_points array.
drm/amdkfd: Fix watch_id bounds checking in debug address watch v2
Fix this by checking that watch_id is within MAX_WATCH_ADDRESSES before
using it. Also use BIT(watch_id) to test and clear bits safely.
This keeps the behavior unchanged for valid watch IDs and avoids
undefined behavior for invalid ones.
Fixes the below:
drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_debug.c:448
kfd_dbg_trap_clear_dev_address_watch() error: buffer overflow
'pdd->watch_points' 4 <= u32max user_rl='0-3,2147483648-u32max' uncapped
drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_debug.c
433 int kfd_dbg_trap_clear_dev_address_watch(struct kfd_process_device *pdd,
434 uint32_t watch_id)
435 {
436 int r;
437
438 if (!kfd_dbg_owns_dev_watch_id(pdd, watch_id))
kfd_dbg_owns_dev_watch_id() doesn't check for negative values so if
watch_id is larger than INT_MAX it leads to a buffer overflow.
(Negative shifts are undefined).
439 return -EINVAL;
440
441 if (!pdd->dev->kfd->shared_resources.enable_mes) {
442 r = debug_lock_and_unmap(pdd->dev->dqm);
443 if (r)
444 return r;
445 }
446
447 amdgpu_gfx_off_ctrl(pdd->dev->adev, false);
--> 448 pdd->watch_points[watch_id] =
pdd->dev->kfd2kgd->clear_address_watch(
449 pdd->dev->adev,
450 watch_id);
v2: (as per, Jonathan Kim)
- Add early watch_id >= MAX_WATCH_ADDRESSES validation in the set path to
match the clear path.
- Drop the redundant bounds check in kfd_dbg_owns_dev_watch_id().
Fixes: e0f85f4690d0 ("drm/amdkfd: add debug set and clear address watch points
operation")
Reported-by: Dan Carpenter <[email protected]>
Cc: Jonathan Kim <[email protected]>
Cc: Felix Kuehling <[email protected]>
Cc: Alex Deucher <[email protected]>
Cc: Christian König <[email protected]>
Signed-off-by: Srinivasan Shanmugam <[email protected]>
Reviewed-by: Jonathan Kim <[email protected]>
---
drivers/gpu/drm/amd/amdkfd/kfd_debug.c | 20 ++++++++++++--------
1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_debug.c
b/drivers/gpu/drm/amd/amdkfd/kfd_debug.c
index 1dae317858e9..bedb95ce9659 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_debug.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_debug.c
@@ -404,27 +404,25 @@ static int kfd_dbg_get_dev_watch_id(struct
kfd_process_device *pdd, int *watch_i
return -ENOMEM;
}
-static void kfd_dbg_clear_dev_watch_id(struct kfd_process_device *pdd, int
watch_id)
+static void kfd_dbg_clear_dev_watch_id(struct kfd_process_device *pdd, u32
watch_id)
{
spin_lock(&pdd->dev->watch_points_lock);
/* process owns device watch point so safe to clear */
- if ((pdd->alloc_watch_ids >> watch_id) & 0x1) {
- pdd->alloc_watch_ids &= ~(0x1 << watch_id);
- pdd->dev->alloc_watch_ids &= ~(0x1 << watch_id);
+ if (pdd->alloc_watch_ids & BIT(watch_id)) {
+ pdd->alloc_watch_ids &= ~BIT(watch_id);
+ pdd->dev->alloc_watch_ids &= ~BIT(watch_id);
}
spin_unlock(&pdd->dev->watch_points_lock);
}
-static bool kfd_dbg_owns_dev_watch_id(struct kfd_process_device *pdd, int
watch_id)
+static bool kfd_dbg_owns_dev_watch_id(struct kfd_process_device *pdd, u32
watch_id)
{
bool owns_watch_id = false;
spin_lock(&pdd->dev->watch_points_lock);
- owns_watch_id = watch_id < MAX_WATCH_ADDRESSES &&
- ((pdd->alloc_watch_ids >> watch_id) & 0x1);
-
+ owns_watch_id = pdd->alloc_watch_ids & BIT(watch_id);
spin_unlock(&pdd->dev->watch_points_lock);
return owns_watch_id;
@@ -435,6 +433,9 @@ int kfd_dbg_trap_clear_dev_address_watch(struct
kfd_process_device *pdd,
{
int r;
+ if (watch_id >= MAX_WATCH_ADDRESSES)
+ return -EINVAL;
+
if (!kfd_dbg_owns_dev_watch_id(pdd, watch_id))
return -EINVAL;
@@ -472,6 +473,9 @@ int kfd_dbg_trap_set_dev_address_watch(struct
kfd_process_device *pdd,
if (r)
return r;
+ if (*watch_id >= MAX_WATCH_ADDRESSES)
+ return -EINVAL;
+
if (!pdd->dev->kfd->shared_resources.enable_mes) {
r = debug_lock_and_unmap(pdd->dev->dqm);
if (r) {
--
2.34.1
