pds_core: Fix possible double free in error handling path

PATCH Superseded hyper 6 participants 10 messages Mar 1, 2024 – Mar 7, 2024

Subsystems (3)

Networking Network drivers AMD pds core driver

lore

Share & tools

lore

hyper

author

2024-03-01 02:23 UTC #0 lore raw

PATCH 1/1 pds_core: Fix possible double free in error handling path

When auxiliary_device_add() returns error and then calls auxiliary_device_uninit(), Callback function pdsc_auxbus_dev_release calls kfree(padev) to free memory. We shouldn't call kfree(padev) again in the error handling path.

Fix this by returning error directly after calling auxiliary_device_uninit().

Fixes 4569cce43bc6 ("pds_core: add auxiliary_bus devices")

Signed-off-by hyper <[email protected]>

---
 drivers/net/ethernet/amd/pds_core/auxbus.c | 2 ++
 1 file changed, 2 insertions(+)

drivers/net/ethernet/amd/pds_core/auxbus.c +2 -0

index 11c23a7f3172..d6eedd78d5cc 100644
@@ -174,6 +174,8 @@ static struct pds_auxiliary_dev *pdsc_auxbus_dev_register(struct pdsc *cf,
 
 err_out_uninit:
 	auxiliary_device_uninit(aux_dev);
+	return ERR_PTR(err);
+
 err_out:
 	kfree(padev);
 	return ERR_PTR(err);

Signature · 1 line

2.36.1

Nelson, Shannon

2024-03-01 17:55 UTC #1 lore raw

Quoted 1 line · hyper

Please specify the networking tree in your patch subject, something like [PATCH net] in this case.

Quoted 29 lines

When auxiliary_device_add() returns error and then calls auxiliary_device_uninit(), Callback function pdsc_auxbus_dev_release calls kfree(padev) to free memory. We shouldn't call kfree(padev) again in the error handling path.
Fix this by returning error directly after calling auxiliary_device_uninit().
Fixes 4569cce43bc6 ("pds_core: add auxiliary_bus devices")
Signed-off-by hyper <[email protected]>
---
  drivers/net/ethernet/amd/pds_core/auxbus.c | 2 ++
  1 file changed, 2 insertions(+)
drivers/net/ethernet/amd/pds_core/auxbus.c +2 -1
index 11c23a7f3172..d6eedd78d5cc 100644
@@ -174,6 +174,8 @@ static struct pds_auxiliary_dev *pdsc_auxbus_dev_register(struct pdsc *cf,
 
  err_out_uninit:
         auxiliary_device_uninit(aux_dev);
+       return ERR_PTR(err);
+
  err_out:
         kfree(padev);
         return ERR_PTR(err);
--

Quoted 2 lines

2.36.1

Yes, I think you've got the right idea here, and this is probably a reasonable solution.

However, usually the error handling exit code stacks on itself, but here it becomes two separate independent chunks - a slightly different pattern. Since these are both very short bits I'd be tempted to "enhance" that independence by putting the error handling back to where the errors happened, something like

drivers/net/ethernet/amd/pds_core/auxbus.c +0 -0

index a3c79848a69a..2babea110991 100644
@@ -160,23 +160,19 @@ static struct pds_auxiliary_dev *pdsc_auxbus_dev_register(struct pdsc *cf,
        if (err < 0) {
                dev_warn(cf->dev, "auxiliary_device_init of %s failed:

%pe\n",

                         name, ERR_PTR(err));
-               goto err_out;
+               kfree(padev);
+               return ERR_PTR(err);
        }

        err = auxiliary_device_add(aux_dev);
        if (err) {
                dev_warn(cf->dev, "auxiliary_device_add of %s failed: 
%pe\n",
                         name, ERR_PTR(err));
-               goto err_out_uninit;
+               auxiliary_device_uninit(aux_dev);
+               return ERR_PTR(err);
        }

        return padev;

-
-err_out_uninit:
- auxiliary_device_uninit(aux_dev);
-err_out:
- kfree(padev);
- return ERR_PTR(err);
}

Some might disagree. I like this a little better, but I could go either way.

Thoughts?

sln

hyper

author

2024-03-03 08:50 UTC #2 lore raw

PATCH v2 1/1 net: pds_core: Fix possible double free in error handling path

Fix this by cleaning up the redundant kfree() and putting the error handling back to where the errors happened.

Fixes 4569cce43bc6 ("pds_core: add auxiliary_bus devices")

Signed-off-by hyper <[email protected]>

---
 drivers/net/ethernet/amd/pds_core/auxbus.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

drivers/net/ethernet/amd/pds_core/auxbus.c +4 -8

index 11c23a7f3172..fd1a5149c003 100644
@@ -160,23 +160,19 @@ static struct pds_auxiliary_dev *pdsc_auxbus_dev_register(struct pdsc *cf,
 	if (err < 0) {
 		dev_warn(cf->dev, "auxiliary_device_init of %s failed: %pe\n",
 			 name, ERR_PTR(err));
-		goto err_out;
+		kfree(padev);
+		return ERR_PTR(err);
 	}
 
 	err = auxiliary_device_add(aux_dev);
 	if (err) {
 		dev_warn(cf->dev, "auxiliary_device_add of %s failed: %pe\n",
 			 name, ERR_PTR(err));
-		goto err_out_uninit;
+		auxiliary_device_uninit(aux_dev);
+		return ERR_PTR(err);
 	}
 
 	return padev;
-
-err_out_uninit:
-	auxiliary_device_uninit(aux_dev);
-err_out:
-	kfree(padev);
-	return ERR_PTR(err);
 }
 
 int pdsc_auxbus_dev_del(struct pdsc *cf, struct pdsc *pf)

Signature · 1 line

2.36.1

Nelson, Shannon

Reviewed-by on v2 1/1

2024-03-04 17:22 UTC #3 lore raw

Re: [PATCH net V2] net: pds_core: Fix possible double free in error handling path

Quoted 11 lines · hyper

When auxiliary_device_add() returns error and then calls auxiliary_device_uninit(), Callback function pdsc_auxbus_dev_release calls kfree(padev) to free memory. We shouldn't call kfree(padev) again in the error handling path.
Fix this by cleaning up the redundant kfree() and putting the error handling back to where the errors happened.
Fixes 4569cce43bc6 ("pds_core: add auxiliary_bus devices")
Signed-off-by hyper <[email protected]>

Thanks.

Reviewed-by Shannon Nelson <[email protected]>

Quoted 39 lines

---
  drivers/net/ethernet/amd/pds_core/auxbus.c | 12 ++++--------
  1 file changed, 4 insertions(+), 8 deletions(-)

drivers/net/ethernet/amd/pds_core/auxbus.c +4 -9

index 11c23a7f3172..fd1a5149c003 100644
@@ -160,23 +160,19 @@ static struct pds_auxiliary_dev *pdsc_auxbus_dev_register(struct pdsc *cf,
         if (err < 0) {
                 dev_warn(cf->dev, "auxiliary_device_init of %s failed: %pe\n",
                          name, ERR_PTR(err));
-               goto err_out;
+               kfree(padev);
+               return ERR_PTR(err);
         }
 
         err = auxiliary_device_add(aux_dev);
         if (err) {
                 dev_warn(cf->dev, "auxiliary_device_add of %s failed: %pe\n",
                          name, ERR_PTR(err));
-               goto err_out_uninit;
+               auxiliary_device_uninit(aux_dev);
+               return ERR_PTR(err);
         }
 
         return padev;
-
-err_out_uninit:
-       auxiliary_device_uninit(aux_dev);
-err_out:
-       kfree(padev);
-       return ERR_PTR(err);
  }
 
  int pdsc_auxbus_dev_del(struct pdsc *cf, struct pdsc *pf)
--

2.36.1

Breno Leitao

Reviewed-by on v2 1/1

2024-03-04 20:26 UTC #4 lore raw

Re: [PATCH net V2] net: pds_core: Fix possible double free in error handling path

In reply to hyper

Quoted 10 lines · hyper

When auxiliary_device_add() returns error and then calls auxiliary_device_uninit(), Callback function pdsc_auxbus_dev_release calls kfree(padev) to free memory. We shouldn't call kfree(padev) again in the error handling path.
Fix this by cleaning up the redundant kfree() and putting the error handling back to where the errors happened.
Fixes 4569cce43bc6 ("pds_core: add auxiliary_bus devices")
Signed-off-by hyper <[email protected]>

I liked this v2 better.

Reviewed-by Breno Leitao <[email protected]>

Paolo Abeni

maintainer

2024-03-05 14:58 UTC #5 lore raw

Re: [PATCH net V2] net: pds_core: Fix possible double free in error handling path

In reply to hyper

Quoted 10 lines · hyper

When auxiliary_device_add() returns error and then calls auxiliary_device_uninit(), Callback function pdsc_auxbus_dev_release calls kfree(padev) to free memory. We shouldn't call kfree(padev) again in the error handling path.
Fix this by cleaning up the redundant kfree() and putting the error handling back to where the errors happened.
Fixes 4569cce43bc6 ("pds_core: add auxiliary_bus devices")
Signed-off-by hyper <[email protected]>

Note that submitters are required to use real identity:

https://elixir.bootlin.com/linux/v6.8-rc7/source/Documentation/process/submitting-patches.rst#L438

Could you please repost avoiding the nick name?

You can retain the already collected acks.

Thanks,

Paolo

Yongzhi Liu

author

2024-03-06 10:57 UTC #6 lore raw

PATCH v3 1/1 net: pds_core: Fix possible double free in error handling path

Fix this by cleaning up the redundant kfree() and putting the error handling back to where the errors happened.

Fixes 4569cce43bc6 ("pds_core: add auxiliary_bus devices")

Signed-off-by Yongzhi Liu <[email protected]>

---
 drivers/net/ethernet/amd/pds_core/auxbus.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

drivers/net/ethernet/amd/pds_core/auxbus.c +4 -8

index 11c23a7f3172..fd1a5149c003 100644
@@ -160,23 +160,19 @@ static struct pds_auxiliary_dev *pdsc_auxbus_dev_register(struct pdsc *cf,
 	if (err < 0) {
 		dev_warn(cf->dev, "auxiliary_device_init of %s failed: %pe\n",
 			 name, ERR_PTR(err));
-		goto err_out;
+		kfree(padev);
+		return ERR_PTR(err);
 	}
 
 	err = auxiliary_device_add(aux_dev);
 	if (err) {
 		dev_warn(cf->dev, "auxiliary_device_add of %s failed: %pe\n",
 			 name, ERR_PTR(err));
-		goto err_out_uninit;
+		auxiliary_device_uninit(aux_dev);
+		return ERR_PTR(err);
 	}
 
 	return padev;
-
-err_out_uninit:
-	auxiliary_device_uninit(aux_dev);
-err_out:
-	kfree(padev);
-	return ERR_PTR(err);
 }
 
 int pdsc_auxbus_dev_del(struct pdsc *cf, struct pdsc *pf)

Signature · 1 line

2.36.1

Wojciech Drewek

Reviewed-by on v3 1/1

2024-03-06 13:45 UTC #7 lore raw

Re: [PATCH net V3] net: pds_core: Fix possible double free in error handling path

Quoted 11 lines · Yongzhi Liu

When auxiliary_device_add() returns error and then calls auxiliary_device_uninit(), Callback function pdsc_auxbus_dev_release calls kfree(padev) to free memory. We shouldn't call kfree(padev) again in the error handling path.
Fix this by cleaning up the redundant kfree() and putting the error handling back to where the errors happened.
Fixes 4569cce43bc6 ("pds_core: add auxiliary_bus devices")
Signed-off-by Yongzhi Liu <[email protected]>
---

Reviewed-by Wojciech Drewek <[email protected]>

Quoted 35 lines

 drivers/net/ethernet/amd/pds_core/auxbus.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

drivers/net/ethernet/amd/pds_core/auxbus.c +4 -8

index 11c23a7f3172..fd1a5149c003 100644
@@ -160,23 +160,19 @@ static struct pds_auxiliary_dev *pdsc_auxbus_dev_register(struct pdsc *cf,
 	if (err < 0) {
 		dev_warn(cf->dev, "auxiliary_device_init of %s failed: %pe\n",
 			 name, ERR_PTR(err));
-		goto err_out;
+		kfree(padev);
+		return ERR_PTR(err);
 	}
 
 	err = auxiliary_device_add(aux_dev);
 	if (err) {
 		dev_warn(cf->dev, "auxiliary_device_add of %s failed: %pe\n",
 			 name, ERR_PTR(err));
-		goto err_out_uninit;
+		auxiliary_device_uninit(aux_dev);
+		return ERR_PTR(err);
 	}
 
 	return padev;
-
-err_out_uninit:
-	auxiliary_device_uninit(aux_dev);
-err_out:
-	kfree(padev);
-	return ERR_PTR(err);
 }
 
 int pdsc_auxbus_dev_del(struct pdsc *cf, struct pdsc *pf)

Nelson, Shannon

Reviewed-by on v3 1/1

2024-03-06 17:05 UTC #8 lore raw

Re: [PATCH net V3] net: pds_core: Fix possible double free in error handling path

In reply to Yongzhi Liu

Quoted 10 lines · Yongzhi Liu

When auxiliary_device_add() returns error and then calls auxiliary_device_uninit(), Callback function pdsc_auxbus_dev_release calls kfree(padev) to free memory. We shouldn't call kfree(padev) again in the error handling path.
Fix this by cleaning up the redundant kfree() and putting the error handling back to where the errors happened.
Fixes 4569cce43bc6 ("pds_core: add auxiliary_bus devices")
Signed-off-by Yongzhi Liu <[email protected]>

Reviewed-by Shannon Nelson <[email protected]>

Quoted 39 lines

---
  drivers/net/ethernet/amd/pds_core/auxbus.c | 12 ++++--------
  1 file changed, 4 insertions(+), 8 deletions(-)

drivers/net/ethernet/amd/pds_core/auxbus.c +4 -9

index 11c23a7f3172..fd1a5149c003 100644
@@ -160,23 +160,19 @@ static struct pds_auxiliary_dev *pdsc_auxbus_dev_register(struct pdsc *cf,
         if (err < 0) {
                 dev_warn(cf->dev, "auxiliary_device_init of %s failed: %pe\n",
                          name, ERR_PTR(err));
-               goto err_out;
+               kfree(padev);
+               return ERR_PTR(err);
         }
 
         err = auxiliary_device_add(aux_dev);
         if (err) {
                 dev_warn(cf->dev, "auxiliary_device_add of %s failed: %pe\n",
                          name, ERR_PTR(err));
-               goto err_out_uninit;
+               auxiliary_device_uninit(aux_dev);
+               return ERR_PTR(err);
         }
 
         return padev;
-
-err_out_uninit:
-       auxiliary_device_uninit(aux_dev);
-err_out:
-       kfree(padev);
-       return ERR_PTR(err);
  }
 
  int pdsc_auxbus_dev_del(struct pdsc *cf, struct pdsc *pf)
--

2.36.1

[email protected]

CI Applied

2024-03-07 11:10 UTC #9 lore raw

Re: [PATCH net V3] net: pds_core: Fix possible double free in error handling path

In reply to Yongzhi Liu

This patch was applied to netdev/net.git (main) by Paolo Abeni <[email protected]>:

Hello:

This patch was applied to netdev/net.git (main)
by Paolo Abeni <[email protected]>:

Quoted 9 lines · On Wed, 6 Mar 2024 18:57:14 +0800 you wrote:

When auxiliary_device_add() returns error and then calls auxiliary_device_uninit(), Callback function pdsc_auxbus_dev_release calls kfree(padev) to free memory. We shouldn't call kfree(padev) again in the error handling path.
Fix this by cleaning up the redundant kfree() and putting the error handling back to where the errors happened.
[...]

Here is the summary with links:
- [net,V3] net: pds_core: Fix possible double free in error handling path

    https://git.kernel.org/netdev/net/c/ba18deddd6d5

You are awesome, thank you!

Signature · 2 lines

Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html