[PATCH] edac: versalnet: Use kasprintf() to simplify string allocation.

[PATCH] edac: versalnet: Use kasprintf() to simplify string allocation.

[Ayaan Mirza Baig]

Replace the kmalloc() + sprintf() pattern with a single call
to kasprintf(). This is cleaner, simpler, and avoids potential
buffer overflows from the fixed-size 32-byte allocation.

Signed-off-by: Ayaan Mirza Baig <ayaanmirzabaig85@gmail.com>
---
 drivers/edac/versalnet_edac.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/edac/versalnet_edac.c b/drivers/edac/versalnet_edac.c
index 1ded4c3f0213..c79509b0a464 100644
--- a/drivers/edac/versalnet_edac.c
+++ b/drivers/edac/versalnet_edac.c
@@ -812,8 +812,7 @@ static int init_versalnet(struct mc_priv *priv, struct platform_device *pdev)
 
 		dev = kzalloc(sizeof(*dev), GFP_KERNEL);
 		dev->release = versal_edac_release;
-		name = kmalloc(32, GFP_KERNEL);
-		sprintf(name, "versal-net-ddrmc5-edac-%d", i);
+		name = kasprintf(GFP_KERNEL, "versal-net-ddrmc5-edac-%d", i);
 		dev->init_name = name;
 		rc = device_register(dev);
 		if (rc)
-- 
2.51.0

Re: [PATCH] edac: versalnet: Use kasprintf() to simplify string allocation.

[Borislav Petkov]

On Sun, Nov 16, 2025 at 01:26:53AM +0530, Ayaan Mirza Baig wrote:
> Replace the kmalloc() + sprintf() pattern with a single call
> to kasprintf(). This is cleaner, simpler, and avoids potential
> buffer overflows from the fixed-size 32-byte allocation.
> 
> Signed-off-by: Ayaan Mirza Baig <ayaanmirzabaig85@gmail.com>
> ---
>  drivers/edac/versalnet_edac.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/edac/versalnet_edac.c b/drivers/edac/versalnet_edac.c
> index 1ded4c3f0213..c79509b0a464 100644
> --- a/drivers/edac/versalnet_edac.c
> +++ b/drivers/edac/versalnet_edac.c
> @@ -812,8 +812,7 @@ static int init_versalnet(struct mc_priv *priv, struct platform_device *pdev)
>  
>  		dev = kzalloc(sizeof(*dev), GFP_KERNEL);
>  		dev->release = versal_edac_release;
> -		name = kmalloc(32, GFP_KERNEL);
> -		sprintf(name, "versal-net-ddrmc5-edac-%d", i);
> +		name = kasprintf(GFP_KERNEL, "versal-net-ddrmc5-edac-%d", i);

I like that.

However, kasprintf() can return NULL so you need to handle that. I know,
I know, the original code doesn't do it either and yes, we have been working
towards cleaning up that whole area. But pls fix that while you're at it.

Also, Shubhrajyoti, that ->init_name needs to be freed somewhere on the
destroy path... remove_versalnet() perhaps which does device_unregister()...

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

[PATCH v2] edac: versalnet: Use kasprintf() to simplify string allocation.

[Ayaan Mirza Baig]

Replace the kmalloc() + sprintf() pattern with a single call
to kasprintf(). This is cleaner, simpler, and avoids potential
buffer overflows from the fixed-size 32-byte allocation.
Handle the potential NULL return from kasprintf().

Signed-off-by: Ayaan Mirza Baig <ayaanmirzabaig85@gmail.com>

v2:
 - Add NULL check for kasprintf() as requested by reviewer.
---
 drivers/edac/versalnet_edac.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/edac/versalnet_edac.c b/drivers/edac/versalnet_edac.c
index 1ded4c3f0213..69a9da8c58a3 100644
--- a/drivers/edac/versalnet_edac.c
+++ b/drivers/edac/versalnet_edac.c
@@ -812,8 +812,12 @@ static int init_versalnet(struct mc_priv *priv, struct platform_device *pdev)
 
 		dev = kzalloc(sizeof(*dev), GFP_KERNEL);
 		dev->release = versal_edac_release;
-		name = kmalloc(32, GFP_KERNEL);
-		sprintf(name, "versal-net-ddrmc5-edac-%d", i);
+		name = kasprintf(GFP_KERNEL, "versal-net-ddrmc5-edac-%d", i);
+		if (!name) {
+			kfree(dev);
+			return -ENOMEM;
+		}
+
 		dev->init_name = name;
 		rc = device_register(dev);
 		if (rc)
-- 
2.51.0

RE: [PATCH v2] edac: versalnet: Use kasprintf() to simplify string allocation.

[Zhuo, Qiuxu]

> From: Ayaan Mirza Baig <ayaanmirzabaig85@gmail.com>
> Sent: Monday, November 17, 2025 3:59 AM
> To: bp@alien8.de
> Cc: shubhrajyoti.datta@amd.com; linux-edac@vger.kernel.org; Ayaan Mirza
> Baig <ayaanmirzabaig85@gmail.com>
> Subject: [PATCH v2] edac: versalnet: Use kasprintf() to simplify string
> allocation.
> 
> Replace the kmalloc() + sprintf() pattern with a single call to kasprintf(). This is
> cleaner, simpler, and avoids potential buffer overflows from the fixed-size 32-
> byte allocation.
> Handle the potential NULL return from kasprintf().
> 
> Signed-off-by: Ayaan Mirza Baig <ayaanmirzabaig85@gmail.com>
> 
> v2:
>  - Add NULL check for kasprintf() as requested by reviewer.
> ---
>  drivers/edac/versalnet_edac.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/edac/versalnet_edac.c b/drivers/edac/versalnet_edac.c
> index 1ded4c3f0213..69a9da8c58a3 100644
> --- a/drivers/edac/versalnet_edac.c
> +++ b/drivers/edac/versalnet_edac.c
> @@ -812,8 +812,12 @@ static int init_versalnet(struct mc_priv *priv, struct
> platform_device *pdev)
> 
>  		dev = kzalloc(sizeof(*dev), GFP_KERNEL);
>  		dev->release = versal_edac_release;
> -		name = kmalloc(32, GFP_KERNEL);
> -		sprintf(name, "versal-net-ddrmc5-edac-%d", i);
> +		name = kasprintf(GFP_KERNEL, "versal-net-ddrmc5-edac-%d",
> i);
> +		if (!name) {
> +			kfree(dev);
> +			return -ENOMEM;

As Boris previously suggested:

  a) On this error, it needs to go through the destroy path to clean up what has been
allocated. 

  b) It also needs to add code to free "dev->init_name" in the destroy path properly,
which was also missed in the original implementation.

> +		}
> +
>  		dev->init_name = name;
>  		rc = device_register(dev);
>  		if (rc)
> --
> 2.51.0
> 

[PATCH v3] edac: versalnet: Use kasprintf() to simplify string allocation and fix error paths.

[Ayaan Mirza Baig]

Replace the kmalloc() + sprintf() pattern with a single call
to kasprintf(). This is cleaner, simpler, and avoids potential
buffer overflows from the fixed-size 32-byte allocation.
Handle possible NULL return from kasprintf() on allocation
failure and ensure proper cleanup on error paths.

Also free dev->init_name in the device release function
to avoid leak on normal removal.

Signed-off-by: Ayaan Mirza Baig <ayaanmirzabaig85@gmail.com>

v2:
 - Add NULL check for kasprintf() as requested by reviewer.

v3:
 - Free dev->init_name in versal_edac_release() to fix the existing leak.
---
 drivers/edac/versalnet_edac.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/edac/versalnet_edac.c b/drivers/edac/versalnet_edac.c
index 1ded4c3f0213..360d4f83ed89 100644
--- a/drivers/edac/versalnet_edac.c
+++ b/drivers/edac/versalnet_edac.c
@@ -15,6 +15,7 @@
 #include <ras/ras_event.h>
 
 #include "edac_module.h"
+#include "../../include/linux/device.h"
 
 /* Granularity of reported error in bytes */
 #define MC5_ERR_GRAIN			1
@@ -755,6 +756,7 @@ static struct rpmsg_driver amd_rpmsg_driver = {
 
 static void versal_edac_release(struct device *dev)
 {
+	kfree(dev->init_name);
 	kfree(dev);
 }
 
@@ -812,12 +814,19 @@ static int init_versalnet(struct mc_priv *priv, struct platform_device *pdev)
 
 		dev = kzalloc(sizeof(*dev), GFP_KERNEL);
 		dev->release = versal_edac_release;
-		name = kmalloc(32, GFP_KERNEL);
-		sprintf(name, "versal-net-ddrmc5-edac-%d", i);
+		name = kasprintf(GFP_KERNEL, "versal-net-ddrmc5-edac-%d", i);
+		if (!name) {
+			kfree(dev);
+			return -ENOMEM;
+		}
+
 		dev->init_name = name;
 		rc = device_register(dev);
-		if (rc)
+		if (rc) {
+			kfree(dev->init_name);
+			kfree(dev);
 			goto err_alloc;
+		}
 
 		mci->pdev = dev;
 
-- 
2.51.0

RE: [PATCH v3] edac: versalnet: Use kasprintf() to simplify string allocation and fix error paths.

[Zhuo, Qiuxu]

> From: Ayaan Mirza Baig <ayaanmirzabaig85@gmail.com>
> Sent: Monday, November 17, 2025 7:02 PM
> To: Zhuo, Qiuxu <qiuxu.zhuo@intel.com>
> Cc: ayaanmirzabaig85@gmail.com; bp@alien8.de; linux-
> edac@vger.kernel.org; shubhrajyoti.datta@amd.com
> Subject: [PATCH v3] edac: versalnet: Use kasprintf() to simplify string
> allocation and fix error paths.
> 
> Replace the kmalloc() + sprintf() pattern with a single call to kasprintf(). This is
> cleaner, simpler, and avoids potential buffer overflows from the fixed-size 32-
> byte allocation.
> Handle possible NULL return from kasprintf() on allocation failure and ensure
> proper cleanup on error paths.
> 
> Also free dev->init_name in the device release function to avoid leak on
> normal removal.
> 
> Signed-off-by: Ayaan Mirza Baig <ayaanmirzabaig85@gmail.com>
> 
> v2:
>  - Add NULL check for kasprintf() as requested by reviewer.
> 
> v3:
>  - Free dev->init_name in versal_edac_release() to fix the existing leak.
> ---
>  drivers/edac/versalnet_edac.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/edac/versalnet_edac.c b/drivers/edac/versalnet_edac.c
> index 1ded4c3f0213..360d4f83ed89 100644
> --- a/drivers/edac/versalnet_edac.c
> +++ b/drivers/edac/versalnet_edac.c
> @@ -15,6 +15,7 @@
>  #include <ras/ras_event.h>
> 
>  #include "edac_module.h"
> +#include "../../include/linux/device.h"
> 
>  /* Granularity of reported error in bytes */
>  #define MC5_ERR_GRAIN			1
> @@ -755,6 +756,7 @@ static struct rpmsg_driver amd_rpmsg_driver = {
> 
>  static void versal_edac_release(struct device *dev)  {
> +	kfree(dev->init_name);
>  	kfree(dev);
>  }
> 
> @@ -812,12 +814,19 @@ static int init_versalnet(struct mc_priv *priv, struct
> platform_device *pdev)
> 
>  		dev = kzalloc(sizeof(*dev), GFP_KERNEL);
>  		dev->release = versal_edac_release;
> -		name = kmalloc(32, GFP_KERNEL);
> -		sprintf(name, "versal-net-ddrmc5-edac-%d", i);
> +		name = kasprintf(GFP_KERNEL, "versal-net-ddrmc5-edac-%d",
> i);
> +		if (!name) {
> +			kfree(dev);
> +			return -ENOMEM;

On this failure, I think it should "goto err_alloc;" to free the allocated mci instances 
instead of directly returning -ENOMEM. 

> +		}
> +
>  		dev->init_name = name;
>  		rc = device_register(dev);
> -		if (rc)
> +		if (rc) {
> +			kfree(dev->init_name);
> +			kfree(dev);
>  			goto err_alloc;
> +		}
> 
>  		mci->pdev = dev;
> 
> --
> 2.51.0

Re: [PATCH v3] edac: versalnet: Use kasprintf() to simplify string allocation and fix error paths.

[Ayaan Mirza Baig]

On Tue, Nov 18, 2025 at 02:02:01PM +0000, Zhuo, Qiuxu wrote:
> > From: Ayaan Mirza Baig <ayaanmirzabaig85@gmail.com>
> > Sent: Monday, November 17, 2025 7:02 PM
> > To: Zhuo, Qiuxu <qiuxu.zhuo@intel.com>
> > Cc: ayaanmirzabaig85@gmail.com; bp@alien8.de; linux-
> > edac@vger.kernel.org; shubhrajyoti.datta@amd.com
> > Subject: [PATCH v3] edac: versalnet: Use kasprintf() to simplify string
> > allocation and fix error paths.
> > 
> > Replace the kmalloc() + sprintf() pattern with a single call to kasprintf(). This is
> > cleaner, simpler, and avoids potential buffer overflows from the fixed-size 32-
> > byte allocation.
> > Handle possible NULL return from kasprintf() on allocation failure and ensure
> > proper cleanup on error paths.
> > 
> > Also free dev->init_name in the device release function to avoid leak on
> > normal removal.
> > 
> > Signed-off-by: Ayaan Mirza Baig <ayaanmirzabaig85@gmail.com>
> > 
> > v2:
> >  - Add NULL check for kasprintf() as requested by reviewer.
> > 
> > v3:
> >  - Free dev->init_name in versal_edac_release() to fix the existing leak.
> > ---
> >  drivers/edac/versalnet_edac.c | 15 ++++++++++++---
> >  1 file changed, 12 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/edac/versalnet_edac.c b/drivers/edac/versalnet_edac.c
> > index 1ded4c3f0213..360d4f83ed89 100644
> > --- a/drivers/edac/versalnet_edac.c
> > +++ b/drivers/edac/versalnet_edac.c
> > @@ -15,6 +15,7 @@
> >  #include <ras/ras_event.h>
> > 
> >  #include "edac_module.h"
> > +#include "../../include/linux/device.h"
> > 
> >  /* Granularity of reported error in bytes */
> >  #define MC5_ERR_GRAIN			1
> > @@ -755,6 +756,7 @@ static struct rpmsg_driver amd_rpmsg_driver = {
> > 
> >  static void versal_edac_release(struct device *dev)  {
> > +	kfree(dev->init_name);
> >  	kfree(dev);
> >  }
> > 
> > @@ -812,12 +814,19 @@ static int init_versalnet(struct mc_priv *priv, struct
> > platform_device *pdev)
> > 
> >  		dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> >  		dev->release = versal_edac_release;
> > -		name = kmalloc(32, GFP_KERNEL);
> > -		sprintf(name, "versal-net-ddrmc5-edac-%d", i);
> > +		name = kasprintf(GFP_KERNEL, "versal-net-ddrmc5-edac-%d",
> > i);
> > +		if (!name) {
> > +			kfree(dev);
> > +			return -ENOMEM;
> 
> On this failure, I think it should "goto err_alloc;" to free the allocated mci instances 
> instead of directly returning -ENOMEM. 

Okay, thanks. I did overlook this, my bad. I apologize for these mistakes I keep
making again and again. I'll make sure there are no more errors in v4.

> 
> > +		}
> > +
> >  		dev->init_name = name;
> >  		rc = device_register(dev);
> > -		if (rc)
> > +		if (rc) {
> > +			kfree(dev->init_name);
> > +			kfree(dev);
> >  			goto err_alloc;
> > +		}
> > 
> >  		mci->pdev = dev;
> > 
> > --
> > 2.51.0
> 

Re: [PATCH v3] edac: versalnet: Use kasprintf() to simplify string allocation and fix error paths.

[Borislav Petkov]

On Wed, Nov 19, 2025 at 12:16:22AM +0530, Ayaan Mirza Baig wrote:
> Okay, thanks. I did overlook this, my bad. I apologize for these mistakes
> I keep making again and again. I'll make sure there are no more errors in
> v4.

Don't worry about the mistakes - if you make them, you learn the best this
way. :-)

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette