Message ID | 20230418065521.453001-1-d.dulov@aladdin.ru |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:b0ea:0:b0:3b6:4342:cba0 with SMTP id b10csp2634179vqo; Tue, 18 Apr 2023 00:04:12 -0700 (PDT) X-Google-Smtp-Source: AKy350aNVyD7xEPuPYzmSjbRoHPydGBicN9ijeA1Z+RkoKCl7KoOuYZA7BhK0t1gqtdWCCyGDVq1 X-Received: by 2002:a05:6a20:1790:b0:dd:cae3:641c with SMTP id bl16-20020a056a20179000b000ddcae3641cmr13299087pzb.55.1681801452384; Tue, 18 Apr 2023 00:04:12 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1681801452; cv=none; d=google.com; s=arc-20160816; b=wqfYkxJXrpKs35p5p4L9ww9i1/ohhKLh5PBAj2Se/j4Yw5WP4u9rR/nOS8ZBaaeTp4 tzw6vEUNSNDFMRkjUmVzyjpvHWLRX0mX9wzGHl3KQ0tv6/iNZUy9ed0R7NcZEU+VK6Uy dJ76YjOCfpDkGBimPmIG7XuZO/QF/viUUUVyjYYDyC/V4gyq8yQhFBjl7ZvDAiwYLzUF E0tPaqMcCLHo1D91SBx8fEgzVlOc7Ln/Npuwb7GdExSeQSNOZDCxbmmdcByJiB7d49+X mCQUMmr2PHG87K9zOy3mcRsE6H9pxg/v8VOKpgIlPc23K+GKsLDOmAXWCYmUYLJgwLPt U4dw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from; bh=5iL9yRst2huEhQkbbzmnxGefvNNvJjPU9j7tB6eRqJA=; b=XRiRD9ZsbObRcdWUq5NaQT+PJbF5cKwNSiXEaNwrYDqnvpMXMD3xuRCSzlA/HX9bXd C5mkT/6ygMlqgK/sYgIx728G82waERtkS4FcIAQRQXlkNk9Xviv+PS8xEoORPg3yOcbO cKHztHqd9mGAIlO6Fn6iUKgsAO9zNeh2K1D1uBdC0/63RQYtSLCzihsFXnty3u0F6as2 0S3/eMhhX0rTjbHNyClP+3iak9l3DuM2ncoTpf33aXR4VYuoWSEL0iaBF9V5CrgbwgbG rdfcr6k7YmSfCXU1ve/CDXlbuiUR6/boc68n5YtG6DNSdmoPz/dou/vGMHQ7851ciwLU iUJg== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=aladdin.ru Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id me15-20020a17090b17cf00b002496f81766bsi170388pjb.123.2023.04.18.00.03.57; Tue, 18 Apr 2023 00:04:12 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=aladdin.ru Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229706AbjDRG40 (ORCPT <rfc822;leviz.kernel.dev@gmail.com> + 99 others); Tue, 18 Apr 2023 02:56:26 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:50880 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229535AbjDRG4Y (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Tue, 18 Apr 2023 02:56:24 -0400 Received: from mail-out.aladdin-rd.ru (mail-out.aladdin-rd.ru [91.199.251.16]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 85D935BA5 for <linux-kernel@vger.kernel.org>; Mon, 17 Apr 2023 23:55:57 -0700 (PDT) From: Daniil Dulov <d.dulov@aladdin.ru> To: Felix Kuehling <Felix.Kuehling@amd.com> CC: Daniil Dulov <d.dulov@aladdin.ru>, Alex Deucher <alexander.deucher@amd.com>, =?utf-8?q?Christian_K=C3=B6nig?= <christian.koenig@amd.com>, David Airlie <airlied@linux.ie>, Daniel Vetter <daniel@ffwll.ch>, Oak Zeng <ozeng@amd.com>, <amd-gfx@lists.freedesktop.org>, <dri-devel@lists.freedesktop.org>, <linux-kernel@vger.kernel.org>, <lvc-project@linuxtesting.org> Subject: [PATCH] drm/amdkfd: Fix potential deallocation of previously deallocated memory. Date: Mon, 17 Apr 2023 23:55:21 -0700 Message-ID: <20230418065521.453001-1-d.dulov@aladdin.ru> X-Mailer: git-send-email 2.25.1 MIME-Version: 1.0 Content-Transfer-Encoding: 7BIT Content-Type: text/plain; charset=US-ASCII X-Originating-IP: [10.0.20.32] X-ClientProxiedBy: EXCH-2016-02.aladdin.ru (192.168.1.102) To EXCH-2016-01.aladdin.ru (192.168.1.101) X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,SPF_HELO_NONE, SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1763496639477765377?= X-GMAIL-MSGID: =?utf-8?q?1763496639477765377?= |
Series |
drm/amdkfd: Fix potential deallocation of previously deallocated memory.
|
|
Commit Message
Daniil Dulov
April 18, 2023, 6:55 a.m. UTC
Pointer mqd_mem_obj can be deallocated in kfd_gtt_sa_allocate().
The function then returns non-zero value, which causes the second deallocation.
Found by Linux Verification Center (linuxtesting.org) with SVACE.
Fixes: d1f8f0d17d40 ("drm/amdkfd: Move non-sdma mqd allocation out of init_mqd")
Signed-off-by: Daniil Dulov <d.dulov@aladdin.ru>
---
drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
Comments
Hi Daniil, On Mon, Apr 17, 2023 at 11:55:21PM -0700, Daniil Dulov wrote: > Pointer mqd_mem_obj can be deallocated in kfd_gtt_sa_allocate(). > The function then returns non-zero value, which causes the second deallocation. > > Found by Linux Verification Center (linuxtesting.org) with SVACE. > > Fixes: d1f8f0d17d40 ("drm/amdkfd: Move non-sdma mqd allocation out of init_mqd") > Signed-off-by: Daniil Dulov <d.dulov@aladdin.ru> > --- > drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c > index 3b6f5963180d..bce11c5b07d6 100644 > --- a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c > @@ -119,7 +119,8 @@ static struct kfd_mem_obj *allocate_mqd(struct kfd_dev *kfd, > } > > if (retval) { > - kfree(mqd_mem_obj); > + if (mqd_mem_obj) > + kfree(mqd_mem_obj); I think this is not needed. kfree() returns immediately if mqd_mem_obj is NULL. Andi > return NULL; > } > > -- > 2.25.1
On 18/04/2023 10:47, Andi Shyti wrote: > Hi Daniil, > > On Mon, Apr 17, 2023 at 11:55:21PM -0700, Daniil Dulov wrote: >> Pointer mqd_mem_obj can be deallocated in kfd_gtt_sa_allocate(). >> The function then returns non-zero value, which causes the second deallocation. >> >> Found by Linux Verification Center (linuxtesting.org) with SVACE. >> >> Fixes: d1f8f0d17d40 ("drm/amdkfd: Move non-sdma mqd allocation out of init_mqd") >> Signed-off-by: Daniil Dulov <d.dulov@aladdin.ru> >> --- >> drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c >> index 3b6f5963180d..bce11c5b07d6 100644 >> --- a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c >> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c >> @@ -119,7 +119,8 @@ static struct kfd_mem_obj *allocate_mqd(struct kfd_dev *kfd, >> } >> >> if (retval) { >> - kfree(mqd_mem_obj); >> + if (mqd_mem_obj) >> + kfree(mqd_mem_obj); > > I think this is not needed. kfree() returns immediately if > mqd_mem_obj is NULL. > Yep, the tool has to be fixed because such patch is just misleading. However different point - the commit description actually describes entirely different case: double free. Maybe the issue is true, just the fix is wrong? Best regards, Krzysztof
On Tue, Apr 18, 2023 at 12:07:15PM +0200, Krzysztof Kozlowski wrote: > On 18/04/2023 10:47, Andi Shyti wrote: > > Hi Daniil, > > > > On Mon, Apr 17, 2023 at 11:55:21PM -0700, Daniil Dulov wrote: > >> Pointer mqd_mem_obj can be deallocated in kfd_gtt_sa_allocate(). > >> The function then returns non-zero value, which causes the second deallocation. > >> > >> Found by Linux Verification Center (linuxtesting.org) with SVACE. > >> > >> Fixes: d1f8f0d17d40 ("drm/amdkfd: Move non-sdma mqd allocation out of init_mqd") > >> Signed-off-by: Daniil Dulov <d.dulov@aladdin.ru> > >> --- > >> drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c | 3 ++- > >> 1 file changed, 2 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c > >> index 3b6f5963180d..bce11c5b07d6 100644 > >> --- a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c > >> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c > >> @@ -119,7 +119,8 @@ static struct kfd_mem_obj *allocate_mqd(struct kfd_dev *kfd, > >> } > >> > >> if (retval) { > >> - kfree(mqd_mem_obj); > >> + if (mqd_mem_obj) > >> + kfree(mqd_mem_obj); > > > > I think this is not needed. kfree() returns immediately if > > mqd_mem_obj is NULL. > > > > Yep, the tool has to be fixed because such patch is just misleading. > However different point - the commit description actually describes > entirely different case: double free. Maybe the issue is true, just the > fix is wrong? Yes, indeed, the fix is wrong, but the bug exists. I'm pasting the original function: if (kfd->cwsr_enabled && (q->type == KFD_QUEUE_TYPE_COMPUTE)) { mqd_mem_obj = kzalloc(sizeof(struct kfd_mem_obj), GFP_KERNEL); if (!mqd_mem_obj) return NULL; ... } else { retval = kfd_gtt_sa_allocate(kfd, sizeof(struct v9_mqd), &mqd_mem_obj); } if (retval) { kfree(mqd_mem_obj); return NULL; } The "kfd_gtt_sa_allocate()" function allocates mqd_mem_obj and if an error occurs internally frees it, without setting it to NULL; retval is true and we kfree a memory that has already been freed. The real fix is to move the "if (retval)" inside the if. It would basically be: diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c index fdbfd725841ff..31d47d687bd62 100644 --- a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c +++ b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c @@ -115,18 +115,20 @@ static struct kfd_mem_obj *allocate_mqd(struct kfd_dev *kfd, &(mqd_mem_obj->gtt_mem), &(mqd_mem_obj->gpu_addr), (void *)&(mqd_mem_obj->cpu_ptr), true); + + if (retval) { + kfree(mqd_mem_obj); + return NULL; + } + } else { retval = kfd_gtt_sa_allocate(kfd, sizeof(struct v9_mqd), &mqd_mem_obj); - } - - if (retval) { - kfree(mqd_mem_obj); - return NULL; + if (retval) + return NULL; } return mqd_mem_obj; - } Maybe with some clever refactoring we could reduce some code duplication. Daniil will you look into this? Andi
> Daniil will you look into this?
and, because this is a bug fix, if you do want to send a real
fix, plase add to the commit message:
Fixes: d1f8f0d17d40 ("drm/amdkfd: Move non-sdma mqd allocation out of init_mqd")
Cc: Oak Zeng <oak.zeng@intel.com>
Cc: <stable@vger.kernel.org> # v5.3+
Andi
PS: please note that Oak's e-mail has changed.
Thank you for your feedback! I will do it as soon as possible.
Daniil
________________________________
От: Andi Shyti <andi.shyti@linux.intel.com>
Отправлено: вторник, 18 апреля 2023 г., 20:44
Кому: Andi Shyti <andi.shyti@linux.intel.com>
Копия: Krzysztof Kozlowski <krzk@kernel.org>; Daniil Dulov <D.Dulov@aladdin.ru>; Felix Kuehling <Felix.Kuehling@amd.com>; lvc-project@linuxtesting.org <lvc-project@linuxtesting.org>; David Airlie <airlied@linux.ie>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>; amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>; dri-devel@lists.freedesktop.org <dri-devel@lists.freedesktop.org>; Alex Deucher <alexander.deucher@amd.com>; Oak Zeng <oak.zeng@intel.com>; Christian König <christian.koenig@amd.com>
Тема: Re: [PATCH] drm/amdkfd: Fix potential deallocation of previously deallocated memory.
> Daniil will you look into this?
and, because this is a bug fix, if you do want to send a real
fix, plase add to the commit message:
Fixes: d1f8f0d17d40 ("drm/amdkfd: Move non-sdma mqd allocation out of init_mqd")
Cc: Oak Zeng <oak.zeng@intel.com>
Cc: <stable@vger.kernel.org> # v5.3+
Andi
PS: please note that Oak's e-mail has changed.
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c index 3b6f5963180d..bce11c5b07d6 100644 --- a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c +++ b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v9.c @@ -119,7 +119,8 @@ static struct kfd_mem_obj *allocate_mqd(struct kfd_dev *kfd, } if (retval) { - kfree(mqd_mem_obj); + if (mqd_mem_obj) + kfree(mqd_mem_obj); return NULL; }