linux-kernel - Re: [PATCH] ALSA: usb-audio: Fix a potential memory leak in scarlett2_init

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]

Date:   Sun, 03 Sep 2023 18:37:19 +0200
From:   Takashi Iwai <tiwai@...e.de>
To:     Christophe JAILLET <christophe.jaillet@...adoo.fr>
Cc:     Jaroslav Kysela <perex@...ex.cz>, Takashi Iwai <tiwai@...e.com>,
        linux-kernel@...r.kernel.org, kernel-janitors@...r.kernel.org,
        alsa-devel@...a-project.org
Subject: Re: [PATCH] ALSA: usb-audio: Fix a potential memory leak in scarlett2_init_notify()

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