Linux-USB Archive on lore.kernel.org
 help / color / Atom feed
From: Johan Hovold <johan@kernel.org>
To: Mathias Nyman <mathias.nyman@linux.intel.com>
Cc: gregkh@linuxfoundation.org, linux-usb@vger.kernel.org,
	"# v5 . 3" <stable@vger.kernel.org>,
	Alan Stern <stern@rowland.harvard.edu>
Subject: Re: [PATCH 8/8] xhci: Fix NULL pointer dereference in xhci_clear_tt_buffer_complete()
Date: Mon, 7 Oct 2019 16:02:45 +0200
Message-ID: <20191007140245.GD13531@localhost> (raw)
In-Reply-To: <1570190373-30684-9-git-send-email-mathias.nyman@linux.intel.com>

[ +CC: Alan ]

On Fri, Oct 04, 2019 at 02:59:33PM +0300, Mathias Nyman wrote:
> udev stored in ep->hcpriv might be NULL if tt buffer is cleared
> due to a halted control endpoint during device enumeration
> 
> xhci_clear_tt_buffer_complete is called by hub_tt_work() once it's
> scheduled,  and by then usb core might have freed and allocated a
> new udev for the next enumeration attempt.
>
> Fixes: ef513be0a905 ("usb: xhci: Add Clear_TT_Buffer")
> Cc: <stable@vger.kernel.org> # v5.3
> Reported-by: Johan Hovold <johan@kernel.org>
> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> ---
>  drivers/usb/host/xhci.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 00f3804f7aa7..517ec3206f6e 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -5238,8 +5238,16 @@ static void xhci_clear_tt_buffer_complete(struct usb_hcd *hcd,
>  	unsigned int ep_index;
>  	unsigned long flags;
>  
> +	/*
> +	 * udev might be NULL if tt buffer is cleared during a failed device
> +	 * enumeration due to a halted control endpoint. Usb core might
> +	 * have allocated a new udev for the next enumeration attempt.
> +	 */
> +
>  	xhci = hcd_to_xhci(hcd);
>  	udev = (struct usb_device *)ep->hcpriv;
> +	if (!udev)
> +		return;

I didn't have time to look into this myself last week, or comment on the
patch before Greg picked it up, but this clearly isn't the right fix.

As your comment suggests, ep->hcpriv may indeed be NULL here if USB core
have allocated a new udev. But this only happens after USB has freed the
old usb_device and the new one happens to get the same address.

Note that even the usb_host_endpoint itself (ep) has then been freed and
reallocated since it is member of struct usb_device, and it is the
use-after-free that needs fixing.

I've even been able to trigger another NULL-deref in this function
before a new udev has been allocated, due to the virt dev having been
freed by xhci_free_dev as part of usb_release_dev:

[   19.627771] usb 2-2.4: unable to read config index 0 descriptor/start: -32
[   19.627966] usb 2-2.4: chopping to 0 config(s)
[   19.628133] usb 2-2.4: can't read configurations, error -32
[   19.629017] usb 2-2.4: usb_release_dev - udev = ffff930b14d82000

udev is freed here

[   19.629258] usb 2-2-port4: attempt power cycle
[   19.629461] xhci_clear_tt_buffer_complete - udev = ffff930b14d82000

use-after-free when tt work is scheduled (note than udev is non-NULL
since udev hasn't been reallocated and initialised yet):

[   19.629643] xhci_clear_tt_buffer_complete - xhci->devs[4] = 0000000000000000

virt dev is NULL after having been freed by xhci_free_dev()

[   19.629876] BUG: kernel NULL pointer dereference, address: 0000000000000030

and is later dereferenced

[   19.630034] #PF: supervisor write access in kernel mode
[   19.630155] #PF: error_code(0x0002) - not-present page
[   19.630270] PGD 0 P4D 0 
[   19.630341] Oops: 0002 [#1] SMP
[   19.630425] CPU: 2 PID: 110 Comm: kworker/2:2 Not tainted 5.4.0-rc1 #28
[   19.630572] Hardware name:  /D34010WYK, BIOS WYLPT10H.86A.0051.2019.0322.1320 03/22/2019
[   19.636141] Workqueue: events hub_tt_work
[   19.638125] RIP: 0010:xhci_clear_tt_buffer_complete.cold.69+0x9b/0xcd

It seems the xhci clear-tt implementation was incomplete since it did
not take care to wait for any ongoing work before disabling the
endpoint. EHCI does this in ehci_endpoint_disable(), but xhci doesn't
even implement that callback.

As this may be something you could end up hitting in other paths as
well, perhaps we should even consider reverting the offending commit
pending a more complete implementation?

>  	slot_id = udev->slot_id;
>  	ep_index = xhci_get_endpoint_index(&ep->desc);

For reference, my original report is here:

	https://lkml.kernel.org/r/20190930103107.GC13531@localhost

Johan

  reply index

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-04 11:59 [PATCH 0/8] xhci fixes for usb-linus Mathias Nyman
2019-10-04 11:59 ` [PATCH 1/8] xhci: Fix false warning message about wrong bounce buffer write length Mathias Nyman
2019-10-04 11:59 ` [PATCH 2/8] xhci: Prevent device initiated U1/U2 link pm if exit latency is too long Mathias Nyman
2019-10-04 11:59 ` [PATCH 3/8] xhci: Check all endpoints for LPM timeout Mathias Nyman
2019-10-04 11:59 ` [PATCH 4/8] xhci: Fix USB 3.1 capability detection on early xHCI 1.1 spec based hosts Mathias Nyman
2019-10-04 11:59 ` [PATCH 5/8] usb: xhci: wait for CNR controller not ready bit in xhci resume Mathias Nyman
2019-10-04 11:59 ` [PATCH 6/8] xhci: Prevent deadlock when xhci adapter breaks during init Mathias Nyman
2019-10-04 11:59 ` [PATCH 7/8] xhci: Increase STS_SAVE timeout in xhci_suspend() Mathias Nyman
     [not found]   ` <20191006120750.5334F2087E@mail.kernel.org>
2019-10-07 18:51     ` Kai-Heng Feng
2019-10-04 11:59 ` [PATCH 8/8] xhci: Fix NULL pointer dereference in xhci_clear_tt_buffer_complete() Mathias Nyman
2019-10-07 14:02   ` Johan Hovold [this message]
2019-10-08  8:15     ` Mathias Nyman
2019-10-11 12:47       ` Mathias Nyman
2019-10-11 12:58         ` [RFT PATCH] xhci: Fix use-after-free regression in xhci clear hub TT implementation Mathias Nyman
2019-10-14 10:16           ` Johan Hovold
2019-10-14 13:18             ` Mathias Nyman

Reply instructions:

You may reply publically 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=20191007140245.GD13531@localhost \
    --to=johan@kernel.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-usb@vger.kernel.org \
    --cc=mathias.nyman@linux.intel.com \
    --cc=stable@vger.kernel.org \
    --cc=stern@rowland.harvard.edu \
    /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

Linux-USB Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-usb/0 linux-usb/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-usb linux-usb/ https://lore.kernel.org/linux-usb \
		linux-usb@vger.kernel.org
	public-inbox-index linux-usb

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-usb


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git