| Message ID | fc275ed315b9157952dcf2744ee7bdb78defdb5f.1693746347.git.christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ALSA: usb-audio: Fix a potential memory leak in scarlett2_init_notify() | expand |
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, 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 > > > > > >
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 *
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(-)