[PATCH] usb: xhci: Fix NULL pointer dereference as part of completion

[PATCH] usb: xhci: Fix NULL pointer dereference as part of completion

[Sriharsha Allenki]

On platforms with IOMMU enabled, multiple SGs can be
coalesced into one by the IOMMU driver. In that case
the SG list processing as part of the completion of
a urb on a bulk endpoint can result into a NULL pointer
dereference with the below stack dump.

<6> Unable to handle kernel NULL pointer dereference at virtual address 0000000c
<6> pgd = c0004000
<6> [0000000c] *pgd=00000000
<6> Internal error: Oops: 5 [#1] PREEMPT SMP ARM
<2> PC is at xhci_queue_bulk_tx+0x454/0x80c
<2> LR is at xhci_queue_bulk_tx+0x44c/0x80c
<2> pc : [<c08907c4>]    lr : [<c08907bc>]    psr: 000000d3
<2> sp : ca337c80  ip : 00000000  fp : ffffffff
<2> r10: 00000000  r9 : 50037000  r8 : 00004000
<2> r7 : 00000000  r6 : 00004000  r5 : 00000000  r4 : 00000000
<2> r3 : 00000000  r2 : 00000082  r1 : c2c1a200  r0 : 00000000
<2> Flags: nzcv  IRQs off  FIQs off  Mode SVC_32  ISA ARM  Segment none
<2> Control: 10c0383d  Table: b412c06a  DAC: 00000051
<6> Process usb-storage (pid: 5961, stack limit = 0xca336210)
<snip>
<2> [<c08907c4>] (xhci_queue_bulk_tx)
<2> [<c0881b3c>] (xhci_urb_enqueue)
<2> [<c0831068>] (usb_hcd_submit_urb)
<2> [<c08350b4>] (usb_sg_wait)
<2> [<c089f384>] (usb_stor_bulk_transfer_sglist)
<2> [<c089f2c0>] (usb_stor_bulk_srb)
<2> [<c089fe38>] (usb_stor_Bulk_transport)
<2> [<c089f468>] (usb_stor_invoke_transport)
<2> [<c08a11b4>] (usb_stor_control_thread)
<2> [<c014a534>] (kthread)

The above NULL pointer dereference is the result of block_len and
the sent_len set to zero after the first SG of the list when IOMMU
driver is enabled. Because of this the loop of processing the SGs
has run more than num_sgs which resulted in a sg_next on the
last SG of the list which has SG_END set.

Fix this by check for the sg before any attributes of the sg are
accessed.

Fixes: f9c589e142d04 ("xhci: TD-fragment, align the unsplittable case with a bounce buffer")
Cc: stable@vger.kernel.org
Signed-off-by: Sriharsha Allenki <sallenki@codeaurora.org>
---
 drivers/usb/host/xhci-ring.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index a78787bb5133..18141b38f7bf 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -3399,8 +3399,8 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
 			/* New sg entry */
 			--num_sgs;
 			sent_len -= block_len;
-			if (num_sgs != 0) {
-				sg = sg_next(sg);
+			sg = sg_next(sg);
+			if (num_sgs != 0 && sg) {
 				block_len = sg_dma_len(sg);
 				addr = (u64) sg_dma_address(sg);
 				addr += sent_len;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

Re: [PATCH] usb: xhci: Fix NULL pointer dereference as part of completion

[Mathias Nyman]

On 5.5.2020 9.14, Sriharsha Allenki wrote:
> On platforms with IOMMU enabled, multiple SGs can be
> coalesced into one by the IOMMU driver. In that case
> the SG list processing as part of the completion of
> a urb on a bulk endpoint can result into a NULL pointer
> dereference with the below stack dump.
> 
> <6> Unable to handle kernel NULL pointer dereference at virtual address 0000000c
> <6> pgd = c0004000
> <6> [0000000c] *pgd=00000000
> <6> Internal error: Oops: 5 [#1] PREEMPT SMP ARM
> <2> PC is at xhci_queue_bulk_tx+0x454/0x80c
> <2> LR is at xhci_queue_bulk_tx+0x44c/0x80c
> <2> pc : [<c08907c4>]    lr : [<c08907bc>]    psr: 000000d3
> <2> sp : ca337c80  ip : 00000000  fp : ffffffff
> <2> r10: 00000000  r9 : 50037000  r8 : 00004000
> <2> r7 : 00000000  r6 : 00004000  r5 : 00000000  r4 : 00000000
> <2> r3 : 00000000  r2 : 00000082  r1 : c2c1a200  r0 : 00000000
> <2> Flags: nzcv  IRQs off  FIQs off  Mode SVC_32  ISA ARM  Segment none
> <2> Control: 10c0383d  Table: b412c06a  DAC: 00000051
> <6> Process usb-storage (pid: 5961, stack limit = 0xca336210)
> <snip>
> <2> [<c08907c4>] (xhci_queue_bulk_tx)
> <2> [<c0881b3c>] (xhci_urb_enqueue)
> <2> [<c0831068>] (usb_hcd_submit_urb)
> <2> [<c08350b4>] (usb_sg_wait)
> <2> [<c089f384>] (usb_stor_bulk_transfer_sglist)
> <2> [<c089f2c0>] (usb_stor_bulk_srb)
> <2> [<c089fe38>] (usb_stor_Bulk_transport)
> <2> [<c089f468>] (usb_stor_invoke_transport)
> <2> [<c08a11b4>] (usb_stor_control_thread)
> <2> [<c014a534>] (kthread)
> 
> The above NULL pointer dereference is the result of block_len and
> the sent_len set to zero after the first SG of the list when IOMMU
> driver is enabled. Because of this the loop of processing the SGs
> has run more than num_sgs which resulted in a sg_next on the
> last SG of the list which has SG_END set.
> 
> Fix this by check for the sg before any attributes of the sg are
> accessed.
> 
> Fixes: f9c589e142d04 ("xhci: TD-fragment, align the unsplittable case with a bounce buffer")
> Cc: stable@vger.kernel.org
> Signed-off-by: Sriharsha Allenki <sallenki@codeaurora.org>
> ---
>  drivers/usb/host/xhci-ring.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index a78787bb5133..18141b38f7bf 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -3399,8 +3399,8 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
>  			/* New sg entry */
>  			--num_sgs;
>  			sent_len -= block_len;
> -			if (num_sgs != 0) {
> -				sg = sg_next(sg);
> +			sg = sg_next(sg);
> +			if (num_sgs != 0 && sg) {
>  				block_len = sg_dma_len(sg);
>  				addr = (u64) sg_dma_address(sg);
>  				addr += sent_len;
> 

Nice catch, thanks.

Adding to queue.

-Mathias