| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1f4134b0-3330-4254-b7b7-d17bafe81d4a@amd.com>
Date: Wed, 27 Nov 2024 19:19:49 -0500
From: Felix Kuehling <felix.kuehling@....com>
To: Christian König <christian.koenig@....com>,
Vamsi Krishna Brahmajosyula <vamsi-krishna.brahmajosyula@...adcom.com>,
stable@...r.kernel.org, gregkh@...uxfoundation.org
Cc: Philip.Yang@....com, alexander.deucher@....com, Xinhui.Pan@....com,
airlied@...il.com, daniel@...ll.ch, amd-gfx@...ts.freedesktop.org,
dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
ajay.kaher@...adcom.com, alexey.makhalov@...adcom.com,
vasavi.sirnapalli@...adcom.com, Sasha Levin <sashal@...nel.org>
Subject: Re: [PATCH v6.1] drm/amdkfd: amdkfd_free_gtt_mem clear the correct
pointer
On 2024-11-13 07:13, Christian König wrote:
> Am 13.11.24 um 13:10 schrieb Vamsi Krishna Brahmajosyula:
>> From: Philip Yang <Philip.Yang@....com>
>>
>> [ Upstream commit c86ad39140bbcb9dc75a10046c2221f657e8083b ]
>>
>> Pass pointer reference to amdgpu_bo_unref to clear the correct pointer,
>> otherwise amdgpu_bo_unref clear the local variable, the original pointer
>> not set to NULL, this could cause use-after-free bug.
>>
>> Signed-off-by: Philip Yang <Philip.Yang@....com>
>> Reviewed-by: Felix Kuehling <felix.kuehling@....com>
>> Acked-by: Christian König <christian.koenig@....com>
>> Signed-off-by: Alex Deucher <alexander.deucher@....com>
>> Signed-off-by: Sasha Levin <sashal@...nel.org>
>> Signed-off-by: Vamsi Krishna Brahmajosyula
>> <vamsi-krishna.brahmajosyula@...adcom.com>
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c | 14 +++++++-------
>> drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h | 2 +-
>> drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 2 +-
>> drivers/gpu/drm/amd/amdkfd/kfd_device.c | 4 ++--
>> .../gpu/drm/amd/amdkfd/kfd_device_queue_manager.c | 2 +-
>> drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager.c | 2 +-
>> drivers/gpu/drm/amd/amdkfd/kfd_process.c | 2 +-
>> .../gpu/drm/amd/amdkfd/kfd_process_queue_manager.c | 4 ++--
>> 8 files changed, 16 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
>> index 5d9a34601a1a..c31e5f9d63da 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
>> @@ -344,15 +344,15 @@ int amdgpu_amdkfd_alloc_gtt_mem(struct
>> amdgpu_device *adev, size_t size,
>> return r;
>> }
>> -void amdgpu_amdkfd_free_gtt_mem(struct amdgpu_device *adev, void
>> *mem_obj)
>> +void amdgpu_amdkfd_free_gtt_mem(struct amdgpu_device *adev, void
>> **mem_obj)
>> {
>> - struct amdgpu_bo *bo = (struct amdgpu_bo *) mem_obj;
>> + struct amdgpu_bo **bo = (struct amdgpu_bo **) mem_obj;
>> - amdgpu_bo_reserve(bo, true);
>> - amdgpu_bo_kunmap(bo);
>> - amdgpu_bo_unpin(bo);
>> - amdgpu_bo_unreserve(bo);
>> - amdgpu_bo_unref(&(bo));
>> + amdgpu_bo_reserve(*bo, true);
>> + amdgpu_bo_kunmap(*bo);
>> + amdgpu_bo_unpin(*bo);
>> + amdgpu_bo_unreserve(*bo);
>> + amdgpu_bo_unref(bo);
>> }
>> int amdgpu_amdkfd_alloc_gws(struct amdgpu_device *adev, size_t size,
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
>> index 4b694886715c..c7672a1d1560 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
>> @@ -210,7 +210,7 @@ int amdgpu_amdkfd_evict_userptr(struct kgd_mem
>> *mem, struct mm_struct *mm)
>> int amdgpu_amdkfd_alloc_gtt_mem(struct amdgpu_device *adev, size_t
>> size,
>> void **mem_obj, uint64_t *gpu_addr,
>> void **cpu_ptr, bool mqd_gfx9);
>> -void amdgpu_amdkfd_free_gtt_mem(struct amdgpu_device *adev, void
>> *mem_obj);
>> +void amdgpu_amdkfd_free_gtt_mem(struct amdgpu_device *adev, void
>> **mem_obj);
>
> Why is that a pointer to a void* in the first place? It looks like all
> callers should work with an amdgpu_bo object as well.
Historical reasons. When KFD was a separate module, mem_obj was opaque
to KFD, so the call from KFD to KGD used a void*.
Regards,
Felix
>
> Regards,
> Christian.
>
>> int amdgpu_amdkfd_alloc_gws(struct amdgpu_device *adev, size_t size,
>> void **mem_obj);
>> void amdgpu_amdkfd_free_gws(struct amdgpu_device *adev, void
>> *mem_obj);
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>> b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>> index e3cd66c4d95d..f83574107eb8 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>> @@ -408,7 +408,7 @@ static int kfd_ioctl_create_queue(struct file
>> *filep, struct kfd_process *p,
>> err_create_queue:
>> if (wptr_bo)
>> - amdgpu_amdkfd_free_gtt_mem(dev->adev, wptr_bo);
>> + amdgpu_amdkfd_free_gtt_mem(dev->adev, (void **)&wptr_bo);
>> err_wptr_map_gart:
>> err_alloc_doorbells:
>> err_bind_process:
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device.c
>> b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
>> index 27820f0a282d..e2c055abfea9 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
>> @@ -673,7 +673,7 @@ bool kgd2kfd_device_init(struct kfd_dev *kfd,
>> kfd_doorbell_error:
>> kfd_gtt_sa_fini(kfd);
>> kfd_gtt_sa_init_error:
>> - amdgpu_amdkfd_free_gtt_mem(kfd->adev, kfd->gtt_mem);
>> + amdgpu_amdkfd_free_gtt_mem(kfd->adev, &kfd->gtt_mem);
>> alloc_gtt_mem_failure:
>> if (kfd->gws)
>> amdgpu_amdkfd_free_gws(kfd->adev, kfd->gws);
>> @@ -693,7 +693,7 @@ void kgd2kfd_device_exit(struct kfd_dev *kfd)
>> kfd_doorbell_fini(kfd);
>> ida_destroy(&kfd->doorbell_ida);
>> kfd_gtt_sa_fini(kfd);
>> - amdgpu_amdkfd_free_gtt_mem(kfd->adev, kfd->gtt_mem);
>> + amdgpu_amdkfd_free_gtt_mem(kfd->adev, &kfd->gtt_mem);
>> if (kfd->gws)
>> amdgpu_amdkfd_free_gws(kfd->adev, kfd->gws);
>> }
>> 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 1b7b29426480..3ab0a796af06 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
>> @@ -2392,7 +2392,7 @@ static void deallocate_hiq_sdma_mqd(struct
>> kfd_dev *dev,
>> {
>> WARN(!mqd, "No hiq sdma mqd trunk to free");
>> - amdgpu_amdkfd_free_gtt_mem(dev->adev, mqd->gtt_mem);
>> + amdgpu_amdkfd_free_gtt_mem(dev->adev, &mqd->gtt_mem);
>> }
>> void device_queue_manager_uninit(struct device_queue_manager *dqm)
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager.c
>> b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager.c
>> index 623ccd227b7d..c733d6888c30 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager.c
>> @@ -204,7 +204,7 @@ void kfd_free_mqd_cp(struct mqd_manager *mm, void
>> *mqd,
>> struct kfd_mem_obj *mqd_mem_obj)
>> {
>> if (mqd_mem_obj->gtt_mem) {
>> - amdgpu_amdkfd_free_gtt_mem(mm->dev->adev,
>> mqd_mem_obj->gtt_mem);
>> + amdgpu_amdkfd_free_gtt_mem(mm->dev->adev,
>> &mqd_mem_obj->gtt_mem);
>> kfree(mqd_mem_obj);
>> } else {
>> kfd_gtt_sa_free(mm->dev, mqd_mem_obj);
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>> b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>> index 5bca6abd55ae..9582c9449fff 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>> @@ -1052,7 +1052,7 @@ static void kfd_process_destroy_pdds(struct
>> kfd_process *p)
>> if (pdd->dev->shared_resources.enable_mes)
>> amdgpu_amdkfd_free_gtt_mem(pdd->dev->adev,
>> - pdd->proc_ctx_bo);
>> + &pdd->proc_ctx_bo);
>> /*
>> * before destroying pdd, make sure to report availability
>> * for auto suspend
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
>> b/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
>> index 99aa8a8399d6..1918a3c06ac8 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c
>> @@ -441,9 +441,9 @@ int pqm_destroy_queue(struct
>> process_queue_manager *pqm, unsigned int qid)
>> if (dev->shared_resources.enable_mes) {
>> amdgpu_amdkfd_free_gtt_mem(dev->adev,
>> - pqn->q->gang_ctx_bo);
>> + &pqn->q->gang_ctx_bo);
>> if (pqn->q->wptr_bo)
>> - amdgpu_amdkfd_free_gtt_mem(dev->adev, pqn->q->wptr_bo);
>> + amdgpu_amdkfd_free_gtt_mem(dev->adev, (void
>> **)&pqn->q->wptr_bo);
>> }
>> uninit_queue(pqn->q);
>
Powered by blists - more mailing lists