Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752227AbcLEJhD (ORCPT ); Mon, 5 Dec 2016 04:37:03 -0500 Received: from mga11.intel.com ([192.55.52.93]:11679 "EHLO mga11.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751427AbcLEJhA (ORCPT ); Mon, 5 Dec 2016 04:37:00 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,747,1477983600"; d="scan'208";a="1077541408" Subject: Re: [PATCH 1/1] usb: xhci: fix possible wild pointer To: Mathias Nyman References: <1480645767-10931-1-git-send-email-baolu.lu@linux.intel.com> <58417993.4040307@linux.intel.com> Cc: linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, stable@vger.kernel.org From: Lu Baolu Message-ID: <58452E42.9010504@linux.intel.com> Date: Mon, 5 Dec 2016 17:07:14 +0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 MIME-Version: 1.0 In-Reply-To: <58417993.4040307@linux.intel.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2674 Lines: 74 Hi, On 12/02/2016 09:39 PM, Mathias Nyman wrote: > On 02.12.2016 04:29, Lu Baolu wrote: >> handle_cmd_completion() frees a command structure which might >> be still referenced by xhci->current_cmd. This might cause >> problem when xhci->current_cmd is accessed after that. >> >> A real-life case could be like this. The host takes a very long >> time to respond to a command, and the command timer is fired at >> the same time when the command completion event arrives. The >> command completion handler frees xhci->current_cmd before the >> timer function can grab xhci->lock. Afterward, timer function >> grabs the lock and go ahead with checking and setting members >> of xhci->current_cmd. >> >> Cc: # v3.16+ >> Signed-off-by: Lu Baolu >> --- >> drivers/usb/host/xhci-ring.c | 16 +++++++++++----- >> 1 file changed, 11 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c >> index bdf6b13..13e05f6 100644 >> --- a/drivers/usb/host/xhci-ring.c >> +++ b/drivers/usb/host/xhci-ring.c >> @@ -1267,14 +1267,16 @@ void xhci_handle_command_timeout(unsigned long data) >> bool second_timeout = false; >> xhci = (struct xhci_hcd *) data; >> >> - /* mark this command to be cancelled */ >> spin_lock_irqsave(&xhci->lock, flags); >> - if (xhci->current_cmd) { >> - if (xhci->current_cmd->status == COMP_CMD_ABORT) >> - second_timeout = true; >> - xhci->current_cmd->status = COMP_CMD_ABORT; >> + if (!xhci->current_cmd) { >> + spin_unlock_irqrestore(&xhci->lock, flags); >> + return; >> } >> >> + if (xhci->current_cmd->status == COMP_CMD_ABORT) >> + second_timeout = true; >> + xhci->current_cmd->status = COMP_CMD_ABORT; >> + >> /* Make sure command ring is running before aborting it */ >> hw_ring_state = xhci_read_64(xhci, &xhci->op_regs->cmd_ring); >> if ((xhci->cmd_ring_state & CMD_RING_STATE_RUNNING) && >> @@ -1422,6 +1424,10 @@ static void handle_cmd_completion(struct xhci_hcd *xhci, >> xhci->current_cmd = list_entry(cmd->cmd_list.next, >> struct xhci_command, cmd_list); >> mod_timer(&xhci->cmd_timer, jiffies + XHCI_CMD_DEFAULT_TIMEOUT); >> + } else if (xhci->current_cmd == cmd) { >> + xhci->current_cmd = NULL; >> + } else { >> + xhci_warn(xhci, "WARN current_cmd doesn't match command\n"); >> } >> >> event_handled: >> > > Thanks, > > I might do some tweaking to (or remove) the warn message when applying if > that is ok Sure. > > -Mathias > Best regards, Lu Baolu