Messages in this thread | | | Subject | Re: [PATCH] drm/amdkfd: Fix a wild pointer dereference in svm_range_add() | From | Felix Kuehling <> | Date | Tue, 7 Dec 2021 19:54:52 -0500 |
| |
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); > >> } >
| |