diff mbox series

[v2] sound: core: fix device ownership model in card and pcm

Message ID 20230802174451.3611976-1-cujomalainey@chromium.org (mailing list archive)
State New, archived
Headers show
Series [v2] sound: core: fix device ownership model in card and pcm | expand

Commit Message

Curtis Malainey Aug. 2, 2023, 5:43 p.m. UTC 
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/aoa/soundbus/i2sbus/pcm.c |  4 ++--
 sound/core/control.c            | 21 +++++++++++++++------
 sound/core/control_led.c        |  4 ++--
 sound/core/pcm.c                | 30 ++++++++++++++++++++----------
 sound/usb/media.c               |  4 ++--
 7 files changed, 43 insertions(+), 24 deletions(-)

Comments

Greg Kroah-Hartman Aug. 3, 2023, 6:49 a.m. UTC | #1 
On Wed, Aug 02, 2023 at 10:43:49AM -0700, cujomalainey@chromium.org wrote:
> 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/aoa/soundbus/i2sbus/pcm.c |  4 ++--
>  sound/core/control.c            | 21 +++++++++++++++------
>  sound/core/control_led.c        |  4 ++--
>  sound/core/pcm.c                | 30 ++++++++++++++++++++----------
>  sound/usb/media.c               |  4 ++--
>  7 files changed, 43 insertions(+), 24 deletions(-)
> 

Hi,

This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
a patch that has triggered this response.  He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created.  Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.

You are receiving this message because of the following common error(s)
as indicated below:

- This looks like a new version of a previously submitted patch, but you
  did not list below the --- line any changes from the previous version.
  Please read the section entitled "The canonical patch format" in the
  kernel file, Documentation/process/submitting-patches.rst for what
  needs to be done here to properly describe this.


If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.

thanks,

greg k-h's patch email bot
Dan Carpenter Aug. 3, 2023, 9:46 a.m. UTC | #2 
Since you're going to have to resend the patch anyway could you modify
this commit message some more?

On Wed, Aug 02, 2023 at 10:43:49AM -0700, cujomalainey@chromium.org wrote:
> 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).

I don't understand what "root control of memory is handled by card_dev,
all other devices are no-ops" means.  At first I thought this was
refering to code that is out of tree but now I think we are talking
about a CONFIG_DEBUG option.  Could you spell out which option we are
talking about?

> 
> This model does not work though in a kernel hacking environment where
> KASAN and delayed release on kobj is enabled.

I don't think KASAN has anything to do with the bug, right?  KASAN just
finds the bug, it doesn't cause it.  The bug is always there regardless.
The "delayed release" is CONFIG_DEBUG_KOBJECT_RELEASE.  Could you please
mention that specifically.  Say something like:

    "KASAN detected a use after free when CONFIG_DEBUG_KOBJECT_RELEASE
     is enabled, which, hopefully, no one does on a production system."

I feel like a KASAN stack trace might help clarify where the use after
free happens.

> 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.

Ah...  I think I understand.

   "The CONFIG_DEBUG_KOBJECT_RELEASE introduces an element of
    randomness to the release process so we could free the card_dev
    before the child objects resulting in a use after free.  But
    if we don't enable that the releases happen in a nice fixed
    order."

> 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.

Does this second use after free happen regardless of
CONFIG_DEBUG_KOBJECT_RELEASE?

> 
> 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>
> ---

Also I know it's complicated here, but could you try identify a Fixes
tag where this bug is introduced or first starts affecting the things?
This looks like a pretty core bug so it's possible it predates git.  I'm
not sure what to do in that case.  I normally just mention it under the
--- cut off line.

regards,
dan carpenter
diff mbox series

Patch

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/aoa/soundbus/i2sbus/pcm.c b/sound/aoa/soundbus/i2sbus/pcm.c
index a9e502a6cdeb8..07df5cc0f2d7c 100644
--- a/sound/aoa/soundbus/i2sbus/pcm.c
+++ b/sound/aoa/soundbus/i2sbus/pcm.c
@@ -972,7 +972,7 @@  i2sbus_attach_codec(struct soundbus_dev *dev, struct snd_card *card,
 			goto out_put_ci_module;
 		snd_pcm_set_ops(dev->pcm, SNDRV_PCM_STREAM_PLAYBACK,
 				&i2sbus_playback_ops);
-		dev->pcm->streams[SNDRV_PCM_STREAM_PLAYBACK].dev.parent =
+		dev->pcm->streams[SNDRV_PCM_STREAM_PLAYBACK].dev->parent =
 			&dev->ofdev.dev;
 		i2sdev->out.created = 1;
 	}
@@ -989,7 +989,7 @@  i2sbus_attach_codec(struct soundbus_dev *dev, struct snd_card *card,
 			goto out_put_ci_module;
 		snd_pcm_set_ops(dev->pcm, SNDRV_PCM_STREAM_CAPTURE,
 				&i2sbus_record_ops);
-		dev->pcm->streams[SNDRV_PCM_STREAM_CAPTURE].dev.parent =
+		dev->pcm->streams[SNDRV_PCM_STREAM_CAPTURE].dev->parent =
 			&dev->ofdev.dev;
 		i2sdev->in.created = 1;
 	}
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;