linux-kernel - Re: [PATCH] usb: gadget: f_midi: Fixing wMaxPacketSize exceeded issue during MIDI bind retries

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: <2024122007-flail-traverse-b7b8@gregkh>
Date: Fri, 20 Dec 2024 16:15:41 +0100
From: Greg KH <gregkh@...uxfoundation.org>
To: Selvarasu Ganesan <selvarasu.g@...sung.com>
Cc: quic_jjohnson@...cinc.com, kees@...nel.org, abdul.rahim@...ahoo.com,
	m.grzeschik@...gutronix.de, linux-usb@...r.kernel.org,
	linux-kernel@...r.kernel.org, jh0801.jung@...sung.com,
	dh10.jung@...sung.com, naushad@...sung.com, akash.m5@...sung.com,
	rc93.raju@...sung.com, taehyun.cho@...sung.com,
	hongpooh.kim@...sung.com, eomji.oh@...sung.com,
	shijie.cai@...sung.com, alim.akhtar@...sung.com,
	stable@...r.kernel.org
Subject: Re: [PATCH] usb: gadget: f_midi: Fixing wMaxPacketSize exceeded
 issue during MIDI bind retries

On Fri, Dec 20, 2024 at 07:02:06PM +0530, Selvarasu Ganesan wrote:
> 
> On 12/20/2024 5:54 PM, Greg KH wrote:
> > On Wed, Dec 18, 2024 at 03:51:50PM +0530, Selvarasu Ganesan wrote:
> >> On 12/18/2024 11:01 AM, Greg KH wrote:
> >>> On Sun, Dec 08, 2024 at 08:53:20PM +0530, Selvarasu Ganesan wrote:
> >>>> The current implementation sets the wMaxPacketSize of bulk in/out
> >>>> endpoints to 1024 bytes at the end of the f_midi_bind function. However,
> >>>> in cases where there is a failure in the first midi bind attempt,
> >>>> consider rebinding.
> >>> What considers rebinding?  Your change does not modify that.
> >> Hi Greg,
> >> Thanks for your review comments.
> >>
> >>
> >> Here the term "rebind" in this context refers to attempting to bind the
> >> MIDI function a second time in certain scenarios.
> >> The situations where rebinding is considered include:
> >>
> >>    * When there is a failure in the first UDC write attempt, which may be
> >>      caused by other functions bind along with MIDI
> >>    * Runtime composition change : Example : MIDI,ADB to MIDI. Or MIDI to
> >>      MIDI,ADB
> >>
> >> The issue arises during the second time the "f_midi_bind" function is
> >> called. The problem lies in the fact that the size of
> >> "bulk_in_desc.wMaxPacketSize" is set to 1024 during the first call,
> >> which exceeds the hardware capability of the dwc3 TX/RX FIFO
> >> (ep->maxpacket_limit = 512).
> > Ok, but then why not properly reset ALL of the options/values when a
> > failure happens, not just this one when the initialization happens
> > again?  Odds are you might be missing the change of something else here
> > as well, right?
> Are you suggesting that we reset the entire value of 
> usb_endpoint_descriptor before call usb_ep_autoconfig? If so, Sorry I am 
> not clear on your reasoning for wanting to reset all options/values. 
> After all, all values will be overwritten 
> afterusb_ep_autoconfig.Additionally, the wMaxPacketSize is the only 
> value being checked during the EP claim process (usb_ep_autoconfig), and 
> it has caused issues where claiming wMaxPacketSize is grater than 
> ep->maxpacket_limit.

Then fix up that value on failure, if things fail you should reset it
back to a "known good state", right?  And what's wrong with resetting
all of the values anyway, wouldn't that be the correct thing to do?

> > Also, cleaning up from an error is a better thing to do than forcing
> > something to be set all the time when you don't have anything gone
> > wrong.
> As I previously mentioned, this is a general approach to set 
> wMaxPacketSize before claiming the endpoint. This is because the 
> usb_ep_autoconfig treats endpoint descriptors as if they were full 
> speed. Following the same pattern as other function drivers, that 
> approach allows us to claim the EP with using a full-speed descriptor. 
> We can use the same approach here instead of resetting wMaxPacketSize 
> every time.
> 
> The following provided code is used to claim an EP with a full-speed 
> bulk descriptor in MIDI. Its also working solution.  But, We thinking 
> that it may unnecessarily complicate the code as it only utilizes the 
> full descriptor for obtaining the EP address here. What you think shall 
> we go with below approach instead of rest wMaxPacketSize before call 
> usb_ep_autoconfig?

I don't know, what do you think is best to do?  You are the one having
problems and will need to fix any bugs that your changes will cause :)

thanks,

greg k-h