Re: [PATCH] drm/amdkfd: Fix a wild pointer dereference in svm_range_add()

[Felix Kuehling <felix.kuehling@amd.com>]

Am 2021-11-30 um 11:51 a.m. schrieb philip yang:
>
>
> On 2021-11-30 6:26 a.m., Zhou Qingyang wrote:
>> In svm_range_add(), the return value of svm_range_new() is assigned
>> to prange and &prange->insert_list is used in list_add(). There is a
>> a dereference of &prange->insert_list in list_add(), which could lead
>> to a wild pointer dereference on failure of vm_range_new() if
>> CONFIG_DEBUG_LIST is unset in .config file.
>>
>> Fix this bug by adding a check of prange.
>>
>> This bug was found by a static analyzer. The analysis employs
>> differential checking to identify inconsistent security operations
>> (e.g., checks or kfrees) between two code paths and confirms that the
>> inconsistent operations are not recovered in the current function or
>> the callers, so they constitute bugs.
>>
>> Note that, as a bug found by static analysis, it can be a false
>> positive or hard to trigger. Multiple researchers have cross-reviewed
>> the bug.
>>
>> Builds with CONFIG_DRM_AMDGPU=m, CONFIG_HSA_AMD=y, and
>> CONFIG_HSA_AMD_SVM=y show no new warnings, and our static analyzer no
>> longer warns about this code.
>>
>> Fixes: 42de677f7999 ("drm/amdkfd: register svm range")
>> Signed-off-by: Zhou Qingyang <zhou1615@umn.edu>
> Reviewed-by: Philip Yang <Philip.Yang@amd.com>

The patch looks good to me. It's an obvious bug and definitely not a
false positive. The patch description is a bit verbose. Is this
auto-generated output from the static checker? It could be replaced with
something more concise. Especially the comment about this possibly being
a false positive should not be in the final submission.

Regards,
  Felix


>> ---
>>  drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>> index 58b89b53ebe6..e40c2211901d 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>> @@ -2940,6 +2940,9 @@ svm_range_add(struct kfd_process *p, uint64_t start, uint64_t size,
>>  
>>  	if (left) {
>>  		prange = svm_range_new(svms, last - left + 1, last);
>> +		if (!prange)
>> +			return -ENOMEM;
>> +
>>  		list_add(&prange->insert_list, insert_list);
>>  		list_add(&prange->update_list, update_list);
>>  	}