linux-kernel - Re: [PATCH] ALSA: usb-audio: Fix double-free in snd_usb_add_audio

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]

Message-ID: <s5hh9fmenkr.wl-tiwai@suse.de>
Date:	Thu, 31 Mar 2016 16:20:20 +0200
From:	Takashi Iwai <tiwai@...e.de>
To:	Vladis Dronov <vdronov@...hat.com>
Cc:	Jaroslav Kysela <perex@...ex.cz>, alsa-devel@...a-project.org,
	linux-kernel@...r.kernel.org, linux-sound@...r.kernel.org
Subject: Re: [PATCH] ALSA: usb-audio: Fix double-free in snd_usb_add_audio_stream()

On Thu, 31 Mar 2016 16:03:55 +0200,
Vladis Dronov wrote:
> 
> Hello, Takashi, all,
> 
> > No, it has nothing to do with the double-free bug itself.  Such an
> > optimization shouldn't be put in a fix patch
> 
> This piece of code move alone fixes the double-free bug in
> create_fixed_stream_quirk(), so I believe it is related.

The merits you pointed are irrelevant from the double-free bug.

> Besides, a lot of stuff
> is created and initialized in snd_usb_add_audio_stream() and while I do not see
> another use-after-free bug, it could be there. By moving this code we avoid
> these potential bugs we have not hit yet.

It's a different issue.  The only judgment now is which one is clearer
to understand.  The code efficiency isn't a question for this bug,
since the error condition is very rare, and it's no hot path, after
all.

> But anyway. If you still believe this code should not be moved, please, confirm,
> I'll suggest the next patch version without it.

Right, I don't want to move it.


> > Vladis, if you take someone's patch as the base, you have to carry the
> > original authorship somewhere...
> 
> Yes, I was thinking about it, I was just not sure how should I do it. Will the
> following form be fine? Or somehow else?
> 
> Based on a patch by Takashi Iwai" <tiwai@...e.de>

Yes, usually such a line should be enough.


> > > + * if not, create a new pcm stream. the caller must remove fp from
> > > + * the substream fmt_list in the error path to avoid double-free.
> >
> > This comment isn't true.  The caller may leave it as is, too, like my
> > first version.  It just depends on the code.
> 
> Yes. Is the following rewrite acceptable for the next patch version?
> 
>  * if not, create a new pcm stream. Note, fp is added to the substream fmt_list
>  * and will be freed on the chip instance release. Do not free fp or do remove
>  * it from the substream fmt_list to avoid double-free.

Yes, that looks more correct.


thanks,

Takashi