[PATCH] ALSA: usb-audio: Fix a potential memory leak in scarlett2_init_notify()
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 *
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;@@ -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;@@ -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;+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;@@ -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;@@ -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;+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
participants (2)
-
Christophe JAILLET -
Takashi Iwai