From: Curtis Malainey cujomalainey@chromium.org
The current implementation of how devices are released is valid for production use cases (root control of memory is handled by card_dev, all other devices are no-ops).
This model does not work though in a kernel hacking environment where KASAN and delayed release on kobj is enabled. If the card_dev device is released before any of the child objects a use-after-free bug is caught by KASAN as the delayed release still has a reference to the devices that were freed by the card_dev release. Also both snd_card and snd_pcm both own two devices internally, so even if they released independently, the shared struct would result in another use after free.
Solution is to move the child devices into their own memory so they can be handled independently and released on their own schedule.
Signed-off-by: Curtis Malainey cujomalainey@chromium.org Cc: Doug Anderson dianders@chromium.org --- include/sound/core.h | 2 +- include/sound/pcm.h | 2 +- sound/core/control.c | 21 +++++++++++++++------ sound/core/control_led.c | 4 ++-- sound/core/pcm.c | 30 ++++++++++++++++++++---------- sound/usb/media.c | 4 ++-- 6 files changed, 41 insertions(+), 22 deletions(-)
diff --git a/include/sound/core.h b/include/sound/core.h index f6e0dd648b80c..a08ab8c8cfb6d 100644 --- a/include/sound/core.h +++ b/include/sound/core.h @@ -96,7 +96,7 @@ struct snd_card { private data */ struct list_head devices; /* devices */
- struct device ctl_dev; /* control device */ + struct device *ctl_dev; /* control device */ unsigned int last_numid; /* last used numeric ID */ struct rw_semaphore controls_rwsem; /* controls lock (list and values) */ rwlock_t ctl_files_rwlock; /* ctl_files list lock */ diff --git a/include/sound/pcm.h b/include/sound/pcm.h index 19f564606ac42..0243a13e9ac47 100644 --- a/include/sound/pcm.h +++ b/include/sound/pcm.h @@ -510,7 +510,7 @@ struct snd_pcm_str { #endif #endif struct snd_kcontrol *chmap_kctl; /* channel-mapping controls */ - struct device dev; + struct device *dev; };
struct snd_pcm { diff --git a/sound/core/control.c b/sound/core/control.c index 8386b53acdcd4..8bbaf3dffce62 100644 --- a/sound/core/control.c +++ b/sound/core/control.c @@ -2315,7 +2315,7 @@ static int snd_ctl_dev_register(struct snd_device *device) int err;
err = snd_register_device(SNDRV_DEVICE_TYPE_CONTROL, card, -1, - &snd_ctl_f_ops, card, &card->ctl_dev); + &snd_ctl_f_ops, card, card->ctl_dev); if (err < 0) return err; down_read(&card->controls_rwsem); @@ -2351,7 +2351,7 @@ static int snd_ctl_dev_disconnect(struct snd_device *device) up_read(&snd_ctl_layer_rwsem); up_read(&card->controls_rwsem);
- return snd_unregister_device(&card->ctl_dev); + return snd_unregister_device(card->ctl_dev); }
/* @@ -2373,10 +2373,15 @@ static int snd_ctl_dev_free(struct snd_device *device) xa_destroy(&card->ctl_hash); #endif up_write(&card->controls_rwsem); - put_device(&card->ctl_dev); + put_device(card->ctl_dev); return 0; }
+static void snd_ctl_dev_release(struct device *dev) +{ + kfree(dev); +} + /* * create control core: * called from init.c @@ -2394,13 +2399,17 @@ int snd_ctl_create(struct snd_card *card) return -ENXIO; if (snd_BUG_ON(card->number < 0 || card->number >= SNDRV_CARDS)) return -ENXIO; + card->ctl_dev = kzalloc(sizeof(*card->ctl_dev), GFP_KERNEL); + if (!card->ctl_dev) + return -ENOMEM;
- snd_device_initialize(&card->ctl_dev, card); - dev_set_name(&card->ctl_dev, "controlC%d", card->number); + snd_device_initialize(card->ctl_dev, card); + card->ctl_dev->release = snd_ctl_dev_release; + dev_set_name(card->ctl_dev, "controlC%d", card->number);
err = snd_device_new(card, SNDRV_DEV_CONTROL, card, &ops); if (err < 0) - put_device(&card->ctl_dev); + put_device(card->ctl_dev); return err; }
diff --git a/sound/core/control_led.c b/sound/core/control_led.c index ee77547bf8dcb..760e46cf25cc8 100644 --- a/sound/core/control_led.c +++ b/sound/core/control_led.c @@ -688,7 +688,7 @@ static void snd_ctl_led_sysfs_add(struct snd_card *card) goto cerr; led->cards[card->number] = led_card; snprintf(link_name, sizeof(link_name), "led-%s", led->name); - WARN(sysfs_create_link(&card->ctl_dev.kobj, &led_card->dev.kobj, link_name), + WARN(sysfs_create_link(&card->ctl_dev->kobj, &led_card->dev.kobj, link_name), "can't create symlink to controlC%i device\n", card->number); WARN(sysfs_create_link(&led_card->dev.kobj, &card->card_dev.kobj, "card"), "can't create symlink to card%i\n", card->number); @@ -714,7 +714,7 @@ static void snd_ctl_led_sysfs_remove(struct snd_card *card) if (!led_card) continue; snprintf(link_name, sizeof(link_name), "led-%s", led->name); - sysfs_remove_link(&card->ctl_dev.kobj, link_name); + sysfs_remove_link(&card->ctl_dev->kobj, link_name); sysfs_remove_link(&led_card->dev.kobj, "card"); device_unregister(&led_card->dev); led->cards[card->number] = NULL; diff --git a/sound/core/pcm.c b/sound/core/pcm.c index 9d95e37311230..9026ccc56dbe7 100644 --- a/sound/core/pcm.c +++ b/sound/core/pcm.c @@ -604,7 +604,7 @@ static const struct attribute_group *pcm_dev_attr_groups[]; #ifdef CONFIG_PM_SLEEP static int do_pcm_suspend(struct device *dev) { - struct snd_pcm_str *pstr = container_of(dev, struct snd_pcm_str, dev); + struct snd_pcm_str *pstr = dev_get_drvdata(dev);
if (!pstr->pcm->no_device_suspend) snd_pcm_suspend_all(pstr->pcm); @@ -622,6 +622,11 @@ static const struct device_type pcm_dev_type = { .pm = &pcm_dev_pm_ops, };
+static void snd_pcm_dev_release(struct device *dev) +{ + kfree(dev); +} + /** * snd_pcm_new_stream - create a new PCM stream * @pcm: the pcm instance @@ -641,6 +646,10 @@ int snd_pcm_new_stream(struct snd_pcm *pcm, int stream, int substream_count) struct snd_pcm_str *pstr = &pcm->streams[stream]; struct snd_pcm_substream *substream, *prev;
+ pstr->dev = kzalloc(sizeof(*pstr->dev), GFP_KERNEL); + if (!pstr->dev) + return -ENOMEM; + dev_set_drvdata(pstr->dev, pstr); #if IS_ENABLED(CONFIG_SND_PCM_OSS) mutex_init(&pstr->oss.setup_mutex); #endif @@ -650,10 +659,11 @@ int snd_pcm_new_stream(struct snd_pcm *pcm, int stream, int substream_count) if (!substream_count) return 0;
- snd_device_initialize(&pstr->dev, pcm->card); - pstr->dev.groups = pcm_dev_attr_groups; - pstr->dev.type = &pcm_dev_type; - dev_set_name(&pstr->dev, "pcmC%iD%i%c", pcm->card->number, pcm->device, + snd_device_initialize(pstr->dev, pcm->card); + pstr->dev->release = snd_pcm_dev_release; + pstr->dev->groups = pcm_dev_attr_groups; + pstr->dev->type = &pcm_dev_type; + dev_set_name(pstr->dev, "pcmC%iD%i%c", pcm->card->number, pcm->device, stream == SNDRV_PCM_STREAM_PLAYBACK ? 'p' : 'c');
if (!pcm->internal) { @@ -699,7 +709,7 @@ int snd_pcm_new_stream(struct snd_pcm *pcm, int stream, int substream_count) prev = substream; } return 0; -} +} EXPORT_SYMBOL(snd_pcm_new_stream);
static int _snd_pcm_new(struct snd_card *card, const char *id, int device, @@ -847,7 +857,7 @@ static void snd_pcm_free_stream(struct snd_pcm_str * pstr) #endif free_chmap(pstr); if (pstr->substream_count) - put_device(&pstr->dev); + put_device(pstr->dev); }
#if IS_ENABLED(CONFIG_SND_PCM_OSS) @@ -1017,7 +1027,7 @@ void snd_pcm_detach_substream(struct snd_pcm_substream *substream) static ssize_t pcm_class_show(struct device *dev, struct device_attribute *attr, char *buf) { - struct snd_pcm_str *pstr = container_of(dev, struct snd_pcm_str, dev); + struct snd_pcm_str *pstr = dev_get_drvdata(dev); struct snd_pcm *pcm = pstr->pcm; const char *str; static const char *strs[SNDRV_PCM_CLASS_LAST + 1] = { @@ -1078,7 +1088,7 @@ static int snd_pcm_dev_register(struct snd_device *device) /* register pcm */ err = snd_register_device(devtype, pcm->card, pcm->device, &snd_pcm_f_ops[cidx], pcm, - &pcm->streams[cidx].dev); + pcm->streams[cidx].dev); if (err < 0) { list_del_init(&pcm->list); goto unlock; @@ -1125,7 +1135,7 @@ static int snd_pcm_dev_disconnect(struct snd_device *device)
pcm_call_notify(pcm, n_disconnect); for (cidx = 0; cidx < 2; cidx++) { - snd_unregister_device(&pcm->streams[cidx].dev); + snd_unregister_device(pcm->streams[cidx].dev); free_chmap(&pcm->streams[cidx]); } mutex_unlock(&pcm->open_mutex); diff --git a/sound/usb/media.c b/sound/usb/media.c index 840f42cb9272c..d48db6f3ae659 100644 --- a/sound/usb/media.c +++ b/sound/usb/media.c @@ -35,7 +35,7 @@ int snd_media_stream_init(struct snd_usb_substream *subs, struct snd_pcm *pcm, { struct media_device *mdev; struct media_ctl *mctl; - struct device *pcm_dev = &pcm->streams[stream].dev; + struct device *pcm_dev = pcm->streams[stream].dev; u32 intf_type; int ret = 0; u16 mixer_pad; @@ -163,7 +163,7 @@ void snd_media_stop_pipeline(struct snd_usb_substream *subs)
static int snd_media_mixer_init(struct snd_usb_audio *chip) { - struct device *ctl_dev = &chip->card->ctl_dev; + struct device *ctl_dev = chip->card->ctl_dev; struct media_intf_devnode *ctl_intf; struct usb_mixer_interface *mixer; struct media_device *mdev = chip->media_dev;