From patchwork Thu May 4 09:12:33 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Xinming Hu X-Patchwork-Id: 9711123 X-Patchwork-Delegate: kvalo@adurom.com Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 0D62860387 for ; Thu, 4 May 2017 09:14:59 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id ECBAE28652 for ; Thu, 4 May 2017 09:14:58 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id DEACC28657; Thu, 4 May 2017 09:14:58 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 4114728652 for ; Thu, 4 May 2017 09:14:58 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932131AbdEDJOJ (ORCPT ); Thu, 4 May 2017 05:14:09 -0400 Received: from mx0b-0016f401.pphosted.com ([67.231.156.173]:38683 "EHLO mx0b-0016f401.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932090AbdEDJMt (ORCPT ); Thu, 4 May 2017 05:12:49 -0400 Received: from pps.filterd (m0045851.ppops.net [127.0.0.1]) by mx0b-0016f401.pphosted.com (8.16.0.21/8.16.0.21) with SMTP id v44900iv018556; Thu, 4 May 2017 02:12:35 -0700 Received: from sc-exch02.marvell.com ([199.233.58.182]) by mx0b-0016f401.pphosted.com with ESMTP id 2a72kuqm4w-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-SHA384 bits=256 verify=NOT); Thu, 04 May 2017 02:12:35 -0700 Received: from SC-EXCH02.marvell.com (10.93.176.82) by SC-EXCH02.marvell.com (10.93.176.82) with Microsoft SMTP Server (TLS) id 15.0.1210.3; Thu, 4 May 2017 02:12:33 -0700 Received: from SC-EXCH02.marvell.com ([fe80::980b:63cf:b3c0:7c95]) by SC-EXCH02.marvell.com ([fe80::980b:63cf:b3c0:7c95%20]) with mapi id 15.00.1210.000; Thu, 4 May 2017 02:12:33 -0700 From: Xinming Hu To: Arend Van Spriel , Xinming Hu , Linux Wireless CC: Kalle Valo , Brian Norris , Dmitry Torokhov , "rajatja@google.com" , Zhiyuan Yang , Cathy Luo Subject: RE: Re: [PATCH 2/6] mwifiex: usb: urb->context sanity check in complete handler Thread-Topic: Re: [PATCH 2/6] mwifiex: usb: urb->context sanity check in complete handler Thread-Index: AQHSxLaQnDzjn1j6o0+MZq0bfIDA7w== Date: Thu, 4 May 2017 09:12:33 +0000 Message-ID: <75f211ce26f441a0949b8e688c82d90d@SC-EXCH02.marvell.com> References: <1493812123-12053-1-git-send-email-huxinming820@gmail.com> <1493812123-12053-2-git-send-email-huxinming820@gmail.com> In-Reply-To: Accept-Language: en-US, zh-CN Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ms-exchange-transport-fromentityheader: Hosted x-originating-ip: [10.93.176.43] MIME-Version: 1.0 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:, , definitions=2017-05-04_06:, , signatures=0 X-Proofpoint-Details: rule=outbound_notspam policy=outbound score=0 priorityscore=1501 malwarescore=0 suspectscore=0 phishscore=0 bulkscore=0 spamscore=0 clxscore=1011 lowpriorityscore=0 impostorscore=0 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1703280000 definitions=main-1705040150 Sender: linux-wireless-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-wireless@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 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 > > > > urb/context might be freed in cornel case, add sanity check to avoid > > use-after-free. > > > > Signed-off-by: Xinming Hu > > --- > > 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 --- 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; }