linux-kernel - Re: [PATCH v2] ALSA: pcm: fix multiple memory leak bugs

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:   Wed, 07 Aug 2019 09:18:52 +0200
From:   Takashi Iwai <tiwai@...e.de>
To:     Wenwen Wang <wenwen@...uga.edu>
Cc:     "moderated list:SOUND" <alsa-devel@...a-project.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Allison Randal <allison@...utok.net>,
        Jaroslav Kysela <perex@...ex.cz>,
        Takashi Iwai <tiwai@...e.com>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] ALSA: pcm: fix multiple memory leak bugs

On Wed, 07 Aug 2019 09:09:59 +0200,
Wenwen Wang wrote:
> 
> In hiface_pcm_init(), 'rt' is firstly allocated through kzalloc(). Later
> on, hiface_pcm_init_urb() is invoked to initialize 'rt->out_urbs[i]'. In
> hiface_pcm_init_urb(), 'rt->out_urbs[i].buffer' is allocated through
> kzalloc().  However, if hiface_pcm_init_urb() fails, both 'rt' and
> 'rt->out_urbs[i].buffer' are not deallocated, leading to memory leak bugs.
> Also, 'rt->out_urbs[i].buffer' is not deallocated if snd_pcm_new() fails.
> 
> To fix the above issues, free 'rt' and 'rt->out_urbs[i].buffer'.
> 
> Signed-off-by: Wenwen Wang <wenwen@...uga.edu>
> ---
>  sound/usb/hiface/pcm.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/sound/usb/hiface/pcm.c b/sound/usb/hiface/pcm.c
> index 14fc1e1..9b132aa 100644
> --- a/sound/usb/hiface/pcm.c
> +++ b/sound/usb/hiface/pcm.c
> @@ -599,12 +599,18 @@ int hiface_pcm_init(struct hiface_chip *chip, u8
> extra_freq)
>         for (i = 0; i < PCM_N_URBS; i++) {
>                 ret = hiface_pcm_init_urb(&rt->out_urbs[i], chip, OUT_EP,
>                                     hiface_pcm_out_urb_handler);
> -               if (ret < 0)
> +               if (ret < 0) {
> +                       for (; i >= 0; i--)
> +                               kfree(rt->out_urbs[i].buffer);
> +                       kfree(rt);
>                         return ret;
> +               }
>         }
> 
>         ret = snd_pcm_new(chip->card, "USB-SPDIF Audio", 0, 1, 0, &pcm);
>         if (ret < 0) {
> +               for (i = 0; i < PCM_N_URBS; i++)
> +                       kfree(rt->out_urbs[i].buffer);
>                 kfree(rt);
>                 dev_err(&chip->dev->dev, "Cannot create pcm instance\n");
>                 return ret;

The fixes look correct, but since we can unconditionally call kfree()
for NULL, both error paths can be unified as:

	for (i = 0; i < PCM_N_URBS; i++)
		kfree(rt->out_urbs[i].buffer);
	kfree(rt);

and this would be better to be put in the common path at the end and
do "goto error" or such from both places.


BTW, your patch doesn't seem cleanly applicable in anyway because the
tabs are converted to spaces.  Please check the mail setup.

Also, please try to make the subject line more unique.  This is about
hiface driver, so "ALSA: hiface: xxx" should be more appropriate.


thanks,

Takashi