[PATCH] platform/x86/amd: Fix memory leak in wbrf_record()

[PATCH] platform/x86/amd: Fix memory leak in wbrf_record()

[Zilin Guan]

The tmp buffer is allocated using kcalloc() but is not freed if
acpi_evaluate_dsm() fails. This causes a memory leak in the error path.

Fix this by adding a free_tmp label and jumping to it when obj is NULL,
ensuring tmp is properly freed.

Fixes: 58e82a62669d ("platform/x86/amd: Add support for AMD ACPI based Wifi band RFI mitigation feature")
Co-developed-by: Jianhao Xu <jianhao.xu@seu.edu.cn>
Signed-off-by: Jianhao Xu <jianhao.xu@seu.edu.cn>
Signed-off-by: Zilin Guan <zilin@seu.edu.cn>
---
 drivers/platform/x86/amd/wbrf.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/amd/wbrf.c b/drivers/platform/x86/amd/wbrf.c
index dd197b3aebe0..b35e9369b62a 100644
--- a/drivers/platform/x86/amd/wbrf.c
+++ b/drivers/platform/x86/amd/wbrf.c
@@ -104,8 +104,10 @@ static int wbrf_record(struct acpi_device *adev, uint8_t action, struct wbrf_ran
 	obj = acpi_evaluate_dsm(adev->handle, &wifi_acpi_dsm_guid,
 				WBRF_REVISION, WBRF_RECORD, &argv4);
 
-	if (!obj)
-		return -EINVAL;
+	if (!obj) {
+		ret = -EINVAL;
+		goto free_tmp;
+	}
 
 	if (obj->type != ACPI_TYPE_INTEGER) {
 		ret = -EINVAL;
@@ -118,6 +120,7 @@ static int wbrf_record(struct acpi_device *adev, uint8_t action, struct wbrf_ran
 
 out:
 	ACPI_FREE(obj);
+free_tmp:
 	kfree(tmp);
 
 	return ret;
-- 
2.34.1

Re: [PATCH] platform/x86/amd: Fix memory leak in wbrf_record()

[Markus Elfring]

…
> Fix this by adding a free_tmp label and jumping to it when obj is NULL,
> ensuring tmp is properly freed.

How do you think about to increase the application of scope-based resource management?

Regards,
Markus

Re: [PATCH] platform/x86/amd: Fix memory leak in wbrf_record()

[Zilin Guan]

On Tue, Dec 30, 2025 at 09:51:06AM+0100, Markus Elfring wrote:
> …
> > Fix this by adding a free_tmp label and jumping to it when obj is NULL,
> > ensuring tmp is properly freed.
> 
> How do you think about to increase the application of scope-based resource management?
> 
> Regards,
> Markus

Thanks for the suggestion.

While scope-based resource management is a valid improvement, I prefer to 
keep this patch focused solely on fixing the memory leak using the 
existing coding style of the file. This ensures the fix is minimal and 
easier to backport.

Such refactoring is arguably outside the scope of this fix.

Regards,
Zilin Guan

Re: [PATCH] platform/x86/amd: Fix memory leak in wbrf_record()

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1227 bytes --]

On Tue, 30 Dec 2025, Zilin Guan wrote:
> On Tue, Dec 30, 2025 at 09:51:06AM+0100, Markus Elfring wrote:
> > …
> > > Fix this by adding a free_tmp label and jumping to it when obj is NULL,
> > > ensuring tmp is properly freed.
> > 
> > How do you think about to increase the application of scope-based 
> > resource management? 
> > 
> > Regards,
> > Markus
> 
> Thanks for the suggestion.
> 
> While scope-based resource management is a valid improvement, I prefer to 
> keep this patch focused solely on fixing the memory leak using the 
> existing coding style of the file. This ensures the fix is minimal and 
> easier to backport.
> 
> Such refactoring is arguably outside the scope of this fix.

Hi,

Lets not get too attached to the ways of the past, using __free() is
what we want to use even as a fix here. Adding goto labels adds 
complexity, not reduces it, so I don't buy the easier backport argument
(on backport each goto target should be carefully reviewed which is 
something __free() does not require because it simply is better 
interface).

You just need to remember to move also the variable declaration down to 
the alloc site as per the guidance in cleanup.h.

-- 
 i.

Re: [PATCH] platform/x86/amd: Fix memory leak in wbrf_record()

[Zilin Guan]

On Fri, Jan 02, 2026 at 01:29:20PM +0200, "Ilpo Järvinen" wrote:
> Hi,
> 
> Lets not get too attached to the ways of the past, using __free() is
> what we want to use even as a fix here. Adding goto labels adds 
> complexity, not reduces it, so I don't buy the easier backport argument
> (on backport each goto target should be carefully reviewed which is 
> something __free() does not require because it simply is better 
> interface).
> 
> You just need to remember to move also the variable declaration down to 
> the alloc site as per the guidance in cleanup.h.

Hi Ilpo,

Thanks for the feedback. I agree that using __free() is a better approach 
here to reduce complexity. I have updated the patch to use scope-based 
resource management and will send a v2 shortly.

Best regards,
Zilin Guan