[PATCH RESEND] xhci: Use xhci_get_virt_ep() to validate ep_index

[PATCH RESEND] xhci: Use xhci_get_virt_ep() to validate ep_index

[Mayank Rana]

ring_doorbell_for_active_rings() API is being called from
multiple context. This specific API tries to get virt_dev
based endpoint using passed slot_id and ep_index. Some caller
API is having check against slot_id and ep_index using
xhci_get_virt_ep() API whereas xhci_handle_cmd_config_ep() API
only check ep_index against -1 value but not upper bound i.e.
EP_CTX_PER_DEV. Hence use xhci_get_virt_ep() API to get virt_dev
based endpoint which checks both slot_id and ep_index to get
valid endpoint.

Signed-off-by: Mayank Rana <quic_mrana@quicinc.com>
---
 drivers/usb/host/xhci-ring.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index d0b6806..3bab4f3 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -62,6 +62,9 @@ static int queue_command(struct xhci_hcd *xhci, struct xhci_command *cmd,
 			 u32 field1, u32 field2,
 			 u32 field3, u32 field4, bool command_must_succeed);
 
+static struct xhci_virt_ep *xhci_get_virt_ep(struct xhci_hcd *xhci,
+			unsigned int slot_id, unsigned int ep_index);
+
 /*
  * Returns zero if the TRB isn't in this segment, otherwise it returns the DMA
  * address of the TRB.
@@ -457,7 +460,9 @@ static void ring_doorbell_for_active_rings(struct xhci_hcd *xhci,
 	unsigned int stream_id;
 	struct xhci_virt_ep *ep;
 
-	ep = &xhci->devs[slot_id]->eps[ep_index];
+	ep = xhci_get_virt_ep(xhci, slot_id, ep_index);
+	if (!ep)
+		return;
 
 	/* A ring has pending URBs if its TD list is not empty */
 	if (!(ep->ep_state & EP_HAS_STREAMS)) {
-- 
2.7.4

Re: [PATCH RESEND] xhci: Use xhci_get_virt_ep() to validate ep_index

[Mathias Nyman]

On 28.4.2022 22.04, Mayank Rana wrote:
> ring_doorbell_for_active_rings() API is being called from
> multiple context. This specific API tries to get virt_dev
> based endpoint using passed slot_id and ep_index. Some caller
> API is having check against slot_id and ep_index using
> xhci_get_virt_ep() API whereas xhci_handle_cmd_config_ep() API
> only check ep_index against -1 value but not upper bound i.e.
> EP_CTX_PER_DEV. Hence use xhci_get_virt_ep() API to get virt_dev
> based endpoint which checks both slot_id and ep_index to get
> valid endpoint.

ep_index upper bound is known to be in range as EP_CTX_PER_DEV is 31,
and ep_index = fls(u32 value)  - 1 - 1; 

We can change to use xhci_get_virt_ep(), but this would be more useful
earlier in xhci_handle_cmd_config_ep() where we touch the ep before
calling ring_doorbell_for_active_rings()

Also note that this codepath is only used for some prototype
xHC controller that probably never made it to the market about 10 years ago. 

Thanks
Mathias

Re: [PATCH RESEND] xhci: Use xhci_get_virt_ep() to validate ep_index

[Greg KH]

On Fri, Apr 29, 2022 at 12:49:59PM +0300, Mathias Nyman wrote:
> On 28.4.2022 22.04, Mayank Rana wrote:
> > ring_doorbell_for_active_rings() API is being called from
> > multiple context. This specific API tries to get virt_dev
> > based endpoint using passed slot_id and ep_index. Some caller
> > API is having check against slot_id and ep_index using
> > xhci_get_virt_ep() API whereas xhci_handle_cmd_config_ep() API
> > only check ep_index against -1 value but not upper bound i.e.
> > EP_CTX_PER_DEV. Hence use xhci_get_virt_ep() API to get virt_dev
> > based endpoint which checks both slot_id and ep_index to get
> > valid endpoint.
> 
> ep_index upper bound is known to be in range as EP_CTX_PER_DEV is 31,
> and ep_index = fls(u32 value)  - 1 - 1; 
> 
> We can change to use xhci_get_virt_ep(), but this would be more useful
> earlier in xhci_handle_cmd_config_ep() where we touch the ep before
> calling ring_doorbell_for_active_rings()
> 
> Also note that this codepath is only used for some prototype
> xHC controller that probably never made it to the market about 10 years ago.

Can we just delete the codepath entirely then?

thanks,

greg k-h

Re: [PATCH RESEND] xhci: Use xhci_get_virt_ep() to validate ep_index

[Mathias Nyman]

On 29.4.2022 13.02, Greg KH wrote:
> On Fri, Apr 29, 2022 at 12:49:59PM +0300, Mathias Nyman wrote:
>> On 28.4.2022 22.04, Mayank Rana wrote:
>>> ring_doorbell_for_active_rings() API is being called from
>>> multiple context. This specific API tries to get virt_dev
>>> based endpoint using passed slot_id and ep_index. Some caller
>>> API is having check against slot_id and ep_index using
>>> xhci_get_virt_ep() API whereas xhci_handle_cmd_config_ep() API
>>> only check ep_index against -1 value but not upper bound i.e.
>>> EP_CTX_PER_DEV. Hence use xhci_get_virt_ep() API to get virt_dev
>>> based endpoint which checks both slot_id and ep_index to get
>>> valid endpoint.
>>
>> ep_index upper bound is known to be in range as EP_CTX_PER_DEV is 31,
>> and ep_index = fls(u32 value)  - 1 - 1; 
>>
>> We can change to use xhci_get_virt_ep(), but this would be more useful
>> earlier in xhci_handle_cmd_config_ep() where we touch the ep before
>> calling ring_doorbell_for_active_rings()
>>
>> Also note that this codepath is only used for some prototype
>> xHC controller that probably never made it to the market about 10 years ago.
> 
> Can we just delete the codepath entirely then?

Probably.
Commit ac9d8fe7c6a8 USB: xhci: Add quirk for Fresco Logic xHCI hardware.
that added this states:

"This patch is for prototype hardware that will be given to other companies
for evaluation purposes only, and should not reach consumer hands.  Fresco
Logic's next chip rev should have this bug fixed."

Should we print some warning instead if this controller is used?
just in case. 
    
Thanks
Mathias

Re: [PATCH RESEND] xhci: Use xhci_get_virt_ep() to validate ep_index

[Greg KH]

On Fri, Apr 29, 2022 at 01:23:50PM +0300, Mathias Nyman wrote:
> On 29.4.2022 13.02, Greg KH wrote:
> > On Fri, Apr 29, 2022 at 12:49:59PM +0300, Mathias Nyman wrote:
> >> On 28.4.2022 22.04, Mayank Rana wrote:
> >>> ring_doorbell_for_active_rings() API is being called from
> >>> multiple context. This specific API tries to get virt_dev
> >>> based endpoint using passed slot_id and ep_index. Some caller
> >>> API is having check against slot_id and ep_index using
> >>> xhci_get_virt_ep() API whereas xhci_handle_cmd_config_ep() API
> >>> only check ep_index against -1 value but not upper bound i.e.
> >>> EP_CTX_PER_DEV. Hence use xhci_get_virt_ep() API to get virt_dev
> >>> based endpoint which checks both slot_id and ep_index to get
> >>> valid endpoint.
> >>
> >> ep_index upper bound is known to be in range as EP_CTX_PER_DEV is 31,
> >> and ep_index = fls(u32 value)  - 1 - 1; 
> >>
> >> We can change to use xhci_get_virt_ep(), but this would be more useful
> >> earlier in xhci_handle_cmd_config_ep() where we touch the ep before
> >> calling ring_doorbell_for_active_rings()
> >>
> >> Also note that this codepath is only used for some prototype
> >> xHC controller that probably never made it to the market about 10 years ago.
> > 
> > Can we just delete the codepath entirely then?
> 
> Probably.
> Commit ac9d8fe7c6a8 USB: xhci: Add quirk for Fresco Logic xHCI hardware.
> that added this states:
> 
> "This patch is for prototype hardware that will be given to other companies
> for evaluation purposes only, and should not reach consumer hands.  Fresco
> Logic's next chip rev should have this bug fixed."
> 
> Should we print some warning instead if this controller is used?
> just in case. 

Would be a good idea, see if that hardware did actually get out into the
wild.

thanks,

greg k-h

Re: [PATCH RESEND] xhci: Use xhci_get_virt_ep() to validate ep_index

[Mathias Nyman]

On 29.4.2022 12.49, Mathias Nyman wrote:
> On 28.4.2022 22.04, Mayank Rana wrote:
>> ring_doorbell_for_active_rings() API is being called from
>> multiple context. This specific API tries to get virt_dev
>> based endpoint using passed slot_id and ep_index. Some caller
>> API is having check against slot_id and ep_index using
>> xhci_get_virt_ep() API whereas xhci_handle_cmd_config_ep() API
>> only check ep_index against -1 value but not upper bound i.e.
>> EP_CTX_PER_DEV. Hence use xhci_get_virt_ep() API to get virt_dev
>> based endpoint which checks both slot_id and ep_index to get
>> valid endpoint.
> 
> ep_index upper bound is known to be in range as EP_CTX_PER_DEV is 31,
> and ep_index = fls(u32 value)  - 1 - 1; 
> 
> We can change to use xhci_get_virt_ep(), but this would be more useful
> earlier in xhci_handle_cmd_config_ep() where we touch the ep before
> calling ring_doorbell_for_active_rings()
> 

After a second look I would appreciate if you could clean up
ep_index checking in xhci_handle_cmd_config_ep()

It currenty does some horrible typecasting.
ep_index is an unsigned int, so the fls() -1 operation might wrap it around.
Checking this was solved by typecasting a -1 to an unsigned int.

if (ep_index != (unsigned int) -1)

Thanks
Mathias

Re: [PATCH RESEND] xhci: Use xhci_get_virt_ep() to validate ep_index

[Mayank Rana]

On 4/29/2022 3:13 AM, Mathias Nyman wrote:
> On 29.4.2022 12.49, Mathias Nyman wrote:
>> On 28.4.2022 22.04, Mayank Rana wrote:
>>> ring_doorbell_for_active_rings() API is being called from
>>> multiple context. This specific API tries to get virt_dev
>>> based endpoint using passed slot_id and ep_index. Some caller
>>> API is having check against slot_id and ep_index using
>>> xhci_get_virt_ep() API whereas xhci_handle_cmd_config_ep() API
>>> only check ep_index against -1 value but not upper bound i.e.
>>> EP_CTX_PER_DEV. Hence use xhci_get_virt_ep() API to get virt_dev
>>> based endpoint which checks both slot_id and ep_index to get
>>> valid endpoint.
>> ep_index upper bound is known to be in range as EP_CTX_PER_DEV is 31,
>> and ep_index = fls(u32 value)  - 1 - 1;
>>
>> We can change to use xhci_get_virt_ep(), but this would be more useful
>> earlier in xhci_handle_cmd_config_ep() where we touch the ep before
>> calling ring_doorbell_for_active_rings()
>>
> After a second look I would appreciate if you could clean up
> ep_index checking in xhci_handle_cmd_config_ep()
>
> It currenty does some horrible typecasting.
> ep_index is an unsigned int, so the fls() -1 operation might wrap it around.
> Checking this was solved by typecasting a -1 to an unsigned int.
>
> if (ep_index != (unsigned int) -1)
>
> Thanks
> Mathias

Thanks Mathias for review and suggestion here.
let me try to clean up xhci_handle_cmd_config_ep() API based ep_index 
usage.

Re: [PATCH RESEND] xhci: Use xhci_get_virt_ep() to validate ep_index

[Mathias Nyman]

On 29.4.2022 22.01, Mayank Rana wrote:
> On 4/29/2022 3:13 AM, Mathias Nyman wrote:
>> On 29.4.2022 12.49, Mathias Nyman wrote:
>>> On 28.4.2022 22.04, Mayank Rana wrote:
>>>> ring_doorbell_for_active_rings() API is being called from
>>>> multiple context. This specific API tries to get virt_dev
>>>> based endpoint using passed slot_id and ep_index. Some caller
>>>> API is having check against slot_id and ep_index using
>>>> xhci_get_virt_ep() API whereas xhci_handle_cmd_config_ep() API
>>>> only check ep_index against -1 value but not upper bound i.e.
>>>> EP_CTX_PER_DEV. Hence use xhci_get_virt_ep() API to get virt_dev
>>>> based endpoint which checks both slot_id and ep_index to get
>>>> valid endpoint.
>>> ep_index upper bound is known to be in range as EP_CTX_PER_DEV is 31,
>>> and ep_index = fls(u32 value)  - 1 - 1;
>>>
>>> We can change to use xhci_get_virt_ep(), but this would be more useful
>>> earlier in xhci_handle_cmd_config_ep() where we touch the ep before
>>> calling ring_doorbell_for_active_rings()
>>>
>> After a second look I would appreciate if you could clean up
>> ep_index checking in xhci_handle_cmd_config_ep()
>>
>> It currenty does some horrible typecasting.
>> ep_index is an unsigned int, so the fls() -1 operation might wrap it around.
>> Checking this was solved by typecasting a -1 to an unsigned int.
>>
>> if (ep_index != (unsigned int) -1)
>>
>> Thanks
>> Mathias
> 
> Thanks Mathias for review and suggestion here.
> let me try to clean up xhci_handle_cmd_config_ep() API based ep_index usage.
> 

Please don't spend too much time on this,
I'm going to remove this code as Greg suggested.

Should have replied earlier, sorry about the delay

Thanks
-Mathias