Re: [PATCH 1/1] usb: xhci: fix possible wild pointer
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(>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(>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, >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(>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
Re: [PATCH 1/1] usb: xhci: fix possible wild pointer
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(>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(>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, >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(>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
Re: [PATCH 1/1] usb: xhci: fix possible wild pointer
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(>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(>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, >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(>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 -Mathias
Re: [PATCH 1/1] usb: xhci: fix possible wild pointer
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(>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(>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, >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(>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 -Mathias
Re: [PATCH 1/1] usb: xhci: fix possible wild pointer
On 2 December 2016 at 12:40, Lu Baoluwrote: > Hi, > > On 12/02/2016 12:18 PM, Baolin Wang wrote: >> 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. > > Below code does: Yes, I just means you did that, but the original code did not. Sorry for confusing. > > xhci->current_cmd = list_entry(cmd->cmd_list.next, >struct xhci_command, cmd_list); > mod_timer(>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"); > } > > > Best regards, > Lu Baolu > >> 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(>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(>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, >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(>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 majord...@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html >> >> > -- Baolin.wang Best Regards
Re: [PATCH 1/1] usb: xhci: fix possible wild pointer
On 2 December 2016 at 12:40, Lu Baolu wrote: > Hi, > > On 12/02/2016 12:18 PM, Baolin Wang wrote: >> 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. > > Below code does: Yes, I just means you did that, but the original code did not. Sorry for confusing. > > xhci->current_cmd = list_entry(cmd->cmd_list.next, >struct xhci_command, cmd_list); > mod_timer(>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"); > } > > > Best regards, > Lu Baolu > >> 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(>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(>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, >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(>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 majord...@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html >> >> > -- Baolin.wang Best Regards
Re: [PATCH 1/1] usb: xhci: fix possible wild pointer
Hi, On 12/02/2016 12:18 PM, Baolin Wang wrote: > On 2 December 2016 at 10:29, Lu Baoluwrote: >> 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. Below code does: xhci->current_cmd = list_entry(cmd->cmd_list.next, struct xhci_command, cmd_list); mod_timer(>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"); } Best regards, Lu Baolu > 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(>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(>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, >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(>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 majord...@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > >
Re: [PATCH 1/1] usb: xhci: fix possible wild pointer
Hi, On 12/02/2016 12:18 PM, Baolin Wang wrote: > 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. Below code does: xhci->current_cmd = list_entry(cmd->cmd_list.next, struct xhci_command, cmd_list); mod_timer(>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"); } Best regards, Lu Baolu > 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(>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(>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, >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(>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 majord...@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > >
Re: [PATCH 1/1] usb: xhci: fix possible wild pointer
On 2 December 2016 at 10:29, Lu Baoluwrote: > 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(>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(>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, >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(>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 majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Baolin.wang Best Regards
Re: [PATCH 1/1] usb: xhci: fix possible wild pointer
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(>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(>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, >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(>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 majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Baolin.wang Best Regards