[linux-next,v2,1/3] platform/x86/intel/tpmi: Fix double free in tpmi_create_device()
Message ID | 20230309040107.534716-2-dzm91@hust.edu.cn |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:5915:0:0:0:0:0 with SMTP id v21csp87859wrd; Wed, 8 Mar 2023 20:21:35 -0800 (PST) X-Google-Smtp-Source: AK7set/eYr/AFyEQe+4aErcWVBW+IYJOsRZb5hwXQog6vIZjCEAWJ78Ioz/jDfUlRfFvOvitLE4n X-Received: by 2002:a17:902:d2c9:b0:19a:c4a0:5b1b with SMTP id n9-20020a170902d2c900b0019ac4a05b1bmr25586926plc.1.1678335695623; Wed, 08 Mar 2023 20:21:35 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1678335695; cv=none; d=google.com; s=arc-20160816; b=RtSo7k+hSQEKF/ZPPkywMFrTsqNexv1uiCaOZ315v+COJIeXaXldhd2GRosIBjUoDb gfhqZg+8evSXoEtJikhqSwanaUgZoF1Vmi5CRg62L/cIcXpjZgGmgFY6mE+UFlkcVED6 KtUYASFBtcLXU7UZNk5s+tEuX/sXOdFI4smT3tzrV8gpdWu91GxLpyPov7zbb9QJQWcj 2YFjMo3tSXJ0QX5Uy0ddC6qT2/AgCoaMlEK862Og5LWVwIOpw2hu2Ov/XnjjcEs+Hkst bttXBObrHUB5ND/xw6GdNEpJiYSsrxuN41cpTwtZDd7VfLkv5bpfF+7iGeXmVDNCairu 7J/Q== 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 :references:in-reply-to:message-id:date:subject:cc:to:from; bh=S1UmzfbYLGxgntmMfhKviSF9Su7iadKSR2IbvN+sctg=; b=DMlvz82ipwN16CRW4YgLwAm2T4UUHarX9CknQMnjNOy4DfAlQwdzV8CajMIYXYSu+k n4zsL+ej5UEgnyG6trwLwLi/hHwt7j3SSyxaCkY0dkvRMJ5obtA6EOmsxisyeRHJ310I yMqsR4HM0ujlN/LZmRAmORiMQiJ2SOvkl7ystDRDH8q4pULajtna/fdCTBJtG4wpTZFN HYXRGOSaTSuoLsx0OF0aPY7k3CHuqAvQ9tnuLmKdw7Hbd44slN5UO7/u8BdM3Shmv9jc svYlQON37y9cc8aiPdVLLhxHAAbkWvtr85s9YVKl3Q8oKnc+Vpi/5F2OO1pAPnQv0Qbv MvDw== 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 Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id cm22-20020a17090afa1600b00230c717f66esi1102685pjb.172.2023.03.08.20.21.20; Wed, 08 Mar 2023 20:21:35 -0800 (PST) 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 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229947AbjCIEES (ORCPT <rfc822;carlos.wei.hk@gmail.com> + 99 others); Wed, 8 Mar 2023 23:04:18 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:33256 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229708AbjCIEEP (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Wed, 8 Mar 2023 23:04:15 -0500 Received: from hust.edu.cn (mail.hust.edu.cn [202.114.0.240]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id ED44169CD6 for <linux-kernel@vger.kernel.org>; Wed, 8 Mar 2023 20:04:13 -0800 (PST) Received: from localhost.localdomain ([172.16.0.254]) (user=dzm91@hust.edu.cn mech=LOGIN bits=0) by mx1.hust.edu.cn with ESMTP id 32943g7q028914-32943g7u028914 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Thu, 9 Mar 2023 12:03:54 +0800 From: Dongliang Mu <dzm91@hust.edu.cn> To: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>, Hans de Goede <hdegoede@redhat.com>, Mark Gross <markgross@kernel.org> Cc: Dongliang Mu <dzm91@hust.edu.cn>, platform-driver-x86@vger.kernel.org, linux-kernel@vger.kernel.org Subject: [PATCH linux-next v2 1/3] platform/x86/intel/tpmi: Fix double free in tpmi_create_device() Date: Thu, 9 Mar 2023 12:01:05 +0800 Message-Id: <20230309040107.534716-2-dzm91@hust.edu.cn> X-Mailer: git-send-email 2.39.2 In-Reply-To: <20230309040107.534716-1-dzm91@hust.edu.cn> References: <20230309040107.534716-1-dzm91@hust.edu.cn> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-FEAS-AUTH-USER: dzm91@hust.edu.cn X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,SPF_HELO_PASS, SPF_PASS 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?1759862530596653374?= X-GMAIL-MSGID: =?utf-8?q?1759862530596653374?= |
Series |
[linux-next,v2,1/3] platform/x86/intel/tpmi: Fix double free in tpmi_create_device()
|
|
Commit Message
Dongliang Mu
March 9, 2023, 4:01 a.m. UTC
The previous commit 6a192c0cbf38 ("platform/x86/intel/tpmi: Fix
double free reported by Smatch") incorrectly handle the deallocation of
res variable. As shown in the comment, intel_vsec_add_aux handles all
the deallocation of res and feature_vsec_dev. Therefore, kfree(res) can
still cause double free if intel_vsec_add_aux returns error.
Fix this by adjusting the error handling part in tpmi_create_device,
following the function intel_vsec_add_dev.
Fixes: 6a192c0cbf38 ("platform/x86/intel/tpmi: Fix double free reported by Smatch")
Signed-off-by: Dongliang Mu <dzm91@hust.edu.cn>
---
drivers/platform/x86/intel/tpmi.c | 17 ++++-------------
1 file changed, 4 insertions(+), 13 deletions(-)
Comments
On Thu, Mar 09, 2023 at 12:01:05PM +0800, Dongliang Mu wrote: > The previous commit 6a192c0cbf38 ("platform/x86/intel/tpmi: Fix > double free reported by Smatch") incorrectly handle the deallocation of > res variable. As shown in the comment, intel_vsec_add_aux handles all > the deallocation of res and feature_vsec_dev. Therefore, kfree(res) can > still cause double free if intel_vsec_add_aux returns error. > > Fix this by adjusting the error handling part in tpmi_create_device, > following the function intel_vsec_add_dev. > > Fixes: 6a192c0cbf38 ("platform/x86/intel/tpmi: Fix double free reported by Smatch") > Signed-off-by: Dongliang Mu <dzm91@hust.edu.cn> > --- Yeah. These patches are right. The earlier fix still has a double free. Devres stuff is confusing... Reviewed-by: Dan Carpenter <error27@gmail.com> regards, dan carpenter
Hi, On 3/9/23 05:01, Dongliang Mu wrote: > The previous commit 6a192c0cbf38 ("platform/x86/intel/tpmi: Fix > double free reported by Smatch") incorrectly handle the deallocation of > res variable. As shown in the comment, intel_vsec_add_aux handles all > the deallocation of res and feature_vsec_dev. Therefore, kfree(res) can > still cause double free if intel_vsec_add_aux returns error. > > Fix this by adjusting the error handling part in tpmi_create_device, > following the function intel_vsec_add_dev. > > Fixes: 6a192c0cbf38 ("platform/x86/intel/tpmi: Fix double free reported by Smatch") > Signed-off-by: Dongliang Mu <dzm91@hust.edu.cn> IIRC then after this v2 was posted I still saw some comments on the original v1 which was not posted on the list. Without the v1 comments being on the list and this archived, I have lost track of what the status of these patches is. Srinivas, can you let me know if I should merge these, or if more changes are necessary ? From the off-list discussion of v1 I got the impression more changes are necessary, but I'm not sure. Regards, Hans > --- > drivers/platform/x86/intel/tpmi.c | 17 ++++------------- > 1 file changed, 4 insertions(+), 13 deletions(-) > > diff --git a/drivers/platform/x86/intel/tpmi.c b/drivers/platform/x86/intel/tpmi.c > index c999732b0f1e..882fe5e4763f 100644 > --- a/drivers/platform/x86/intel/tpmi.c > +++ b/drivers/platform/x86/intel/tpmi.c > @@ -215,8 +215,8 @@ static int tpmi_create_device(struct intel_tpmi_info *tpmi_info, > > feature_vsec_dev = kzalloc(sizeof(*feature_vsec_dev), GFP_KERNEL); > if (!feature_vsec_dev) { > - ret = -ENOMEM; > - goto free_res; > + kfree(res); > + return -ENOMEM; > } > > snprintf(feature_id_name, sizeof(feature_id_name), "tpmi-%s", name); > @@ -242,17 +242,8 @@ static int tpmi_create_device(struct intel_tpmi_info *tpmi_info, > * feature_vsec_dev memory is also freed as part of device > * delete. > */ > - ret = intel_vsec_add_aux(vsec_dev->pcidev, &vsec_dev->auxdev.dev, > - feature_vsec_dev, feature_id_name); > - if (ret) > - goto free_res; > - > - return 0; > - > -free_res: > - kfree(res); > - > - return ret; > + return intel_vsec_add_aux(vsec_dev->pcidev, &vsec_dev->auxdev.dev, > + feature_vsec_dev, feature_id_name); > } > > static int tpmi_create_devices(struct intel_tpmi_info *tpmi_info)
Hi Hans, On Thu, 2023-03-16 at 15:25 +0100, Hans de Goede wrote: > Hi, > > On 3/9/23 05:01, Dongliang Mu wrote: > > The previous commit 6a192c0cbf38 ("platform/x86/intel/tpmi: Fix > > double free reported by Smatch") incorrectly handle the > > deallocation of > > res variable. As shown in the comment, intel_vsec_add_aux handles > > all > > the deallocation of res and feature_vsec_dev. Therefore, kfree(res) > > can > > still cause double free if intel_vsec_add_aux returns error. > > > > Fix this by adjusting the error handling part in > > tpmi_create_device, > > following the function intel_vsec_add_dev. > > > > Fixes: 6a192c0cbf38 ("platform/x86/intel/tpmi: Fix double free > > reported by Smatch") > > Signed-off-by: Dongliang Mu <dzm91@hust.edu.cn> Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> > > IIRC then after this v2 was posted I still saw some comments on the > original v1 which was not posted on the list. Without the v1 comments > being on the list and this archived, I have lost track of what the > status of these patches is. > > Srinivas, can you let me know if I should merge these, or if more > changes are necessary ? > > From the off-list discussion of v1 I got the impression more changes > are necessary, but I'm not sure. I was looking for changes submitted by the following patch " [PATCH linux-next v2 3/3] drivers/platform/x86/intel: fix a memory leak in intel_vsec_add_aux " Since I was not copied on this, I was unaware. So I was requesting this change. Thanks, Srinivas > > Regards, > > Hans > > > > > > --- > > drivers/platform/x86/intel/tpmi.c | 17 ++++------------- > > 1 file changed, 4 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/platform/x86/intel/tpmi.c > > b/drivers/platform/x86/intel/tpmi.c > > index c999732b0f1e..882fe5e4763f 100644 > > --- a/drivers/platform/x86/intel/tpmi.c > > +++ b/drivers/platform/x86/intel/tpmi.c > > @@ -215,8 +215,8 @@ static int tpmi_create_device(struct > > intel_tpmi_info *tpmi_info, > > > > feature_vsec_dev = kzalloc(sizeof(*feature_vsec_dev), > > GFP_KERNEL); > > if (!feature_vsec_dev) { > > - ret = -ENOMEM; > > - goto free_res; > > + kfree(res); > > + return -ENOMEM; > > } > > > > snprintf(feature_id_name, sizeof(feature_id_name), "tpmi- > > %s", name); > > @@ -242,17 +242,8 @@ static int tpmi_create_device(struct > > intel_tpmi_info *tpmi_info, > > * feature_vsec_dev memory is also freed as part of device > > * delete. > > */ > > - ret = intel_vsec_add_aux(vsec_dev->pcidev, &vsec_dev- > > >auxdev.dev, > > - feature_vsec_dev, > > feature_id_name); > > - if (ret) > > - goto free_res; > > - > > - return 0; > > - > > -free_res: > > - kfree(res); > > - > > - return ret; > > + return intel_vsec_add_aux(vsec_dev->pcidev, &vsec_dev- > > >auxdev.dev, > > + feature_vsec_dev, > > feature_id_name); > > } > > > > static int tpmi_create_devices(struct intel_tpmi_info *tpmi_info) >
On 2023/3/17 02:18, srinivas pandruvada wrote: > Hi Hans, > > On Thu, 2023-03-16 at 15:25 +0100, Hans de Goede wrote: >> Hi, >> >> On 3/9/23 05:01, Dongliang Mu wrote: >>> The previous commit 6a192c0cbf38 ("platform/x86/intel/tpmi: Fix >>> double free reported by Smatch") incorrectly handle the >>> deallocation of >>> res variable. As shown in the comment, intel_vsec_add_aux handles >>> all >>> the deallocation of res and feature_vsec_dev. Therefore, kfree(res) >>> can >>> still cause double free if intel_vsec_add_aux returns error. >>> >>> Fix this by adjusting the error handling part in >>> tpmi_create_device, >>> following the function intel_vsec_add_dev. >>> >>> Fixes: 6a192c0cbf38 ("platform/x86/intel/tpmi: Fix double free >>> reported by Smatch") >>> Signed-off-by: Dongliang Mu <dzm91@hust.edu.cn> > Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> > >> IIRC then after this v2 was posted I still saw some comments on the >> original v1 which was not posted on the list. Without the v1 comments >> being on the list and this archived, I have lost track of what the >> status of these patches is. >> >> Srinivas, can you let me know if I should merge these, or if more >> changes are necessary ? >> >> From the off-list discussion of v1 I got the impression more changes >> are necessary, but I'm not sure. > I was looking for changes submitted by the following patch > " > [PATCH linux-next v2 3/3] drivers/platform/x86/intel: fix a memory leak > in intel_vsec_add_aux > " > > Since I was not copied on this, I was unaware. So I was requesting this > change. > > Thanks, > Srinivas Hi Srinivas and Hans, How about folding these three patches into one patch and resend a v3 patch? This will get all people together and avoid the previous embarrassing sitation. Dongliang Mu > >> Regards, >> >> Hans >> >> >> >> >>> --- >>> drivers/platform/x86/intel/tpmi.c | 17 ++++------------- >>> 1 file changed, 4 insertions(+), 13 deletions(-) >>> >>> diff --git a/drivers/platform/x86/intel/tpmi.c >>> b/drivers/platform/x86/intel/tpmi.c >>> index c999732b0f1e..882fe5e4763f 100644 >>> --- a/drivers/platform/x86/intel/tpmi.c >>> +++ b/drivers/platform/x86/intel/tpmi.c >>> @@ -215,8 +215,8 @@ static int tpmi_create_device(struct >>> intel_tpmi_info *tpmi_info, >>> >>> feature_vsec_dev = kzalloc(sizeof(*feature_vsec_dev), >>> GFP_KERNEL); >>> if (!feature_vsec_dev) { >>> - ret = -ENOMEM; >>> - goto free_res; >>> + kfree(res); >>> + return -ENOMEM; >>> } >>> >>> snprintf(feature_id_name, sizeof(feature_id_name), "tpmi- >>> %s", name); >>> @@ -242,17 +242,8 @@ static int tpmi_create_device(struct >>> intel_tpmi_info *tpmi_info, >>> * feature_vsec_dev memory is also freed as part of device >>> * delete. >>> */ >>> - ret = intel_vsec_add_aux(vsec_dev->pcidev, &vsec_dev- >>>> auxdev.dev, >>> - feature_vsec_dev, >>> feature_id_name); >>> - if (ret) >>> - goto free_res; >>> - >>> - return 0; >>> - >>> -free_res: >>> - kfree(res); >>> - >>> - return ret; >>> + return intel_vsec_add_aux(vsec_dev->pcidev, &vsec_dev- >>>> auxdev.dev, >>> + feature_vsec_dev, >>> feature_id_name); >>> } >>> >>> static int tpmi_create_devices(struct intel_tpmi_info *tpmi_info)
Hi, On 3/17/23 02:28, Dongliang Mu wrote: > > On 2023/3/17 02:18, srinivas pandruvada wrote: >> Hi Hans, >> >> On Thu, 2023-03-16 at 15:25 +0100, Hans de Goede wrote: >>> Hi, >>> >>> On 3/9/23 05:01, Dongliang Mu wrote: >>>> The previous commit 6a192c0cbf38 ("platform/x86/intel/tpmi: Fix >>>> double free reported by Smatch") incorrectly handle the >>>> deallocation of >>>> res variable. As shown in the comment, intel_vsec_add_aux handles >>>> all >>>> the deallocation of res and feature_vsec_dev. Therefore, kfree(res) >>>> can >>>> still cause double free if intel_vsec_add_aux returns error. >>>> >>>> Fix this by adjusting the error handling part in >>>> tpmi_create_device, >>>> following the function intel_vsec_add_dev. >>>> >>>> Fixes: 6a192c0cbf38 ("platform/x86/intel/tpmi: Fix double free >>>> reported by Smatch") >>>> Signed-off-by: Dongliang Mu <dzm91@hust.edu.cn> >> Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> >> >>> IIRC then after this v2 was posted I still saw some comments on the >>> original v1 which was not posted on the list. Without the v1 comments >>> being on the list and this archived, I have lost track of what the >>> status of these patches is. >>> >>> Srinivas, can you let me know if I should merge these, or if more >>> changes are necessary ? >>> >>> From the off-list discussion of v1 I got the impression more changes >>> are necessary, but I'm not sure. >> I was looking for changes submitted by the following patch >> " >> [PATCH linux-next v2 3/3] drivers/platform/x86/intel: fix a memory leak >> in intel_vsec_add_aux >> " >> >> Since I was not copied on this, I was unaware. So I was requesting this >> change. >> >> Thanks, >> Srinivas > > Hi Srinivas and Hans, > > How about folding these three patches into one patch and resend a v3 patch? > > This will get all people together and avoid the previous embarrassing sitation. If I understand things correctly then patch 1/3 needs 3/3 to function correctly, right ? I would not fold them together, smaller patches are easier to review / understand, but maybe change the order and put patch 3/3 first? (so make it 1/3) ? I can even do that when applying if you agree that is the better order. Regards, Hans >>>> --- >>>> drivers/platform/x86/intel/tpmi.c | 17 ++++------------- >>>> 1 file changed, 4 insertions(+), 13 deletions(-) >>>> >>>> diff --git a/drivers/platform/x86/intel/tpmi.c >>>> b/drivers/platform/x86/intel/tpmi.c >>>> index c999732b0f1e..882fe5e4763f 100644 >>>> --- a/drivers/platform/x86/intel/tpmi.c >>>> +++ b/drivers/platform/x86/intel/tpmi.c >>>> @@ -215,8 +215,8 @@ static int tpmi_create_device(struct >>>> intel_tpmi_info *tpmi_info, >>>> feature_vsec_dev = kzalloc(sizeof(*feature_vsec_dev), >>>> GFP_KERNEL); >>>> if (!feature_vsec_dev) { >>>> - ret = -ENOMEM; >>>> - goto free_res; >>>> + kfree(res); >>>> + return -ENOMEM; >>>> } >>>> snprintf(feature_id_name, sizeof(feature_id_name), "tpmi- >>>> %s", name); >>>> @@ -242,17 +242,8 @@ static int tpmi_create_device(struct >>>> intel_tpmi_info *tpmi_info, >>>> * feature_vsec_dev memory is also freed as part of device >>>> * delete. >>>> */ >>>> - ret = intel_vsec_add_aux(vsec_dev->pcidev, &vsec_dev- >>>>> auxdev.dev, >>>> - feature_vsec_dev, >>>> feature_id_name); >>>> - if (ret) >>>> - goto free_res; >>>> - >>>> - return 0; >>>> - >>>> -free_res: >>>> - kfree(res); >>>> - >>>> - return ret; >>>> + return intel_vsec_add_aux(vsec_dev->pcidev, &vsec_dev- >>>>> auxdev.dev, >>>> + feature_vsec_dev, >>>> feature_id_name); >>>> } >>>> static int tpmi_create_devices(struct intel_tpmi_info *tpmi_info)
Hi, ... ... > > > > Hi Srinivas and Hans, > > > > How about folding these three patches into one patch and resend a > > v3 patch? > > > > This will get all people together and avoid the previous > > embarrassing sitation. > > If I understand things correctly then patch 1/3 needs 3/3 to function > correctly, right ? > > I would not fold them together, smaller patches are easier to review > / understand, but maybe change the order and put patch 3/3 first? (so > make it 1/3) ? That should be correct order. The patch 3/3 should be the first. > > I can even do that when applying if you agree that is the better > order. Agree. Thanks, Srinivas > > Regards, > > Hans > > > > > > > > --- > > > > > drivers/platform/x86/intel/tpmi.c | 17 ++++------------- > > > > > 1 file changed, 4 insertions(+), 13 deletions(-) > > > > > > > > > > diff --git a/drivers/platform/x86/intel/tpmi.c > > > > > b/drivers/platform/x86/intel/tpmi.c > > > > > index c999732b0f1e..882fe5e4763f 100644 > > > > > --- a/drivers/platform/x86/intel/tpmi.c > > > > > +++ b/drivers/platform/x86/intel/tpmi.c > > > > > @@ -215,8 +215,8 @@ static int tpmi_create_device(struct > > > > > intel_tpmi_info *tpmi_info, > > > > > feature_vsec_dev = > > > > > kzalloc(sizeof(*feature_vsec_dev), > > > > > GFP_KERNEL); > > > > > if (!feature_vsec_dev) { > > > > > - ret = -ENOMEM; > > > > > - goto free_res; > > > > > + kfree(res); > > > > > + return -ENOMEM; > > > > > } > > > > > snprintf(feature_id_name, sizeof(feature_id_name), > > > > > "tpmi- > > > > > %s", name); > > > > > @@ -242,17 +242,8 @@ static int tpmi_create_device(struct > > > > > intel_tpmi_info *tpmi_info, > > > > > * feature_vsec_dev memory is also freed as part of > > > > > device > > > > > * delete. > > > > > */ > > > > > - ret = intel_vsec_add_aux(vsec_dev->pcidev, &vsec_dev- > > > > > > auxdev.dev, > > > > > - feature_vsec_dev, > > > > > feature_id_name); > > > > > - if (ret) > > > > > - goto free_res; > > > > > - > > > > > - return 0; > > > > > - > > > > > -free_res: > > > > > - kfree(res); > > > > > - > > > > > - return ret; > > > > > + return intel_vsec_add_aux(vsec_dev->pcidev, > > > > > &vsec_dev- > > > > > > auxdev.dev, > > > > > + feature_vsec_dev, > > > > > feature_id_name); > > > > > } > > > > > static int tpmi_create_devices(struct intel_tpmi_info > > > > > *tpmi_info) >
Hi Dongliang, > ... ... > Hi Srinivas and Hans, > > How about folding these three patches into one patch and resend a v3 > patch? > > This will get all people together and avoid the previous embarrassing > sitation. This is NOT an embarrassing situation. Thanks for finding and fixing the issue. Thanks, Srinivas > > Dongliang Mu > > > > > > Regards, > > > > > > Hans > > > > > > > > > > > > > > > > --- > > > > drivers/platform/x86/intel/tpmi.c | 17 ++++------------- > > > > 1 file changed, 4 insertions(+), 13 deletions(-) > > > > > > > > diff --git a/drivers/platform/x86/intel/tpmi.c > > > > b/drivers/platform/x86/intel/tpmi.c > > > > index c999732b0f1e..882fe5e4763f 100644 > > > > --- a/drivers/platform/x86/intel/tpmi.c > > > > +++ b/drivers/platform/x86/intel/tpmi.c > > > > @@ -215,8 +215,8 @@ static int tpmi_create_device(struct > > > > intel_tpmi_info *tpmi_info, > > > > > > > > feature_vsec_dev = kzalloc(sizeof(*feature_vsec_dev), > > > > GFP_KERNEL); > > > > if (!feature_vsec_dev) { > > > > - ret = -ENOMEM; > > > > - goto free_res; > > > > + kfree(res); > > > > + return -ENOMEM; > > > > } > > > > > > > > snprintf(feature_id_name, sizeof(feature_id_name), > > > > "tpmi- > > > > %s", name); > > > > @@ -242,17 +242,8 @@ static int tpmi_create_device(struct > > > > intel_tpmi_info *tpmi_info, > > > > * feature_vsec_dev memory is also freed as part of > > > > device > > > > * delete. > > > > */ > > > > - ret = intel_vsec_add_aux(vsec_dev->pcidev, &vsec_dev- > > > > > auxdev.dev, > > > > - feature_vsec_dev, > > > > feature_id_name); > > > > - if (ret) > > > > - goto free_res; > > > > - > > > > - return 0; > > > > - > > > > -free_res: > > > > - kfree(res); > > > > - > > > > - return ret; > > > > + return intel_vsec_add_aux(vsec_dev->pcidev, &vsec_dev- > > > > > auxdev.dev, > > > > + feature_vsec_dev, > > > > feature_id_name); > > > > } > > > > > > > > static int tpmi_create_devices(struct intel_tpmi_info > > > > *tpmi_info)
On 2023/3/17 18:23, srinivas pandruvada wrote: > Hi, > > ... > ... > >>> Hi Srinivas and Hans, >>> >>> How about folding these three patches into one patch and resend a >>> v3 patch? >>> >>> This will get all people together and avoid the previous >>> embarrassing sitation. >> If I understand things correctly then patch 1/3 needs 3/3 to function >> correctly, right ? >> >> I would not fold them together, smaller patches are easier to review >> / understand, but maybe change the order and put patch 3/3 first? (so >> make it 1/3) ? > That should be correct order. The patch 3/3 should be the first. Oh, yeah. The memory leak fix 3/3 should be the first. This is more reasonable. BTW, I separated this memory leak fix due to that the kernel mainline is still vulnerable to this memory leak problem. > >> I can even do that when applying if you agree that is the better >> order. > Agree. > > Thanks, > Srinivas > >> Regards, >> >> Hans >> >> >> >>>>>> --- >>>>>> drivers/platform/x86/intel/tpmi.c | 17 ++++------------- >>>>>> 1 file changed, 4 insertions(+), 13 deletions(-) >>>>>> >>>>>> diff --git a/drivers/platform/x86/intel/tpmi.c >>>>>> b/drivers/platform/x86/intel/tpmi.c >>>>>> index c999732b0f1e..882fe5e4763f 100644 >>>>>> --- a/drivers/platform/x86/intel/tpmi.c >>>>>> +++ b/drivers/platform/x86/intel/tpmi.c >>>>>> @@ -215,8 +215,8 @@ static int tpmi_create_device(struct >>>>>> intel_tpmi_info *tpmi_info, >>>>>> feature_vsec_dev = >>>>>> kzalloc(sizeof(*feature_vsec_dev), >>>>>> GFP_KERNEL); >>>>>> if (!feature_vsec_dev) { >>>>>> - ret = -ENOMEM; >>>>>> - goto free_res; >>>>>> + kfree(res); >>>>>> + return -ENOMEM; >>>>>> } >>>>>> snprintf(feature_id_name, sizeof(feature_id_name), >>>>>> "tpmi- >>>>>> %s", name); >>>>>> @@ -242,17 +242,8 @@ static int tpmi_create_device(struct >>>>>> intel_tpmi_info *tpmi_info, >>>>>> * feature_vsec_dev memory is also freed as part of >>>>>> device >>>>>> * delete. >>>>>> */ >>>>>> - ret = intel_vsec_add_aux(vsec_dev->pcidev, &vsec_dev- >>>>>>> auxdev.dev, >>>>>> - feature_vsec_dev, >>>>>> feature_id_name); >>>>>> - if (ret) >>>>>> - goto free_res; >>>>>> - >>>>>> - return 0; >>>>>> - >>>>>> -free_res: >>>>>> - kfree(res); >>>>>> - >>>>>> - return ret; >>>>>> + return intel_vsec_add_aux(vsec_dev->pcidev, >>>>>> &vsec_dev- >>>>>>> auxdev.dev, >>>>>> + feature_vsec_dev, >>>>>> feature_id_name); >>>>>> } >>>>>> static int tpmi_create_devices(struct intel_tpmi_info >>>>>> *tpmi_info)
On 2023/3/17 18:27, srinivas pandruvada wrote: > Hi Dongliang, > > ... > ... > > >> Hi Srinivas and Hans, >> >> How about folding these three patches into one patch and resend a v3 >> patch? >> >> This will get all people together and avoid the previous embarrassing >> sitation. > This is NOT an embarrassing situation. > Thanks for finding and fixing the issue. > > Thanks, > Srinivas > Hi Srinivas, Any conclusion about this patch set? >> Dongliang Mu >> >>>> Regards, >>>> >>>> Hans >>>> >>>> >>>> >>>> >>>>> --- >>>>> drivers/platform/x86/intel/tpmi.c | 17 ++++------------- >>>>> 1 file changed, 4 insertions(+), 13 deletions(-) >>>>> >>>>> diff --git a/drivers/platform/x86/intel/tpmi.c >>>>> b/drivers/platform/x86/intel/tpmi.c >>>>> index c999732b0f1e..882fe5e4763f 100644 >>>>> --- a/drivers/platform/x86/intel/tpmi.c >>>>> +++ b/drivers/platform/x86/intel/tpmi.c >>>>> @@ -215,8 +215,8 @@ static int tpmi_create_device(struct >>>>> intel_tpmi_info *tpmi_info, >>>>> >>>>> feature_vsec_dev = kzalloc(sizeof(*feature_vsec_dev), >>>>> GFP_KERNEL); >>>>> if (!feature_vsec_dev) { >>>>> - ret = -ENOMEM; >>>>> - goto free_res; >>>>> + kfree(res); >>>>> + return -ENOMEM; >>>>> } >>>>> >>>>> snprintf(feature_id_name, sizeof(feature_id_name), >>>>> "tpmi- >>>>> %s", name); >>>>> @@ -242,17 +242,8 @@ static int tpmi_create_device(struct >>>>> intel_tpmi_info *tpmi_info, >>>>> * feature_vsec_dev memory is also freed as part of >>>>> device >>>>> * delete. >>>>> */ >>>>> - ret = intel_vsec_add_aux(vsec_dev->pcidev, &vsec_dev- >>>>>> auxdev.dev, >>>>> - feature_vsec_dev, >>>>> feature_id_name); >>>>> - if (ret) >>>>> - goto free_res; >>>>> - >>>>> - return 0; >>>>> - >>>>> -free_res: >>>>> - kfree(res); >>>>> - >>>>> - return ret; >>>>> + return intel_vsec_add_aux(vsec_dev->pcidev, &vsec_dev- >>>>>> auxdev.dev, >>>>> + feature_vsec_dev, >>>>> feature_id_name); >>>>> } >>>>> >>>>> static int tpmi_create_devices(struct intel_tpmi_info >>>>> *tpmi_info)
On Mon, 2023-03-20 at 10:43 +0800, Dongliang Mu wrote: > > On 2023/3/17 18:27, srinivas pandruvada wrote: > > Hi Dongliang, > > > > ... > > ... > > > > > > > Hi Srinivas and Hans, > > > > > > How about folding these three patches into one patch and resend a > > > v3 > > > patch? > > > > > > This will get all people together and avoid the previous > > > embarrassing > > > sitation. > > This is NOT an embarrassing situation. > > Thanks for finding and fixing the issue. > > > > Thanks, > > Srinivas > > > Hi Srinivas, > > Any conclusion about this patch set? Hans can reorder patches as he suggested and apply. Thanks, Srinivas > > > > Dongliang Mu > > > > > > > > Regards, > > > > > > > > > > Hans > > > > > > > > > > > > > > > > > > > > > > > > > > --- > > > > > > drivers/platform/x86/intel/tpmi.c | 17 ++++------------- > > > > > > 1 file changed, 4 insertions(+), 13 deletions(-) > > > > > > > > > > > > diff --git a/drivers/platform/x86/intel/tpmi.c > > > > > > b/drivers/platform/x86/intel/tpmi.c > > > > > > index c999732b0f1e..882fe5e4763f 100644 > > > > > > --- a/drivers/platform/x86/intel/tpmi.c > > > > > > +++ b/drivers/platform/x86/intel/tpmi.c > > > > > > @@ -215,8 +215,8 @@ static int tpmi_create_device(struct > > > > > > intel_tpmi_info *tpmi_info, > > > > > > > > > > > > feature_vsec_dev = > > > > > > kzalloc(sizeof(*feature_vsec_dev), > > > > > > GFP_KERNEL); > > > > > > if (!feature_vsec_dev) { > > > > > > - ret = -ENOMEM; > > > > > > - goto free_res; > > > > > > + kfree(res); > > > > > > + return -ENOMEM; > > > > > > } > > > > > > > > > > > > snprintf(feature_id_name, > > > > > > sizeof(feature_id_name), > > > > > > "tpmi- > > > > > > %s", name); > > > > > > @@ -242,17 +242,8 @@ static int tpmi_create_device(struct > > > > > > intel_tpmi_info *tpmi_info, > > > > > > * feature_vsec_dev memory is also freed as part > > > > > > of > > > > > > device > > > > > > * delete. > > > > > > */ > > > > > > - ret = intel_vsec_add_aux(vsec_dev->pcidev, > > > > > > &vsec_dev- > > > > > > > auxdev.dev, > > > > > > - feature_vsec_dev, > > > > > > feature_id_name); > > > > > > - if (ret) > > > > > > - goto free_res; > > > > > > - > > > > > > - return 0; > > > > > > - > > > > > > -free_res: > > > > > > - kfree(res); > > > > > > - > > > > > > - return ret; > > > > > > + return intel_vsec_add_aux(vsec_dev->pcidev, > > > > > > &vsec_dev- > > > > > > > auxdev.dev, > > > > > > + feature_vsec_dev, > > > > > > feature_id_name); > > > > > > } > > > > > > > > > > > > static int tpmi_create_devices(struct intel_tpmi_info > > > > > > *tpmi_info)
Hi, On 3/9/23 05:01, Dongliang Mu wrote: > The previous commit 6a192c0cbf38 ("platform/x86/intel/tpmi: Fix > double free reported by Smatch") incorrectly handle the deallocation of > res variable. As shown in the comment, intel_vsec_add_aux handles all > the deallocation of res and feature_vsec_dev. Therefore, kfree(res) can > still cause double free if intel_vsec_add_aux returns error. > > Fix this by adjusting the error handling part in tpmi_create_device, > following the function intel_vsec_add_dev. > > Fixes: 6a192c0cbf38 ("platform/x86/intel/tpmi: Fix double free reported by Smatch") > Signed-off-by: Dongliang Mu <dzm91@hust.edu.cn> Note this patch causes the following compiler warnings: CC [M] drivers/platform/x86/intel/tpmi.o drivers/platform/x86/intel/tpmi.c: In function ‘tpmi_create_device’: drivers/platform/x86/intel/tpmi.c:206:13: warning: unused variable ‘ret’ [-Wunused-variable] 206 | int ret, i; | ^~~ I have fixed this and squashed the fix into the patch while applying it, so there is no need to send a new version: Thank you for your patch, I've applied this patch to my review-hans branch: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans Note it will show up in my review-hans branch once I've pushed my local branch there, which might take a while. Once I've run some tests on this branch the patches there will be added to the platform-drivers-x86/for-next branch and eventually will be included in the pdx86 pull-request to Linus for the next merge-window. Regards, Hans > --- > drivers/platform/x86/intel/tpmi.c | 17 ++++------------- > 1 file changed, 4 insertions(+), 13 deletions(-) > > diff --git a/drivers/platform/x86/intel/tpmi.c b/drivers/platform/x86/intel/tpmi.c > index c999732b0f1e..882fe5e4763f 100644 > --- a/drivers/platform/x86/intel/tpmi.c > +++ b/drivers/platform/x86/intel/tpmi.c > @@ -215,8 +215,8 @@ static int tpmi_create_device(struct intel_tpmi_info *tpmi_info, > > feature_vsec_dev = kzalloc(sizeof(*feature_vsec_dev), GFP_KERNEL); > if (!feature_vsec_dev) { > - ret = -ENOMEM; > - goto free_res; > + kfree(res); > + return -ENOMEM; > } > > snprintf(feature_id_name, sizeof(feature_id_name), "tpmi-%s", name); > @@ -242,17 +242,8 @@ static int tpmi_create_device(struct intel_tpmi_info *tpmi_info, > * feature_vsec_dev memory is also freed as part of device > * delete. > */ > - ret = intel_vsec_add_aux(vsec_dev->pcidev, &vsec_dev->auxdev.dev, > - feature_vsec_dev, feature_id_name); > - if (ret) > - goto free_res; > - > - return 0; > - > -free_res: > - kfree(res); > - > - return ret; > + return intel_vsec_add_aux(vsec_dev->pcidev, &vsec_dev->auxdev.dev, > + feature_vsec_dev, feature_id_name); > } > > static int tpmi_create_devices(struct intel_tpmi_info *tpmi_info)
diff --git a/drivers/platform/x86/intel/tpmi.c b/drivers/platform/x86/intel/tpmi.c index c999732b0f1e..882fe5e4763f 100644 --- a/drivers/platform/x86/intel/tpmi.c +++ b/drivers/platform/x86/intel/tpmi.c @@ -215,8 +215,8 @@ static int tpmi_create_device(struct intel_tpmi_info *tpmi_info, feature_vsec_dev = kzalloc(sizeof(*feature_vsec_dev), GFP_KERNEL); if (!feature_vsec_dev) { - ret = -ENOMEM; - goto free_res; + kfree(res); + return -ENOMEM; } snprintf(feature_id_name, sizeof(feature_id_name), "tpmi-%s", name); @@ -242,17 +242,8 @@ static int tpmi_create_device(struct intel_tpmi_info *tpmi_info, * feature_vsec_dev memory is also freed as part of device * delete. */ - ret = intel_vsec_add_aux(vsec_dev->pcidev, &vsec_dev->auxdev.dev, - feature_vsec_dev, feature_id_name); - if (ret) - goto free_res; - - return 0; - -free_res: - kfree(res); - - return ret; + return intel_vsec_add_aux(vsec_dev->pcidev, &vsec_dev->auxdev.dev, + feature_vsec_dev, feature_id_name); } static int tpmi_create_devices(struct intel_tpmi_info *tpmi_info)