Re: Patch "xhci: fix possible null pointer deref during xhci urb enqueue" has been added to the 6.7-stab

Re: Patch "xhci: fix possible null pointer deref during xhci urb enqueue" has been added to the 6.7-stab

[Pascal Ernster]

[2024-02-23 18:45] Sasha Levin:
> This is a note to let you know that I've just added the patch titled
> 
>      xhci: fix possible null pointer deref during xhci urb enqueue
> 
> to the 6.7-stable tree which can be found at:
>      http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
> 
> The filename of the patch is:
>       xhci-fix-possible-null-pointer-deref-during-xhci-urb.patch
> and it can be found in the queue-6.7 subdirectory.
> 
> If you, or anyone else, feels it should not be added to the stable tree,
> please let <stable@vger.kernel.org> know about it.
> 
> 
> 
> commit fb9100c2c6b7b172650ba25283cc4cf9af1d082c
> Author: Mathias Nyman <mathias.nyman@linux.intel.com>
> Date:   Fri Dec 1 17:06:47 2023 +0200
> 
>      xhci: fix possible null pointer deref during xhci urb enqueue
>      
>      [ Upstream commit e2e2aacf042f52854c92775b7800ba668e0bdfe4 ]
>      
>      There is a short gap between urb being submitted and actually added to the
>      endpoint queue (linked). If the device is disconnected during this time
>      then usb core is not yet aware of the pending urb, and device may be freed
>      just before xhci_urq_enqueue() continues, dereferencing the freed device.
>      
>      Freeing the device is protected by the xhci spinlock, so make sure we take
>      and keep the lock while checking that device exists, dereference it, and
>      add the urb to the queue.
>      
>      Remove the unnecessary URB check, usb core checks it before calling
>      xhci_urb_enqueue()
>      
>      Suggested-by: Kuen-Han Tsai <khtsai@google.com>
>      Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
>      Link: https://lore.kernel.org/r/20231201150647.1307406-20-mathias.nyman@linux.intel.com
>      Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>      Signed-off-by: Sasha Levin <sashal@kernel.org>
> 
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 884b0898d9c95..ddb686301af5d 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -1522,24 +1522,7 @@ static int xhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
>   	struct urb_priv	*urb_priv;
>   	int num_tds;
>   
> -	if (!urb)
> -		return -EINVAL;
> -	ret = xhci_check_args(hcd, urb->dev, urb->ep,
> -					true, true, __func__);
> -	if (ret <= 0)
> -		return ret ? ret : -EINVAL;
> -
> -	slot_id = urb->dev->slot_id;
>   	ep_index = xhci_get_endpoint_index(&urb->ep->desc);
> -	ep_state = &xhci->devs[slot_id]->eps[ep_index].ep_state;
> -
> -	if (!HCD_HW_ACCESSIBLE(hcd))
> -		return -ESHUTDOWN;
> -
> -	if (xhci->devs[slot_id]->flags & VDEV_PORT_ERROR) {
> -		xhci_dbg(xhci, "Can't queue urb, port error, link inactive\n");
> -		return -ENODEV;
> -	}
>   
>   	if (usb_endpoint_xfer_isoc(&urb->ep->desc))
>   		num_tds = urb->number_of_packets;
> @@ -1578,12 +1561,35 @@ static int xhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
>   
>   	spin_lock_irqsave(&xhci->lock, flags);
>   
> +	ret = xhci_check_args(hcd, urb->dev, urb->ep,
> +			      true, true, __func__);
> +	if (ret <= 0) {
> +		ret = ret ? ret : -EINVAL;
> +		goto free_priv;
> +	}
> +
> +	slot_id = urb->dev->slot_id;
> +
> +	if (!HCD_HW_ACCESSIBLE(hcd)) {
> +		ret = -ESHUTDOWN;
> +		goto free_priv;
> +	}
> +
> +	if (xhci->devs[slot_id]->flags & VDEV_PORT_ERROR) {
> +		xhci_dbg(xhci, "Can't queue urb, port error, link inactive\n");
> +		ret = -ENODEV;
> +		goto free_priv;
> +	}
> +
>   	if (xhci->xhc_state & XHCI_STATE_DYING) {
>   		xhci_dbg(xhci, "Ep 0x%x: URB %p submitted for non-responsive xHCI host.\n",
>   			 urb->ep->desc.bEndpointAddress, urb);
>   		ret = -ESHUTDOWN;
>   		goto free_priv;
>   	}
> +
> +	ep_state = &xhci->devs[slot_id]->eps[ep_index].ep_state;
> +
>   	if (*ep_state & (EP_GETTING_STREAMS | EP_GETTING_NO_STREAMS)) {
>   		xhci_warn(xhci, "WARN: Can't enqueue URB, ep in streams transition state %x\n",
>   			  *ep_state);


Hi, this patch is causing my laptop (Dell Precision 7530) to crash 
during early boot with a kernel 6.7.6 with all the patches from your 
current stable-queue applied on top 
(https://git.kernel.org/pub/scm/linux/kernel/git/stable/stable-queue.git/tree/queue-6.7?id=8294c8ea0d96dc8271e87053f7b6731ee5b986ca).

Booting with "module_blacklist=xhci_pci,xhci_pci_renesas" stops the 
crashes. This patch was already thrown out a few weeks ago because it 
was causing problems:

https://lore.kernel.org/stable/2024020331-confetti-ducking-8afb@gregkh/


Regards
Pascal

Re: Patch "xhci: fix possible null pointer deref during xhci urb enqueue" has been added to the 6.7-stab

[Greg Kroah-Hartman]

On Sun, Feb 25, 2024 at 06:31:04PM +0100, Pascal Ernster wrote:
> [2024-02-23 18:45] Sasha Levin:
> > This is a note to let you know that I've just added the patch titled
> > 
> >      xhci: fix possible null pointer deref during xhci urb enqueue
> > 
> > to the 6.7-stable tree which can be found at:
> >      http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
> > 
> > The filename of the patch is:
> >       xhci-fix-possible-null-pointer-deref-during-xhci-urb.patch
> > and it can be found in the queue-6.7 subdirectory.
> > 
> > If you, or anyone else, feels it should not be added to the stable tree,
> > please let <stable@vger.kernel.org> know about it.
> > 
> > 
> > 
> > commit fb9100c2c6b7b172650ba25283cc4cf9af1d082c
> > Author: Mathias Nyman <mathias.nyman@linux.intel.com>
> > Date:   Fri Dec 1 17:06:47 2023 +0200
> > 
> >      xhci: fix possible null pointer deref during xhci urb enqueue
> >      [ Upstream commit e2e2aacf042f52854c92775b7800ba668e0bdfe4 ]
> >      There is a short gap between urb being submitted and actually added to the
> >      endpoint queue (linked). If the device is disconnected during this time
> >      then usb core is not yet aware of the pending urb, and device may be freed
> >      just before xhci_urq_enqueue() continues, dereferencing the freed device.
> >      Freeing the device is protected by the xhci spinlock, so make sure we take
> >      and keep the lock while checking that device exists, dereference it, and
> >      add the urb to the queue.
> >      Remove the unnecessary URB check, usb core checks it before calling
> >      xhci_urb_enqueue()
> >      Suggested-by: Kuen-Han Tsai <khtsai@google.com>
> >      Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> >      Link: https://lore.kernel.org/r/20231201150647.1307406-20-mathias.nyman@linux.intel.com
> >      Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >      Signed-off-by: Sasha Levin <sashal@kernel.org>
> > 
> > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> > index 884b0898d9c95..ddb686301af5d 100644
> > --- a/drivers/usb/host/xhci.c
> > +++ b/drivers/usb/host/xhci.c
> > @@ -1522,24 +1522,7 @@ static int xhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
> >   	struct urb_priv	*urb_priv;
> >   	int num_tds;
> > -	if (!urb)
> > -		return -EINVAL;
> > -	ret = xhci_check_args(hcd, urb->dev, urb->ep,
> > -					true, true, __func__);
> > -	if (ret <= 0)
> > -		return ret ? ret : -EINVAL;
> > -
> > -	slot_id = urb->dev->slot_id;
> >   	ep_index = xhci_get_endpoint_index(&urb->ep->desc);
> > -	ep_state = &xhci->devs[slot_id]->eps[ep_index].ep_state;
> > -
> > -	if (!HCD_HW_ACCESSIBLE(hcd))
> > -		return -ESHUTDOWN;
> > -
> > -	if (xhci->devs[slot_id]->flags & VDEV_PORT_ERROR) {
> > -		xhci_dbg(xhci, "Can't queue urb, port error, link inactive\n");
> > -		return -ENODEV;
> > -	}
> >   	if (usb_endpoint_xfer_isoc(&urb->ep->desc))
> >   		num_tds = urb->number_of_packets;
> > @@ -1578,12 +1561,35 @@ static int xhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
> >   	spin_lock_irqsave(&xhci->lock, flags);
> > +	ret = xhci_check_args(hcd, urb->dev, urb->ep,
> > +			      true, true, __func__);
> > +	if (ret <= 0) {
> > +		ret = ret ? ret : -EINVAL;
> > +		goto free_priv;
> > +	}
> > +
> > +	slot_id = urb->dev->slot_id;
> > +
> > +	if (!HCD_HW_ACCESSIBLE(hcd)) {
> > +		ret = -ESHUTDOWN;
> > +		goto free_priv;
> > +	}
> > +
> > +	if (xhci->devs[slot_id]->flags & VDEV_PORT_ERROR) {
> > +		xhci_dbg(xhci, "Can't queue urb, port error, link inactive\n");
> > +		ret = -ENODEV;
> > +		goto free_priv;
> > +	}
> > +
> >   	if (xhci->xhc_state & XHCI_STATE_DYING) {
> >   		xhci_dbg(xhci, "Ep 0x%x: URB %p submitted for non-responsive xHCI host.\n",
> >   			 urb->ep->desc.bEndpointAddress, urb);
> >   		ret = -ESHUTDOWN;
> >   		goto free_priv;
> >   	}
> > +
> > +	ep_state = &xhci->devs[slot_id]->eps[ep_index].ep_state;
> > +
> >   	if (*ep_state & (EP_GETTING_STREAMS | EP_GETTING_NO_STREAMS)) {
> >   		xhci_warn(xhci, "WARN: Can't enqueue URB, ep in streams transition state %x\n",
> >   			  *ep_state);
> 
> 
> Hi, this patch is causing my laptop (Dell Precision 7530) to crash during
> early boot with a kernel 6.7.6 with all the patches from your current
> stable-queue applied on top (https://git.kernel.org/pub/scm/linux/kernel/git/stable/stable-queue.git/tree/queue-6.7?id=8294c8ea0d96dc8271e87053f7b6731ee5b986ca).
> 
> Booting with "module_blacklist=xhci_pci,xhci_pci_renesas" stops the crashes.
> This patch was already thrown out a few weeks ago because it was causing
> problems:
> 
> https://lore.kernel.org/stable/2024020331-confetti-ducking-8afb@gregkh/

Yeah, thanks, this one needs to be dropped as it depended on previous
patches to work properly, I'll go drop them now, thanks!

greg k-h