lkml.org 
[lkml]   [2021]   [Dec]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] drm/amdkfd: Fix a wild pointer dereference in svm_range_add()
From
Date
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@amd.com
> <mailto:felix.kuehling@amd.com>> 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@umn.edu
> <mailto:zhou1615@umn.edu>>
> > Reviewed-by: Philip Yang <Philip.Yang@amd.com
> <mailto: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);
> >>      }
>

\
 
 \ /
  Last update: 2021-12-08 01:56    [W:0.043 / U:0.272 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site