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@xxxxxxx>
> Reviewed-by: Philip Yang <Philip.Yang@xxxxxxx>
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);
>> }
