| Message ID | 20200505061446.21850-1-sallenki@codeaurora.org (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 3c6f8cb92c9178fc0c66b580ea3df1fa3ac1155a |
| Headers | show |
| Series | usb: xhci: Fix NULL pointer dereference as part of completion | expand |
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
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;
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(-)