From patchwork Fri Oct 16 21:59:25 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Doug Anderson X-Patchwork-Id: 7421101 Return-Path: X-Original-To: patchwork-linux-rockchip@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id A390C9F37F for ; Fri, 16 Oct 2015 22:00:17 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 7E527207E8 for ; Fri, 16 Oct 2015 22:00:15 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.9]) (using TLSv1.2 with cipher AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 58EB8207E6 for ; Fri, 16 Oct 2015 22:00:14 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1ZnD2q-0002n9-Rw; Fri, 16 Oct 2015 22:00:12 +0000 Received: from mail-pa0-x22f.google.com ([2607:f8b0:400e:c03::22f]) by bombadil.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1ZnD2o-0001cS-S4 for linux-rockchip@lists.infradead.org; Fri, 16 Oct 2015 22:00:11 +0000 Received: by pabws5 with SMTP id ws5so1244902pab.1 for ; Fri, 16 Oct 2015 14:59:49 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=from:to:cc:subject:date:message-id:mime-version:content-type :content-transfer-encoding; bh=39/1OBg3a2NWZHTEVLD3MRew3PZxm3jmwKKQjyB6QZQ=; b=ggi0zWQzgc34lRv8N5LvxDH72xh1flHk2x0ZYd/akebWUTbBUpt9jMYeGJH53c0qsH 2jl25bkIoySFR5rQ3BzhkIzrp/5rwzycgOE41NE8x1zkWEWPMujkT+otjqGMaIxxug2a bHBG22S2almZDFvkQQOOe450HN84W72Iqiz/0= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:date:message-id:mime-version :content-type:content-transfer-encoding; bh=39/1OBg3a2NWZHTEVLD3MRew3PZxm3jmwKKQjyB6QZQ=; b=FiryShNwSiXSD5olDUffDolXBAPEAVURDy8SBL71MOLpFbGC9VNvCONmxWUPqHScfc 9z01bce9mbqNiIXSynQqwyBRn+Td6cNqaywfYXQQ5adEMi0oHbQhmfgjMPLwhyJiJWQW yMEhoODcWlIEw/aLWYu8ftr2n5a9BON3OqzXHZ3bwtDcFOTA0bT+j9hynUv2eQekk7if 3iPMA+qAcE5D8nyVKaRtVgOmbefhmIPA/HBx0fCg2JuX59APiYi9qtcLNneKXu8KozQI ScJeXp9H2BmexO0ejnRrIK2cwJaAtN0IU1oKcnbiSCcZTVdUbqAeRNzqPoyl7WaPClyq lvCw== X-Gm-Message-State: ALoCoQlRCsNH8gY6TTH0aYOw13vF+VEa2qiwzmXEMTJJndC3CiX3laUEiAUwi37suGoRx+T1I9cP X-Received: by 10.68.87.226 with SMTP id bb2mr7532503pbb.84.1445032789449; Fri, 16 Oct 2015 14:59:49 -0700 (PDT) Received: from tictac.mtv.corp.google.com ([172.22.65.76]) by smtp.gmail.com with ESMTPSA id rm9sm23135902pab.14.2015.10.16.14.59.48 (version=TLS1_2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Fri, 16 Oct 2015 14:59:48 -0700 (PDT) From: Douglas Anderson To: John Youn , balbi@ti.com Subject: [PATCH v3] usb: dwc2: host: Fix use after free w/ simultaneous irqs Date: Fri, 16 Oct 2015 14:59:25 -0700 Message-Id: <1445032765-17342-1-git-send-email-dianders@chromium.org> X-Mailer: git-send-email 2.6.0.rc2.230.g3dd15c0 MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20151016_150010_967097_18CBD489 X-CRM114-Status: GOOD ( 17.72 ) X-Spam-Score: -2.7 (--) X-BeenThere: linux-rockchip@lists.infradead.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: Upstream kernel work for Rockchip platforms List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: gregory.herrero@intel.com, =?UTF-8?q?Heiko=20St=C3=BCbner?= , johnyoun@synopsys.com, gregkh@linuxfoundation.org, linux-usb@vger.kernel.org, Doug Anderson , linux-kernel@vger.kernel.org, linux-rockchip@lists.infradead.org, yousaf.kaukab@intel.com, Yunzhi Li , Julius Werner , dinguyen@opensource.altera.com Sender: "Linux-rockchip" Errors-To: linux-rockchip-bounces+patchwork-linux-rockchip=patchwork.kernel.org@lists.infradead.org X-Spam-Status: No, score=-4.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_MED, T_DKIM_INVALID, T_RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP From: Doug Anderson While plugging / unplugging on a DWC2 host port with "slub_debug=FZPUA" enabled, I found a crash that was quite obviously a use after free. It appears that in some cases when we handle the various sub-cases of HCINT we may end up freeing the QTD. If there is more than one bit set in HCINT we may then end up continuing to use the QTD, which is bad. Let's be paranoid and check for this after each sub-case. This should be safe since we officially have the "hsotg->lock" (it was grabbed in dwc2_handle_hcd_intr). The specific crash I found was: Unable to handle kernel paging request at virtual address 6b6b6b9f At the time of the crash, the kernel reported: (dwc2_hc_nak_intr+0x5c/0x198) (dwc2_handle_hcd_intr+0xa84/0xbf8) (_dwc2_hcd_irq+0x1c/0x20) (usb_hcd_irq+0x34/0x48) Popping into kgdb found that "*qtd" was filled with "0x6b", AKA qtd had been freed and filled with slub_debug poison. kgdb gave a little better stack crawl: 0 dwc2_hc_nak_intr (hsotg=hsotg@entry=0xec42e058, chan=chan@entry=0xec546dc0, chnum=chnum@entry=4, qtd=qtd@entry=0xec679600) at drivers/usb/dwc2/hcd_intr.c:1237 1 dwc2_hc_n_intr (chnum=4, hsotg=0xec42e058) at drivers/usb/dwc2/hcd_intr.c:2041 2 dwc2_hc_intr (hsotg=0xec42e058) at drivers/usb/dwc2/hcd_intr.c:2078 3 dwc2_handle_hcd_intr (hsotg=0xec42e058) at drivers/usb/dwc2/hcd_intr.c:2128 4 _dwc2_hcd_irq (hcd=) at drivers/usb/dwc2/hcd.c:2837 5 usb_hcd_irq (irq=, __hcd=) at drivers/usb/core/hcd.c:2353 Popping up to frame #1 (dwc2_hc_n_intr) found: (gdb) print /x hcint $12 = 0x12 AKA: #define HCINTMSK_CHHLTD (1 << 1) #define HCINTMSK_NAK (1 << 4) Further debugging found that by simulating receiving those two interrupts at the same time it was trivial to replicate the use-after-free. See for a patch and instructions. This lead to getting the following stack crawl of the actual free: 0 arch_kgdb_breakpoint () at arch/arm/include/asm/outercache.h:103 1 kgdb_breakpoint () at kernel/debug/debug_core.c:1054 2 dwc2_hcd_qtd_unlink_and_free (hsotg=, qh=, qtd=0xe4479a00) at drivers/usb/dwc2/hcd.h:488 3 dwc2_deactivate_qh (free_qtd=, qh=0xe5efa280, hsotg=0xed424618) at drivers/usb/dwc2/hcd_intr.c:671 4 dwc2_release_channel (hsotg=hsotg@entry=0xed424618, chan=chan@entry=0xed5be000, qtd=, halt_status=) at drivers/usb/dwc2/hcd_intr.c:742 5 dwc2_halt_channel (hsotg=0xed424618, chan=0xed5be000, qtd=, halt_status=) at drivers/usb/dwc2/hcd_intr.c:804 6 dwc2_complete_non_periodic_xfer (chnum=, halt_status=, qtd=, chan=, hsotg=) at drivers/usb/dwc2/hcd_intr.c:889 7 dwc2_hc_xfercomp_intr (hsotg=hsotg@entry=0xed424618, chan=chan@entry=0xed5be000, chnum=chnum@entry=6, qtd=qtd@entry=0xe4479a00) at drivers/usb/dwc2/hcd_intr.c:1065 8 dwc2_hc_chhltd_intr_dma (qtd=0xe4479a00, chnum=6, chan=0xed5be000, hsotg=0xed424618) at drivers/usb/dwc2/hcd_intr.c:1823 9 dwc2_hc_chhltd_intr (qtd=0xe4479a00, chnum=6, chan=0xed5be000, hsotg=0xed424618) at drivers/usb/dwc2/hcd_intr.c:1944 10 dwc2_hc_n_intr (chnum=6, hsotg=0xed424618) at drivers/usb/dwc2/hcd_intr.c:2052 11 dwc2_hc_intr (hsotg=0xed424618) at drivers/usb/dwc2/hcd_intr.c:2097 12 dwc2_handle_hcd_intr (hsotg=0xed424618) at drivers/usb/dwc2/hcd_intr.c:2147 13 _dwc2_hcd_irq (hcd=) at drivers/usb/dwc2/hcd.c:2837 14 usb_hcd_irq (irq=, __hcd=) at drivers/usb/core/hcd.c:2353 Though we could add specific code to handle this case, adding the general purpose code to check for all cases where qtd might be freed seemed safer. Signed-off-by: Douglas Anderson --- Changes in v3: - Don't pass NULL if qtd freed, just return (John Youn) - Don't keep track of interrupts left: list_first_entry() is fast. Changes in v2: - Add static as correctly pointed by kbuild test robot drivers/usb/dwc2/hcd_intr.c | 67 ++++++++++++++++++++++++++++++++++++++------- 1 file changed, 57 insertions(+), 10 deletions(-) diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c index f70c970..6d0cd368 100644 --- a/drivers/usb/dwc2/hcd_intr.c +++ b/drivers/usb/dwc2/hcd_intr.c @@ -1949,6 +1949,21 @@ static void dwc2_hc_chhltd_intr(struct dwc2_hsotg *hsotg, } } +/* + * Check if the given qtd is still the top of the list (and thus valid). + * + * If dwc2_hcd_qtd_unlink_and_free() has been called since we grabbed + * the qtd from the top of the list, this will return false (otherwise true). + */ +static bool dwc2_check_qtd_still_ok(struct dwc2_qtd *qtd, + struct list_head *qtd_list) +{ + struct dwc2_qtd *cur_head; + + cur_head = list_first_entry(qtd_list, struct dwc2_qtd, qtd_list_entry); + return (cur_head == qtd); +} + /* Handles interrupt for a specific Host Channel */ static void dwc2_hc_n_intr(struct dwc2_hsotg *hsotg, int chnum) { @@ -2031,27 +2046,59 @@ static void dwc2_hc_n_intr(struct dwc2_hsotg *hsotg, int chnum) */ hcint &= ~HCINTMSK_NYET; } - if (hcint & HCINTMSK_CHHLTD) + + if (hcint & HCINTMSK_CHHLTD) { dwc2_hc_chhltd_intr(hsotg, chan, chnum, qtd); - if (hcint & HCINTMSK_AHBERR) + if (!dwc2_check_qtd_still_ok(qtd, &chan->qh->qtd_list)) + goto exit; + } + if (hcint & HCINTMSK_AHBERR) { dwc2_hc_ahberr_intr(hsotg, chan, chnum, qtd); - if (hcint & HCINTMSK_STALL) + if (!dwc2_check_qtd_still_ok(qtd, &chan->qh->qtd_list)) + goto exit; + } + if (hcint & HCINTMSK_STALL) { dwc2_hc_stall_intr(hsotg, chan, chnum, qtd); - if (hcint & HCINTMSK_NAK) + if (!dwc2_check_qtd_still_ok(qtd, &chan->qh->qtd_list)) + goto exit; + } + if (hcint & HCINTMSK_NAK) { dwc2_hc_nak_intr(hsotg, chan, chnum, qtd); - if (hcint & HCINTMSK_ACK) + if (!dwc2_check_qtd_still_ok(qtd, &chan->qh->qtd_list)) + goto exit; + } + if (hcint & HCINTMSK_ACK) { dwc2_hc_ack_intr(hsotg, chan, chnum, qtd); - if (hcint & HCINTMSK_NYET) + if (!dwc2_check_qtd_still_ok(qtd, &chan->qh->qtd_list)) + goto exit; + } + if (hcint & HCINTMSK_NYET) { dwc2_hc_nyet_intr(hsotg, chan, chnum, qtd); - if (hcint & HCINTMSK_XACTERR) + if (!dwc2_check_qtd_still_ok(qtd, &chan->qh->qtd_list)) + goto exit; + } + if (hcint & HCINTMSK_XACTERR) { dwc2_hc_xacterr_intr(hsotg, chan, chnum, qtd); - if (hcint & HCINTMSK_BBLERR) + if (!dwc2_check_qtd_still_ok(qtd, &chan->qh->qtd_list)) + goto exit; + } + if (hcint & HCINTMSK_BBLERR) { dwc2_hc_babble_intr(hsotg, chan, chnum, qtd); - if (hcint & HCINTMSK_FRMOVRUN) + if (!dwc2_check_qtd_still_ok(qtd, &chan->qh->qtd_list)) + goto exit; + } + if (hcint & HCINTMSK_FRMOVRUN) { dwc2_hc_frmovrun_intr(hsotg, chan, chnum, qtd); - if (hcint & HCINTMSK_DATATGLERR) + if (!dwc2_check_qtd_still_ok(qtd, &chan->qh->qtd_list)) + goto exit; + } + if (hcint & HCINTMSK_DATATGLERR) { dwc2_hc_datatglerr_intr(hsotg, chan, chnum, qtd); + if (!dwc2_check_qtd_still_ok(qtd, &chan->qh->qtd_list)) + goto exit; + } +exit: chan->hcint = 0; }