netdev - 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: <f2xnihnjrvh6qqqqtqev6zx47pjhxd5kpgdahibdsgtg7ran2d@z6yerx5rddsr>
Date: Tue, 28 Oct 2025 22:08:44 +0530
From: Abdun Nihaal <nihaal@....iitm.ac.in>
To: Simon Horman <horms@...nel.org>
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 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