Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756134AbcLBESp (ORCPT ); Thu, 1 Dec 2016 23:18:45 -0500 Received: from mail-yb0-f177.google.com ([209.85.213.177]:36146 "EHLO mail-yb0-f177.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753657AbcLBESn (ORCPT ); Thu, 1 Dec 2016 23:18:43 -0500 MIME-Version: 1.0 In-Reply-To: <1480645767-10931-1-git-send-email-baolu.lu@linux.intel.com> References: <1480645767-10931-1-git-send-email-baolu.lu@linux.intel.com> From: Baolin Wang Date: Fri, 2 Dec 2016 12:18:42 +0800 Message-ID: Subject: Re: [PATCH 1/1] usb: xhci: fix possible wild pointer To: Lu Baolu Cc: Mathias Nyman , USB , LKML , stable@vger.kernel.org Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2971 Lines: 74 On 2 December 2016 at 10: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 Nice catch. I was also curious where set xhci->current_cmd to be NULL when current command is freed. Tested-by: Baolin Wang > --- > 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: > -- > 2.1.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-usb" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Baolin.wang Best Regards