sound/usb/midi2.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-)
If usb_alloc_coherent() or usb_urb_ep_type_check() fail, we should release
the resources previously allocated.
Fixes: ff49d1df79ae ("ALSA: usb-audio: USB MIDI 2.0 UMP support")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
sound/usb/midi2.c | 17 ++++++++++++++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/sound/usb/midi2.c b/sound/usb/midi2.c
index a27e244650c8..4109c82adff6 100644
--- a/sound/usb/midi2.c
+++ b/sound/usb/midi2.c
@@ -302,7 +302,8 @@ static int alloc_midi_urbs(struct snd_usb_midi2_endpoint *ep)
ctx->urb = usb_alloc_urb(0, GFP_KERNEL);
if (!ctx->urb) {
dev_err(&ep->dev->dev, "URB alloc failed\n");
- return -ENOMEM;
+ err = -ENOMEM;
+ goto err_free_all;
}
ctx->ep = ep;
buffer = usb_alloc_coherent(ep->dev, len, GFP_KERNEL,
@@ -310,7 +311,8 @@ static int alloc_midi_urbs(struct snd_usb_midi2_endpoint *ep)
if (!buffer) {
dev_err(&ep->dev->dev,
"URB buffer alloc failed (size %d)\n", len);
- return -ENOMEM;
+ err = -ENOMEM;
+ goto err_free_cur_urb;
}
if (ep->interval)
usb_fill_int_urb(ctx->urb, ep->dev, ep->pipe,
@@ -322,13 +324,22 @@ static int alloc_midi_urbs(struct snd_usb_midi2_endpoint *ep)
if (err < 0) {
dev_err(&ep->dev->dev, "invalid MIDI EP %x\n",
endpoint);
- return err;
+ goto err_free_cur_dma;
}
ctx->urb->transfer_flags = URB_NO_TRANSFER_DMA_MAP;
ep->num_urbs++;
}
ep->urb_free = ep->urb_free_mask = GENMASK(ep->num_urbs - 1, 0);
return 0;
+
+err_free_cur_dma:
+ usb_free_coherent(ep->dev, len, buffer, ctx->urb->transfer_dma);
+err_free_cur_urb:
+ usb_free_urb(ctx->urb);
+ ctx->urb = NULL;
+err_free_all:
+ free_midi_urbs(ep);
+ return err;
}
static struct snd_usb_midi2_endpoint *
--
2.34.1On Sun, 03 Sep 2023 15:06:00 +0200,
Christophe JAILLET wrote:
>
> If usb_alloc_coherent() or usb_urb_ep_type_check() fail, we should release
> the resources previously allocated.
Those are freed in the caller side, start_input_streams() instead.
thanks,
Takashi
>
> Fixes: ff49d1df79ae ("ALSA: usb-audio: USB MIDI 2.0 UMP support")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> sound/usb/midi2.c | 17 ++++++++++++++---
> 1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/sound/usb/midi2.c b/sound/usb/midi2.c
> index a27e244650c8..4109c82adff6 100644
> --- a/sound/usb/midi2.c
> +++ b/sound/usb/midi2.c
> @@ -302,7 +302,8 @@ static int alloc_midi_urbs(struct snd_usb_midi2_endpoint *ep)
> ctx->urb = usb_alloc_urb(0, GFP_KERNEL);
> if (!ctx->urb) {
> dev_err(&ep->dev->dev, "URB alloc failed\n");
> - return -ENOMEM;
> + err = -ENOMEM;
> + goto err_free_all;
> }
> ctx->ep = ep;
> buffer = usb_alloc_coherent(ep->dev, len, GFP_KERNEL,
> @@ -310,7 +311,8 @@ static int alloc_midi_urbs(struct snd_usb_midi2_endpoint *ep)
> if (!buffer) {
> dev_err(&ep->dev->dev,
> "URB buffer alloc failed (size %d)\n", len);
> - return -ENOMEM;
> + err = -ENOMEM;
> + goto err_free_cur_urb;
> }
> if (ep->interval)
> usb_fill_int_urb(ctx->urb, ep->dev, ep->pipe,
> @@ -322,13 +324,22 @@ static int alloc_midi_urbs(struct snd_usb_midi2_endpoint *ep)
> if (err < 0) {
> dev_err(&ep->dev->dev, "invalid MIDI EP %x\n",
> endpoint);
> - return err;
> + goto err_free_cur_dma;
> }
> ctx->urb->transfer_flags = URB_NO_TRANSFER_DMA_MAP;
> ep->num_urbs++;
> }
> ep->urb_free = ep->urb_free_mask = GENMASK(ep->num_urbs - 1, 0);
> return 0;
> +
> +err_free_cur_dma:
> + usb_free_coherent(ep->dev, len, buffer, ctx->urb->transfer_dma);
> +err_free_cur_urb:
> + usb_free_urb(ctx->urb);
> + ctx->urb = NULL;
> +err_free_all:
> + free_midi_urbs(ep);
> + return err;
> }
>
> static struct snd_usb_midi2_endpoint *
> --
> 2.34.1
>
Le 03/09/2023 à 16:23, Takashi Iwai a écrit :
> On Sun, 03 Sep 2023 15:06:00 +0200,
> Christophe JAILLET wrote:
>>
>> If usb_alloc_coherent() or usb_urb_ep_type_check() fail, we should release
>> the resources previously allocated.
>
> Those are freed in the caller side, start_input_streams() instead.
Thanks for the fast review.
Hmpm, If IIUC, resources allocated *before* the ending "ep->num_urbs++"
still need to be freed here, otherwise free_midi_urbs() in the caller
will not free them.
Do you agree?
If yes, I can send v2 which would look like:
usb_alloc_urb()
if (err)
return -ENOMEM
usb_alloc_coherent()
if (err) {
usb_free_urb()
urb = NULL
return -ENOMEM
}
usb_urb_ep_type_check()
if (err) {
usb_free_coherent()
usb_free_urb()
urb = NULL
return -err
}
Or, if yuo prefer, with an error handling path just like below, but
without the final free_midi_urbs() + a comment explaining that the
caller does this part of job instead.
CJ
>
>
> thanks,
>
> Takashi
>
>>
>> Fixes: ff49d1df79ae ("ALSA: usb-audio: USB MIDI 2.0 UMP support")
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>> ---
>> sound/usb/midi2.c | 17 ++++++++++++++---
>> 1 file changed, 14 insertions(+), 3 deletions(-)
>>
>> diff --git a/sound/usb/midi2.c b/sound/usb/midi2.c
>> index a27e244650c8..4109c82adff6 100644
>> --- a/sound/usb/midi2.c
>> +++ b/sound/usb/midi2.c
>> @@ -302,7 +302,8 @@ static int alloc_midi_urbs(struct snd_usb_midi2_endpoint *ep)
>> ctx->urb = usb_alloc_urb(0, GFP_KERNEL);
>> if (!ctx->urb) {
>> dev_err(&ep->dev->dev, "URB alloc failed\n");
>> - return -ENOMEM;
>> + err = -ENOMEM;
>> + goto err_free_all;
>> }
>> ctx->ep = ep;
>> buffer = usb_alloc_coherent(ep->dev, len, GFP_KERNEL,
>> @@ -310,7 +311,8 @@ static int alloc_midi_urbs(struct snd_usb_midi2_endpoint *ep)
>> if (!buffer) {
>> dev_err(&ep->dev->dev,
>> "URB buffer alloc failed (size %d)\n", len);
>> - return -ENOMEM;
>> + err = -ENOMEM;
>> + goto err_free_cur_urb;
>> }
>> if (ep->interval)
>> usb_fill_int_urb(ctx->urb, ep->dev, ep->pipe,
>> @@ -322,13 +324,22 @@ static int alloc_midi_urbs(struct snd_usb_midi2_endpoint *ep)
>> if (err < 0) {
>> dev_err(&ep->dev->dev, "invalid MIDI EP %x\n",
>> endpoint);
>> - return err;
>> + goto err_free_cur_dma;
>> }
>> ctx->urb->transfer_flags = URB_NO_TRANSFER_DMA_MAP;
>> ep->num_urbs++;
>> }
>> ep->urb_free = ep->urb_free_mask = GENMASK(ep->num_urbs - 1, 0);
>> return 0;
>> +
>> +err_free_cur_dma:
>> + usb_free_coherent(ep->dev, len, buffer, ctx->urb->transfer_dma);
>> +err_free_cur_urb:
>> + usb_free_urb(ctx->urb);
>> + ctx->urb = NULL;
>> +err_free_all:
>> + free_midi_urbs(ep);
>> + return err;
>> }
>>
>> static struct snd_usb_midi2_endpoint *
>> --
>> 2.34.1
>>
>
On Sun, 03 Sep 2023 17:04:47 +0200,
Christophe JAILLET wrote:
>
> Le 03/09/2023 à 16:23, Takashi Iwai a écrit :
> > On Sun, 03 Sep 2023 15:06:00 +0200,
> > Christophe JAILLET wrote:
> >>
> >> If usb_alloc_coherent() or usb_urb_ep_type_check() fail, we should release
> >> the resources previously allocated.
> >
> > Those are freed in the caller side, start_input_streams() instead.
>
> Thanks for the fast review.
>
> Hmpm, If IIUC, resources allocated *before* the ending
> "ep->num_urbs++" still need to be freed here, otherwise
> free_midi_urbs() in the caller will not free them.
>
> Do you agree?
>
> If yes, I can send v2 which would look like:
> usb_alloc_urb()
> if (err)
> return -ENOMEM
>
> usb_alloc_coherent()
> if (err) {
> usb_free_urb()
> urb = NULL
> return -ENOMEM
> }
>
> usb_urb_ep_type_check()
> if (err) {
> usb_free_coherent()
> usb_free_urb()
> urb = NULL
> return -err
> }
>
> Or, if yuo prefer, with an error handling path just like below, but
> without the final free_midi_urbs() + a comment explaining that the
> caller does this part of job instead.
Indeed. The fix would be rather a oneliner like below, though:
--- a/sound/usb/midi2.c
+++ b/sound/usb/midi2.c
@@ -265,7 +265,7 @@ static void free_midi_urbs(struct snd_usb_midi2_endpoint *ep)
if (!ep)
return;
- for (i = 0; i < ep->num_urbs; ++i) {
+ for (i = 0; i < NUM_URBS; ++i) {
ctx = &ep->urbs[i];
if (!ctx->urb)
break;
That was the intended behavior of free_midi_urbs().
Takashi
Le 03/09/2023 à 18:37, Takashi Iwai a écrit :
> On Sun, 03 Sep 2023 17:04:47 +0200,
...
> Indeed. The fix would be rather a oneliner like below, though:
Looks much better than mine :)
I let you send the patch, it is your solution.
Just for my understanding, how is snd_ump_ops used, especially .open?
I've not been able to figure out where it was called.
In alloc_midi_urbs(), if usb_alloc_coherent() fails, then
ctx->urb->transfer_buffer could be anything because usb_fill_xxx_urb()
is not called.
So there could be an edge case where your fix could still be incomplete.
For the start_input_streams() caller, this is fine, because the
corresponding memory is kzalloc()'ed in start_input_streams() at some
point, but I've not been able to check for snd_usb_midi_v2_open().
CJ
>
> --- a/sound/usb/midi2.c
> +++ b/sound/usb/midi2.c
> @@ -265,7 +265,7 @@ static void free_midi_urbs(struct snd_usb_midi2_endpoint *ep)
>
> if (!ep)
> return;
> - for (i = 0; i < ep->num_urbs; ++i) {
> + for (i = 0; i < NUM_URBS; ++i) {
> ctx = &ep->urbs[i];
> if (!ctx->urb)
> break;
>
> That was the intended behavior of free_midi_urbs().
>
>
> Takashi
>
On Sun, 03 Sep 2023 21:42:55 +0200,
Christophe JAILLET wrote:
>
> Le 03/09/2023 à 18:37, Takashi Iwai a écrit :
> > On Sun, 03 Sep 2023 17:04:47 +0200,
> ...
>
> > Indeed. The fix would be rather a oneliner like below, though:
>
> Looks much better than mine :)
>
> I let you send the patch, it is your solution.
>
>
>
> Just for my understanding, how is snd_ump_ops used, especially .open?
> I've not been able to figure out where it was called.
It's called via rawmidi open (the snd_ump_endpoint is a sort of child
class of snd_rawmidi).
> In alloc_midi_urbs(), if usb_alloc_coherent() fails, then
> ctx->urb->transfer_buffer could be anything because usb_fill_xxx_urb()
> is not called.
> So there could be an edge case where your fix could still be incomplete.
Each URB is allocated in the loop via usb_alloc_urb(), and it does
zero-initialize the object, hence the buffer is supposed to be NULL
until it's set up via usb_fill_xxx().
thanks,
Takashi
> For the start_input_streams() caller, this is fine, because the
> corresponding memory is kzalloc()'ed in start_input_streams() at some
> point, but I've not been able to check for snd_usb_midi_v2_open().
>
> CJ
>
> >
> > --- a/sound/usb/midi2.c
> > +++ b/sound/usb/midi2.c
> > @@ -265,7 +265,7 @@ static void free_midi_urbs(struct snd_usb_midi2_endpoint *ep)
> > if (!ep)
> > return;
> > - for (i = 0; i < ep->num_urbs; ++i) {
> > + for (i = 0; i < NUM_URBS; ++i) {
> > ctx = &ep->urbs[i];
> > if (!ctx->urb)
> > break;
> >
> > That was the intended behavior of free_midi_urbs().
> >
> >
> > Takashi
> >
>
On Mon, 04 Sep 2023 16:08:15 +0200,
Takashi Iwai wrote:
>
> On Sun, 03 Sep 2023 21:42:55 +0200,
> Christophe JAILLET wrote:
> >
> > Le 03/09/2023 à 18:37, Takashi Iwai a écrit :
> > > On Sun, 03 Sep 2023 17:04:47 +0200,
> > ...
> > For the start_input_streams() caller, this is fine, because the
> > corresponding memory is kzalloc()'ed in start_input_streams() at some
> > point, but I've not been able to check for snd_usb_midi_v2_open().
Oh I overlooked that point. Yes, it's a missing call, although the
memory leaks as free_midi_urbs() is called also at the destructor,
free_midi2_endpoint(), too. But it's definitely better to call at the
error path, too. Will fix it up together and submit the proper fix
patch.
thanks,
Takashi
> >
> > CJ
> >
> > >
> > > --- a/sound/usb/midi2.c
> > > +++ b/sound/usb/midi2.c
> > > @@ -265,7 +265,7 @@ static void free_midi_urbs(struct snd_usb_midi2_endpoint *ep)
> > > if (!ep)
> > > return;
> > > - for (i = 0; i < ep->num_urbs; ++i) {
> > > + for (i = 0; i < NUM_URBS; ++i) {
> > > ctx = &ep->urbs[i];
> > > if (!ctx->urb)
> > > break;
> > >
> > > That was the intended behavior of free_midi_urbs().
> > >
> > >
> > > Takashi
> > >
> >
>
© 2016 - 2025 Red Hat, Inc.