From: Doug Anderson <dianders@chromium.org>
To: John Youn <John.Youn@synopsys.com>, Felipe Balbi <balbi@ti.com>
Cc: "Yunzhi Li" <lyz@rock-chips.com>,
"Heiko Stübner" <heiko@sntech.de>,
"open list:ARM/Rockchip SoC..."
<linux-rockchip@lists.infradead.org>,
"Julius Werner" <jwerner@chromium.org>,
"Herrero, Gregory" <gregory.herrero@intel.com>,
"Kaukab, Yousaf" <yousaf.kaukab@intel.com>,
"Dinh Nguyen" <dinguyen@opensource.altera.com>,
"Doug Anderson" <dianders@chromium.org>,
"John Youn" <johnyoun@synopsys.com>,
"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
"linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v3] usb: dwc2: host: Fix use after free w/ simultaneous irqs
Date: Fri, 16 Oct 2015 15:57:40 -0700 [thread overview]
Message-ID: <CAD=FV=XQa4coFjHupn7rCGSn-m7itZB9Hs9uU_ckgumtcbtx6w@mail.gmail.com> (raw)
In-Reply-To: <1445032765-17342-1-git-send-email-dianders@chromium.org>
Hi,
On Fri, Oct 16, 2015 at 2:59 PM, Douglas Anderson <dianders@chromium.org> wrote:
> From: Doug Anderson <dianders@chromium.org>
>
> 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=<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>
> ---
> 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(-)
I just tested this latest patch together with the descriptor DMA
patches. I didn't track down why, but something about that series
ended up making "qh" NULL after an interrupt. I'll post up v4 that
fixes that. Sorry for the noise.
-Doug
prev parent reply other threads:[~2015-10-16 22:57 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-10-16 21:59 [PATCH v3] usb: dwc2: host: Fix use after free w/ simultaneous irqs Douglas Anderson
2015-10-16 22:57 ` Doug Anderson [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAD=FV=XQa4coFjHupn7rCGSn-m7itZB9Hs9uU_ckgumtcbtx6w@mail.gmail.com' \
--to=dianders@chromium.org \
--cc=John.Youn@synopsys.com \
--cc=balbi@ti.com \
--cc=dinguyen@opensource.altera.com \
--cc=gregkh@linuxfoundation.org \
--cc=gregory.herrero@intel.com \
--cc=heiko@sntech.de \
--cc=johnyoun@synopsys.com \
--cc=jwerner@chromium.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-rockchip@lists.infradead.org \
--cc=linux-usb@vger.kernel.org \
--cc=lyz@rock-chips.com \
--cc=yousaf.kaukab@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).