[PATCH v4] usb: gadget: aspeed_udc: validate endpoint index for ast udc

[PATCH v4] usb: gadget: aspeed_udc: validate endpoint index for ast udc

[Ma Ke]

We should verify the bound of the array to assure that host
may not manipulate the index to point past endpoint array.

Found by static analysis.

Signed-off-by: Ma Ke <make24@iscas.ac.cn>
---
Changes in v4:
- used a consistent email address to send patches, sorry for my negligence.
Changes in v3:
- added the changelog as suggested.
Changes in v2:
- used the correct macro-defined constants as suggested;
- explained the method for finding and testing vulnerabilities.
---
 drivers/usb/gadget/udc/aspeed_udc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/gadget/udc/aspeed_udc.c b/drivers/usb/gadget/udc/aspeed_udc.c
index 3916c8e2ba01..d972ef4644bc 100644
--- a/drivers/usb/gadget/udc/aspeed_udc.c
+++ b/drivers/usb/gadget/udc/aspeed_udc.c
@@ -1009,6 +1009,8 @@ static void ast_udc_getstatus(struct ast_udc_dev *udc)
 		break;
 	case USB_RECIP_ENDPOINT:
 		epnum = crq.wIndex & USB_ENDPOINT_NUMBER_MASK;
+		if (epnum >= AST_UDC_NUM_ENDPOINTS)
+			goto stall;
 		status = udc->ep[epnum].stopped;
 		break;
 	default:
-- 
2.25.1

Re: [PATCH v4] usb: gadget: aspeed_udc: validate endpoint index for ast udc

[Markus Elfring]

> We should verify the bound of the array to assure that host
> may not manipulate the index to point past endpoint array.

Why did you not choose an imperative wording for your change description?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.10-rc5#n94


> Found by static analysis.

Were any special tools involved?


How do you think about to add any tags (like “Fixes” and “Cc”) accordingly?

Regards,
Markus

Re: [PATCH v4] usb: gadget: aspeed_udc: validate endpoint index for ast udc

[Greg Kroah-Hartman]

On Tue, Jun 25, 2024 at 02:00:15PM +0200, Markus Elfring wrote:
> > We should verify the bound of the array to assure that host
> > may not manipulate the index to point past endpoint array.
> 
> Why did you not choose an imperative wording for your change description?
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.10-rc5#n94
> 
> 
> > Found by static analysis.
> 
> Were any special tools involved?
> 
> 
> How do you think about to add any tags (like “Fixes” and “Cc”) accordingly?


Hi,

This is the semi-friendly patch-bot of Greg Kroah-Hartman.

Markus, you seem to have sent a nonsensical or otherwise pointless
review comment to a patch submission on a Linux kernel developer mailing
list.  I strongly suggest that you not do this anymore.  Please do not
bother developers who are actively working to produce patches and
features with comments that, in the end, are a waste of time.

Patch submitter, please ignore Markus's suggestion; you do not need to
follow it at all.  The person/bot/AI that sent it is being ignored by
almost all Linux kernel maintainers for having a persistent pattern of
behavior of producing distracting and pointless commentary, and
inability to adapt to feedback.  Please feel free to also ignore emails
from them.

thanks,

greg k-h's patch email bot

Re: [PATCH v4] usb: gadget: aspeed_udc: validate endpoint index for ast udc

[Greg Kroah-Hartman]

On Tue, Jun 25, 2024 at 02:00:15PM +0200, Markus Elfring wrote:
> > We should verify the bound of the array to assure that host
> > may not manipulate the index to point past endpoint array.
> 
> Why did you not choose an imperative wording for your change description?
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.10-rc5#n94

Markus, please stop reviewing USB patches.  This is not helpful at all,
and causes new developers extra work for no reason at all.

You have been warned many times about this, and many people have talked
to you about this.  If you continue, you will have to be banned the
mailing lists, again.

greg k-h

Re: [PATCH v4] usb: gadget: aspeed_udc: validate endpoint index for ast udc

[Markus Elfring]

>>> We should verify the bound of the array to assure that host
>>> may not manipulate the index to point past endpoint array.
>>
>> Why did you not choose an imperative wording for your change description?
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.10-rc5#n94
>
> Markus, please stop reviewing USB patches.  This is not helpful at all,
> and causes new developers extra work for no reason at all.

How does this feedback fit to the linked information source?

Regards,
Markus

Re: [PATCH v4] usb: gadget: aspeed_udc: validate endpoint index for ast udc

[Greg Kroah-Hartman]

On Tue, Jun 25, 2024 at 02:50:25PM +0200, Markus Elfring wrote:
> >>> We should verify the bound of the array to assure that host
> >>> may not manipulate the index to point past endpoint array.
> >>
> >> Why did you not choose an imperative wording for your change description?
> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.10-rc5#n94
> >
> > Markus, please stop reviewing USB patches.  This is not helpful at all,
> > and causes new developers extra work for no reason at all.
> 
> How does this feedback fit to the linked information source?

That is not what I wrote.

I wrote, "Please stop reviewing USB patches."

Please stop now.

greg k-h

Re: [PATCH v4] usb: gadget: aspeed_udc: validate endpoint index for ast udc

[Markus Elfring]

>>>>> We should verify the bound of the array to assure that host
>>>>> may not manipulate the index to point past endpoint array.
>>>>
>>>> Why did you not choose an imperative wording for your change description?
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.10-rc5#n94
>>>
>>> Markus, please stop reviewing USB patches.  This is not helpful at all,
>>> and causes new developers extra work for no reason at all.
>>
>> How does this feedback fit to the linked information source?
>
> That is not what I wrote.

You indicated concerns according to patch review processes,
didn't you?

See also:
* Patch submission notes
  https://elixir.bootlin.com/linux/v6.10-rc5/source/Documentation/process/maintainer-tip.rst#L100

* Contributor Covenant Code of Conduct
  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/code-of-conduct.rst?h=v6.10-rc5#n3


> I wrote, "Please stop reviewing USB patches."
>
> Please stop now.

I might be going to influence evolution of this software area in other ways
under other circumstances.

Regards,
Markus

Re: [PATCH v4] usb: gadget: aspeed_udc: validate endpoint index for ast udc

[Greg Kroah-Hartman]

On Tue, Jun 25, 2024 at 05:20:07PM +0200, Markus Elfring wrote:
> >>>>> We should verify the bound of the array to assure that host
> >>>>> may not manipulate the index to point past endpoint array.
> >>>>
> >>>> Why did you not choose an imperative wording for your change description?
> >>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.10-rc5#n94
> >>>
> >>> Markus, please stop reviewing USB patches.  This is not helpful at all,
> >>> and causes new developers extra work for no reason at all.
> >>
> >> How does this feedback fit to the linked information source?
> >
> > That is not what I wrote.
> 
> You indicated concerns according to patch review processes,
> didn't you?
> 
> See also:
> * Patch submission notes
>   https://elixir.bootlin.com/linux/v6.10-rc5/source/Documentation/process/maintainer-tip.rst#L100

This is not the tip tree.

> * Contributor Covenant Code of Conduct
>   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/code-of-conduct.rst?h=v6.10-rc5#n3

I do not see how this is relevant here.

> > I wrote, "Please stop reviewing USB patches."
> >
> > Please stop now.
> 
> I might be going to influence evolution of this software area in other ways
> under other circumstances.

Please take some time and find other projects to help out.

greg k-h

Re: [v4] usb: gadget: aspeed_udc: validate endpoint index for ast udc

[Markus Elfring]

>> You indicated concerns according to patch review processes,
>> didn't you?
>>
>> See also:
>> * Patch submission notes
>>   https://elixir.bootlin.com/linux/v6.10-rc5/source/Documentation/process/maintainer-tip.rst#L100
>
> This is not the tip tree.

I know.

But I got the impression that some information sources
(also from the Linux development reference documentation)
can provide advices and further guidance for recurring patch review concerns.


>> I might be going to influence evolution of this software area in other ways
>> under other circumstances.
>
> Please take some time and find other projects to help out.

I found several opportunities already to improve something through the years.

Concrete example for a selected data representation:
https://patchwork.kernel.org/project/linux-usb/list/?submitter=170303&archive=both

Regards,
Markus

Re: [RFC] usb: Patch review processes?

[Markus Elfring]

>> You indicated concerns according to patch review processes,
>> didn't you?
>>
>> See also:
>> * Patch submission notes
>>   https://elixir.bootlin.com/linux/v6.10-rc5/source/Documentation/process/maintainer-tip.rst#L100
>
> This is not the tip tree.

Would you eventually like to support the creation and maintenance of a document
like “Documentation/process/maintainer-usb.rst”?

Regards,
Markus

Re: [PATCH v4] usb: gadget: aspeed_udc: validate endpoint index for ast udc

[Andrew Jeffery]

On Tue, 2024-06-25 at 10:23 +0800, Ma Ke wrote:
> We should verify the bound of the array to assure that host
> may not manipulate the index to point past endpoint array.
> 
> Found by static analysis.
> 
> Signed-off-by: Ma Ke <make24@iscas.ac.cn>

Reviewed-by: Andrew Jeffery <andrew@codeconstruct.com.au>