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

[Ayaan Mirza Baig <ayaanmirzabaig85@gmail.com>]

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
>