linux-kernel - Re: [PATCH] usb: hub: Fix unbalanced reference count and memory leak

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]

Date:	Thu, 28 Jul 2016 10:13:12 -0400 (EDT)
From:	Alan Stern <stern@...land.harvard.edu>
To:	Viresh Kumar <viresh.kumar@...aro.org>
cc:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	<linaro-kernel@...ts.linaro.org>, Alex Elder <elder@...aro.org>,
	Johan Hovold <johan@...oldconsulting.com>,
	"#4 . 4+" <stable@...r.kernel.org>, <linux-usb@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] usb: hub: Fix unbalanced reference count and memory leak

On Wed, 27 Jul 2016, Viresh Kumar wrote:

> If the hub gets disconnected while the core is still activating it, this
> can result in leaking memory of few USB structures.
> 
> This will happen if we have done a kref_get() from hub_activate() and
> scheduled a delayed work item for HUB_INIT2/3. Now if hub_disconnect()
> gets called before the delayed work expires, then we will cancel the
> work from hub_quiesce(), but wouldn't do a kref_put(). And so the
> unbalance.
> 
> kmemleak reports this as (with the commit e50293ef9775 backported to
> 3.10 kernel with other changes, though the same is true for mainline as
> well):

...

> Fix this by putting the reference in hub_quiesce() if we canceled a
> pending work.
> 
> CC: <stable@...r.kernel.org> #4.4+
> Fixes: e50293ef9775 ("USB: fix invalid memory access in hub_activate()")
> Signed-off-by: Viresh Kumar <viresh.kumar@...aro.org>
> ---
> Greg,
> 
> This is tested over 3.10 with backported patches only, sorry didn't had
> a mainline setup to test this out. :(

Arg.  This is exactly the sort of thing I should have foreseen when 
writing the earlier commit.

>  drivers/usb/core/hub.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index bee13517676f..3173693fa8e3 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -1315,7 +1315,8 @@ static void hub_quiesce(struct usb_hub *hub, enum hub_quiescing_type type)
>  	struct usb_device *hdev = hub->hdev;
>  	int i;
>  
> -	cancel_delayed_work_sync(&hub->init_work);
> +	if (cancel_delayed_work_sync(&hub->init_work))
> +		kref_put(&hub->kref, hub_release);
>  
>  	/* hub_wq and related activity won't re-trigger */
>  	hub->quiescing = 1;

Another possibility is to remove the cancel_delayed_work_sync call 
entirely.  Either way, you can add

Acked-by: Alan Stern <stern@...land.harvard.edu>

Alan Stern