Re: [PATCH] xhci: Fix dereference from empty xhci->cmd_list

[OGAWA Hirofumi]

Mathias Nyman  writes:

>> OGAWA Hirofumi  writes:
>>> If race with timeout or stop event on empty cmd_list was happened,
>>> handle_cmd_completion() deferences wrong pointer from xhci->cmd_list.
>>>
>>> So this makes sure we don't dereference wrong pointer from empty
>>> xhci->cmd_list.
>>>
>>> Signed-off-by: OGAWA Hirofumi 
>>> ---
>
> Thanks, nice catch.
>
> What kernel is this based on, There is a partial fix in 4.8:
>
> commit 33be126510974e2eb9679f1ca9bca4f67ee4c4c7
>  xhci: always handle "Command Ring Stopped" events
>
> Looking at the existing code the case you describe could be possible if
> handle_cmd_completion is racing with:
> -  command timed out twice and xhci_cleanup_command_queue() is called
> -  or command timed out and we fail to restart command ring (assume host is 
> dead)
>
> It would be interesting to know how this was triggered, were there any error 
> messages like:
> "Abort command ring failed"
> or with xhci debugging enabled something like:
> "command timed out twice, ring start fail?"

Ah, sorry for confusing. I wrote this for older kernel (3.8.x, and not
for vanilla). So 4.8 might not have race what I saw actually.
(33be126510974e2eb9679f1ca9bca4f67ee4c4c7 seems to be prevent it)

However, even if 4.8 fixed that case, if there is spurious event or
driver state confusing, fetch and dereference empty list is possible and
bad thing. So, IMO, it would be better to make sure to avoid it.

>>> +   if (list_empty(>cmd_list))
>>> +   cmd = NULL;
>>> +   else {
>>> +   cmd = list_first_entry(>cmd_list, struct xhci_command,
>>> +  cmd_list);
>>> +   }
>>
>> These few lines could become a nice helper. Something like:
>>
>> static inline struct xhci_command *next_command(struct xhci_hcd *xhci)
>> {
>>  if (list_empty(>cmd_list))
>>  return NULL;
>>  return list_first_entry(>cmd_list, struct xhci_command, cmd_list);
>> }
>>
>
> Both works for me, helper function, or code in this patch.

There are some opencode list_head usages too. It might be better to
change at once as list cleanup if need.

E.g.

/* restart timer if this wasn't the last command */
if (cmd->cmd_list.next != >cmd_list) {

is

if (!list_is_last(>cmd_list, >cmd_list)) {

Well, so, if next_command() cleanup is not required, I'd like to use as
is, in this patch.

Thanks.
-- 
OGAWA Hirofumi 
--
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] xhci: Fix dereference from empty xhci->cmd_list

[Mathias Nyman]

Hi

On 18.10.2016 16:07, Felipe Balbi wrote:


Hi,

OGAWA Hirofumi  writes:

If race with timeout or stop event on empty cmd_list was happened,
handle_cmd_completion() deferences wrong pointer from xhci->cmd_list.

So this makes sure we don't dereference wrong pointer from empty
xhci->cmd_list.

Signed-off-by: OGAWA Hirofumi 
---


Thanks, nice catch.

What kernel is this based on, There is a partial fix in 4.8:

commit 33be126510974e2eb9679f1ca9bca4f67ee4c4c7
xhci: always handle "Command Ring Stopped" events


Looking at the existing code the case you describe could be possible if
handle_cmd_completion is racing with:
-  command timed out twice and xhci_cleanup_command_queue() is called
-  or command timed out and we fail to restart command ring (assume host is 
dead)

It would be interesting to know how this was triggered, were there any error 
messages like:
"Abort command ring failed"
or with xhci debugging enabled something like:
"command timed out twice, ring start fail?"



  drivers/usb/host/xhci-ring.c |   12 +---
  1 file changed, 9 insertions(+), 3 deletions(-)

diff -puN drivers/usb/host/xhci-ring.c~xhci-fix-empty-list 
drivers/usb/host/xhci-ring.c
--- linux/drivers/usb/host/xhci-ring.c~xhci-fix-empty-list  2016-10-18 
21:20:07.973055018 +0900
+++ linux-hirofumi/drivers/usb/host/xhci-ring.c 2016-10-18 21:21:35.949060365 
+0900
@@ -1336,7 +1336,12 @@ static void handle_cmd_completion(struct
return;
}

-   cmd = list_entry(xhci->cmd_list.next, struct xhci_command, cmd_list);




+   if (list_empty(>cmd_list))
+   cmd = NULL;
+   else {
+   cmd = list_first_entry(>cmd_list, struct xhci_command,
+  cmd_list);
+   }


These few lines could become a nice helper. Something like:

static inline struct xhci_command *next_command(struct xhci_hcd *xhci)
{
 if (list_empty(>cmd_list))
return NULL;
return list_first_entry(>cmd_list, struct xhci_command, cmd_list);
}



Both works for me, helper function, or code in this patch.


@@ -1350,9 +1355,10 @@ static void handle_cmd_completion(struct


Before the lines below, there's a call to del_timer() and a
tracepoint. I have a feeling we don't want those to happen if the list
is corrupt. Perhaps you should skip del_timer() and adding a log to
trace buffer, no?


I think those are fine, they shouldn't matter. With a empty command list
we don't wan't any command timeout timer to trigger, and the trancepoint
prints out info about the command trb on the command ring and event
trb on event ring. It doesn't touch the struct command  in the command list.

-Mathias

--
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] xhci: Fix dereference from empty xhci->cmd_list

[Felipe Balbi]

Hi,

OGAWA Hirofumi  writes:
> If race with timeout or stop event on empty cmd_list was happened,
> handle_cmd_completion() deferences wrong pointer from xhci->cmd_list.
>
> So this makes sure we don't dereference wrong pointer from empty
> xhci->cmd_list.
>
> Signed-off-by: OGAWA Hirofumi 
> ---
>
>  drivers/usb/host/xhci-ring.c |   12 +---
>  1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff -puN drivers/usb/host/xhci-ring.c~xhci-fix-empty-list 
> drivers/usb/host/xhci-ring.c
> --- linux/drivers/usb/host/xhci-ring.c~xhci-fix-empty-list2016-10-18 
> 21:20:07.973055018 +0900
> +++ linux-hirofumi/drivers/usb/host/xhci-ring.c   2016-10-18 
> 21:21:35.949060365 +0900
> @@ -1336,7 +1336,12 @@ static void handle_cmd_completion(struct
>   return;
>   }
>  
> - cmd = list_entry(xhci->cmd_list.next, struct xhci_command, cmd_list);


> + if (list_empty(>cmd_list))
> + cmd = NULL;
> + else {
> + cmd = list_first_entry(>cmd_list, struct xhci_command,
> +cmd_list);
> + }

These few lines could become a nice helper. Something like:

static inline struct xhci_command *next_command(struct xhci_hcd *xhci)
{
if (list_empty(>cmd_list))
return NULL;
return list_first_entry(>cmd_list, struct xhci_command, cmd_list);
}

> @@ -1350,9 +1355,10 @@ static void handle_cmd_completion(struct

Before the lines below, there's a call to del_timer() and a
tracepoint. I have a feeling we don't want those to happen if the list
is corrupt. Perhaps you should skip del_timer() and adding a log to
trace buffer, no?

>   return;
>   }
>  
> - if (cmd->command_trb != xhci->cmd_ring->dequeue) {
> + if (cmd == NULL || cmd->command_trb != xhci->cmd_ring->dequeue) {
>   xhci_err(xhci,
> -  "Command completion event does not match command\n");
> +  "Command completion event %s\n",
> +  cmd ? "does not match command" : "on empty list");
>   return;
>   }

-- 
balbi


signature.asc
Description: PGP signature

[PATCH] xhci: Fix dereference from empty xhci->cmd_list

[OGAWA Hirofumi]

If race with timeout or stop event on empty cmd_list was happened,
handle_cmd_completion() deferences wrong pointer from xhci->cmd_list.

So this makes sure we don't dereference wrong pointer from empty
xhci->cmd_list.

Signed-off-by: OGAWA Hirofumi 
---

 drivers/usb/host/xhci-ring.c |   12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff -puN drivers/usb/host/xhci-ring.c~xhci-fix-empty-list 
drivers/usb/host/xhci-ring.c
--- linux/drivers/usb/host/xhci-ring.c~xhci-fix-empty-list  2016-10-18 
21:20:07.973055018 +0900
+++ linux-hirofumi/drivers/usb/host/xhci-ring.c 2016-10-18 21:21:35.949060365 
+0900
@@ -1336,7 +1336,12 @@ static void handle_cmd_completion(struct
return;
}
 
-   cmd = list_entry(xhci->cmd_list.next, struct xhci_command, cmd_list);
+   if (list_empty(>cmd_list))
+   cmd = NULL;
+   else {
+   cmd = list_first_entry(>cmd_list, struct xhci_command,
+  cmd_list);
+   }
 
del_timer(>cmd_timer);
 
@@ -1350,9 +1355,10 @@ static void handle_cmd_completion(struct
return;
}
 
-   if (cmd->command_trb != xhci->cmd_ring->dequeue) {
+   if (cmd == NULL || cmd->command_trb != xhci->cmd_ring->dequeue) {
xhci_err(xhci,
-"Command completion event does not match command\n");
+"Command completion event %s\n",
+cmd ? "does not match command" : "on empty list");
return;
}
 
_

-- 
OGAWA Hirofumi 
--
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