Hello, Takashi, Jaroslav, all,
Please, see the research and the following patch on a double-free bug in [snd-usb-audio].
1) The upstream commits 0f886ca1, 902eb7fd and 447d6275f (many thanks to Takashi Iwai) revealed that there is a double-free bug in [snd-usb-audio] module due to alloc/free logic flaw in snd_usb_add_audio_stream() function. A double-free leads to kernel structure/list/slab corruptions and shortly to null-deref, GPF or lockup.
2.1) Let me describe what happens with the current code in usb_audio_probe(), create_fixed_stream_quirk() and snd_usb_add_audio_stream():
> [ sound/usb/card.c ]
> static int usb_audio_probe(struct usb_interface *intf,
> const struct usb_device_id *usb_id)
> {
> ...
> if (quirk && quirk->ifnum != QUIRK_NO_INTERFACE) {
> /* need some special handlings */
> err = snd_usb_create_quirk(chip, intf, &usb_audio_driver, quirk);
> if (err < 0)
> goto __error;
> }
> ...
> __error:
> if (chip) {
> if (!chip->num_interfaces)
> snd_card_free(chip->card);
Somewhere in the middle of usb_audio_probe() the function snd_usb_create_quirk() is called, and if it returns with an error and no interfaces were created, the sound card is destroyed with "snd_card_free(chip->card)".
2.2) create_fixed_stream_quirk() is called from snd_usb_create_quirk():
> [ sound/usb/quirks.c ]
> static int create_fixed_stream_quirk(struct snd_usb_audio *chip,
> struct usb_interface *iface,
> struct usb_driver *driver,
> const struct snd_usb_audio_quirk *quirk)
> {
> struct audioformat *fp;
> struct usb_host_interface *alts;
> struct usb_interface_descriptor *altsd;
> ...
> fp = kmemdup(quirk->data, sizeof(*fp), GFP_KERNEL);
> ...
> (*) 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;
>
> if (fp->iface != get_iface_desc(&iface->altsetting[0])->bInterfaceNumber ||
> fp->altset_idx >= iface->num_altsetting) {
> err = -EINVAL;
> goto error;
> }
> alts = &iface->altsetting[fp->altset_idx];
> altsd = get_iface_desc(alts);
> if (altsd->bNumEndpoints < 1) {
> err = -EINVAL;
> goto error;
> }
> ...
> error:
> kfree(fp);
> kfree(rate_table);
> return err;
> }
2.3) *fp is allocated and passed to snd_usb_add_audio_stream() where snd_usb_init_substream() is called:
> [ sound/usb/stream.c ]
> int snd_usb_add_audio_stream(struct snd_usb_audio *chip,
> int stream,
> struct audioformat *fp)
> {
> struct snd_usb_stream *as;
> struct snd_usb_substream *subs;
> struct snd_pcm *pcm;
> ...
> /* create a new pcm */
> as = kzalloc(sizeof(*as), GFP_KERNEL);
> ...
> snd_usb_init_substream(as, stream, fp);
2.4) In turn snd_usb_init_substream() adds audioformat *fp by its &fp->list to substream's fmt_list list:
> [ sound/usb/stream.c ]
> static void snd_usb_init_substream(struct snd_usb_stream *as,
> int stream,
> struct audioformat *fp)
> {
> struct snd_usb_substream *subs = &as->substream[stream];
>
> INIT_LIST_HEAD(&subs->fmt_list);
> ...
> list_add_tail(&fp->list, &subs->fmt_list);
> subs->num_formats++;
2.5) Things go bad from this point in case snd_usb_add_audio_stream() or the caller go the error path. The bug is that the caller frees (see "error: kfree(fp);" code) *fp on the error path, BUT the pointer to the already-freed memory remains in the substream's fmt_list list.
The double-free happens after create_fixed_stream_quirk() returns with an error and usb_audio_probe() calls snd_card_free()->...->snd_usb_audio_pcm_free()->free_substream(). As subs->fmt_list is already corrupted, iterating it with list_for_each_entry_safe() leads to any and unpredictable results.
> static void free_substream(struct snd_usb_substream *subs)
> {
> struct audioformat *fp, *n;
> ...
> list_for_each_entry_safe(fp, n, &subs->fmt_list, list) {
> ...
> kfree(fp);
3.1) The crash is reproduceable by faking the USB device with no endpoints as described in https://bugzilla.redhat.com/show_bug.cgi?id=1283355 and https://bugzilla.redhat.com/show_bug.cgi?id=1283358.
Please see as a proof the following kernel log with debug printing added to the code. First, *fp is added to fmt_list in snd_usb_init_substream():
[ 322.797223] usb 1-1: new full-speed USB device number 2 using xhci_hcd
[ 322.974083] usb 1-1: config 1 interface 0 altsetting 0 has 3 endpoint descriptors, different from the interface descriptor's value: 0
[ 322.987031] usb 1-1: New USB device found, idVendor=045e, idProduct=0283
[ 322.998318] usb 1-1: New USB device strings: Mfr=1, Product=2, SerialNumber=3
[ 323.083913] media: Linux media interface: v0.10
[ 323.231249] init_substream: add_tail() fp=ffff88003364ba80 fp->list.next=ffff8800b1e898b8 fp->list.prev=ffff8800b1e898b8 fmt_list=ffff8800b1e898b8 fmt_list.next=ffff88003364ba80 prev=ffff88003364ba80 num_formats=1
As you can see we have a correct list here with head at fmt_list=ffff8800b1e898b8 and single element fp=ffff88003364ba80.
3.2) The code finds out that there are too few endpoints present and goes the error path (to the "error:" label):
[ 323.307927] usb 1-1: too few endpoints
[ 323.312964] trace-before-free: substream-0 ffff8800b1e89818 numf 1 fmt_list ffff8800b1e898b8 next ffff88003364ba80
[ 323.353759] fp=ffff88003364ba80 next=ffff8800b1e898b8 prev=ffff8800b1e898b8 rate=(null) chmap=(null)
[ 323.362118] struct sane
As you can see the substream's fmt_list is sane at this point.
3.3) After "kfree(fp)" in the error path of create_fixed_stream_quirk() *fp is freed, BUT the pointer to the freed memory remains in fmt_list. After *fp is freed the list is corrupted and contains trash:
[ 323.371752] KFREE(fp) ffff88003364ba80
[ 323.380383] trace-after-free: substream-0 ffff8800b1e89818 numf 1 fmt_list ffff8800b1e898b8 next ffff88003364ba80
[ 323.400003] fp=ffff88003364ba80 next=ffff88003364bf80 prev=ffff8800b1e898b8 rate=(null) chmap=(null)
[ 323.406786] fp=ffff88003364bf80 next= (null) prev= (null) rate=(null) chmap=(null)
[ 323.422211] next == NULL: FAIL, struct INSANE
[ 323.436706] KFREE(rate_table) (null)
3.4) After error return from create_fixed_stream_quirk() the sound card is destroyed with "snd_card_free(chip->card)" in usb_audio_probe(). In the end free_substream() is called:
[ 323.511256] usb 1-1: snd_usb_create_quirk() failed: -22
[ 323.565108] list_for_each_entry_safe(): fp=ffff88003364ba80 n=ffff88003364bf80
[ 323.586337] kfree fp ffff88003364ba80 <<< DOUBLE-FREE
[ 323.588509] loop-end: fp=ffff88003364ba80 n=ffff88003364bf80
[ 323.599969] list_for_each_entry_safe(): fp=ffff88003364bf80 n=(null)
[ 323.610460] kfree fp ffff88003364bf80 <<< FREEING SOMEONE ELSE'S MEMORY
[ 323.613181] loop-end: fp=ffff88003364bf80 n=(null) <<< NULL-PTR DEREF
...
[ 324.247113] BUG: unable to handle kernel NULL pointer dereference at (null)
[ 324.247533] IP: [<ffffffffc02d40ef>] free_substream.part.0+0xef/0x100 [snd_usb_audio]
[ 324.248088] PGD 0
[ 324.248088] Oops: 0000 [#1] SMP
[ 324.248088] Modules linked in: snd_usb_audio(+) snd_usbmidi_lib snd_hwdep ...
[ 324.248088] CPU: 2 PID: 767 Comm: systemd-udevd Not tainted 4.5.0-vladis #25
[ 324.248088] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.8.2-0-g33fbe13 by qemu-project.org 04/01/2014
[ 324.248088] task: ffff880034718000 ti: ffff8800b2fbc000 task.ti: ffff8800b2fbc000
[ 324.248088] RIP: 0010:[<ffffffffc02d40ef>] [<ffffffffc02d40ef>] free_substream.part.0+0xef/0x100 [snd_usb_audio]
[ 324.248088] RSP: 0018:ffff8800b2fbf898 EFLAGS: 00010217
...
[ 324.248088] Call Trace:
[ 324.248088] [<ffffffffc02d43fa>] snd_usb_audio_pcm_free+0x9a/0xa0 [snd_usb_audio]
[ 324.248088] [<ffffffffc028d982>] snd_pcm_free+0x32/0xa0 [snd_pcm]
[ 324.248088] [<ffffffffc028da02>] snd_pcm_dev_free+0x12/0x20 [snd_pcm]
[ 324.248088] [<ffffffffc027d279>] __snd_device_free+0x29/0x70 [snd]
[ 324.248088] [<ffffffffc027d660>] snd_device_free_all+0x30/0x60 [snd]
[ 324.248088] [<ffffffffc02777a4>] release_card_device+0x34/0x90 [snd]
[ 324.248088] [<ffffffff815ae2b2>] device_release+0x32/0x90
[ 324.248088] [<ffffffff81455f8a>] kobject_release+0x7a/0x190
[ 324.248088] [<ffffffff81455e47>] kobject_put+0x27/0x50
[ 324.248088] [<ffffffff815ae5f7>] put_device+0x17/0x20
[ 324.248088] [<ffffffffc0277b57>] snd_card_free+0x67/0x90 [snd]
[ 324.248088] [<ffffffffc02c4f14>] usb_audio_probe+0x754/0x9d0 [snd_usb_audio]
...
4.1) The suggested patch consists of 2 changes. First, I suppose we should move the code in create_fixed_stream_quirk() marked as "(*)" (see above) after "if (altsd->bNumEndpoints < 1)" check. This way no allocations is done if we go an error path. I've checked that fp->iface and fp->altset_idx are not changed in snd_usb_add_audio_stream() and functions called from it, so it is safe to swap these 2 pieces of code.
4.2) The problem with double-free still remains. I've verified that all the callers of snd_usb_add_audio_stream() free *fp in case of error:
$ git grep snd_usb_add_audio_stream
sound/usb/quirks.c: err = snd_usb_add_audio_stream(chip, stream, fp);
> * err = snd_usb_add_audio_stream(chip, stream, fp);
> * if (err < 0)
> * goto error;
> * error:
> * kfree(fp);
> * kfree(rate_table);
> * return err;
sound/usb/quirks.c: err = snd_usb_add_audio_stream(chip, stream, fp);
> * err = snd_usb_add_audio_stream(chip, stream, fp);
> * if (err < 0) {
> * kfree(fp);
> * return err;
> * }
sound/usb/stream.c: err = snd_usb_add_audio_stream(chip, stream, fp);
> * err = snd_usb_add_audio_stream(chip, stream, fp);
> * if (err < 0) {
> * kfree(fp->rate_table);
> * kfree(fp->chmap);
> * kfree(fp);
> * return err;
> * }
This means that snd_usb_add_audio_stream() should remove *fp from the substream's fmt_list list on the error path, if it was already added. Such places are:
> int snd_usb_add_audio_stream(struct snd_usb_audio *chip, int stream, struct audioformat *fp)
> {
> struct snd_usb_stream *as;
> struct snd_usb_substream *subs;
> struct snd_pcm *pcm;
> int err;
>
> list_for_each_entry(as, &chip->pcm_list, list) {
> subs = &as->substream[stream];
> ...
> list_add_tail(&fp->list, &subs->fmt_list); <<< ADDING fp HERE
> subs->num_formats++;
> return 0; <<< NO ERROR PATH HERE
> }
> }
>
> /* look for an empty stream */
> list_for_each_entry(as, &chip->pcm_list, list) {
> subs = &as->substream[stream];
> ...
> snd_usb_init_substream(as, stream, fp); <<< ADDING fp HERE, IF add_chmap() FAILS
> return add_chmap(as->pcm, stream, subs); <<< fp SHOULD BE REMOVED FROM fmt_list
> }
>
> /* create a new pcm */
> as = kzalloc(sizeof(*as), GFP_KERNEL);
> err = snd_pcm_new(chip->card, "USB Audio", chip->pcm_devs,
> ...
> if (err < 0) { <<< fp IS NOT ADDED YET HERE, SO FINE
> kfree(as);
> return err;
> }
> ...
> snd_usb_init_substream(as, stream, fp); <<< ADDING fp HERE
> ... <<< IF add_chmap() FAILS fp SHOULD
> return add_chmap(pcm, stream, &as->substream[stream]); <<< BE REMOVED FROM fmt_list
> }
add_chmap() itself does not add anything to fmt_list list, so we indeed need to remove only the single list element from the list. Having all the above in mind, the patch follows.
4.3) How to handle possible error paths after successful call to snd_usb_add_audio_stream() in create_fixed_stream_quirk() is d
iscussable. Properly it should be like the below, but I believe it is overcomplication here would and stick to a simple error_after_add_audio_stream: label:
> error2:
> snd_usb_del_audio_stream(...something...);
> error:
> kfree(fp);
> kfree(rate_table);
> return err;
Best regards,
Vladis Dronov | Red Hat, Inc. | Product Security Engineer
There is a double-free bug in [snd-usb-audio] module due to alloc/free logic
flaw in snd_usb_add_audio_stream() function. This leads to kernel structures
corruption and panic. Fix the code flow and alloc/free logic so there is no
double-free.
The detailed analysis: https://bugzilla.redhat.com/show_bug.cgi?id=1283358
Reported-by: Ralf Spenneberg <[email protected]>
Signed-off-by: Vladis Dronov <[email protected]>
---
sound/usb/quirks.c | 17 ++++++++++++-----
sound/usb/stream.c | 10 ++++++++--
2 files changed, 20 insertions(+), 7 deletions(-)
diff --git a/sound/usb/quirks.c b/sound/usb/quirks.c
index fb62bce..1d41b47 100644
--- a/sound/usb/quirks.c
+++ b/sound/usb/quirks.c
@@ -164,11 +164,6 @@ static int create_fixed_stream_quirk(struct snd_usb_audio *chip,
fp->rate_table = rate_table;
}
- 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;
if (fp->iface != get_iface_desc(&iface->altsetting[0])->bInterfaceNumber ||
fp->altset_idx >= iface->num_altsetting) {
err = -EINVAL;
@@ -181,6 +176,17 @@ static int create_fixed_stream_quirk(struct snd_usb_audio *chip,
goto error;
}
+ 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;
+
+ /* From this point error paths should jump to
+ * error_after_add_audio_stream: not to error: as fp
+ * and rate_table will be freed on stream removal
+ */
+
fp->protocol = altsd->bInterfaceProtocol;
if (fp->datainterval == 0)
@@ -195,6 +201,7 @@ static int create_fixed_stream_quirk(struct snd_usb_audio *chip,
error:
kfree(fp);
kfree(rate_table);
+ error_after_add_audio_stream:
return err;
}
diff --git a/sound/usb/stream.c b/sound/usb/stream.c
index 51258a1..f8ed8b49 100644
--- a/sound/usb/stream.c
+++ b/sound/usb/stream.c
@@ -349,7 +349,10 @@ int snd_usb_add_audio_stream(struct snd_usb_audio *chip,
if (err < 0)
return err;
snd_usb_init_substream(as, stream, fp);
- return add_chmap(as->pcm, stream, subs);
+ err = add_chmap(as->pcm, stream, subs);
+ if (err < 0)
+ list_del_init(&fp->list);
+ return err;
}
/* create a new pcm */
@@ -391,7 +394,10 @@ int snd_usb_add_audio_stream(struct snd_usb_audio *chip,
snd_usb_proc_pcm_format_add(as);
- return add_chmap(pcm, stream, &as->substream[stream]);
+ err = add_chmap(pcm, stream, &as->substream[stream]);
+ if (err < 0)
+ list_del_init(&fp->list);
+ return err;
}
static int parse_uac_endpoint_attributes(struct snd_usb_audio *chip,
--
2.5.5
Hi Vladis,
[auto build test WARNING on sound/for-next]
[also build test WARNING on v4.6-rc1 next-20160330]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]
url: https://github.com/0day-ci/linux/commits/Vladis-Dronov/ALSA-usb-audio-Fix-double-free-in-snd_usb_add_audio_stream/20160331-030636
base: https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next
config: x86_64-acpi-redef (attached as .config)
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All warnings (new ones prefixed by >>):
sound/usb/quirks.c: In function 'create_fixed_stream_quirk':
>> sound/usb/quirks.c:204:2: warning: label 'error_after_add_audio_stream' defined but not used [-Wunused-label]
error_after_add_audio_stream:
^
vim +/error_after_add_audio_stream +204 sound/usb/quirks.c
188 */
189
190 fp->protocol = altsd->bInterfaceProtocol;
191
192 if (fp->datainterval == 0)
193 fp->datainterval = snd_usb_parse_datainterval(chip, alts);
194 if (fp->maxpacksize == 0)
195 fp->maxpacksize = le16_to_cpu(get_endpoint(alts, 0)->wMaxPacketSize);
196 usb_set_interface(chip->dev, fp->iface, 0);
197 snd_usb_init_pitch(chip, fp->iface, alts, fp);
198 snd_usb_init_sample_rate(chip, fp->iface, alts, fp, fp->rate_max);
199 return 0;
200
201 error:
202 kfree(fp);
203 kfree(rate_table);
> 204 error_after_add_audio_stream:
205 return err;
206 }
207
208 static int create_auto_pcm_quirk(struct snd_usb_audio *chip,
209 struct usb_interface *iface,
210 struct usb_driver *driver)
211 {
212 struct usb_host_interface *alts;
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Wed, 30 Mar 2016 21:03:22 +0200,
Vladis Dronov wrote:
>
> There is a double-free bug in [snd-usb-audio] module due to alloc/free logic
> flaw in snd_usb_add_audio_stream() function. This leads to kernel structures
> corruption and panic. Fix the code flow and alloc/free logic so there is no
> double-free.
>
> The detailed analysis: https://bugzilla.redhat.com/show_bug.cgi?id=1283358
>
> Reported-by: Ralf Spenneberg <[email protected]>
> Signed-off-by: Vladis Dronov <[email protected]>
Thanks for the report. But how about a simpler fix like below?
Takashi
---
diff --git a/sound/usb/quirks.c b/sound/usb/quirks.c
index fb62bce2435c..0e154ae7924e 100644
--- a/sound/usb/quirks.c
+++ b/sound/usb/quirks.c
@@ -150,6 +150,7 @@ static int create_fixed_stream_quirk(struct snd_usb_audio *chip,
usb_audio_err(chip, "cannot memdup\n");
return -ENOMEM;
}
+ INIT_LIST_HEAD(&fp->list);
if (fp->nr_rates > MAX_NR_RATES) {
kfree(fp);
return -EINVAL;
@@ -193,8 +194,11 @@ static int create_fixed_stream_quirk(struct snd_usb_audio *chip,
return 0;
error:
- kfree(fp);
- kfree(rate_table);
+ /* linked fp will be removed in snd_usb_audio_pcm_free() */
+ if (list_empty(&fp->list)) {
+ kfree(fp);
+ kfree(rate_table);
+ }
return err;
}
On Wed, 30 Mar 2016 22:31:15 +0200,
Takashi Iwai wrote:
>
> On Wed, 30 Mar 2016 21:03:22 +0200,
> Vladis Dronov wrote:
> >
> > There is a double-free bug in [snd-usb-audio] module due to alloc/free logic
> > flaw in snd_usb_add_audio_stream() function. This leads to kernel structures
> > corruption and panic. Fix the code flow and alloc/free logic so there is no
> > double-free.
> >
> > The detailed analysis: https://bugzilla.redhat.com/show_bug.cgi?id=1283358
> >
> > Reported-by: Ralf Spenneberg <[email protected]>
> > Signed-off-by: Vladis Dronov <[email protected]>
>
> 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.
thanks,
Takashi
-- 8< --
From: Takashi Iwai <[email protected]>
Subject: [PATCH] ALSA: usb-audio: Fix double free in create_fixed_stream_quirk() error paths
create_fixed_stream_quirk() function allocates the audioformat object
by itself and frees it upon error before returning. However, once
when 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 this failure in the error path by unlinking the
audioformat object before freeing it.
[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 <[email protected]>
Reported-by: Vladis Dronov <[email protected]>
Cc: <[email protected]> # see the note above
Signed-off-by: Takashi Iwai <[email protected]>
---
sound/usb/quirks.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/sound/usb/quirks.c b/sound/usb/quirks.c
index fb62bce2435c..a5a9ecaafb37 100644
--- a/sound/usb/quirks.c
+++ b/sound/usb/quirks.c
@@ -150,6 +150,7 @@ static int create_fixed_stream_quirk(struct snd_usb_audio *chip,
usb_audio_err(chip, "cannot memdup\n");
return -ENOMEM;
}
+ INIT_LIST_HEAD(&fp->list);
if (fp->nr_rates > MAX_NR_RATES) {
kfree(fp);
return -EINVAL;
@@ -193,6 +194,7 @@ static int create_fixed_stream_quirk(struct snd_usb_audio *chip,
return 0;
error:
+ list_del(&fp->list); /* unlink for avoiding double-free */
kfree(fp);
kfree(rate_table);
return err;
--
2.7.4
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.
1) 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;
2) 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.
3) 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 <[email protected]>
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 <[email protected]>
Cc: <[email protected]> # see the note above
Signed-off-by: Vladis Dronov <[email protected]>
---
sound/usb/quirks.c | 15 ++++++++++-----
sound/usb/stream.c | 5 ++++-
2 files changed, 14 insertions(+), 6 deletions(-)
diff --git a/sound/usb/quirks.c b/sound/usb/quirks.c
index fb62bce..dda5682 100644
--- a/sound/usb/quirks.c
+++ b/sound/usb/quirks.c
@@ -150,6 +150,7 @@ static int create_fixed_stream_quirk(struct snd_usb_audio *chip,
usb_audio_err(chip, "cannot memdup\n");
return -ENOMEM;
}
+ INIT_LIST_HEAD(&fp->list);
if (fp->nr_rates > MAX_NR_RATES) {
kfree(fp);
return -EINVAL;
@@ -164,11 +165,6 @@ static int create_fixed_stream_quirk(struct snd_usb_audio *chip,
fp->rate_table = rate_table;
}
- 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;
if (fp->iface != get_iface_desc(&iface->altsetting[0])->bInterfaceNumber ||
fp->altset_idx >= iface->num_altsetting) {
err = -EINVAL;
@@ -181,6 +177,12 @@ static int create_fixed_stream_quirk(struct snd_usb_audio *chip,
goto error;
}
+ 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;
+
fp->protocol = altsd->bInterfaceProtocol;
if (fp->datainterval == 0)
@@ -193,6 +195,7 @@ static int create_fixed_stream_quirk(struct snd_usb_audio *chip,
return 0;
error:
+ list_del(&fp->list); /* unlink for avoiding double-free */
kfree(fp);
kfree(rate_table);
return err;
@@ -469,6 +472,7 @@ static int create_uaxx_quirk(struct snd_usb_audio *chip,
fp->ep_attr = get_endpoint(alts, 0)->bmAttributes;
fp->datainterval = 0;
fp->maxpacksize = le16_to_cpu(get_endpoint(alts, 0)->wMaxPacketSize);
+ INIT_LIST_HEAD(&fp->list);
switch (fp->maxpacksize) {
case 0x120:
@@ -492,6 +496,7 @@ static int create_uaxx_quirk(struct snd_usb_audio *chip,
? SNDRV_PCM_STREAM_CAPTURE : SNDRV_PCM_STREAM_PLAYBACK;
err = snd_usb_add_audio_stream(chip, stream, fp);
if (err < 0) {
+ list_del(&fp->list); /* unlink for avoiding double-free */
kfree(fp);
return err;
}
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.
*/
int snd_usb_add_audio_stream(struct snd_usb_audio *chip,
int stream,
@@ -677,6 +678,7 @@ int snd_usb_parse_audio_interface(struct snd_usb_audio *chip, int iface_no)
* (fp->maxpacksize & 0x7ff);
fp->attributes = parse_uac_endpoint_attributes(chip, alts, protocol, iface_no);
fp->clock = clock;
+ INIT_LIST_HEAD(&fp->list);
/* some quirks for attributes here */
@@ -725,6 +727,7 @@ int snd_usb_parse_audio_interface(struct snd_usb_audio *chip, int iface_no)
dev_dbg(&dev->dev, "%u:%d: add audio endpoint %#x\n", iface_no, altno, fp->endpoint);
err = snd_usb_add_audio_stream(chip, stream, fp);
if (err < 0) {
+ list_del(&fp->list); /* unlink for avoiding double-free */
kfree(fp->rate_table);
kfree(fp->chmap);
kfree(fp);
--
2.5.5
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.
>
> 1) 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.)
> 2) 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.
> 3) 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 <[email protected]>
> 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 <[email protected]>
> Cc: <[email protected]> # see the note above
> Signed-off-by: Vladis Dronov <[email protected]>
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
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. 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.
But anyway. If you still believe this code should not be moved, please, confirm,
I'll suggest the next patch version without 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" <[email protected]>
> > + * 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.
Best regards,
Vladis Dronov | Red Hat, Inc. | Product Security Engineer
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" <[email protected]>
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
From: Vladis Dronov <[email protected]>
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.
Based on a patch by Takashi Iwai" <[email protected]>
[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 <[email protected]>
Cc: <[email protected]> # see the note above
Signed-off-by: Vladis Dronov <[email protected]>
---
sound/usb/quirks.c | 4 ++++
sound/usb/stream.c | 6 +++++-
2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/sound/usb/quirks.c b/sound/usb/quirks.c
index fb62bce..6178bb5 100644
--- a/sound/usb/quirks.c
+++ b/sound/usb/quirks.c
@@ -150,6 +150,7 @@ static int create_fixed_stream_quirk(struct snd_usb_audio *chip,
usb_audio_err(chip, "cannot memdup\n");
return -ENOMEM;
}
+ INIT_LIST_HEAD(&fp->list);
if (fp->nr_rates > MAX_NR_RATES) {
kfree(fp);
return -EINVAL;
@@ -193,6 +194,7 @@ static int create_fixed_stream_quirk(struct snd_usb_audio *chip,
return 0;
error:
+ list_del(&fp->list); /* unlink for avoiding double-free */
kfree(fp);
kfree(rate_table);
return err;
@@ -469,6 +471,7 @@ static int create_uaxx_quirk(struct snd_usb_audio *chip,
fp->ep_attr = get_endpoint(alts, 0)->bmAttributes;
fp->datainterval = 0;
fp->maxpacksize = le16_to_cpu(get_endpoint(alts, 0)->wMaxPacketSize);
+ INIT_LIST_HEAD(&fp->list);
switch (fp->maxpacksize) {
case 0x120:
@@ -492,6 +495,7 @@ static int create_uaxx_quirk(struct snd_usb_audio *chip,
? SNDRV_PCM_STREAM_CAPTURE : SNDRV_PCM_STREAM_PLAYBACK;
err = snd_usb_add_audio_stream(chip, stream, fp);
if (err < 0) {
+ list_del(&fp->list); /* unlink for avoiding double-free */
kfree(fp);
return err;
}
diff --git a/sound/usb/stream.c b/sound/usb/stream.c
index 51258a1..6fe7f21 100644
--- a/sound/usb/stream.c
+++ b/sound/usb/stream.c
@@ -316,7 +316,9 @@ 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. 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.
*/
int snd_usb_add_audio_stream(struct snd_usb_audio *chip,
int stream,
@@ -677,6 +679,7 @@ int snd_usb_parse_audio_interface(struct snd_usb_audio *chip, int iface_no)
* (fp->maxpacksize & 0x7ff);
fp->attributes = parse_uac_endpoint_attributes(chip, alts, protocol, iface_no);
fp->clock = clock;
+ INIT_LIST_HEAD(&fp->list);
/* some quirks for attributes here */
@@ -725,6 +728,7 @@ int snd_usb_parse_audio_interface(struct snd_usb_audio *chip, int iface_no)
dev_dbg(&dev->dev, "%u:%d: add audio endpoint %#x\n", iface_no, altno, fp->endpoint);
err = snd_usb_add_audio_stream(chip, stream, fp);
if (err < 0) {
+ list_del(&fp->list); /* unlink for avoiding double-free */
kfree(fp->rate_table);
kfree(fp->chmap);
kfree(fp);
--
2.5.5
On Thu, 31 Mar 2016 18:05:43 +0200,
Vladis Dronov wrote:
>
> From: Vladis Dronov <[email protected]>
> 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.
>
> Based on a patch by Takashi Iwai" <[email protected]>
>
> [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 <[email protected]>
> Cc: <[email protected]> # see the note above
> Signed-off-by: Vladis Dronov <[email protected]>
Applied, thanks.
Takashi