linux-kernel - Re: [PATCH v2 2/3] usb: dwc3: gadget: Add support for snps,reserved-endpoints property

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z6zvcF1oe4TklTlK@smile.fi.intel.com>
Date: Wed, 12 Feb 2025 20:58:57 +0200
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Thinh Nguyen <Thinh.Nguyen@...opsys.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Felipe Balbi <balbi@...nel.org>,
	"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, Ferry Toth <fntoth@...il.com>
Subject: Re: [PATCH v2 2/3] usb: dwc3: gadget: Add support for
 snps,reserved-endpoints property

On Wed, Feb 12, 2025 at 12:36:04PM +0200, Andy Shevchenko wrote:
> On Wed, Feb 12, 2025 at 01:10:17AM +0000, Thinh Nguyen wrote:
> > On Mon, Feb 03, 2025, Andy Shevchenko wrote:

...

> > > static bool dwc3_gadget_endpoint_trbs_complete(struct dwc3_ep *dep,
> 
> > >  		for (i = 0; i < DWC3_ENDPOINTS_NUM; i++) {
> > >  			dep = dwc->eps[i];
> > > +			if (!dep)
> > > +				continue;
> > 
> > It should be fine to ignore this check here. Something must be really
> > wrong if there's an interrupt pointing to an endpoint that we shouldn't
> > be touching. If we do add a check, we should print a warn or something
> > here. But that should be a patch separate from this.
> 
> Theoretically everything is possible as it may be HW integration bug,
> for example. But are you asking about separate patch even from the rest
> of the checks? Please, elaborate what do you want to see.

Re-reading the code again, I don't understand. If we get to this loop
ever (theoretically it might be an old IP with the reserved endpoints),
we crash the kernel on the first gap in the array. And since the function
is called on an endpoint, it doesn't mean that all endpoints are allocated,
so I do not see the justification to issue a warning here.
Or do you imply that DWC3_VER_IS_PRIOR(DWC3, 183A) may not have an HW
integration similar to what we have on Intel Merrifield?

For now I'm going to leave this check as is.

...

> > > static void dwc3_endpoint_interrupt(struct dwc3 *dwc,
> 
> > >  	dep = dwc->eps[epnum];
> > > +	if (!dep)
> > > +		return;
> > 
> > Same here.

Here I agree and I will add a warning message. Indeed, if we get and interrupt
for undefined endpoint, something is not correct.

-- 
With Best Regards,
Andy Shevchenko