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

[Felix Kuehling]

On 2021-11-30 12:49 p.m., Qingyang Zhou wrote:
> Dear Felix:
>
> This patch is not auto-generated, and as a matter of fact, it is 
> requested by the Linux Community.
>
> As you can see from my email address, I am a researcher from the 
> University of Minnesota, and because of the unpleasant event that 
> happened in April, all the patches from our university must contain 
> enough information for the Linux Community to verify. Still I feel so 
> sorry to take up your time.

Hi Qingyang,

Sorry for the late response. I was about to apply your patch when I 
realized that it's not unwinding things correctly in the new failure 
case. I think I'll refactor svm_range_add and svm_range_handle_overlap a 
bit to make sure the unwinding is handled correctly and only needs to be 
done in one place instead of two.

I'll copy you on the final patch.

Regards,
  Felix


> yours sincerely,
> zhou qingyang.
>
>
> On Wed, Dec 1, 2021 at 1:35 AM Felix Kuehling <felix.kuehling@xxxxxxx 
> <mailto:felix.kuehling@xxxxxxx>> wrote:
>
>     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
>     <mailto:zhou1615@xxxxxxx>>
>     > Reviewed-by: Philip Yang <Philip.Yang@xxxxxxx
>     <mailto: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);
>     >>      }