2013/12/16 David Laight <David.Laight@xxxxxxxxxx>:
>> From: Wang, Lin X
>> 2013/12/16 David Laight <David.Laight@xxxxxxxxxx>:
>> >> From: Of Wang, Lin X
>> >> Sent: 15 December 2013 14:21
>> >> To: Sarah Sharp (sarah.a.sharp@xxxxxxxxxxxxxxx)
>> >> Cc: linux-usb@xxxxxxxxxxxxxxx
>> >> Subject: [PATCH] xhci: fix array index out of the bounds in function last_trb() and
>> >> last_trb_on_last_seg()
>> >>
>> >> In function last_trb() and last_trb_on_last_seg(), incorrect array index value 'TRBS_PER_SEGMENT'
>> is
>> >> used to determine the last element in an event ring segment, which lead to the out-of-bounds
>> >> of index. But according to the current logic of event ring operation, this "bug" brings no problems
>> >> and the driver works as desired. This patch only fix this array index out-of-bounds issue and there
>> is
>> >> no need no modify corresponding ring operation.
>> >>
>> >> 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 d26cd94..3f93b07 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]) &&
>> >
>> > Taking the address of the element beyond the end of an array is valid C.
>>
>> It's vaild in C doesn't mean that it‘s reasonable, besides,
>> 'TRBS_PER_SEGMENT - 1' is widely used in current implementation to
>> identily the last element in an array, using 'TRBS_PER_SEGMENT' here
>> result in
>> a lack of unity in style and easy to get lost.
>
> There is a big difference between the 'event' ring (written by the xhci hardware)
> and all the other rings (read by the hardware).
> The last entry of the non-event rings is always a LINK trb (pointing to the
> next ring segment) so must not be overwritten.
> The event rings have a separate array that defines the segments and the hardware
> writes real data into the last ring entry.
Hi David,
I know all these difference clearly, inc_deq() is indeed a common
function for different rings, but lasr_trb() & last_trb_on_last_seg()
inside it use different condition to determine the last trb in an
event ring and an non-event ring; and sorry, i still not find why last
trb in an event ring skipped by H/W according to the current logic.
Thanks!
Lin
>
> The operation of the event ring is different from the other rings and it was
> (IMHO) a mistake to have used common functions for the two.
>
> David
>
>
>
--
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
