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

[Simon Horman <horms@kernel.org>]

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 */