ALSA: hda: Fix potential deadlock at codec unbinding

When a codec is unbound dynamically via sysfs while its stream is in
use, we may face a potential deadlock at the proc remove or a UAF.
This happens since the hda_pcm is managed by a linked list, as it
handles the hda_pcm object release via kref.

When a PCM is opened at the unbinding time, the release of hda_pcm
gets delayed and it ends up with the close of the PCM stream releasing
the associated hda_pcm object of its own.  The hda_pcm destructor
contains the PCM device release that includes the removal of procfs
entries.  And, this removal has the sync of the close of all in-use
files -- which would never finish because it's called from the PCM
file descriptor itself, i.e. it's trying to shoot its foot.

For addressing the deadlock above, this patch changes the way to
manage and release the hda_pcm object.  The kref of hda_pcm is
dropped, and instead a simple refcount is introduced in hda_codec for
keeping the track of the active PCM streams, and at each PCM open and
close, this refcount is adjusted accordingly.  At unbinding, the
driver calls snd_device_disconnect() for each PCM stream, then
synchronizes with the refcount finish, and finally releases the object
resources.

Fixes: bbbc7e8502c9 ("ALSA: hda - Allocate hda_pcm objects dynamically")
Link: https://lore.kernel.org/r/20211116072459.18930-1-tiwai@suse.de
Signed-off-by: Takashi Iwai <tiwai@suse.de>

diff --git a/include/sound/hda_codec.h b/include/sound/hda_codec.h
index 0e45963bb767f767f85eeb59e8bc6dc8603959a2..82d9daa17851773c979beac8c5761dd959eb852a 100644 (file)
--- a/include/sound/hda_codec.h
+++ b/include/sound/hda_codec.h
@@ -8,7 +8,7 @@
 #ifndef __SOUND_HDA_CODEC_H
 #define __SOUND_HDA_CODEC_H
 
-#include <linux/kref.h>
+#include <linux/refcount.h>
 #include <linux/mod_devicetable.h>
 #include <sound/info.h>
 #include <sound/control.h>
@@ -166,8 +166,8 @@ struct hda_pcm {
        bool own_chmap;         /* codec driver provides own channel maps */
        /* private: */
        struct hda_codec *codec;
-       struct kref kref;
        struct list_head list;
+       unsigned int disconnected:1;
 };
 
 /* codec information */
@@ -187,6 +187,8 @@ struct hda_codec {
 
        /* PCM to create, set by patch_ops.build_pcms callback */
        struct list_head pcm_list_head;
+       refcount_t pcm_ref;
+       wait_queue_head_t remove_sleep;
 
        /* codec specific info */
        void *spec;
@@ -420,7 +422,7 @@ void snd_hda_codec_cleanup_for_unbind(struct hda_codec *codec);
 
 static inline void snd_hda_codec_pcm_get(struct hda_pcm *pcm)
 {
-       kref_get(&pcm->kref);
+       refcount_inc(&pcm->codec->pcm_ref);
 }
 void snd_hda_codec_pcm_put(struct hda_pcm *pcm);
 

diff --git a/sound/pci/hda/hda_bind.c b/sound/pci/hda/hda_bind.c
index 1c8bffc3eec6e674b36065bf447400db71aa0835..7153bd53e189345df4a8c8b4bcf554c3a184a608 100644 (file)
--- a/sound/pci/hda/hda_bind.c
+++ b/sound/pci/hda/hda_bind.c
@@ -156,6 +156,11 @@ static int hda_codec_driver_remove(struct device *dev)
                return codec->bus->core.ext_ops->hdev_detach(&codec->core);
        }
 
+       refcount_dec(&codec->pcm_ref);
+       snd_hda_codec_disconnect_pcms(codec);
+       wait_event(codec->remove_sleep, !refcount_read(&codec->pcm_ref));
+       snd_power_sync_ref(codec->bus->card);
+
        if (codec->patch_ops.free)
                codec->patch_ops.free(codec);
        snd_hda_codec_cleanup_for_unbind(codec);

diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index eda70814369bd628676833352f3de2c3c1c3472e..7016b48227bf28d5f35ed54b656d7217eae467d9 100644 (file)
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -703,20 +703,10 @@ get_hda_cvt_setup(struct hda_codec *codec, hda_nid_t nid)
 /*
  * PCM device
  */
-static void release_pcm(struct kref *kref)
-{
-       struct hda_pcm *pcm = container_of(kref, struct hda_pcm, kref);
-
-       if (pcm->pcm)
-               snd_device_free(pcm->codec->card, pcm->pcm);
-       clear_bit(pcm->device, pcm->codec->bus->pcm_dev_bits);
-       kfree(pcm->name);
-       kfree(pcm);
-}
-
 void snd_hda_codec_pcm_put(struct hda_pcm *pcm)
 {
-       kref_put(&pcm->kref, release_pcm);
+       if (refcount_dec_and_test(&pcm->codec->pcm_ref))
+               wake_up(&pcm->codec->remove_sleep);
 }
 EXPORT_SYMBOL_GPL(snd_hda_codec_pcm_put);
 
@@ -731,7 +721,6 @@ struct hda_pcm *snd_hda_codec_pcm_new(struct hda_codec *codec,
                return NULL;
 
        pcm->codec = codec;
-       kref_init(&pcm->kref);
        va_start(args, fmt);
        pcm->name = kvasprintf(GFP_KERNEL, fmt, args);
        va_end(args);
@@ -741,6 +730,7 @@ struct hda_pcm *snd_hda_codec_pcm_new(struct hda_codec *codec,
        }
 
        list_add_tail(&pcm->list, &codec->pcm_list_head);
+       refcount_inc(&codec->pcm_ref);
        return pcm;
 }
 EXPORT_SYMBOL_GPL(snd_hda_codec_pcm_new);
@@ -748,15 +738,31 @@ EXPORT_SYMBOL_GPL(snd_hda_codec_pcm_new);
 /*
  * codec destructor
  */
+void snd_hda_codec_disconnect_pcms(struct hda_codec *codec)
+{
+       struct hda_pcm *pcm;
+
+       list_for_each_entry(pcm, &codec->pcm_list_head, list) {
+               if (pcm->disconnected)
+                       continue;
+               if (pcm->pcm)
+                       snd_device_disconnect(codec->card, pcm->pcm);
+               snd_hda_codec_pcm_put(pcm);
+               pcm->disconnected = 1;
+       }
+}
+
 static void codec_release_pcms(struct hda_codec *codec)
 {
        struct hda_pcm *pcm, *n;
 
        list_for_each_entry_safe(pcm, n, &codec->pcm_list_head, list) {
-               list_del_init(&pcm->list);
+               list_del(&pcm->list);
                if (pcm->pcm)
-                       snd_device_disconnect(codec->card, pcm->pcm);
-               snd_hda_codec_pcm_put(pcm);
+                       snd_device_free(pcm->codec->card, pcm->pcm);
+               clear_bit(pcm->device, pcm->codec->bus->pcm_dev_bits);
+               kfree(pcm->name);
+               kfree(pcm);
        }
 }
 
@@ -769,6 +775,7 @@ void snd_hda_codec_cleanup_for_unbind(struct hda_codec *codec)
                codec->registered = 0;
        }
 
+       snd_hda_codec_disconnect_pcms(codec);
        cancel_delayed_work_sync(&codec->jackpoll_work);
        if (!codec->in_freeing)
                snd_hda_ctls_clear(codec);
@@ -792,6 +799,7 @@ void snd_hda_codec_cleanup_for_unbind(struct hda_codec *codec)
        remove_conn_list(codec);
        snd_hdac_regmap_exit(&codec->core);
        codec->configured = 0;
+       refcount_set(&codec->pcm_ref, 1); /* reset refcount */
 }
 EXPORT_SYMBOL_GPL(snd_hda_codec_cleanup_for_unbind);
 
@@ -958,6 +966,8 @@ int snd_hda_codec_device_new(struct hda_bus *bus, struct snd_card *card,
        snd_array_init(&codec->verbs, sizeof(struct hda_verb *), 8);
        INIT_LIST_HEAD(&codec->conn_list);
        INIT_LIST_HEAD(&codec->pcm_list_head);
+       refcount_set(&codec->pcm_ref, 1);
+       init_waitqueue_head(&codec->remove_sleep);
 
        INIT_DELAYED_WORK(&codec->jackpoll_work, hda_jackpoll_work);
        codec->depop_delay = -1;

diff --git a/sound/pci/hda/hda_local.h b/sound/pci/hda/hda_local.h
index ea8ab8b43337896ba3ebee6a89a94d63fedff2df..4662a47add7ec1693b0de3b83ef080e553b6ea1a 100644 (file)
--- a/sound/pci/hda/hda_local.h
+++ b/sound/pci/hda/hda_local.h
@@ -137,6 +137,7 @@ int __snd_hda_add_vmaster(struct hda_codec *codec, char *name,
 int snd_hda_codec_reset(struct hda_codec *codec);
 void snd_hda_codec_register(struct hda_codec *codec);
 void snd_hda_codec_cleanup_for_unbind(struct hda_codec *codec);
+void snd_hda_codec_disconnect_pcms(struct hda_codec *codec);
 
 #define snd_hda_regmap_sync(codec)     snd_hdac_regmap_sync(&(codec)->core)