On Tue, Nov 12, 2013 at 08:46:27AM +0000, Wang, Lin X wrote:
> Hi Sarah,
Hi Lin,
Sorry for taking so long to respond to this patch.
Yes, I think this is a bug, but the end result is harmless. Please send
an updated version of the patch to me, and Cc the linux-usb mailing
list.
> I found a potential array index out of bounds issue in xhci driver, according to my understanding, the number of TRBs inside a segment is "TRBS_PER_SEGMENT" which is 64 in current driver,
> here "seg->trbs[TRBS_PER_SEGMENT]" means the 65th element in TRB array which cross the border of seg->trbs[]. Is this a bug which should be fixed?
>
> [85767.775733] address: trb = ffff88023ee267f0
> [85767.775734] address: seg-> seg->trbs[TRBS_PER_SEGMENT-1] = ffff88023ee267f0 // for a array with size equal to TRBS_PER_SEGMENT, seg->trbs[TRBS_PER_SEGMENT-1]should be the last element
> in this segment, seg->trbs[TRBS_PER_SEGMENT] as the last element in an array doesn't make sense.
>
> [85767.775735] address: seg->trbs[TRBS_PER_SEGMENT] = ffff88023ee26800 // if segments in segment_pool are not contiguous, &seg->trbs[TRBS_PER_SEGMENT-1] + 16 would not equal to &seg->trbs[TRBS_PER_SEGMENT],
> here might be a coincidence, or "return trb == &seg->trbs[TRBS_PER_SEGMENT]" will never be true.
> >8-------------------------------------------------------------8<
>
> This patch fix array index out of the bounds in function ast_trb() and last_trb_on_last_seg().
This should be last_trb() instead of ast_trb(). Also, please line-wrap
your commit message bodies to at least 80 characters, except when
copy-pasting dmesg or warning output. Finally, this patch doesn't apply
to my for-usb-next-queue branch. Please rebase it.
More comments below.
> Signed-off-by: Lin Wang <lin.x.wang@xxxxxxxxx>
>
> ---
> drivers/usb/host/xhci-ring.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 1e2f3f4..7dc8a24 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -98,21 +98,21 @@ static bool last_trb_on_last_seg(struct xhci_hcd *xhci, struct xhci_ring *ring,
> struct xhci_segment *seg, union xhci_trb *trb)
> {
> if (ring == xhci->event_ring)
> - return (trb == &seg->trbs[TRBS_PER_SEGMENT]) &&
> + return (trb == &seg->trbs[TRBS_PER_SEGMENT - 1]) &&
> (seg->next == xhci->event_ring->first_seg);
> else
> return le32_to_cpu(trb->link.control) & LINK_TOGGLE;
> }
>
> -/* Is this TRB a link TRB or was the last TRB the last TRB in this event ring
> - * segment? I.e. would the updated event TRB pointer step off the end of the
> - * event seg?
> +/* Is this TRB a link TRB or was the last TRB in this event ring segment?
> + * I.e. would the updated event TRB pointer step off the end of the event
> + * seg?
> */
> static int last_trb(struct xhci_hcd *xhci, struct xhci_ring *ring,
> struct xhci_segment *seg, union xhci_trb *trb)
> {
> if (ring == xhci->event_ring)
> - return trb == &seg->trbs[TRBS_PER_SEGMENT];
> + return trb == &seg->trbs[TRBS_PER_SEGMENT - 1];
> else
> return TRB_TYPE_LINK_LE32(trb->link.control);
> }
> --
> 1.7.9.5
>
I drilled down into the behavior of the functions that call both
last_trb and last_trb_on_last_seg, and without your patch, they still
work as desired.
last_trb and last_trb_on_last_seg are called in:
next_trb()
- called from xhci_find_new_dequeue_state, td_to_noop,
xhci_cmd_to_noop, process_isoc_td, and process_bulk_intr_td. These
functions are never called for event rings, so the code you changed
never runs.
update_ring_for_set_deq_completion()
prepare_ring()
inc_enq()
- not called for event rings.
inc_deq()
- is called for event rings.
static void inc_deq(struct xhci_hcd *xhci, struct xhci_ring *ring)
{
unsigned long long addr;
ring->deq_updates++;
/*
* If this is not event ring, and the dequeue pointer
* is not on a link TRB, there is one more usable TRB
*/
if (ring->type != TYPE_EVENT &&
!last_trb(xhci, ring, ring->deq_seg, ring->dequeue))
ring->num_trbs_free++;
do {
/*
* Update the dequeue pointer further if that was a link TRB or
* we're at the end of an event ring segment (which doesn't have
* link TRBS)
*/
if (last_trb(xhci, ring, ring->deq_seg, ring->dequeue)) {
if (ring->type == TYPE_EVENT &&
last_trb_on_last_seg(xhci, ring,
ring->deq_seg, ring->dequeue)) {
ring->cycle_state ^= 1;
}
ring->deq_seg = ring->deq_seg->next;
ring->dequeue = ring->deq_seg->trbs;
} else {
ring->dequeue++;
}
} while (last_trb(xhci, ring, ring->deq_seg, ring->dequeue));
addr = (unsigned long long) xhci_trb_virt_to_dma(ring->deq_seg, ring->dequeue);
}
The current inc_deq code's behavior is this:
- The first call to last_trb is skipped, because the && short circuits,
since this is an event ring.
- last_trb is called while incrementing the dequeue pointer. The event
ring has no link TRBs. If the host has processed the last TRB on the
event ring segment, and inc_deq() is called:
- the first call to last_trb will fail, because the TRB isn't one
past the segment
- the else condition triggers, and ring->dequeue will get incremented
to point to a TRB past the segment
- the while case is evaluated, last_trb will return true, we'll loop
- last_trb() succeeds again, we move the the ring->dequeue pointer
and deq_seg into the next segment.
- since this is all just pointer math, with no dereferences of the
dequeue pointer, moving the dequeue pointer to one TRB past the ring
is harmless.
If your patch is applied last_trb and last_trb_on_last_segment both
return true if the TRB pointer is on the last TRB in the segment. That
means the new behavior becomes:
inc_deq()
- The first call to last_trb is skipped, because the && short circuits,
since this is an event ring.
- last_trb is called while incrementing the dequeue pointer. The event
ring has no link TRBs. If the host has processed the last TRB on the
event ring segment, and inc_deq() is called:
- the first call to last_trb will succeed
- we move the the ring->dequeue pointer and deq_seg into the next segment.
So, yes, you did find a bug, but my analysis shows it's harmless because
it's just pointer math, no dereferences. Please send me a revised
patch, and I'll queue it to the usb-next branch for 3.14.
Sarah Sharp
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
