Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1247070 > unrolled thread

[PATCH] usb: dwc2: host: Fix use after free w/ simultaneous irqs

Back to article view | Back to linux.kernel

Contents

  [PATCH] usb: dwc2: host: Fix use after free w/ simultaneous irqs Douglas Anderson <dianders@chromium.org> - 2015-10-14 20:50 +0200
    Re: [PATCH] usb: dwc2: host: Fix use after free w/ simultaneous irqs kbuild test robot <lkp@intel.com> - 2015-10-14 22:50 +0200
    [RFC PATCH] usb: dwc2: host: dwc2_check_qtd_still_ok() can be static kbuild test robot <lkp@intel.com> - 2015-10-14 22:50 +0200
      Re: [RFC PATCH] usb: dwc2: host: dwc2_check_qtd_still_ok() can be static Doug Anderson <dianders@chromium.org> - 2015-10-15 01:00 +0200

#1247070 — [PATCH] usb: dwc2: host: Fix use after free w/ simultaneous irqs

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.  We only do
this if there are still further bits to process, so the overhead should
be small.  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=<optimized out>) at drivers/usb/dwc2/hcd.c:2837
 5 usb_hcd_irq (irq=<optimized out>, __hcd=<optimized out>) 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 <http://crosreview.com/305712> 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=<optimized out>, qh=<optimized
      out>, qtd=0xe4479a00) at drivers/usb/dwc2/hcd.h:488
 3  dwc2_deactivate_qh (free_qtd=<optimized out>, 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=<optimized out>,
      halt_status=<optimized out>) at drivers/usb/dwc2/hcd_intr.c:742
 5  dwc2_halt_channel (hsotg=0xed424618, chan=0xed5be000, qtd=<optimized
      out>, halt_status=<optimized out>) at
      drivers/usb/dwc2/hcd_intr.c:804
 6  dwc2_complete_non_periodic_xfer (chnum=<optimized out>,
      halt_status=<optimized out>, qtd=<optimized out>, chan=<optimized
      out>, hsotg=<optimized out>) 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=<optimized out>) at drivers/usb/dwc2/hcd.c:2837
 14 usb_hcd_irq (irq=<optimized out>, __hcd=<optimized out>) 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 <dianders@chromium.org>
---
 drivers/usb/dwc2/hcd_intr.c | 80 +++++++++++++++++++++++++++++++++++++++------
 1 file changed, 70 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c
index f70c970..57e71f9d 100644
--- a/drivers/usb/dwc2/hcd_intr.c
+++ b/drivers/usb/dwc2/hcd_intr.c
@@ -1949,6 +1949,25 @@ 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 NULL.  Otherwise
+ * it will be passed back qtd.
+ */
+struct dwc2_qtd *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);
+	if (cur_head == qtd)
+		return qtd;
+
+	return NULL;
+}
+
 /* Handles interrupt for a specific Host Channel */
 static void dwc2_hc_n_intr(struct dwc2_hsotg *hsotg, int chnum)
 {
@@ -2031,26 +2050,67 @@ static void dwc2_hc_n_intr(struct dwc2_hsotg *hsotg, int chnum)
 		 */
 		hcint &= ~HCINTMSK_NYET;
 	}
-	if (hcint & HCINTMSK_CHHLTD)
+
+	if (hcint & HCINTMSK_CHHLTD) {
+		hcint &= ~HCINTMSK_CHHLTD;
 		dwc2_hc_chhltd_intr(hsotg, chan, chnum, qtd);
-	if (hcint & HCINTMSK_AHBERR)
+		if (hcint)
+			qtd = dwc2_check_qtd_still_ok(qtd, &chan->qh->qtd_list);
+	}
+	if (hcint & HCINTMSK_AHBERR) {
+		hcint &= ~HCINTMSK_AHBERR;
 		dwc2_hc_ahberr_intr(hsotg, chan, chnum, qtd);
-	if (hcint & HCINTMSK_STALL)
+		if (hcint)
+			qtd = dwc2_check_qtd_still_ok(qtd, &chan->qh->qtd_list);
+	}
+	if (hcint & HCINTMSK_STALL) {
+		hcint &= ~HCINTMSK_STALL;
 		dwc2_hc_stall_intr(hsotg, chan, chnum, qtd);
-	if (hcint & HCINTMSK_NAK)
+		if (hcint)
+			qtd = dwc2_check_qtd_still_ok(qtd, &chan->qh->qtd_list);
+	}
+	if (hcint & HCINTMSK_NAK) {
+		hcint &= ~HCINTMSK_NAK;
 		dwc2_hc_nak_intr(hsotg, chan, chnum, qtd);
-	if (hcint & HCINTMSK_ACK)
+		if (hcint)
+			qtd = dwc2_check_qtd_still_ok(qtd, &chan->qh->qtd_list);
+	}
+	if (hcint & HCINTMSK_ACK) {
+		hcint &= ~HCINTMSK_ACK;
 		dwc2_hc_ack_intr(hsotg, chan, chnum, qtd);
-	if (hcint & HCINTMSK_NYET)
+		if (hcint)
+			qtd = dwc2_check_qtd_still_ok(qtd, &chan->qh->qtd_list);
+	}
+	if (hcint & HCINTMSK_NYET) {
+		hcint &= ~HCINTMSK_NYET;
 		dwc2_hc_nyet_intr(hsotg, chan, chnum, qtd);
-	if (hcint & HCINTMSK_XACTERR)
+		if (hcint)
+			qtd = dwc2_check_qtd_still_ok(qtd, &chan->qh->qtd_list);
+	}
+	if (hcint & HCINTMSK_XACTERR) {
+		hcint &= ~HCINTMSK_XACTERR;
 		dwc2_hc_xacterr_intr(hsotg, chan, chnum, qtd);
-	if (hcint & HCINTMSK_BBLERR)
+		if (hcint)
+			qtd = dwc2_check_qtd_still_ok(qtd, &chan->qh->qtd_list);
+	}
+	if (hcint & HCINTMSK_BBLERR) {
+		hcint &= ~HCINTMSK_BBLERR;
 		dwc2_hc_babble_intr(hsotg, chan, chnum, qtd);
-	if (hcint & HCINTMSK_FRMOVRUN)
+		if (hcint)
+			qtd = dwc2_check_qtd_still_ok(qtd, &chan->qh->qtd_list);
+	}
+	if (hcint & HCINTMSK_FRMOVRUN) {
+		hcint &= ~HCINTMSK_FRMOVRUN;
 		dwc2_hc_frmovrun_intr(hsotg, chan, chnum, qtd);
-	if (hcint & HCINTMSK_DATATGLERR)
+		if (hcint)
+			qtd = dwc2_check_qtd_still_ok(qtd, &chan->qh->qtd_list);
+	}
+	if (hcint & HCINTMSK_DATATGLERR) {
+		hcint &= ~HCINTMSK_DATATGLERR;
 		dwc2_hc_datatglerr_intr(hsotg, chan, chnum, qtd);
+		if (hcint)
+			qtd = dwc2_check_qtd_still_ok(qtd, &chan->qh->qtd_list);
+	}
 
 	chan->hcint = 0;
 }
-- 
2.6.0.rc2.230.g3dd15c0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1247141

Hi Douglas,

[auto build test WARNING on balbi-usb/next -- if it's inappropriate base, please suggest rules for selecting the more suitable base]

url:    https://github.com/0day-ci/linux/commits/Douglas-Anderson/usb-dwc2-host-Fix-use-after-free-w-simultaneous-irqs/20151015-024824
reproduce:
        # apt-get install sparse
        make ARCH=x86_64 allmodconfig
        make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)

>> drivers/usb/dwc2/hcd_intr.c:1959:17: sparse: symbol 'dwc2_check_qtd_still_ok' was not declared. Should it be static?

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1247142 — [RFC PATCH] usb: dwc2: host: dwc2_check_qtd_still_ok() can be static

Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
---
 hcd_intr.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c
index 57e71f9d..b31a1fb 100644
--- a/drivers/usb/dwc2/hcd_intr.c
+++ b/drivers/usb/dwc2/hcd_intr.c
@@ -1956,7 +1956,7 @@ static void dwc2_hc_chhltd_intr(struct dwc2_hsotg *hsotg,
  * the qtd from the top of the list, this will return NULL.  Otherwise
  * it will be passed back qtd.
  */
-struct dwc2_qtd *dwc2_check_qtd_still_ok(struct dwc2_qtd *qtd,
+static struct dwc2_qtd *dwc2_check_qtd_still_ok(struct dwc2_qtd *qtd,
 					    struct list_head *qtd_list)
 {
 	struct dwc2_qtd *cur_head;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1247245 — Re: [RFC PATCH] usb: dwc2: host: dwc2_check_qtd_still_ok() can be static

Hi,

On Wed, Oct 14, 2015 at 1:44 PM, kbuild test robot <lkp@intel.com> wrote:
>
> Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
> ---
>  hcd_intr.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c
> index 57e71f9d..b31a1fb 100644
> --- a/drivers/usb/dwc2/hcd_intr.c
> +++ b/drivers/usb/dwc2/hcd_intr.c
> @@ -1956,7 +1956,7 @@ static void dwc2_hc_chhltd_intr(struct dwc2_hsotg *hsotg,
>   * the qtd from the top of the list, this will return NULL.  Otherwise
>   * it will be passed back qtd.
>   */
> -struct dwc2_qtd *dwc2_check_qtd_still_ok(struct dwc2_qtd *qtd,
> +static struct dwc2_qtd *dwc2_check_qtd_still_ok(struct dwc2_qtd *qtd,
>                                             struct list_head *qtd_list)
>  {
>         struct dwc2_qtd *cur_head;

Fixed in v2.  https://patchwork.kernel.org/patch/7398911/

-Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web