diff mbox

Re: [PATCH 2/6] mwifiex: usb: urb->context sanity check in complete handler

Message ID 75f211ce26f441a0949b8e688c82d90d@SC-EXCH02.marvell.com (mailing list archive)
State Not Applicable
Delegated to: Kalle Valo
Headers show

Commit Message

Xinming Hu May 4, 2017, 9:12 a.m. UTC 
Hi,

> -----Original Message-----

> From: Arend Van Spriel [mailto:arend.vanspriel@broadcom.com]

> Sent: 2017年5月4日 2:52

> To: Xinming Hu; Linux Wireless

> Cc: Kalle Valo; Brian Norris; Dmitry Torokhov; rajatja@google.com; Zhiyuan

> Yang; Cathy Luo; Xinming Hu

> Subject: [EXT] Re: [PATCH 2/6] mwifiex: usb: urb->context sanity check in

> complete handler

> 

> External Email

> 

> ----------------------------------------------------------------------

> On 3-5-2017 13:48, Xinming Hu wrote:

> > From: Xinming Hu <huxm@marvell.com>

> >

> > urb/context might be freed in cornel case, add sanity check to avoid

> > use-after-free.

> >

> > Signed-off-by: Xinming Hu <huxm@marvell.com>

> > ---

> >  drivers/net/wireless/marvell/mwifiex/usb.c | 10 ++++++++++

> >  1 file changed, 10 insertions(+)

> >

> > diff --git a/drivers/net/wireless/marvell/mwifiex/usb.c

> > b/drivers/net/wireless/marvell/mwifiex/usb.c

> > index 2f7705c..ee5f488 100644

> > --- a/drivers/net/wireless/marvell/mwifiex/usb.c

> > +++ b/drivers/net/wireless/marvell/mwifiex/usb.c

> > @@ -169,6 +169,11 @@ static void mwifiex_usb_rx_complete(struct urb

> *urb)

> >  	int recv_length = urb->actual_length;

> >  	int size, status;

> >

> > +	if (!urb || !urb->context) {

> > +		pr_err("URB or URB context is not valid in USB Rx complete\n");

> > +		return;

> > +	}

> 

> Something is really off if you need !urb here. Furthermore, you are already

> initializing stack variables using fields inside the urb before this check causing a

> null-deref so it is bogus anyway. The completion function is a member in struct

> urb. If your driver has a list of these urb's somewhere and they are free in

> parallel then your design is wrong and this does not solve the use-after-free.

> The urb here will not be NULL and still points to the old memory location as it

> was handed to the usb subsystem. That piece of memory might already been

> handed out to some other piece of the kernel making any access to it invalid

> and potentially crashing the kernel.

> 


Thanks for the review. The cornel case happens when device firmware crash,
previous submitted urb will be holding in usbcore, and given back to device driver
when device disconnected. We actually need kill the holding urb before free its memory.


Will upgrade this fix in V2.

Regards,
Simo

> Regards,

> Arend
diff mbox

Patch

--- a/drivers/net/wireless/marvell/mwifiex/usb.c
+++ b/drivers/net/wireless/marvell/mwifiex/usb.c
@@ -373,6 +373,7 @@  static void mwifiex_usb_free(struct usb_card_rec *card)
        for (i = 0; i < MWIFIEX_TX_DATA_PORT; i++) {
                port = &card->port[i];
                for (j = 0; j < MWIFIEX_TX_DATA_URB; j++) {
+                       usb_kill_urb(port->tx_data_list[j].urb);
                        usb_free_urb(port->tx_data_list[j].urb);
                        port->tx_data_list[j].urb = NULL;
                }