| From cf5c14e45eff2dd348758645311e929eb0af05ab Mon Sep 17 00:00:00 2001 |
| From: Sasha Levin <sashal@kernel.org> |
| Date: Fri, 6 Feb 2026 21:18:11 +0530 |
| Subject: drm/amdkfd: Fix watch_id bounds checking in debug address watch v2 |
| MIME-Version: 1.0 |
| Content-Type: text/plain; charset=UTF-8 |
| Content-Transfer-Encoding: 8bit |
| |
| From: Srinivasan Shanmugam <srinivasan.shanmugam@amd.com> |
| |
| [ Upstream commit 5a19302cab5cec7ae7f1a60c619951e6c17d8742 ] |
| |
| 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 <dan.carpenter@linaro.org> |
| Cc: Jonathan Kim <jonathan.kim@amd.com> |
| Cc: Felix Kuehling <felix.kuehling@amd.com> |
| Cc: Alex Deucher <alexander.deucher@amd.com> |
| Cc: Christian König <christian.koenig@amd.com> |
| Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam@amd.com> |
| Reviewed-by: Jonathan Kim <jonathan.kim@amd.com> |
| Signed-off-by: Alex Deucher <alexander.deucher@amd.com> |
| Signed-off-by: Sasha Levin <sashal@kernel.org> |
| --- |
| 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 eb973f16848d3..267650dcced9d 100644 |
| --- a/drivers/gpu/drm/amd/amdkfd/kfd_debug.c |
| +++ b/drivers/gpu/drm/amd/amdkfd/kfd_debug.c |
| @@ -400,27 +400,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; |
| @@ -431,6 +429,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; |
| |
| @@ -468,6 +469,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.51.0 |
| |