Re: [PATCH net] isdn: mISDN: hfcsusb: fix memory leak in hfcsusb_probe()

[Simon Horman <horms@kernel.org>]

On Tue, Oct 28, 2025 at 10:08:44PM +0530, Abdun Nihaal wrote:
> On Tue, Oct 28, 2025 at 12:32:31PM +0000, Simon Horman wrote:
> > I agree that this is a bug, and that it was introduced in the cited commit.
> 
> Thanks for reviewing the patch.
> 
> > I think it would be good to add something to the commit message
> > regarding how the problem was found, e.g. which tool was used, or
> > by inspection; and what testing has been done, e.g. compile tested only.
> 
> Sure I'll add that to future patches that I send.
> This issue was reported by a prototype static analysis tool.
> And it is compile tested only.

Thanks, I think the including two lines above would work well.

> 
> > I think it would be best to follow the idiomatic pattern and
> > introduce a ladder of goto statements to handle unwind on error.
> > 
> > Something like this (compile tested only!):
> > 
> > diff --git a/drivers/isdn/hardware/mISDN/hfcsusb.c b/drivers/isdn/hardware/mISDN/hfcsusb.c
> > @@ -1921,6 +1920,7 @@ hfcsusb_probe(struct usb_interface *intf, const struct usb_device_id *id)
> >  		probe_alt_setting, vend_idx, cfg_used, *vcf, attr, cfg_found,
> >  		ep_addr, cmptbl[16], small_match, iso_packet_size, packet_size,
> >  		alt_used = 0;
> > +	int err;
> >  
> >  	vend_idx = 0xffff;
> >  	for (i = 0; hfcsusb_idtab[i].idVendor; i++) {
> > @@ -2101,20 +2101,28 @@ hfcsusb_probe(struct usb_interface *intf, const struct usb_device_id *id)
> >  	if (!hw->ctrl_urb) {
> >  		pr_warn("%s: No memory for control urb\n",
> >  			driver_info->vend_name);
> > -		kfree(hw);
> > -		return -ENOMEM;
> > +		err = -ENOMEM;
> > +		goto err_free_urb;
> >  	}
> >  
> >  	pr_info("%s: %s: detected \"%s\" (%s, if=%d alt=%d)\n",
> >  		hw->name, __func__, driver_info->vend_name,
> >  		conf_str[small_match], ifnum, alt_used);
> >  
> > -	if (setup_instance(hw, dev->dev.parent))
> > -		return -EIO;
> > +	if (setup_instance(hw, dev->dev.parent)) {
> > +		err = -EIO;
> > +		goto err_free_hw;
> > +	}
> >  
> >  	hw->intf = intf;
> >  	usb_set_intfdata(hw->intf, hw);
> >  	return 0;
> > +
> > +err_free_urb:
> > +	usb_free_urb(hw->ctrl_urb);
> > +err_free_hw:
> > +	kfree(hw);
> > +	return err;
> >  }
> 
> In this case, since there are only two memory allocations and only two
> error paths involved, I would prefer keeping the frees in place. But I
> agree that for longer error paths the ladder of gotos would be better.
> 
> Let me know. I can send an updated patch in the goto-ladder style, if
> you insist.

Insist is a strong word. But that is my preference.
Because even for two allocations this is the preferred style
for Networking code.