On Thu, 31 Mar 2016 14:36:30 +0200, Vladis Dronov wrote:
Hello, Takashi, all,
Thanks for the report. But how about a simpler fix like below?
Maybe the one below is more straightforward (and even simpler). Let me know if this works enough for you.
- I would still suggest moving the code in create_fixed_stream_quirk() (marked
as (*)) after "if (altsd->bNumEndpoints < 1)" check. This way no allocations is done in snd_usb_add_audio_stream() if we go an error path:
(*) stream = (fp->endpoint & USB_DIR_IN) (*) ? SNDRV_PCM_STREAM_CAPTURE : SNDRV_PCM_STREAM_PLAYBACK; (*) err = snd_usb_add_audio_stream(chip, stream, fp); (*) if (err < 0) (*) goto error;
No, it has nothing to do with the double-free bug itself. Such an optimization shouldn't be put in a fix patch. We need to concentrate on fixing the double-free bug at first. Then do an optimization, but only if really needed. Otherwise it makes hard to understand what's actually doing.
(And, in this particular case, that optimization doesn't look worth; the error condition is really rare, happening only with a malformed firmware, and the path isn't hot at all.)
- While your fix is indeed simpler, it fixes double-free bug only in
create_fixed_stream_quirk(). create_uaxx_quirk(), for example, still has this bug:
err = snd_usb_add_audio_stream(chip, stream, fp); if (err < 0) { <<< in the error path there kfree(fp); <<< is a double-free return err; }
as any other code where snd_usb_add_audio_stream() is used and *fp is freed in case of error.
OK, that's another spot. Let's fix similarly.
- Not deleting fp from the fmt_list inside snd_usb_add_audio_stream() in case
of error moves this necessity to a caller, thus breaking the scope. This forces any caller of snd_usb_add_audio_stream() to fulfill this non-obvious requirement. But I agree that this is simpler and more straightforward. We need just to fix all the places where snd_usb_add_audio_stream() is called (3 as of now), please, have a look on the following patch.
Best regards, Vladis Dronov | Red Hat, Inc. | Product Security Engineer
-- 8< -- From: Vladis Dronov vdronov@redhat.com Subject: [PATCH] ALSA: usb-audio: Fix double-free in error paths after snd_usb_add_audio_stream() call
create_fixed_stream_quirk(), snd_usb_parse_audio_interface() and create_uaxx_quirk() functions allocate the audioformat object by themselves and free it upon error before returning. However, once the object is linked to a stream, it's freed again in snd_usb_audio_pcm_free(), thus it'll be double-freed, eventually resulting in a memory corruption.
This patch fixes these failures in the error paths by unlinking the audioformat object before freeing it. Also it moves a piece of code in create_fixed_stream_quirk() to avoid unnecessary allocations in case of error.
[Note for stable backports: this patch requires the commit 902eb7fd1e4a ('ALSA: usb-audio: Minor code cleanup in create_fixed_stream_quirk()')]
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1283358 Reported-by: Ralf Spenneberg ralf@spenneberg.net Cc: stable@vger.kernel.org # see the note above Signed-off-by: Vladis Dronov vdronov@redhat.com
Vladis, if you take someone's patch as the base, you have to carry the original authorship somewhere...
diff --git a/sound/usb/stream.c b/sound/usb/stream.c index 51258a1..6455003 100644 --- a/sound/usb/stream.c +++ b/sound/usb/stream.c @@ -316,7 +316,8 @@ static struct snd_pcm_chmap_elem *convert_chmap(int channels, unsigned int bits, /*
- add this endpoint to the chip instance.
- if a stream with the same endpoint already exists, append to it.
- if not, create a new pcm stream.
- 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.
thanks,
Takashi