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

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

[Abdun Nihaal]

In hfcsusb_probe(), the memory allocated for ctrl_urb gets leaked when
setup_instance() fails with an error code. Fix that by freeing the urb
before freeing the hw structure.

Fixes: 69f52adb2d53 ("mISDN: Add HFC USB driver")
Signed-off-by: Abdun Nihaal <nihaal@cse.iitm.ac.in>
---
 drivers/isdn/hardware/mISDN/hfcsusb.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/isdn/hardware/mISDN/hfcsusb.c b/drivers/isdn/hardware/mISDN/hfcsusb.c
index e54419a4e731..378d0c92622b 100644
--- a/drivers/isdn/hardware/mISDN/hfcsusb.c
+++ b/drivers/isdn/hardware/mISDN/hfcsusb.c
@@ -1904,7 +1904,6 @@ setup_instance(struct hfcsusb *hw, struct device *parent)
 	mISDN_freebchannel(&hw->bch[1]);
 	mISDN_freebchannel(&hw->bch[0]);
 	mISDN_freedchannel(&hw->dch);
-	kfree(hw);
 	return err;
 }
 
@@ -2109,8 +2108,11 @@ hfcsusb_probe(struct usb_interface *intf, const struct usb_device_id *id)
 		hw->name, __func__, driver_info->vend_name,
 		conf_str[small_match], ifnum, alt_used);
 
-	if (setup_instance(hw, dev->dev.parent))
+	if (setup_instance(hw, dev->dev.parent)) {
+		usb_free_urb(hw->ctrl_urb);
+		kfree(hw);
 		return -EIO;
+	}
 
 	hw->intf = intf;
 	usb_set_intfdata(hw->intf, hw);
-- 
2.43.0

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

[Simon Horman]

On Fri, Oct 24, 2025 at 11:04:55PM +0530, Abdun Nihaal wrote:
> In hfcsusb_probe(), the memory allocated for ctrl_urb gets leaked when
> setup_instance() fails with an error code. Fix that by freeing the urb
> before freeing the hw structure.
> 
> Fixes: 69f52adb2d53 ("mISDN: Add HFC USB driver")
> Signed-off-by: Abdun Nihaal <nihaal@cse.iitm.ac.in>

Thanks,

I agree that this is a bug, and that it was introduced in the cited commit.

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.

> ---
>  drivers/isdn/hardware/mISDN/hfcsusb.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/isdn/hardware/mISDN/hfcsusb.c b/drivers/isdn/hardware/mISDN/hfcsusb.c

...

> @@ -2109,8 +2108,11 @@ hfcsusb_probe(struct usb_interface *intf, const struct usb_device_id *id)
>  		hw->name, __func__, driver_info->vend_name,
>  		conf_str[small_match], ifnum, alt_used);
>  
> -	if (setup_instance(hw, dev->dev.parent))
> +	if (setup_instance(hw, dev->dev.parent)) {
> +		usb_free_urb(hw->ctrl_urb);
> +		kfree(hw);
>  		return -EIO;
> +	}
>  
>  	hw->intf = intf;
>  	usb_set_intfdata(hw->intf, hw);

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;
 }
 
 /* function called when an active device is removed */

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

[Abdun Nihaal]

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.

> 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.

Regards,
Nihaal

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

[Simon Horman]

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.

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

[Abdun Nihaal]

On Wed, Oct 29, 2025 at 04:45:06PM +0000, Simon Horman wrote:
> 
> Insist is a strong word. But that is my preference.
> Because even for two allocations this is the preferred style
> for Networking code.

Sure I'll update and send a v2.