| 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
| ||
|
Message-ID: <Z5uyacORwkWDbqYm-jkeeping@inmusicbrands.com>
Date: Thu, 30 Jan 2025 17:10:01 +0000
From: John Keeping <jkeeping@...usicbrands.com>
To: Takashi Iwai <tiwai@...e.de>
Cc: linux-usb@...r.kernel.org, stable@...r.kernel.org,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Kees Cook <kees@...nel.org>, Abdul Rahim <abdul.rahim@...ahoo.com>,
Michael Grzeschik <m.grzeschik@...gutronix.de>,
Jeff Johnson <quic_jjohnson@...cinc.com>,
Felipe Balbi <balbi@...com>, Daniel Mack <zonque@...il.com>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] usb: gadget: f_midi: fix MIDI Streaming descriptor
lengths
On Thu, Jan 30, 2025 at 01:17:59PM +0100, Takashi Iwai wrote:
> On Thu, 30 Jan 2025 11:59:05 +0100,
> John Keeping wrote:
> >
> > On Thu, Jan 30, 2025 at 11:50:07AM +0100, Takashi Iwai wrote:
> > > On Wed, 29 Jan 2025 18:31:35 +0100,
> > > John Keeping wrote:
> > > >
> > > > On Wed, Jan 29, 2025 at 05:40:04PM +0100, Takashi Iwai wrote:
> > > > > On Wed, 29 Jan 2025 17:05:19 +0100,
> > > > > John Keeping wrote:
> > > > > >
> > > > > > In the two loops before setting the MIDIStreaming descriptors,
> > > > > > ms_in_desc.baAssocJackID[] has entries written for "in_ports" values and
> > > > > > ms_out_desc.baAssocJackID[] has entries written for "out_ports" values.
> > > > > > But the counts and lengths are set the other way round in the
> > > > > > descriptors.
> > > > > >
> > > > > > Fix the descriptors so that the bNumEmbMIDIJack values and the
> > > > > > descriptor lengths match the number of entries populated in the trailing
> > > > > > arrays.
> > > > >
> > > > > Are you sure that it's a correct change?
> > > > >
> > > > > IIUC, the in_ports and out_ports parameters are for external IN and
> > > > > OUT jacks, where an external OUT jack is connected to an embedded IN
> > > > > jack, and an external IN jack is connected to an embedded OUT jack.
> > > >
> > > > I think it depends how the in_ports and out_ports values in configfs are
> > > > interpreted. However, the case where in_ports != out_ports has been
> > > > broken since these files were added!
> > > >
> > > > Without this change, setting in_ports=4 out_ports=2 we end up with:
> > > >
> > > > Endpoint Descriptor:
> > > > [...]
> > > > bEndpointAddress 0x01 EP 1 OUT
> > > > [...]
> > > > MIDIStreaming Endpoint Descriptor:
> > > > bLength 8
> > > > bDescriptorType 37
> > > > bDescriptorSubtype 1 (Invalid)
> > > > bNumEmbMIDIJack 4
> > > > baAssocJackID( 0) 9
> > > > baAssocJackID( 1) 11
> > > > baAssocJackID( 2) 9
> > > > baAssocJackID( 3) 0
> > > > Endpoint Descriptor:
> > > > [...]
> > > > bEndpointAddress 0x81 EP 1 IN
> > > > [...]
> > > > MIDIStreaming Endpoint Descriptor:
> > > > bLength 6
> > > > bDescriptorType 37
> > > > bDescriptorSubtype 1 (Invalid)
> > > > bNumEmbMIDIJack 2
> > > > baAssocJackID( 0) 2
> > > > baAssocJackID( 1) 4
> > > >
> > > > Note that baAssocJackID values 2 and 3 on the OUT endpoint are wrong.
> > > >
> > > > From the same config, the jack definitions are:
> > > >
> > > > 1: IN External
> > > > 2: OUT Embedded, source 1
> > > > 3: IN External
> > > > 4: OUT Embedded, source 3
> > > > 5: IN External
> > > > 6: OUT Embedded, source 5
> > > > 7: IN External
> > > > 8: OUT Embedded, source 7
> > > >
> > > > 9: IN Embedded
> > > > 10: OUT External, source 9
> > > > 11: IN Embedded
> > > > 12: OUT External, source 11
> > > >
> > > > So it seems that the first 2 entries in each endpoint list are correct.
> > > > For the OUT endpoint, jacks 9 and 11 are embedded IN jacks and for the
> > > > IN endpoint, jacks 2 and 4 are embedded OUT jacks.
> > > >
> > > > The problem is that the OUT endpoint lists two extra invalid jack IDs
> > > > and the IN endpoint should list jacks 6 and 8 but does not.
> > > >
> > > > After applying this patch, the endpoint descriptors for the same config
> > > > are:
> > > >
> > > > Endpoint Descriptor:
> > > > [...]
> > > > bEndpointAddress 0x01 EP 1 OUT
> > > > [...]
> > > > MIDIStreaming Endpoint Descriptor:
> > > > bLength 6
> > > > bDescriptorType 37
> > > > bDescriptorSubtype 1 (Invalid)
> > > > bNumEmbMIDIJack 2
> > > > baAssocJackID( 0) 9
> > > > baAssocJackID( 1) 11
> > > > Endpoint Descriptor:
> > > > [...]
> > > > bEndpointAddress 0x81 EP 1 IN
> > > > [...]
> > > > MIDIStreaming Endpoint Descriptor:
> > > > bLength 8
> > > > bDescriptorType 37
> > > > bDescriptorSubtype 1 (Invalid)
> > > > bNumEmbMIDIJack 4
> > > > baAssocJackID( 0) 2
> > > > baAssocJackID( 1) 4
> > > > baAssocJackID( 2) 6
> > > > baAssocJackID( 3) 8
> > > >
> > > > Which lists all the jack IDs where they should be.
> > >
> > > Hmm, I don't get your point. The embedded IN is paired with the
> > > external OUT. That's the intended behavior, no?
> >
> > Yes, all the endpoint assignments are correct - when they appear in the
> > lists!
> >
> > The issue is setting bNumEmbMIDIJack and bLength in the MIDIStreaming
> > Endpoint Descriptors. Without this patch these are set the wrong way
> > round so either some ports do not appear or there are bogus entries
> > containing uninitialized stack memory.
>
> OK, now point taken. The main problem here is the definition of
> in_port and out_ports aren't really clear. If in_ports really
> corresponds to external IN jacks, then we may correct rather like:
>
> --- a/drivers/usb/gadget/function/f_midi.c
> +++ b/drivers/usb/gadget/function/f_midi.c
> @@ -968,7 +968,7 @@ static int f_midi_bind(struct usb_configuration *c, struct usb_function *f)
> midi_function[i++] = (struct usb_descriptor_header *) out_emb;
>
> /* link it to the endpoint */
> - ms_in_desc.baAssocJackID[n] = out_emb->bJackID;
> + ms_out_desc.baAssocJackID[n] = out_emb->bJackID;
> }
>
> /* configure the external OUT jacks, each linked to an embedded IN jack */
> @@ -996,7 +996,7 @@ static int f_midi_bind(struct usb_configuration *c, struct usb_function *f)
> midi_function[i++] = (struct usb_descriptor_header *) out_ext;
>
> /* link it to the endpoint */
> - ms_out_desc.baAssocJackID[n] = in_emb->bJackID;
> + ms_in_desc.baAssocJackID[n] = in_emb->bJackID;
> }
>
> /* configure the endpoint descriptors ... */
>
> OTOH, the current code will make the actual appearance other way
> round, likely more confusing. So I believe your fix makes sense.
It always takes me a few minutes to get used to working in device-side
USB because IN and OUT refer to the host's view.
But I think "in_ports" and "out_ports" here are consistent with that.
"in_ports" are the ports that send MIDI into the USB host, and
"out_ports" are the ports receiving MIDI from the USB host over an OUT
endpoint.
> But it'd be helpful to extend the description a bit more to clarify
> this confusion. I guess this confusion came from the association
> between the embedded and external jacks, and the patch corrects it.
I'll rewrite the commit message to include some more of this context and
send a v2 later today or tomorrow.
Regards,
John
> > > > > > Cc: stable@...r.kernel.org
> > > > > > Fixes: c8933c3f79568 ("USB: gadget: f_midi: allow a dynamic number of input and output ports")
> > > > > > Signed-off-by: John Keeping <jkeeping@...usicbrands.com>
> > > > > > ---
> > > > > > drivers/usb/gadget/function/f_midi.c | 8 ++++----
> > > > > > 1 file changed, 4 insertions(+), 4 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
> > > > > > index 837fcdfa3840f..6cc3d86cb4774 100644
> > > > > > --- a/drivers/usb/gadget/function/f_midi.c
> > > > > > +++ b/drivers/usb/gadget/function/f_midi.c
> > > > > > @@ -1000,11 +1000,11 @@ static int f_midi_bind(struct usb_configuration *c, struct usb_function *f)
> > > > > > }
> > > > > >
> > > > > > /* configure the endpoint descriptors ... */
> > > > > > - ms_out_desc.bLength = USB_DT_MS_ENDPOINT_SIZE(midi->in_ports);
> > > > > > - ms_out_desc.bNumEmbMIDIJack = midi->in_ports;
> > > > > > + ms_out_desc.bLength = USB_DT_MS_ENDPOINT_SIZE(midi->out_ports);
> > > > > > + ms_out_desc.bNumEmbMIDIJack = midi->out_ports;
> > > > > >
> > > > > > - ms_in_desc.bLength = USB_DT_MS_ENDPOINT_SIZE(midi->out_ports);
> > > > > > - ms_in_desc.bNumEmbMIDIJack = midi->out_ports;
> > > > > > + ms_in_desc.bLength = USB_DT_MS_ENDPOINT_SIZE(midi->in_ports);
> > > > > > + ms_in_desc.bNumEmbMIDIJack = midi->in_ports;
> > > > > >
> > > > > > /* ... and add them to the list */
> > > > > > endpoint_descriptor_index = i;
> > > > > > --
> > > > > > 2.48.1
> > > > > >
> > > > > >
Powered by blists - more mailing lists