diff mbox series

[v14,01/53] xhci: fix possible null pointer dereference at secondary interrupter removal

Message ID 20240208231406.27397-2-quic_wcheng@quicinc.com
State New
Headers
Series Introduce QC USB SND audio offloading support |

Commit Message

Wesley Cheng Feb. 8, 2024, 11:13 p.m. UTC 
  From: Mathias Nyman <mathias.nyman@linux.intel.com>

Don't try to remove a secondary interrupter that is known to be invalid.
Also check if the interrupter is valid inside the spinlock that protects
the array of interrupters.

Found by smatch static checker

Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/linux-usb/ffaa0a1b-5984-4a1f-bfd3-9184630a97b9@moroto.mountain/
Fixes: c99b38c41234 ("xhci: add support to allocate several interrupters")
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
Link: https://lore.kernel.org/r/20240125152737.2983959-2-mathias.nyman@linux.intel.com
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
---
 drivers/usb/host/xhci-mem.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Comments

Greg KH Feb. 9, 2024, 10:22 a.m. UTC | #1 
On Thu, Feb 08, 2024 at 03:13:14PM -0800, Wesley Cheng wrote:
> From: Mathias Nyman <mathias.nyman@linux.intel.com>
> 
> Don't try to remove a secondary interrupter that is known to be invalid.
> Also check if the interrupter is valid inside the spinlock that protects
> the array of interrupters.
> 
> Found by smatch static checker
> 
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/linux-usb/ffaa0a1b-5984-4a1f-bfd3-9184630a97b9@moroto.mountain/
> Fixes: c99b38c41234 ("xhci: add support to allocate several interrupters")
> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> Link: https://lore.kernel.org/r/20240125152737.2983959-2-mathias.nyman@linux.intel.com
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>

Wait, this is already in my tree, right?  Why keep sending it?

confused,

greg k-h
Wesley Cheng Feb. 9, 2024, 8:16 p.m. UTC | #2 
Hi Greg,

On 2/9/2024 2:22 AM, Greg KH wrote:
> On Thu, Feb 08, 2024 at 03:13:14PM -0800, Wesley Cheng wrote:
>> From: Mathias Nyman <mathias.nyman@linux.intel.com>
>>
>> Don't try to remove a secondary interrupter that is known to be invalid.
>> Also check if the interrupter is valid inside the spinlock that protects
>> the array of interrupters.
>>
>> Found by smatch static checker
>>
>> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
>> Closes: https://lore.kernel.org/linux-usb/ffaa0a1b-5984-4a1f-bfd3-9184630a97b9@moroto.mountain/
>> Fixes: c99b38c41234 ("xhci: add support to allocate several interrupters")
>> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
>> Link: https://lore.kernel.org/r/20240125152737.2983959-2-mathias.nyman@linux.intel.com
>> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
> 
> Wait, this is already in my tree, right?  Why keep sending it?
> 

Sorry, I noticed this yesterday night as well when I was preparing some 
changes to push elsewhere.  Will remove the ones I saw that were already 
present on usb-next.

Thanks
Wesley Cheng
diff mbox series

Patch

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 4460fa7e9fab..d00d4d937236 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -1861,14 +1861,14 @@  void xhci_remove_secondary_interrupter(struct usb_hcd *hcd, struct xhci_interrup
 	struct xhci_hcd *xhci = hcd_to_xhci(hcd);
 	unsigned int intr_num;
 
+	spin_lock_irq(&xhci->lock);
+
 	/* interrupter 0 is primary interrupter, don't touch it */
-	if (!ir || !ir->intr_num || ir->intr_num >= xhci->max_interrupters)
+	if (!ir || !ir->intr_num || ir->intr_num >= xhci->max_interrupters) {
 		xhci_dbg(xhci, "Invalid secondary interrupter, can't remove\n");
-
-	/* fixme, should we check xhci->interrupter[intr_num] == ir */
-	/* fixme locking */
-
-	spin_lock_irq(&xhci->lock);
+		spin_unlock_irq(&xhci->lock);
+		return;
+	}
 
 	intr_num = ir->intr_num;