There is a possible null-pointer dereference related to the wait-completion
synchronization mechanism in the function xhci_setup_device().

Consider the following execution scenario:

in drivers/usb/host/xhci-mem.c:
  xhci_mem_init()       // 2381
    if (!xhci->dcbaa)   // 2431  xhci->dcbaa can be NULL
    xhci_mem_cleanup()  // 2548
      xhci_cleanup_command_queue()        // 1888
in drivers/usb/host/xhci-ring.c
        xhci_complete_del_and_free_cmd()  // 1619
          complete(cmd->completion);      // 1608

The variable xhci->dcbaa is checked by an if statement at Line 2431. If
xhci->dcbaa is NULL, xhci_mem_cleanup() will be called at Line 2548, which
eventually leads to complete() at Line 1608 that wakes up the
wait_for_completion().

Consider the wait_for_completion() in drivers/usb/host/xhci.c
  xhci_setup_device()
    wait_for_completion(command->completion);       // 4179
    le64_to_cpu(xhci->dcbaa->dev_context_ptrs...)); // 4237

The variable xhci->dcbaa is dereferenced (without being rechecked) after 
the wait_for_completion(), which can introduce a possible null-pointer
dereference.

To address this issue, a NULL check is added to ensure the variable
xhci->dcbaa is not NULL when xhci_dbg_trace() is called.

This potential bug was discovered using an experimental static analysis
tool developed by our team. The tool deduces complete() and 
wait_for_completion() pairs using alias analysis. It then applies data
flow analysis to detect null-pointer dereferences across synchronization
points.

Fixes: 84a99f6fc5d4 ("xhci: add traces for debug messages in xhci_address_device()")
Signed-off-by: Tuo Li <islituo@gmail.com>
---
 drivers/usb/host/xhci.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 899c0effb5d3..1c7322a4e301 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -4229,12 +4229,14 @@ static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev,
 	temp_64 = xhci_read_64(xhci, &xhci->op_regs->dcbaa_ptr);
 	xhci_dbg_trace(xhci, trace_xhci_dbg_address,
 			"Op regs DCBAA ptr = %#016llx", temp_64);
-	xhci_dbg_trace(xhci, trace_xhci_dbg_address,
-		"Slot ID %d dcbaa entry @%p = %#016llx",
-		udev->slot_id,
-		&xhci->dcbaa->dev_context_ptrs[udev->slot_id],
-		(unsigned long long)
-		le64_to_cpu(xhci->dcbaa->dev_context_ptrs[udev->slot_id]));
+	if (xhci->dcbaa) {
+		xhci_dbg_trace(xhci, trace_xhci_dbg_address,
+			"Slot ID %d dcbaa entry @%p = %#016llx",
+			udev->slot_id,
+			&xhci->dcbaa->dev_context_ptrs[udev->slot_id],
+			(unsigned long long)
+			le64_to_cpu(xhci->dcbaa->dev_context_ptrs[udev->slot_id]));
+	}
 	xhci_dbg_trace(xhci, trace_xhci_dbg_address,
 			"Output Context DMA address = %#08llx",
 			(unsigned long long)virt_dev->out_ctx->dma);
-- 
2.43.0

Hi,

> There is a possible null-pointer dereference related to the
> wait-completion synchronization mechanism in the function
> xhci_setup_device().
> 
> Consider the following execution scenario:
> 
> in drivers/usb/host/xhci-mem.c:
>   xhci_mem_init()       // 2381
>     if (!xhci->dcbaa)   // 2431  xhci->dcbaa can be NULL
>     xhci_mem_cleanup()  // 2548
>       xhci_cleanup_command_queue()        // 1888
> in drivers/usb/host/xhci-ring.c
>         xhci_complete_del_and_free_cmd()  // 1619
>           complete(cmd->completion);      // 1608
> 
> The variable xhci->dcbaa is checked by an if statement at Line 2431.
> If xhci->dcbaa is NULL, xhci_mem_cleanup() will be called at Line
> 2548, which eventually leads to complete() at Line 1608 that wakes up
> the wait_for_completion().
> 
> Consider the wait_for_completion() in drivers/usb/host/xhci.c
>   xhci_setup_device()
>     wait_for_completion(command->completion);       // 4179
>     le64_to_cpu(xhci->dcbaa->dev_context_ptrs...)); // 4237
> 
> The variable xhci->dcbaa is dereferenced (without being rechecked)
> after the wait_for_completion(), which can introduce a possible
> null-pointer dereference.

I think it's a false positive, because xhci_mem_init() is only called
on driver initialization and on resume from suspend, immediately after
an explicit xhci_mem_cleanup(), which would have woken up any waiting
tasks (and likely made them crash), but there shouldn't be any.

By the way, is your analyzer not finding the issue that any call to
xhci_mem_cleanup() wakes up everybody waiting on the command queue and
then sets a bunch of things (including xhci->dcbaa) to NULL shortly
thereafter? This race looks like it shouldn't be harder to detect than
the things you are doing already.

Of course, all of that would bring more false positives too. Basically,
you discovered that calling a cleanup function while something else is
still pending, or having some work already pending while initialization
isn't yet complete, may not end well.

> To address this issue, a NULL check is added to ensure the variable
> xhci->dcbaa is not NULL when xhci_dbg_trace() is called.

That's still just bandaid and not a real fix. With static analysis one
must always review the output and ask if the problem is real, what it
really means for the code and what to do about it. Simply ignoring the
missing pointer is rarely the right solution.

Regards,
Michal

> By the way, is your analyzer not finding the issue that any call to
> xhci_mem_cleanup() wakes up everybody waiting on the command queue and
> then sets a bunch of things (including xhci->dcbaa) to NULL shortly
> thereafter? This race looks like it shouldn't be harder to detect than
> the things you are doing already.

Actually, I think neither the above nor the issue you reported is a
problem at all, because xhci_setup_device() checks if command->status
is COMP_COMMAND_ABORTED and returns early in this case. At a quick
glance, other command handlers do the same.

This status code is always assigned by xhci_cleanup_command_queue().

Regards,
Michal