netdev - Re: [PATCH net] net: qmi_wwan: fix Oops while disconnecting

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:	Fri, 22 Jun 2012 20:45:12 +0800
From:	Ming Lei <tom.leiming@...il.com>
To:	Bjørn Mork <bjorn@...k.no>
Cc:	netdev@...r.kernel.org, linux-usb@...r.kernel.org,
	Marius Bjørnstad Kotsbak 
	<marius.kotsbak@...il.com>
Subject: Re: [PATCH net] net: qmi_wwan: fix Oops while disconnecting

On Fri, Jun 22, 2012 at 5:11 PM, Bjørn Mork <bjorn@...k.no> wrote:
> usbnet_disconnect() will set intfdata to NULL before calling
> the minidriver unbind function.  The cdc_wdm subdriver cannot
> know that it is disconnecting until the qmi_wwan unbind
> function has called its disconnect function.  This means that
> we must be able to support the cdc_wdm subdriver operating
> normally while usbnet_disconnect() is running, and in
> particular that intfdata may be NULL.
>
> The only place this matters is in qmi_wwan_cdc_wdm_manage_power
> which is called from cdc_wdm.  Simply testing for NULL
> intfdata there is sufficient to allow it to continue working
> at all times.
>
> Fixes this Oops where a cdc-wdm device was closed while the
> USB device was disconnecting, causing wdm_release to call
> qmi_wwan_cdc_wdm_manage_power after intfdata was set to
> NULL by usbnet_disconnect:

In fact, it should be a general problem in usbnet, there are races
between usbnet_disconnect and .open/.close. Considered that
unregister_netdev, which will sync with .open/.close,  is called in
usbnet_disconnect,  the simplest fix is to move usb_set_intfdata(NULL)
after unregister_netdev.

> Reported-by: Marius Bjørnstad Kotsbak <marius.kotsbak@...il.com>
> Cc: <stable@...r.kernel.org> # v3.4
> Signed-off-by: Bjørn Mork <bjorn@...k.no>
> ---
>  drivers/net/usb/qmi_wwan.c |    4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c
> index 3767a12..b01960f 100644
> --- a/drivers/net/usb/qmi_wwan.c
> +++ b/drivers/net/usb/qmi_wwan.c
> @@ -197,6 +197,10 @@ err:
>  static int qmi_wwan_cdc_wdm_manage_power(struct usb_interface *intf, int on)
>  {
>        struct usbnet *dev = usb_get_intfdata(intf);
> +
> +       /* can be called while disconnecting */
> +       if (!dev)
> +               return 0;
>        return qmi_wwan_manage_power(dev, on);
>  }

Considered that usb_set_intfdata(NULL) will be called after
executing .disconnect in unbind path, it can be removed simply.

So how about the below?

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index e92c057..2eb9e1e 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -1286,7 +1286,6 @@ void usbnet_disconnect (struct usb_interface *intf)
 	struct net_device	*net;

 	dev = usb_get_intfdata(intf);
-	usb_set_intfdata(intf, NULL);
 	if (!dev)
 		return;


Thanks,
-- 
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html