[PATCH 1/2] ALSA: hda: Move irq pending work into hda-intel stream

[PATCH 1/2] ALSA: hda: Move irq pending work into hda-intel stream

[Takashi Iwai]

Currently, the delayed IRQ handling for PCM streams is managed in a
single work embedded in hda_intel, but this is basically a per-stream
thing.  Due to the single work, we can't cancel the work properly at
closing each stream, for example.

For making the IRQ pending work to be stream-based, this patch changes
the following:

- An extended version of azx_dev (i.e. the hd-audio stream object) is
  defined for snd-hda-intel
- The irq_pending flag and irq_pending_work are moved to
  hda_intel_stream, so that they can be hda-intel stream specific
- The stream creation and assignment are refactored so that
  snd-hda-intel can handle individually;
  the snd-hda-intel specific workaround for stream tags is also moved
  to snd-hda-intel itself instead of the common code
- The irq pending work is canceled properly at free / shutdown

While we're at it, changed the bit field flag to bool, as the bit
field doesn't help much in our case.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/hda/common/controller.c     | 26 ++-------
 sound/hda/common/hda_controller.h | 12 +++-
 sound/hda/controllers/intel.c     | 92 +++++++++++++++++++++----------
 sound/hda/controllers/intel.h     | 15 ++++-
 4 files changed, 89 insertions(+), 56 deletions(-)

diff --git a/sound/hda/common/controller.c b/sound/hda/common/controller.c
index 5934e5cdfdfd..89e63a53d683 100644
--- a/sound/hda/common/controller.c
+++ b/sound/hda/common/controller.c
@@ -1264,19 +1264,17 @@ int azx_codec_configure(struct azx *chip)
 }
 EXPORT_SYMBOL_GPL(azx_codec_configure);
 
-static int stream_direction(struct azx *chip, unsigned char index)
+void azx_add_stream(struct azx *chip, struct azx_dev *azx_dev, int idx, int tag)
 {
-	if (index >= chip->capture_index_offset &&
-	    index < chip->capture_index_offset + chip->capture_streams)
-		return SNDRV_PCM_STREAM_CAPTURE;
-	return SNDRV_PCM_STREAM_PLAYBACK;
+	snd_hdac_stream_init(azx_bus(chip), azx_stream(azx_dev), idx,
+			     azx_stream_direction(chip, idx), tag);
 }
+EXPORT_SYMBOL_GPL(azx_add_stream);
 
 /* initialize SD streams */
 int azx_init_streams(struct azx *chip)
 {
 	int i;
-	int stream_tags[2] = { 0, 0 };
 
 	/* initialize each stream (aka device)
 	 * assign the starting bdl address to each stream (device)
@@ -1284,24 +1282,10 @@ int azx_init_streams(struct azx *chip)
 	 */
 	for (i = 0; i < chip->num_streams; i++) {
 		struct azx_dev *azx_dev = kzalloc_obj(*azx_dev);
-		int dir, tag;
 
 		if (!azx_dev)
 			return -ENOMEM;
-
-		dir = stream_direction(chip, i);
-		/* stream tag must be unique throughout
-		 * the stream direction group,
-		 * valid values 1...15
-		 * use separate stream tag if the flag
-		 * AZX_DCAPS_SEPARATE_STREAM_TAG is used
-		 */
-		if (chip->driver_caps & AZX_DCAPS_SEPARATE_STREAM_TAG)
-			tag = ++stream_tags[dir];
-		else
-			tag = i + 1;
-		snd_hdac_stream_init(azx_bus(chip), azx_stream(azx_dev),
-				     i, dir, tag);
+		azx_add_stream(chip, azx_dev, i, i + 1);
 	}
 
 	return 0;
diff --git a/sound/hda/common/hda_controller.h b/sound/hda/common/hda_controller.h
index 7434f38038a0..bc8ee4cc2401 100644
--- a/sound/hda/common/hda_controller.h
+++ b/sound/hda/common/hda_controller.h
@@ -57,13 +57,12 @@ enum {
 struct azx_dev {
 	struct hdac_stream core;
 
-	unsigned int irq_pending:1;
 	/*
 	 * For VIA:
 	 *  A flag to ensure DMA position is 0
 	 *  when link position is not greater than FIFO size
 	 */
-	unsigned int insufficient:1;
+	bool insufficient;
 };
 
 #define azx_stream(dev)		(&(dev)->core)
@@ -206,6 +205,15 @@ int azx_bus_init(struct azx *chip, const char *model);
 int azx_probe_codecs(struct azx *chip, unsigned int max_slots);
 int azx_codec_configure(struct azx *chip);
 int azx_init_streams(struct azx *chip);
+void azx_add_stream(struct azx *chip, struct azx_dev *s, int idx, int tag);
 void azx_free_streams(struct azx *chip);
 
+static inline int azx_stream_direction(struct azx *chip, unsigned char index)
+{
+	if (index >= chip->capture_index_offset &&
+	    index < chip->capture_index_offset + chip->capture_streams)
+		return SNDRV_PCM_STREAM_CAPTURE;
+	return SNDRV_PCM_STREAM_PLAYBACK;
+}
+
 #endif /* __SOUND_HDA_CONTROLLER_H */
diff --git a/sound/hda/controllers/intel.c b/sound/hda/controllers/intel.c
index c87d75dbd8aa..01477bd4fcb9 100644
--- a/sound/hda/controllers/intel.c
+++ b/sound/hda/controllers/intel.c
@@ -615,17 +615,17 @@ static int azx_position_ok(struct azx *chip, struct azx_dev *azx_dev);
 /* called from IRQ */
 static int azx_position_check(struct azx *chip, struct azx_dev *azx_dev)
 {
-	struct hda_intel *hda = container_of(chip, struct hda_intel, chip);
+	struct hda_intel_stream *istream = azx_dev_to_istream(azx_dev);
 	int ok;
 
 	ok = azx_position_ok(chip, azx_dev);
 	if (ok == 1) {
-		azx_dev->irq_pending = 0;
+		istream->irq_pending = false;
 		return ok;
 	} else if (ok == 0) {
 		/* bogus IRQ, process it later */
-		azx_dev->irq_pending = 1;
-		schedule_work(&hda->irq_pending_work);
+		istream->irq_pending = true;
+		schedule_work(&istream->irq_pending_work);
 	}
 	return 0;
 }
@@ -721,11 +721,13 @@ static int azx_position_ok(struct azx *chip, struct azx_dev *azx_dev)
  */
 static void azx_irq_pending_work(struct work_struct *work)
 {
-	struct hda_intel *hda = container_of(work, struct hda_intel, irq_pending_work);
+	struct hda_intel_stream *istream =
+		container_of(work, struct hda_intel_stream, irq_pending_work);
+	struct azx_dev *azx_dev = &istream->azx_dev;
+	struct hda_intel *hda = istream->hda;
 	struct azx *chip = &hda->chip;
 	struct hdac_bus *bus = azx_bus(chip);
-	struct hdac_stream *s;
-	int pending, ok;
+	int ok;
 
 	if (!hda->irq_pending_warned) {
 		dev_info(chip->card->dev,
@@ -735,28 +737,25 @@ static void azx_irq_pending_work(struct work_struct *work)
 	}
 
 	for (;;) {
-		pending = 0;
-		spin_lock_irq(&bus->reg_lock);
-		list_for_each_entry(s, &bus->stream_list, list) {
-			struct azx_dev *azx_dev = stream_to_azx_dev(s);
-			if (!azx_dev->irq_pending ||
-			    !s->substream ||
-			    !s->running)
-				continue;
+		scoped_guard(spinlock_irq, &bus->reg_lock) {
+			if (!istream->irq_pending ||
+			    !azx_dev->core.substream ||
+			    !azx_dev->core.running) {
+				return;
+			}
+
 			ok = azx_position_ok(chip, azx_dev);
-			if (ok > 0) {
-				azx_dev->irq_pending = 0;
-				spin_unlock(&bus->reg_lock);
-				snd_pcm_period_elapsed(s->substream);
-				spin_lock(&bus->reg_lock);
-			} else if (ok < 0) {
-				pending = 0;	/* too early */
-			} else
-				pending++;
+			if (ok < 0)
+				return; /* too early */
+			if (ok > 0)
+				istream->irq_pending = false;
 		}
-		spin_unlock_irq(&bus->reg_lock);
-		if (!pending)
+
+		if (ok) {
+			snd_pcm_period_elapsed(azx_dev->core.substream);
 			return;
+		}
+
 		msleep(1);
 	}
 }
@@ -767,10 +766,11 @@ static void azx_clear_irq_pending(struct azx *chip)
 	struct hdac_bus *bus = azx_bus(chip);
 	struct hdac_stream *s;
 
-	guard(spinlock_irq)(&bus->reg_lock);
 	list_for_each_entry(s, &bus->stream_list, list) {
 		struct azx_dev *azx_dev = stream_to_azx_dev(s);
-		azx_dev->irq_pending = 0;
+		struct hda_intel_stream *istream = azx_dev_to_istream(azx_dev);
+		istream->irq_pending = false;
+		cancel_work_sync(&istream->irq_pending_work);
 	}
 }
 
@@ -1797,7 +1797,6 @@ static int azx_create(struct snd_card *card, struct pci_dev *pci,
 	if (jackpoll_ms[dev] >= 50 && jackpoll_ms[dev] <= 60000)
 		chip->jackpoll_interval = msecs_to_jiffies(jackpoll_ms[dev]);
 	INIT_LIST_HEAD(&chip->pcm_list);
-	INIT_WORK(&hda->irq_pending_work, azx_irq_pending_work);
 	INIT_LIST_HEAD(&hda->list);
 	init_vga_switcheroo(chip);
 	init_completion(&hda->probe_wait);
@@ -1846,6 +1845,39 @@ static int azx_create(struct snd_card *card, struct pci_dev *pci,
 	return 0;
 }
 
+/* create and assign streams */
+static int hda_init_streams(struct azx *chip)
+{
+	int i;
+	int stream_tags[2] = { 0, 0 };
+
+	for (i = 0; i < chip->num_streams; i++) {
+		struct hda_intel_stream *s = kzalloc_obj(*s);
+		int tag, dir;
+
+		if (!s)
+			return -ENOMEM;
+
+		s->hda = container_of(chip, struct hda_intel, chip);
+		INIT_WORK(&s->irq_pending_work, azx_irq_pending_work);
+
+		/* stream tag must be unique throughout
+		 * the stream direction group,
+		 * valid values 1...15
+		 * use separate stream tag if the flag
+		 * AZX_DCAPS_SEPARATE_STREAM_TAG is used
+		 */
+		dir = azx_stream_direction(chip, i);
+		if (chip->driver_caps & AZX_DCAPS_SEPARATE_STREAM_TAG)
+			tag = ++stream_tags[dir];
+		else
+			tag = i + 1;
+		azx_add_stream(chip, &s->azx_dev, i, tag);
+	}
+
+	return 0;
+}
+
 static int azx_first_init(struct azx *chip)
 {
 	int dev = chip->dev_index;
@@ -2000,7 +2032,7 @@ static int azx_first_init(struct azx *chip)
 	}
 
 	/* initialize streams */
-	err = azx_init_streams(chip);
+	err = hda_init_streams(chip);
 	if (err < 0)
 		return err;
 
diff --git a/sound/hda/controllers/intel.h b/sound/hda/controllers/intel.h
index 2d1725f86ef1..4efb3b0fc2d8 100644
--- a/sound/hda/controllers/intel.h
+++ b/sound/hda/controllers/intel.h
@@ -9,9 +9,6 @@
 struct hda_intel {
 	struct azx chip;
 
-	/* for pending irqs */
-	struct work_struct irq_pending_work;
-
 	/* sync probing */
 	struct completion probe_wait;
 	struct delayed_work probe_work;
@@ -35,4 +32,16 @@ struct hda_intel {
 	int probe_retry;	/* being probe-retry */
 };
 
+struct hda_intel_stream {
+	struct azx_dev azx_dev;
+
+	/* for pending irqs */
+	struct hda_intel *hda;
+	struct work_struct irq_pending_work;
+	bool irq_pending;
+};
+
+#define azx_dev_to_istream(azx_dev) \
+	container_of(azx_dev, struct hda_intel_stream, azx_dev)
+
 #endif
-- 
2.54.0

[PATCH 2/2] ALSA: hda/intel: Make sure to cancel irq-pending work at closing PCM stream

[Takashi Iwai]

The pending irq work might be still floating while the assigned stream
has been already closed, which may lead to UAF, especially when
another async work for fasync is involved.

For addressing this, extend the hda_controller_ops for allowing the
extra cleanup procedure that is specific to the controller driver, and
make sure to cancel and sync the pending irq work at each PCM close
before releasing the resources.

Reported-by: Jake Lamberson <lamberson.jake@gmail.com>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/hda/common/controller.c     |  2 ++
 sound/hda/common/hda_controller.h |  2 ++
 sound/hda/controllers/intel.c     | 20 ++++++++++++++++----
 3 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/sound/hda/common/controller.c b/sound/hda/common/controller.c
index 89e63a53d683..a847546753db 100644
--- a/sound/hda/common/controller.c
+++ b/sound/hda/common/controller.c
@@ -97,6 +97,8 @@ static int azx_pcm_close(struct snd_pcm_substream *substream)
 
 	trace_azx_pcm_close(chip, azx_dev);
 	scoped_guard(mutex, &chip->open_mutex) {
+		if (chip->ops->pcm_close)
+			chip->ops->pcm_close(chip, azx_dev);
 		azx_release_device(azx_dev);
 		if (hinfo->ops.close)
 			hinfo->ops.close(hinfo, apcm->codec, substream);
diff --git a/sound/hda/common/hda_controller.h b/sound/hda/common/hda_controller.h
index bc8ee4cc2401..38227f82e704 100644
--- a/sound/hda/common/hda_controller.h
+++ b/sound/hda/common/hda_controller.h
@@ -78,6 +78,8 @@ struct hda_controller_ops {
 	int (*position_check)(struct azx *chip, struct azx_dev *azx_dev);
 	/* enable/disable the link power */
 	int (*link_power)(struct azx *chip, bool enable);
+	/* additional hook for PCM */
+	void (*pcm_close)(struct azx *chip, struct azx_dev *azx_dev);
 };
 
 struct azx_pcm {
diff --git a/sound/hda/controllers/intel.c b/sound/hda/controllers/intel.c
index 01477bd4fcb9..4b03c64e72ab 100644
--- a/sound/hda/controllers/intel.c
+++ b/sound/hda/controllers/intel.c
@@ -761,16 +761,27 @@ static void azx_irq_pending_work(struct work_struct *work)
 }
 
 /* clear irq_pending flags and assure no on-going workq */
+static void hda_intel_stream_clear_irq_pending(struct azx_dev *azx_dev)
+{
+	struct hda_intel_stream *istream = azx_dev_to_istream(azx_dev);
+
+	istream->irq_pending = false;
+	cancel_work_sync(&istream->irq_pending_work);
+}
+
+/* called at PCM close */
+static void hda_intel_pcm_close(struct azx *chip, struct azx_dev *azx_dev)
+{
+	hda_intel_stream_clear_irq_pending(azx_dev);
+}
+
 static void azx_clear_irq_pending(struct azx *chip)
 {
 	struct hdac_bus *bus = azx_bus(chip);
 	struct hdac_stream *s;
 
 	list_for_each_entry(s, &bus->stream_list, list) {
-		struct azx_dev *azx_dev = stream_to_azx_dev(s);
-		struct hda_intel_stream *istream = azx_dev_to_istream(azx_dev);
-		istream->irq_pending = false;
-		cancel_work_sync(&istream->irq_pending_work);
+		hda_intel_stream_clear_irq_pending(stream_to_azx_dev(s));
 	}
 }
 
@@ -2131,6 +2142,7 @@ static const struct dmi_system_id driver_denylist_dmi[] = {
 static const struct hda_controller_ops pci_hda_ops = {
 	.disable_msi_reset_irq = disable_msi_reset_irq,
 	.position_check = azx_position_check,
+	.pcm_close = hda_intel_pcm_close,
 };
 
 static DECLARE_BITMAP(probed_devs, SNDRV_CARDS);
-- 
2.54.0