linux-kernel - Re: [PATCH v2 1/2] USB: core: add a memory pool to urb for host-controller private data

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: <4f421ea5.ac18.196da1614e9.Coremail.00107082@163.com>
Date: Sat, 17 May 2025 01:13:22 +0800 (CST)
From: "David Wang" <00107082@....com>
To: "Oliver Neukum" <oneukum@...e.com>, Jes.Sorensen@...il.com
Cc: mathias.nyman@...el.com, gregkh@...uxfoundation.org,
	stern@...land.harvard.edu, linux-usb@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/2] USB: core: add a memory pool to urb for
 host-controller private data




At 2025-05-14 20:03:02, "Oliver Neukum" <oneukum@...e.com> wrote:
>
>
>On 14.05.25 13:51, David Wang wrote:
>  
>> I am not quite sure about the concern here, do you mean somebody create a urb,
>> and then usb_init_urb() here, and never use urb_destroy to release it?
>
>Yes.
>
>> 
>> That would cause memory leak if urb_destroy is not called......But is this really possible?.
>
>I think a few drivers under drivers/media do so.


I search through codes, some drivers will use usb_free_urb which would invoke urb_destroy;
But there are indeed several drivers use urb as a struct member, which is not directly kmalloced and 
should not be kfreed via usb_free_urb..... It would involve lots of changes.....

On the bright side, when I made the code check, I notice something off:
in drivers/net/wireless/realtek/rtl8xxxu/core.c


5168                 usb_free_urb(&tx_urb->urb);
5868                 usb_free_urb(&rx_urb->urb);
5890                 usb_free_urb(&rx_urb->urb);
5938                         usb_free_urb(&rx_urb->urb);

usb_free_urb would kfree the urb pointer, which would be wrong here since rx_urb and tx_urb defined 
in drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
1944 struct rtl8xxxu_rx_urb {
1945         struct urb urb;
1946         struct ieee80211_hw *hw;
1947         struct list_head list;
1948 };
1949 
1950 struct rtl8xxxu_tx_urb {
1951         struct urb urb;
1952         struct ieee80211_hw *hw;
1953         struct list_head list;
1954 };


Hi, Jes

Would you take a look? I feel usb_free_urb needs a pointer which is allokedd directly, but I would be wrong.....


Thanks
David
>
>	Regards
>		Oliver