linux-kernel - Re: [PATCH net] isdn: mISDN: hfcsusb: fix memory leak in hfcsusb

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aQC333bzOkMNOFAG@horms.kernel.org>
Date: Tue, 28 Oct 2025 12:32:31 +0000
From: Simon Horman <horms@...nel.org>
To: Abdun Nihaal <nihaal@....iitm.ac.in>
Cc: isdn@...ux-pingi.de, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH net] isdn: mISDN: hfcsusb: fix memory leak in
 hfcsusb_probe()

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