[PATCH 0/2] ALSA: echoaudio: Fix the probe error handling

[PATCH 0/2] ALSA: echoaudio: Fix the probe error handling

[Takashi Iwai]

Hi,

this is a patch set to address the regression of the error handling
in the echoaudio driver probe phase.  Similar error patterns are seen
in other drivers, and the newly introduced helper will be used later
for those, too.


Takashi

===

Takashi Iwai (2):
  ALSA: core: Add snd_card_free_on_error() helper
  ALSA: echoaudio: Fix the missing snd_card_free() call at probe error

 include/sound/core.h            |  1 +
 sound/core/init.c               | 28 ++++++++++++++++++++++++++++
 sound/pci/echoaudio/echoaudio.c |  9 +++++++--
 3 files changed, 36 insertions(+), 2 deletions(-)

-- 
2.31.1

[PATCH 1/2] ALSA: core: Add snd_card_free_on_error() helper

[Takashi Iwai]

This is a small helper function to handle the error path more easily
when an error happens during the probe for the device with the
device-managed card.  Since devres releases in the reverser order of
the creations, usually snd_card_free() gets called at the last in the
probe error path unless it already reached snd_card_register() calls.
Due to this nature, when a driver expects the resource releases in
card->private_free, this might be called too lately.

As a workaround, one should call the probe like:

 static int __some_probe(...) { // do real probe.... }

 static int some_probe(...)
 {
	return snd_card_free_on_error(dev, __some_probe(dev, ...));
 }

so that the snd_card_free() is called explicitly at the beginning of
the error path from the probe.

This function will be used in the upcoming fixes to address the
regressions by devres usages.

Fixes: e8ad415b7a55 ("ALSA: core: Add managed card creation")
Cc: <stable@vger.kernel.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 include/sound/core.h |  1 +
 sound/core/init.c    | 28 ++++++++++++++++++++++++++++
 2 files changed, 29 insertions(+)

diff --git a/include/sound/core.h b/include/sound/core.h
index b7e9b58d3c78..6d4cc49584c6 100644
--- a/include/sound/core.h
+++ b/include/sound/core.h
@@ -284,6 +284,7 @@ int snd_card_disconnect(struct snd_card *card);
 void snd_card_disconnect_sync(struct snd_card *card);
 int snd_card_free(struct snd_card *card);
 int snd_card_free_when_closed(struct snd_card *card);
+int snd_card_free_on_error(struct device *dev, int ret);
 void snd_card_set_id(struct snd_card *card, const char *id);
 int snd_card_register(struct snd_card *card);
 int snd_card_info_init(void);
diff --git a/sound/core/init.c b/sound/core/init.c
index 31ba7024e3ad..726a8353201f 100644
--- a/sound/core/init.c
+++ b/sound/core/init.c
@@ -209,6 +209,12 @@ static void __snd_card_release(struct device *dev, void *data)
  * snd_card_register(), the very first devres action to call snd_card_free()
  * is added automatically.  In that way, the resource disconnection is assured
  * at first, then released in the expected order.
+ *
+ * If an error happens at the probe before snd_card_register() is called and
+ * there have been other devres resources, you'd need to free the card manually
+ * via snd_card_free() call in the error; otherwise it may lead to UAF due to
+ * devres call orders.  You can use snd_card_free_on_error() helper for
+ * handling it more easily.
  */
 int snd_devm_card_new(struct device *parent, int idx, const char *xid,
 		      struct module *module, size_t extra_size,
@@ -235,6 +241,28 @@ int snd_devm_card_new(struct device *parent, int idx, const char *xid,
 }
 EXPORT_SYMBOL_GPL(snd_devm_card_new);
 
+/**
+ * snd_card_free_on_error - a small helper for handling devm probe errors
+ * @dev: the managed device object
+ * @ret: the return code from the probe callback
+ *
+ * This function handles the explicit snd_card_free() call at the error from
+ * the probe callback.  It's just a small helper for simplifying the error
+ * handling with the managed devices.
+ */
+int snd_card_free_on_error(struct device *dev, int ret)
+{
+	struct snd_card *card;
+
+	if (!ret)
+		return 0;
+	card = devres_find(dev, __snd_card_release, NULL, NULL);
+	if (card)
+		snd_card_free(card);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(snd_card_free_on_error);
+
 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.31.1

[PATCH 2/2] ALSA: echoaudio: Fix the missing snd_card_free() call at probe error

[Takashi Iwai]

The previous cleanup with devres may lead to the incorrect release
orders at the probe error handling due to the devres's nature.  Until
we register the card, snd_card_free() has to be called at first for
releasing the stuff properly when the driver tries to manage and
release the stuff via card->private_free().

This patch fixes it by calling snd_card_free() on the error from the
probe callback using a new helper function.

Fixes: 9c211bf392bb ("ALSA: echoaudio: Allocate resources with device-managed APIs")
Reported-and-tested-by: Zheyu Ma <zheyuma97@gmail.com>
Cc: <stable@vger.kernel.org>
Link: https://lore.kernel.org/r/CAMhUBjm2AdyEZ_-EgexdNDN7SvY4f89=4=FwAL+c0Mg0O+X50A@mail.gmail.com
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/pci/echoaudio/echoaudio.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/sound/pci/echoaudio/echoaudio.c b/sound/pci/echoaudio/echoaudio.c
index 25b012ef5c3e..c70c3ac4e99a 100644
--- a/sound/pci/echoaudio/echoaudio.c
+++ b/sound/pci/echoaudio/echoaudio.c
@@ -1970,8 +1970,8 @@ static int snd_echo_create(struct snd_card *card,
 }
 
 /* constructor */
-static int snd_echo_probe(struct pci_dev *pci,
-			  const struct pci_device_id *pci_id)
+static int __snd_echo_probe(struct pci_dev *pci,
+			    const struct pci_device_id *pci_id)
 {
 	static int dev;
 	struct snd_card *card;
@@ -2139,6 +2139,11 @@ static int snd_echo_probe(struct pci_dev *pci,
 	return 0;
 }
 
+static int snd_echo_probe(struct pci_dev *pci,
+			  const struct pci_device_id *pci_id)
+{
+	return snd_card_free_on_error(&pci->dev, __snd_echo_probe(pci, pci_id));
+}
 
 
 #if defined(CONFIG_PM_SLEEP)
-- 
2.31.1

Re: [PATCH 1/2] ALSA: core: Add snd_card_free_on_error() helper

[Takashi Sakamoto]

Hi,

On Tue, Apr 12, 2022 at 11:31:40AM +0200, Takashi Iwai wrote:
> This is a small helper function to handle the error path more easily
> when an error happens during the probe for the device with the
> device-managed card.  Since devres releases in the reverser order of
> the creations, usually snd_card_free() gets called at the last in the
> probe error path unless it already reached snd_card_register() calls.
> Due to this nature, when a driver expects the resource releases in
> card->private_free, this might be called too lately.
> 
> As a workaround, one should call the probe like:
> 
>  static int __some_probe(...) { // do real probe.... }
> 
>  static int some_probe(...)
>  {
> 	return snd_card_free_on_error(dev, __some_probe(dev, ...));
>  }
> 
> so that the snd_card_free() is called explicitly at the beginning of
> the error path from the probe.
> 
> This function will be used in the upcoming fixes to address the
> regressions by devres usages.
> 
> Fixes: e8ad415b7a55 ("ALSA: core: Add managed card creation")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  include/sound/core.h |  1 +
>  sound/core/init.c    | 28 ++++++++++++++++++++++++++++
>  2 files changed, 29 insertions(+)
> 
> diff --git a/include/sound/core.h b/include/sound/core.h
> index b7e9b58d3c78..6d4cc49584c6 100644
> --- a/include/sound/core.h
> +++ b/include/sound/core.h
> @@ -284,6 +284,7 @@ int snd_card_disconnect(struct snd_card *card);
>  void snd_card_disconnect_sync(struct snd_card *card);
>  int snd_card_free(struct snd_card *card);
>  int snd_card_free_when_closed(struct snd_card *card);
> +int snd_card_free_on_error(struct device *dev, int ret);
>  void snd_card_set_id(struct snd_card *card, const char *id);
>  int snd_card_register(struct snd_card *card);
>  int snd_card_info_init(void);
> diff --git a/sound/core/init.c b/sound/core/init.c
> index 31ba7024e3ad..726a8353201f 100644
> --- a/sound/core/init.c
> +++ b/sound/core/init.c
> @@ -209,6 +209,12 @@ static void __snd_card_release(struct device *dev, void *data)
>   * snd_card_register(), the very first devres action to call snd_card_free()
>   * is added automatically.  In that way, the resource disconnection is assured
>   * at first, then released in the expected order.
> + *
> + * If an error happens at the probe before snd_card_register() is called and
> + * there have been other devres resources, you'd need to free the card manually
> + * via snd_card_free() call in the error; otherwise it may lead to UAF due to
> + * devres call orders.  You can use snd_card_free_on_error() helper for
> + * handling it more easily.
>   */
>  int snd_devm_card_new(struct device *parent, int idx, const char *xid,
>  		      struct module *module, size_t extra_size,
> @@ -235,6 +241,28 @@ int snd_devm_card_new(struct device *parent, int idx, const char *xid,
>  }
>  EXPORT_SYMBOL_GPL(snd_devm_card_new);
>  
> +/**
> + * snd_card_free_on_error - a small helper for handling devm probe errors
> + * @dev: the managed device object
> + * @ret: the return code from the probe callback
> + *
> + * This function handles the explicit snd_card_free() call at the error from
> + * the probe callback.  It's just a small helper for simplifying the error
> + * handling with the managed devices.
> + */
> +int snd_card_free_on_error(struct device *dev, int ret)
> +{
> +	struct snd_card *card;
> +
> +	if (!ret)
> +		return 0;
> +	card = devres_find(dev, __snd_card_release, NULL, NULL);
> +	if (card)
> +		snd_card_free(card);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(snd_card_free_on_error);
> +
>  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.31.1

The idea looks good itself to me. On the other hand, the name
'snd_card_free_on_error()' is not so suitable since it assumes that
'snd_devm_card_new()' is called in advance, while we have another function,
'snd_card_new()'.

I think it better to use 'snd_devm_card_free_on_error()' instead since
the function doesn't work as expected in the case of 'snd_card_new()'
(the snd_card_free() is not called because nothing found in devres).


Regards

Takashi Sakamoto

Re: [PATCH 1/2] ALSA: core: Add snd_card_free_on_error() helper

[Takashi Iwai]

On Tue, 12 Apr 2022 12:47:47 +0200,
Takashi Sakamoto wrote:
> 
> Hi,
> 
> On Tue, Apr 12, 2022 at 11:31:40AM +0200, Takashi Iwai wrote:
> > This is a small helper function to handle the error path more easily
> > when an error happens during the probe for the device with the
> > device-managed card.  Since devres releases in the reverser order of
> > the creations, usually snd_card_free() gets called at the last in the
> > probe error path unless it already reached snd_card_register() calls.
> > Due to this nature, when a driver expects the resource releases in
> > card->private_free, this might be called too lately.
> > 
> > As a workaround, one should call the probe like:
> > 
> >  static int __some_probe(...) { // do real probe.... }
> > 
> >  static int some_probe(...)
> >  {
> > 	return snd_card_free_on_error(dev, __some_probe(dev, ...));
> >  }
> > 
> > so that the snd_card_free() is called explicitly at the beginning of
> > the error path from the probe.
> > 
> > This function will be used in the upcoming fixes to address the
> > regressions by devres usages.
> > 
> > Fixes: e8ad415b7a55 ("ALSA: core: Add managed card creation")
> > Cc: <stable@vger.kernel.org>
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> >  include/sound/core.h |  1 +
> >  sound/core/init.c    | 28 ++++++++++++++++++++++++++++
> >  2 files changed, 29 insertions(+)
> > 
> > diff --git a/include/sound/core.h b/include/sound/core.h
> > index b7e9b58d3c78..6d4cc49584c6 100644
> > --- a/include/sound/core.h
> > +++ b/include/sound/core.h
> > @@ -284,6 +284,7 @@ int snd_card_disconnect(struct snd_card *card);
> >  void snd_card_disconnect_sync(struct snd_card *card);
> >  int snd_card_free(struct snd_card *card);
> >  int snd_card_free_when_closed(struct snd_card *card);
> > +int snd_card_free_on_error(struct device *dev, int ret);
> >  void snd_card_set_id(struct snd_card *card, const char *id);
> >  int snd_card_register(struct snd_card *card);
> >  int snd_card_info_init(void);
> > diff --git a/sound/core/init.c b/sound/core/init.c
> > index 31ba7024e3ad..726a8353201f 100644
> > --- a/sound/core/init.c
> > +++ b/sound/core/init.c
> > @@ -209,6 +209,12 @@ static void __snd_card_release(struct device *dev, void *data)
> >   * snd_card_register(), the very first devres action to call snd_card_free()
> >   * is added automatically.  In that way, the resource disconnection is assured
> >   * at first, then released in the expected order.
> > + *
> > + * If an error happens at the probe before snd_card_register() is called and
> > + * there have been other devres resources, you'd need to free the card manually
> > + * via snd_card_free() call in the error; otherwise it may lead to UAF due to
> > + * devres call orders.  You can use snd_card_free_on_error() helper for
> > + * handling it more easily.
> >   */
> >  int snd_devm_card_new(struct device *parent, int idx, const char *xid,
> >  		      struct module *module, size_t extra_size,
> > @@ -235,6 +241,28 @@ int snd_devm_card_new(struct device *parent, int idx, const char *xid,
> >  }
> >  EXPORT_SYMBOL_GPL(snd_devm_card_new);
> >  
> > +/**
> > + * snd_card_free_on_error - a small helper for handling devm probe errors
> > + * @dev: the managed device object
> > + * @ret: the return code from the probe callback
> > + *
> > + * This function handles the explicit snd_card_free() call at the error from
> > + * the probe callback.  It's just a small helper for simplifying the error
> > + * handling with the managed devices.
> > + */
> > +int snd_card_free_on_error(struct device *dev, int ret)
> > +{
> > +	struct snd_card *card;
> > +
> > +	if (!ret)
> > +		return 0;
> > +	card = devres_find(dev, __snd_card_release, NULL, NULL);
> > +	if (card)
> > +		snd_card_free(card);
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(snd_card_free_on_error);
> > +
> >  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.31.1
> 
> The idea looks good itself to me. On the other hand, the name
> 'snd_card_free_on_error()' is not so suitable since it assumes that
> 'snd_devm_card_new()' is called in advance, while we have another function,
> 'snd_card_new()'.
> 
> I think it better to use 'snd_devm_card_free_on_error()' instead since
> the function doesn't work as expected in the case of 'snd_card_new()'
> (the snd_card_free() is not called because nothing found in devres).

Yeah, that came to my mind in the first implementations, too, but it
looked too long to me, so I took this term in the submitted version :)

In theory, we can extend it to retrieve the card from the device data,
too, but I don't think worth for it.


thanks,

Takashi