[PATCH RFC 0/9] ALSA: Don't embed struct devices

[PATCH RFC 0/9] ALSA: Don't embed struct devices

[Takashi Iwai]

Hi,

this is another set of patches to attempt papering over the UAF
problems that are seen when the delayed kobject release is enabled, as
initially reported by Curtis:
  https://lore.kernel.org/r/20230801171928.1460120-1-cujomalainey@chromium.org

There was a previous patch set with a different approach (using the
device refcount dependencies), but this is a sort of step-back to the
old way.
  https://lore.kernel.org/r/20230807135207.17708-1-tiwai@suse.de

After discussions and evaluations, we agreed that decoupling the
struct device from each sound component object is the safest (and
easiest) way as of now.  For applying the changes more consistently, I
introduced a new helper for the struct device allocation and
initialization, and applied all components.

A couple of more changes for card_dev refcount managed aren't included
in this patch set, though.  They might be good to have, but this patch
set should suffice for the currently seen UAF problems.

For a long-term solution, we may restructure the device management,
then the struct devices may be embedded again in each object.  But,
it'll need lots of other changes and cleanups, a big TODO.

The latest patches are found in topic/dev-split branch of sound.git
tree.


Takashi

===

Takashi Iwai (9):
  ALSA: core: Introduce snd_device_alloc()
  ALSA: control: Don't embed ctl_dev
  ALSA: pcm: Don't embed device
  ALSA: hwdep: Don't embed device
  ALSA: rawmidi: Don't embed device
  ALSA: compress: Don't embed device
  ALSA: timer: Create device with snd_device_alloc()
  ALSA: seq: Create device with snd_device_alloc()
  ALSA: core: Drop snd_device_initialize()

 include/sound/compress_driver.h |  2 +-
 include/sound/core.h            |  4 ++--
 include/sound/hwdep.h           |  2 +-
 include/sound/pcm.h             |  2 +-
 include/sound/rawmidi.h         |  2 +-
 sound/aoa/soundbus/i2sbus/pcm.c |  4 ++--
 sound/core/compress_offload.c   | 16 ++++++++------
 sound/core/control.c            | 14 ++++++------
 sound/core/control_led.c        |  4 ++--
 sound/core/hwdep.c              | 38 ++++++++++++++++++---------------
 sound/core/init.c               | 28 +++++++++++++++---------
 sound/core/pcm.c                | 22 +++++++++++--------
 sound/core/rawmidi.c            | 29 +++++++++++--------------
 sound/core/seq/seq_clientmgr.c  | 16 ++++++++------
 sound/core/timer.c              | 16 ++++++++------
 sound/core/ump.c                |  8 +++----
 sound/pci/hda/hda_hwdep.c       |  4 ++--
 sound/usb/media.c               |  4 ++--
 18 files changed, 119 insertions(+), 96 deletions(-)

-- 
2.35.3

[PATCH RFC 1/9] ALSA: core: Introduce snd_device_alloc()

[Takashi Iwai]

Introduce a new helper, snd_device_alloc(), for allocating a struct
device that is bound with the sound class.  It's a replacement of
snd_device_initialize().

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 include/sound/core.h |  1 +
 sound/core/init.c    | 31 +++++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/include/sound/core.h b/include/sound/core.h
index f6e0dd648b80..f986fcc5f18f 100644
--- a/include/sound/core.h
+++ b/include/sound/core.h
@@ -239,6 +239,7 @@ extern struct dentry *sound_debugfs_root;
 
 void snd_request_card(int card);
 
+int snd_device_alloc(struct device **dev_p, struct snd_card *card);
 void snd_device_initialize(struct device *dev, struct snd_card *card);
 
 int snd_register_device(int type, struct snd_card *card, int dev,
diff --git a/sound/core/init.c b/sound/core/init.c
index baef2688d0cf..a4de9f00d90f 100644
--- a/sound/core/init.c
+++ b/sound/core/init.c
@@ -134,6 +134,37 @@ void snd_device_initialize(struct device *dev, struct snd_card *card)
 }
 EXPORT_SYMBOL_GPL(snd_device_initialize);
 
+/* the default release callback set in snd_device_alloc() */
+static void default_release_alloc(struct device *dev)
+{
+	kfree(dev);
+}
+
+/**
+ * snd_device_alloc - Allocate and initialize struct device for sound devices
+ * @dev_p: pointer to store the allocated device
+ * @card: card to assign, optional
+ *
+ * For releasing the allocated device, call put_device().
+ */
+int snd_device_alloc(struct device **dev_p, struct snd_card *card)
+{
+	struct device *dev;
+
+	*dev_p = NULL;
+	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+	if (!dev)
+		return -ENOMEM;
+	device_initialize(dev);
+	if (card)
+		dev->parent = &card->card_dev;
+	dev->class = &sound_class;
+	dev->release = default_release_alloc;
+	*dev_p = dev;
+	return 0;
+}
+EXPORT_SYMBOL_GPL(snd_device_alloc);
+
 static int snd_card_init(struct snd_card *card, struct device *parent,
 			 int idx, const char *xid, struct module *module,
 			 size_t extra_size);
-- 
2.35.3

[PATCH RFC 2/9] ALSA: control: Don't embed ctl_dev

[Takashi Iwai]

Embedding the ctl_dev in the snd_card object may result in UAF when
the delayed kobj release is used; at the delayed kobj release, it
still accesses the struct device itself while the card memory (that
embeds the struct device) may be already gone.

As a workaround, detach the struct device from the card object by
allocating via the new snd_device_alloc() helper.  The rest are just
replacing ctl_dev access to the pointer.

This is based on the fix Curtis posted initially.  In this patch, the
changes are split and use the new helper function instead.

Link: https://lore.kernel.org/r/20230801171928.1460120-1-cujomalainey@chromium.org
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 include/sound/core.h     |  2 +-
 sound/core/control.c     | 14 ++++++++------
 sound/core/control_led.c |  4 ++--
 sound/usb/media.c        |  2 +-
 4 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/include/sound/core.h b/include/sound/core.h
index f986fcc5f18f..f3f6b720a278 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/sound/core/control.c b/sound/core/control.c
index 8386b53acdcd..eb975df067fb 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,7 +2373,7 @@ 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;
 }
 
@@ -2395,12 +2395,14 @@ int snd_ctl_create(struct snd_card *card)
 	if (snd_BUG_ON(card->number < 0 || card->number >= SNDRV_CARDS))
 		return -ENXIO;
 
-	snd_device_initialize(&card->ctl_dev, card);
-	dev_set_name(&card->ctl_dev, "controlC%d", card->number);
+	err = snd_device_alloc(&card->ctl_dev, card);
+	if (err < 0)
+		return err;
+	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 ee77547bf8dc..760e46cf25cc 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/usb/media.c b/sound/usb/media.c
index 840f42cb9272..6d11fedb4632 100644
--- a/sound/usb/media.c
+++ b/sound/usb/media.c
@@ -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;
-- 
2.35.3

[PATCH RFC 3/9] ALSA: pcm: Don't embed device

[Takashi Iwai]

So far we use the embedded struct device for each PCM substreams in
struct snd_pcm.  This may result in UAF when the delayed kobj release
is used; each corresponding struct device is still accessed at the
(delayed) device release, while the snd_pcm object may be already
gone.

As a workaround, detach the struct device from the snd_pcm object by
allocating via the new snd_device_alloc() helper.

A caveat is that we store the PCM substream pointer to drvdata since
the device resume and others require the access to it.

This patch is based on the fix Curtis posted initially.  In this
patch, the changes are split and use the new helper function instead.

Link: https://lore.kernel.org/r/20230801171928.1460120-1-cujomalainey@chromium.org
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 include/sound/pcm.h             |  2 +-
 sound/aoa/soundbus/i2sbus/pcm.c |  4 ++--
 sound/core/pcm.c                | 22 +++++++++++++---------
 sound/usb/media.c               |  2 +-
 4 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index 19f564606ac4..0243a13e9ac4 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 a9e502a6cdeb..3680eb6eabc9 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/pcm.c b/sound/core/pcm.c
index 9d95e3731123..317a25b68159 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);
@@ -650,11 +650,14 @@ 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,
+	err = snd_device_alloc(&pstr->dev, pcm->card);
+	if (err < 0)
+		return err;
+	dev_set_name(pstr->dev, "pcmC%iD%i%c", pcm->card->number, pcm->device,
 		     stream == SNDRV_PCM_STREAM_PLAYBACK ? 'p' : 'c');
+	pstr->dev->groups = pcm_dev_attr_groups;
+	pstr->dev->type = &pcm_dev_type;
+	dev_set_drvdata(pstr->dev, pstr);
 
 	if (!pcm->internal) {
 		err = snd_pcm_stream_proc_init(pstr);
@@ -847,7 +850,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 +1020,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 +1081,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 +1128,8 @@ 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);
+		if (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 6d11fedb4632..d48db6f3ae65 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;
-- 
2.35.3

[PATCH RFC 4/9] ALSA: hwdep: Don't embed device

[Takashi Iwai]

Like control and PCM devices, it's better to avoid the embedded struct
device for hwdep (although it's more or less well working), too.
Change it to allocate via snd_device_alloc(), and free the memory at
the common snd_hwdep_free().

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 include/sound/hwdep.h     |  2 +-
 sound/core/hwdep.c        | 38 +++++++++++++++++++++-----------------
 sound/pci/hda/hda_hwdep.c |  4 ++--
 3 files changed, 24 insertions(+), 20 deletions(-)

diff --git a/include/sound/hwdep.h b/include/sound/hwdep.h
index 8d6cdb254039..b0da633184cd 100644
--- a/include/sound/hwdep.h
+++ b/include/sound/hwdep.h
@@ -53,7 +53,7 @@ struct snd_hwdep {
 	wait_queue_head_t open_wait;
 	void *private_data;
 	void (*private_free) (struct snd_hwdep *hwdep);
-	struct device dev;
+	struct device *dev;
 
 	struct mutex open_mutex;
 	int used;			/* reference counter */
diff --git a/sound/core/hwdep.c b/sound/core/hwdep.c
index e95fa275c289..de7476034f2c 100644
--- a/sound/core/hwdep.c
+++ b/sound/core/hwdep.c
@@ -338,9 +338,14 @@ static const struct file_operations snd_hwdep_f_ops =
 	.mmap =		snd_hwdep_mmap,
 };
 
-static void release_hwdep_device(struct device *dev)
+static void snd_hwdep_free(struct snd_hwdep *hwdep)
 {
-	kfree(container_of(dev, struct snd_hwdep, dev));
+	if (!hwdep)
+		return;
+	if (hwdep->private_free)
+		hwdep->private_free(hwdep);
+	put_device(hwdep->dev);
+	kfree(hwdep);
 }
 
 /**
@@ -382,16 +387,20 @@ int snd_hwdep_new(struct snd_card *card, char *id, int device,
 	if (id)
 		strscpy(hwdep->id, id, sizeof(hwdep->id));
 
-	snd_device_initialize(&hwdep->dev, card);
-	hwdep->dev.release = release_hwdep_device;
-	dev_set_name(&hwdep->dev, "hwC%iD%i", card->number, device);
+	err = snd_device_alloc(&hwdep->dev, card);
+	if (err < 0) {
+		snd_hwdep_free(hwdep);
+		return err;
+	}
+
+	dev_set_name(hwdep->dev, "hwC%iD%i", card->number, device);
 #ifdef CONFIG_SND_OSSEMUL
 	hwdep->oss_type = -1;
 #endif
 
 	err = snd_device_new(card, SNDRV_DEV_HWDEP, hwdep, &ops);
 	if (err < 0) {
-		put_device(&hwdep->dev);
+		snd_hwdep_free(hwdep);
 		return err;
 	}
 
@@ -403,12 +412,7 @@ EXPORT_SYMBOL(snd_hwdep_new);
 
 static int snd_hwdep_dev_free(struct snd_device *device)
 {
-	struct snd_hwdep *hwdep = device->device_data;
-	if (!hwdep)
-		return 0;
-	if (hwdep->private_free)
-		hwdep->private_free(hwdep);
-	put_device(&hwdep->dev);
+	snd_hwdep_free(device->device_data);
 	return 0;
 }
 
@@ -426,9 +430,9 @@ static int snd_hwdep_dev_register(struct snd_device *device)
 	list_add_tail(&hwdep->list, &snd_hwdep_devices);
 	err = snd_register_device(SNDRV_DEVICE_TYPE_HWDEP,
 				  hwdep->card, hwdep->device,
-				  &snd_hwdep_f_ops, hwdep, &hwdep->dev);
+				  &snd_hwdep_f_ops, hwdep, hwdep->dev);
 	if (err < 0) {
-		dev_err(&hwdep->dev, "unable to register\n");
+		dev_err(hwdep->dev, "unable to register\n");
 		list_del(&hwdep->list);
 		mutex_unlock(&register_mutex);
 		return err;
@@ -439,12 +443,12 @@ static int snd_hwdep_dev_register(struct snd_device *device)
 	if (hwdep->oss_type >= 0) {
 		if (hwdep->oss_type == SNDRV_OSS_DEVICE_TYPE_DMFM &&
 		    hwdep->device)
-			dev_warn(&hwdep->dev,
+			dev_warn(hwdep->dev,
 				 "only hwdep device 0 can be registered as OSS direct FM device!\n");
 		else if (snd_register_oss_device(hwdep->oss_type,
 						 card, hwdep->device,
 						 &snd_hwdep_f_ops, hwdep) < 0)
-			dev_warn(&hwdep->dev,
+			dev_warn(hwdep->dev,
 				 "unable to register OSS compatibility device\n");
 		else
 			hwdep->ossreg = 1;
@@ -471,7 +475,7 @@ static int snd_hwdep_dev_disconnect(struct snd_device *device)
 	if (hwdep->ossreg)
 		snd_unregister_oss_device(hwdep->oss_type, hwdep->card, hwdep->device);
 #endif
-	snd_unregister_device(&hwdep->dev);
+	snd_unregister_device(hwdep->dev);
 	list_del_init(&hwdep->list);
 	mutex_unlock(&hwdep->open_mutex);
 	mutex_unlock(&register_mutex);
diff --git a/sound/pci/hda/hda_hwdep.c b/sound/pci/hda/hda_hwdep.c
index 125e97fe0b1c..727f39acedfc 100644
--- a/sound/pci/hda/hda_hwdep.c
+++ b/sound/pci/hda/hda_hwdep.c
@@ -114,8 +114,8 @@ int snd_hda_create_hwdep(struct hda_codec *codec)
 #endif
 
 	/* for sysfs */
-	hwdep->dev.groups = snd_hda_dev_attr_groups;
-	dev_set_drvdata(&hwdep->dev, codec);
+	hwdep->dev->groups = snd_hda_dev_attr_groups;
+	dev_set_drvdata(hwdep->dev, codec);
 
 	return 0;
 }
-- 
2.35.3

[PATCH RFC 5/9] ALSA: rawmidi: Don't embed device

[Takashi Iwai]

This patch detaches the struct device from the snd_rawmidi object by
allocating via snd_device_alloc(), just like done for other devices.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 include/sound/rawmidi.h |  2 +-
 sound/core/rawmidi.c    | 29 +++++++++++++----------------
 sound/core/ump.c        |  8 ++++----
 3 files changed, 18 insertions(+), 21 deletions(-)

diff --git a/include/sound/rawmidi.h b/include/sound/rawmidi.h
index b0197b1d1fe4..f31cabf0158c 100644
--- a/include/sound/rawmidi.h
+++ b/include/sound/rawmidi.h
@@ -135,7 +135,7 @@ struct snd_rawmidi {
 	struct mutex open_mutex;
 	wait_queue_head_t open_wait;
 
-	struct device dev;
+	struct device *dev;
 
 	struct snd_info_entry *proc_entry;
 
diff --git a/sound/core/rawmidi.c b/sound/core/rawmidi.c
index 2d3cec908154..ba06484ac4aa 100644
--- a/sound/core/rawmidi.c
+++ b/sound/core/rawmidi.c
@@ -44,11 +44,11 @@ static LIST_HEAD(snd_rawmidi_devices);
 static DEFINE_MUTEX(register_mutex);
 
 #define rmidi_err(rmidi, fmt, args...) \
-	dev_err(&(rmidi)->dev, fmt, ##args)
+	dev_err((rmidi)->dev, fmt, ##args)
 #define rmidi_warn(rmidi, fmt, args...) \
-	dev_warn(&(rmidi)->dev, fmt, ##args)
+	dev_warn((rmidi)->dev, fmt, ##args)
 #define rmidi_dbg(rmidi, fmt, args...) \
-	dev_dbg(&(rmidi)->dev, fmt, ##args)
+	dev_dbg((rmidi)->dev, fmt, ##args)
 
 struct snd_rawmidi_status32 {
 	s32 stream;
@@ -1877,11 +1877,6 @@ static int snd_rawmidi_alloc_substreams(struct snd_rawmidi *rmidi,
 	return 0;
 }
 
-static void release_rawmidi_device(struct device *dev)
-{
-	kfree(container_of(dev, struct snd_rawmidi, dev));
-}
-
 /* used for both rawmidi and ump */
 int snd_rawmidi_init(struct snd_rawmidi *rmidi,
 		     struct snd_card *card, char *id, int device,
@@ -1906,12 +1901,13 @@ int snd_rawmidi_init(struct snd_rawmidi *rmidi,
 	if (id != NULL)
 		strscpy(rmidi->id, id, sizeof(rmidi->id));
 
-	snd_device_initialize(&rmidi->dev, card);
-	rmidi->dev.release = release_rawmidi_device;
+	err = snd_device_alloc(&rmidi->dev, card);
+	if (err < 0)
+		return err;
 	if (rawmidi_is_ump(rmidi))
-		dev_set_name(&rmidi->dev, "umpC%iD%i", card->number, device);
+		dev_set_name(rmidi->dev, "umpC%iD%i", card->number, device);
 	else
-		dev_set_name(&rmidi->dev, "midiC%iD%i", card->number, device);
+		dev_set_name(rmidi->dev, "midiC%iD%i", card->number, device);
 
 	err = snd_rawmidi_alloc_substreams(rmidi,
 					   &rmidi->streams[SNDRV_RAWMIDI_STREAM_INPUT],
@@ -1996,7 +1992,8 @@ int snd_rawmidi_free(struct snd_rawmidi *rmidi)
 	snd_rawmidi_free_substreams(&rmidi->streams[SNDRV_RAWMIDI_STREAM_OUTPUT]);
 	if (rmidi->private_free)
 		rmidi->private_free(rmidi);
-	put_device(&rmidi->dev);
+	put_device(rmidi->dev);
+	kfree(rmidi);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(snd_rawmidi_free);
@@ -2038,7 +2035,7 @@ static int snd_rawmidi_dev_register(struct snd_device *device)
 
 	err = snd_register_device(SNDRV_DEVICE_TYPE_RAWMIDI,
 				  rmidi->card, rmidi->device,
-				  &snd_rawmidi_f_ops, rmidi, &rmidi->dev);
+				  &snd_rawmidi_f_ops, rmidi, rmidi->dev);
 	if (err < 0) {
 		rmidi_err(rmidi, "unable to register\n");
 		goto error;
@@ -2103,7 +2100,7 @@ static int snd_rawmidi_dev_register(struct snd_device *device)
 	return 0;
 
  error_unregister:
-	snd_unregister_device(&rmidi->dev);
+	snd_unregister_device(rmidi->dev);
  error:
 	mutex_lock(&register_mutex);
 	list_del(&rmidi->list);
@@ -2142,7 +2139,7 @@ static int snd_rawmidi_dev_disconnect(struct snd_device *device)
 		rmidi->ossreg = 0;
 	}
 #endif /* CONFIG_SND_OSSEMUL */
-	snd_unregister_device(&rmidi->dev);
+	snd_unregister_device(rmidi->dev);
 	mutex_unlock(&rmidi->open_mutex);
 	mutex_unlock(&register_mutex);
 	return 0;
diff --git a/sound/core/ump.c b/sound/core/ump.c
index 246348766ec1..fbe2892e72fd 100644
--- a/sound/core/ump.c
+++ b/sound/core/ump.c
@@ -13,10 +13,10 @@
 #include <sound/ump.h>
 #include <sound/ump_convert.h>
 
-#define ump_err(ump, fmt, args...)	dev_err(&(ump)->core.dev, fmt, ##args)
-#define ump_warn(ump, fmt, args...)	dev_warn(&(ump)->core.dev, fmt, ##args)
-#define ump_info(ump, fmt, args...)	dev_info(&(ump)->core.dev, fmt, ##args)
-#define ump_dbg(ump, fmt, args...)	dev_dbg(&(ump)->core.dev, fmt, ##args)
+#define ump_err(ump, fmt, args...)	dev_err((ump)->core.dev, fmt, ##args)
+#define ump_warn(ump, fmt, args...)	dev_warn((ump)->core.dev, fmt, ##args)
+#define ump_info(ump, fmt, args...)	dev_info((ump)->core.dev, fmt, ##args)
+#define ump_dbg(ump, fmt, args...)	dev_dbg((ump)->core.dev, fmt, ##args)
 
 static int snd_ump_dev_register(struct snd_rawmidi *rmidi);
 static int snd_ump_dev_unregister(struct snd_rawmidi *rmidi);
-- 
2.35.3

[PATCH RFC 6/9] ALSA: compress: Don't embed device

[Takashi Iwai]

Embedding the struct device to snd_compr object may result in UAF when
the delayed kobj release is used.  Like other devices, let's detach
the struct device from the snd_compr by allocating dynamically via
snd_device_alloc().

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 include/sound/compress_driver.h |  2 +-
 sound/core/compress_offload.c   | 16 ++++++++++------
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h
index d91289c6f00e..bcf872c17dd3 100644
--- a/include/sound/compress_driver.h
+++ b/include/sound/compress_driver.h
@@ -148,7 +148,7 @@ struct snd_compr_ops {
  */
 struct snd_compr {
 	const char *name;
-	struct device dev;
+	struct device *dev;
 	struct snd_compr_ops *ops;
 	void *private_data;
 	struct snd_card *card;
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index 30f73097447b..619371aa9964 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -546,7 +546,7 @@ static int snd_compr_allocate_buffer(struct snd_compr_stream *stream,
 		if (stream->runtime->dma_buffer_p) {
 
 			if (buffer_size > stream->runtime->dma_buffer_p->bytes)
-				dev_err(&stream->device->dev,
+				dev_err(stream->device->dev,
 						"Not enough DMA buffer");
 			else
 				buffer = stream->runtime->dma_buffer_p->area;
@@ -1070,7 +1070,7 @@ static int snd_compress_dev_register(struct snd_device *device)
 	/* register compressed device */
 	ret = snd_register_device(SNDRV_DEVICE_TYPE_COMPRESS,
 				  compr->card, compr->device,
-				  &snd_compr_file_ops, compr, &compr->dev);
+				  &snd_compr_file_ops, compr, compr->dev);
 	if (ret < 0) {
 		pr_err("snd_register_device failed %d\n", ret);
 		return ret;
@@ -1084,7 +1084,7 @@ static int snd_compress_dev_disconnect(struct snd_device *device)
 	struct snd_compr *compr;
 
 	compr = device->device_data;
-	snd_unregister_device(&compr->dev);
+	snd_unregister_device(compr->dev);
 	return 0;
 }
 
@@ -1158,7 +1158,7 @@ static int snd_compress_dev_free(struct snd_device *device)
 
 	compr = device->device_data;
 	snd_compress_proc_done(compr);
-	put_device(&compr->dev);
+	put_device(compr->dev);
 	return 0;
 }
 
@@ -1189,12 +1189,16 @@ int snd_compress_new(struct snd_card *card, int device,
 
 	snd_compress_set_id(compr, id);
 
-	snd_device_initialize(&compr->dev, card);
-	dev_set_name(&compr->dev, "comprC%iD%i", card->number, device);
+	ret = snd_device_alloc(&compr->dev, card);
+	if (ret)
+		return ret;
+	dev_set_name(compr->dev, "comprC%iD%i", card->number, device);
 
 	ret = snd_device_new(card, SNDRV_DEV_COMPRESS, compr, &ops);
 	if (ret == 0)
 		snd_compress_proc_init(compr);
+	else
+		put_device(compr->dev);
 
 	return ret;
 }
-- 
2.35.3

[PATCH RFC 7/9] ALSA: timer: Create device with snd_device_alloc()

[Takashi Iwai]

Align with the other components, and use snd_device_alloc() for the
new sound device for timer, too.  No functional changes.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/core/timer.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/sound/core/timer.c b/sound/core/timer.c
index 9d0d2a5c2e15..e6e551d4a29e 100644
--- a/sound/core/timer.c
+++ b/sound/core/timer.c
@@ -2301,7 +2301,7 @@ static void snd_timer_free_all(void)
 		snd_timer_free(timer);
 }
 
-static struct device timer_dev;
+static struct device *timer_dev;
 
 /*
  *  ENTRY functions
@@ -2311,8 +2311,10 @@ static int __init alsa_timer_init(void)
 {
 	int err;
 
-	snd_device_initialize(&timer_dev, NULL);
-	dev_set_name(&timer_dev, "timer");
+	err = snd_device_alloc(&timer_dev, NULL);
+	if (err < 0)
+		return err;
+	dev_set_name(timer_dev, "timer");
 
 #ifdef SNDRV_OSS_INFO_DEV_TIMERS
 	snd_oss_info_register(SNDRV_OSS_INFO_DEV_TIMERS, SNDRV_CARDS - 1,
@@ -2326,7 +2328,7 @@ static int __init alsa_timer_init(void)
 	}
 
 	err = snd_register_device(SNDRV_DEVICE_TYPE_TIMER, NULL, 0,
-				  &snd_timer_f_ops, NULL, &timer_dev);
+				  &snd_timer_f_ops, NULL, timer_dev);
 	if (err < 0) {
 		pr_err("ALSA: unable to register timer device (%i)\n", err);
 		snd_timer_free_all();
@@ -2337,15 +2339,15 @@ static int __init alsa_timer_init(void)
 	return 0;
 
 put_timer:
-	put_device(&timer_dev);
+	put_device(timer_dev);
 	return err;
 }
 
 static void __exit alsa_timer_exit(void)
 {
-	snd_unregister_device(&timer_dev);
+	snd_unregister_device(timer_dev);
 	snd_timer_free_all();
-	put_device(&timer_dev);
+	put_device(timer_dev);
 	snd_timer_proc_done();
 #ifdef SNDRV_OSS_INFO_DEV_TIMERS
 	snd_oss_info_unregister(SNDRV_OSS_INFO_DEV_TIMERS, SNDRV_CARDS - 1);
-- 
2.35.3

[PATCH RFC 8/9] ALSA: seq: Create device with snd_device_alloc()

[Takashi Iwai]

Align with the other components, and use snd_device_alloc() for the
new sound device for sequencer, too.  No functional changes.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/core/seq/seq_clientmgr.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/sound/core/seq/seq_clientmgr.c b/sound/core/seq/seq_clientmgr.c
index e3f9ea67d019..42a705141050 100644
--- a/sound/core/seq/seq_clientmgr.c
+++ b/sound/core/seq/seq_clientmgr.c
@@ -2721,7 +2721,7 @@ static const struct file_operations snd_seq_f_ops =
 	.compat_ioctl =	snd_seq_ioctl_compat,
 };
 
-static struct device seq_dev;
+static struct device *seq_dev;
 
 /* 
  * register sequencer device 
@@ -2730,15 +2730,17 @@ int __init snd_sequencer_device_init(void)
 {
 	int err;
 
-	snd_device_initialize(&seq_dev, NULL);
-	dev_set_name(&seq_dev, "seq");
+	err = snd_device_alloc(&seq_dev, NULL);
+	if (err < 0)
+		return err;
+	dev_set_name(seq_dev, "seq");
 
 	mutex_lock(&register_mutex);
 	err = snd_register_device(SNDRV_DEVICE_TYPE_SEQUENCER, NULL, 0,
-				  &snd_seq_f_ops, NULL, &seq_dev);
+				  &snd_seq_f_ops, NULL, seq_dev);
 	mutex_unlock(&register_mutex);
 	if (err < 0) {
-		put_device(&seq_dev);
+		put_device(seq_dev);
 		return err;
 	}
 	
@@ -2752,6 +2754,6 @@ int __init snd_sequencer_device_init(void)
  */
 void snd_sequencer_device_done(void)
 {
-	snd_unregister_device(&seq_dev);
-	put_device(&seq_dev);
+	snd_unregister_device(seq_dev);
+	put_device(seq_dev);
 }
-- 
2.35.3

[PATCH RFC 9/9] ALSA: core: Drop snd_device_initialize()

[Takashi Iwai]

Now all users of snd_device_intialize() are gone, let's drop it.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 include/sound/core.h |  1 -
 sound/core/init.c    | 23 -----------------------
 2 files changed, 24 deletions(-)

diff --git a/include/sound/core.h b/include/sound/core.h
index f3f6b720a278..dfef0c9d4b9f 100644
--- a/include/sound/core.h
+++ b/include/sound/core.h
@@ -240,7 +240,6 @@ extern struct dentry *sound_debugfs_root;
 void snd_request_card(int card);
 
 int snd_device_alloc(struct device **dev_p, struct snd_card *card);
-void snd_device_initialize(struct device *dev, struct snd_card *card);
 
 int snd_register_device(int type, struct snd_card *card, int dev,
 			const struct file_operations *f_ops,
diff --git a/sound/core/init.c b/sound/core/init.c
index a4de9f00d90f..d61bde1225f2 100644
--- a/sound/core/init.c
+++ b/sound/core/init.c
@@ -111,29 +111,6 @@ static int get_slot_from_bitmask(int mask, int (*check)(struct module *, int),
 	return mask; /* unchanged */
 }
 
-/* the default release callback set in snd_device_initialize() below;
- * this is just NOP for now, as almost all jobs are already done in
- * dev_free callback of snd_device chain instead.
- */
-static void default_release(struct device *dev)
-{
-}
-
-/**
- * snd_device_initialize - Initialize struct device for sound devices
- * @dev: device to initialize
- * @card: card to assign, optional
- */
-void snd_device_initialize(struct device *dev, struct snd_card *card)
-{
-	device_initialize(dev);
-	if (card)
-		dev->parent = &card->card_dev;
-	dev->class = &sound_class;
-	dev->release = default_release;
-}
-EXPORT_SYMBOL_GPL(snd_device_initialize);
-
 /* the default release callback set in snd_device_alloc() */
 static void default_release_alloc(struct device *dev)
 {
-- 
2.35.3

Re: [PATCH RFC 0/9] ALSA: Don't embed struct devices

[Jaroslav Kysela]

On 16. 08. 23 18:02, Takashi Iwai wrote:
> Hi,
> 
> this is another set of patches to attempt papering over the UAF
> problems that are seen when the delayed kobject release is enabled, as
> initially reported by Curtis:
>    https://lore.kernel.org/r/20230801171928.1460120-1-cujomalainey@chromium.org
> 
> There was a previous patch set with a different approach (using the
> device refcount dependencies), but this is a sort of step-back to the
> old way.
>    https://lore.kernel.org/r/20230807135207.17708-1-tiwai@suse.de
> 
> After discussions and evaluations, we agreed that decoupling the
> struct device from each sound component object is the safest (and
> easiest) way as of now.  For applying the changes more consistently, I
> introduced a new helper for the struct device allocation and
> initialization, and applied all components.
> 
> A couple of more changes for card_dev refcount managed aren't included
> in this patch set, though.  They might be good to have, but this patch
> set should suffice for the currently seen UAF problems.
> 
> For a long-term solution, we may restructure the device management,
> then the struct devices may be embedded again in each object.  But,
> it'll need lots of other changes and cleanups, a big TODO.
> 
> The latest patches are found in topic/dev-split branch of sound.git
> tree.
> 
> 
> Takashi
> 
> ===
> 
> Takashi Iwai (9):
>    ALSA: core: Introduce snd_device_alloc()
>    ALSA: control: Don't embed ctl_dev
>    ALSA: pcm: Don't embed device
>    ALSA: hwdep: Don't embed device
>    ALSA: rawmidi: Don't embed device
>    ALSA: compress: Don't embed device
>    ALSA: timer: Create device with snd_device_alloc()
>    ALSA: seq: Create device with snd_device_alloc()
>    ALSA: core: Drop snd_device_initialize()

For all commits:

Reviewed-by: Jaroslav Kysela <perex@perex.cz>

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [PATCH RFC 0/9] ALSA: Don't embed struct devices

[Curtis Malainey]

On Wed, Aug 16, 2023 at 9:45 AM Jaroslav Kysela <perex@perex.cz> wrote:
>
> On 16. 08. 23 18:02, Takashi Iwai wrote:
> > Hi,
> >
> > this is another set of patches to attempt papering over the UAF
> > problems that are seen when the delayed kobject release is enabled, as
> > initially reported by Curtis:
> >    https://lore.kernel.org/r/20230801171928.1460120-1-cujomalainey@chromium.org
> >
> > There was a previous patch set with a different approach (using the
> > device refcount dependencies), but this is a sort of step-back to the
> > old way.
> >    https://lore.kernel.org/r/20230807135207.17708-1-tiwai@suse.de
> >
> > After discussions and evaluations, we agreed that decoupling the
> > struct device from each sound component object is the safest (and
> > easiest) way as of now.  For applying the changes more consistently, I
> > introduced a new helper for the struct device allocation and
> > initialization, and applied all components.
> >
> > A couple of more changes for card_dev refcount managed aren't included
> > in this patch set, though.  They might be good to have, but this patch
> > set should suffice for the currently seen UAF problems.
> >
> > For a long-term solution, we may restructure the device management,
> > then the struct devices may be embedded again in each object.  But,
> > it'll need lots of other changes and cleanups, a big TODO.

I agree I think we should apply this as proper fixes will be a big
lift. Thanks for refining them.

> >
> > The latest patches are found in topic/dev-split branch of sound.git
> > tree.
> >
> >
> > Takashi
> >
> > ===
> >
> > Takashi Iwai (9):
> >    ALSA: core: Introduce snd_device_alloc()
> >    ALSA: control: Don't embed ctl_dev
> >    ALSA: pcm: Don't embed device
> >    ALSA: hwdep: Don't embed device
> >    ALSA: rawmidi: Don't embed device
> >    ALSA: compress: Don't embed device
> >    ALSA: timer: Create device with snd_device_alloc()
> >    ALSA: seq: Create device with snd_device_alloc()
> >    ALSA: core: Drop snd_device_initialize()
>
> For all commits:
>
> Reviewed-by: Jaroslav Kysela <perex@perex.cz>
>
> --
> Jaroslav Kysela <perex@perex.cz>
> Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
>

For all

Signed-off-by: Curtis Malainey <cujomalainey@chromium.org>
Tested-by: Curtis Malainey <cujomalainey@chromium.org>

Re: [PATCH RFC 0/9] ALSA: Don't embed struct devices

[Takashi Iwai]

On Wed, 16 Aug 2023 18:02:43 +0200,
Takashi Iwai wrote:
> 
> Hi,
> 
> this is another set of patches to attempt papering over the UAF
> problems that are seen when the delayed kobject release is enabled, as
> initially reported by Curtis:
>   https://lore.kernel.org/r/20230801171928.1460120-1-cujomalainey@chromium.org
> 
> There was a previous patch set with a different approach (using the
> device refcount dependencies), but this is a sort of step-back to the
> old way.
>   https://lore.kernel.org/r/20230807135207.17708-1-tiwai@suse.de
> 
> After discussions and evaluations, we agreed that decoupling the
> struct device from each sound component object is the safest (and
> easiest) way as of now.  For applying the changes more consistently, I
> introduced a new helper for the struct device allocation and
> initialization, and applied all components.
> 
> A couple of more changes for card_dev refcount managed aren't included
> in this patch set, though.  They might be good to have, but this patch
> set should suffice for the currently seen UAF problems.
> 
> For a long-term solution, we may restructure the device management,
> then the struct devices may be embedded again in each object.  But,
> it'll need lots of other changes and cleanups, a big TODO.
> 
> The latest patches are found in topic/dev-split branch of sound.git
> tree.
> 
> 
> Takashi
> 
> ===
> 
> Takashi Iwai (9):
>   ALSA: core: Introduce snd_device_alloc()
>   ALSA: control: Don't embed ctl_dev
>   ALSA: pcm: Don't embed device
>   ALSA: hwdep: Don't embed device
>   ALSA: rawmidi: Don't embed device
>   ALSA: compress: Don't embed device
>   ALSA: timer: Create device with snd_device_alloc()
>   ALSA: seq: Create device with snd_device_alloc()
>   ALSA: core: Drop snd_device_initialize()

Although the patch set was sent as RFC, I merged them now for 6.6 with
Acks, as there is no further plan to change.


thanks,

Takashi