[PATCH] ALSA: rawmidi: Fix potential UAF from sequencer destruction

[PATCH] ALSA: rawmidi: Fix potential UAF from sequencer destruction

[John Keeping]

If the sequencer device outlives the rawmidi device, then
snd_rawmidi_dev_seq_free() will run after release_rawmidi_device() has
freed the snd_rawmidi structure.

This can easily be reproduced with CONFIG_DEBUG_KOBJECT_RELEASE.

Keep a reference to the rawmidi device until the sequencer has been
destroyed in order to avoid this.

Signed-off-by: John Keeping <john@metanate.com>
---
 sound/core/rawmidi.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/sound/core/rawmidi.c b/sound/core/rawmidi.c
index 6f30231bdb88..b015f5f69175 100644
--- a/sound/core/rawmidi.c
+++ b/sound/core/rawmidi.c
@@ -1860,6 +1860,7 @@ static void snd_rawmidi_dev_seq_free(struct snd_seq_device *device)
 	struct snd_rawmidi *rmidi = device->private_data;
 
 	rmidi->seq_dev = NULL;
+	put_device(&rmidi->dev);
 }
 #endif
 
@@ -1936,6 +1937,9 @@ static int snd_rawmidi_dev_register(struct snd_device *device)
 #if IS_ENABLED(CONFIG_SND_SEQUENCER)
 	if (!rmidi->ops || !rmidi->ops->dev_register) { /* own registration mechanism */
 		if (snd_seq_device_new(rmidi->card, rmidi->device, SNDRV_SEQ_DEV_ID_MIDISYNTH, 0, &rmidi->seq_dev) >= 0) {
+			/* Ensure we outlive the sequencer (see snd_rawmidi_dev_seq_free). */
+			get_device(&rmidi->dev);
+
 			rmidi->seq_dev->private_data = rmidi;
 			rmidi->seq_dev->private_free = snd_rawmidi_dev_seq_free;
 			sprintf(rmidi->seq_dev->name, "MIDI %d-%d", rmidi->card->number, rmidi->device);
-- 
2.33.0

[PATCH] ALSA: rawmidi: Fix potential UAF from sequencer destruction

[John Keeping]

If the sequencer device outlives the rawmidi device, then
snd_rawmidi_dev_seq_free() will run after release_rawmidi_device() has
freed the snd_rawmidi structure.

This can easily be reproduced with CONFIG_DEBUG_KOBJECT_RELEASE.

Keep a reference to the rawmidi device until the sequencer has been
destroyed in order to avoid this.

Signed-off-by: John Keeping <john@metanate.com>
---
 sound/core/rawmidi.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/sound/core/rawmidi.c b/sound/core/rawmidi.c
index 6f30231bdb88..b015f5f69175 100644
--- a/sound/core/rawmidi.c
+++ b/sound/core/rawmidi.c
@@ -1860,6 +1860,7 @@ static void snd_rawmidi_dev_seq_free(struct snd_seq_device *device)
 	struct snd_rawmidi *rmidi = device->private_data;
 
 	rmidi->seq_dev = NULL;
+	put_device(&rmidi->dev);
 }
 #endif
 
@@ -1936,6 +1937,9 @@ static int snd_rawmidi_dev_register(struct snd_device *device)
 #if IS_ENABLED(CONFIG_SND_SEQUENCER)
 	if (!rmidi->ops || !rmidi->ops->dev_register) { /* own registration mechanism */
 		if (snd_seq_device_new(rmidi->card, rmidi->device, SNDRV_SEQ_DEV_ID_MIDISYNTH, 0, &rmidi->seq_dev) >= 0) {
+			/* Ensure we outlive the sequencer (see snd_rawmidi_dev_seq_free). */
+			get_device(&rmidi->dev);
+
 			rmidi->seq_dev->private_data = rmidi;
 			rmidi->seq_dev->private_free = snd_rawmidi_dev_seq_free;
 			sprintf(rmidi->seq_dev->name, "MIDI %d-%d", rmidi->card->number, rmidi->device);
-- 
2.33.0

Re: [PATCH] ALSA: rawmidi: Fix potential UAF from sequencer destruction

[Takashi Iwai]

On Wed, 29 Sep 2021 13:36:20 +0200,
John Keeping wrote:
> 
> If the sequencer device outlives the rawmidi device, then
> snd_rawmidi_dev_seq_free() will run after release_rawmidi_device() has
> freed the snd_rawmidi structure.
> 
> This can easily be reproduced with CONFIG_DEBUG_KOBJECT_RELEASE.
> 
> Keep a reference to the rawmidi device until the sequencer has been
> destroyed in order to avoid this.
> 
> Signed-off-by: John Keeping <john@metanate.com>

Thanks for the patch.  I wonder, though, how this could be triggered.
Is this the case where the connected sequencer device is being used
while the sound card gets released?  Or is it something else?


thanks,

Takashi

> ---
>  sound/core/rawmidi.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/sound/core/rawmidi.c b/sound/core/rawmidi.c
> index 6f30231bdb88..b015f5f69175 100644
> --- a/sound/core/rawmidi.c
> +++ b/sound/core/rawmidi.c
> @@ -1860,6 +1860,7 @@ static void snd_rawmidi_dev_seq_free(struct snd_seq_device *device)
>  	struct snd_rawmidi *rmidi = device->private_data;
>  
>  	rmidi->seq_dev = NULL;
> +	put_device(&rmidi->dev);
>  }
>  #endif
>  
> @@ -1936,6 +1937,9 @@ static int snd_rawmidi_dev_register(struct snd_device *device)
>  #if IS_ENABLED(CONFIG_SND_SEQUENCER)
>  	if (!rmidi->ops || !rmidi->ops->dev_register) { /* own registration mechanism */
>  		if (snd_seq_device_new(rmidi->card, rmidi->device, SNDRV_SEQ_DEV_ID_MIDISYNTH, 0, &rmidi->seq_dev) >= 0) {
> +			/* Ensure we outlive the sequencer (see snd_rawmidi_dev_seq_free). */
> +			get_device(&rmidi->dev);
> +
>  			rmidi->seq_dev->private_data = rmidi;
>  			rmidi->seq_dev->private_free = snd_rawmidi_dev_seq_free;
>  			sprintf(rmidi->seq_dev->name, "MIDI %d-%d", rmidi->card->number, rmidi->device);
> -- 
> 2.33.0
> 

Re: [PATCH] ALSA: rawmidi: Fix potential UAF from sequencer destruction

[Takashi Iwai]

On Wed, 29 Sep 2021 13:36:20 +0200,
John Keeping wrote:
> 
> If the sequencer device outlives the rawmidi device, then
> snd_rawmidi_dev_seq_free() will run after release_rawmidi_device() has
> freed the snd_rawmidi structure.
> 
> This can easily be reproduced with CONFIG_DEBUG_KOBJECT_RELEASE.
> 
> Keep a reference to the rawmidi device until the sequencer has been
> destroyed in order to avoid this.
> 
> Signed-off-by: John Keeping <john@metanate.com>

Thanks for the patch.  I wonder, though, how this could be triggered.
Is this the case where the connected sequencer device is being used
while the sound card gets released?  Or is it something else?


thanks,

Takashi

> ---
>  sound/core/rawmidi.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/sound/core/rawmidi.c b/sound/core/rawmidi.c
> index 6f30231bdb88..b015f5f69175 100644
> --- a/sound/core/rawmidi.c
> +++ b/sound/core/rawmidi.c
> @@ -1860,6 +1860,7 @@ static void snd_rawmidi_dev_seq_free(struct snd_seq_device *device)
>  	struct snd_rawmidi *rmidi = device->private_data;
>  
>  	rmidi->seq_dev = NULL;
> +	put_device(&rmidi->dev);
>  }
>  #endif
>  
> @@ -1936,6 +1937,9 @@ static int snd_rawmidi_dev_register(struct snd_device *device)
>  #if IS_ENABLED(CONFIG_SND_SEQUENCER)
>  	if (!rmidi->ops || !rmidi->ops->dev_register) { /* own registration mechanism */
>  		if (snd_seq_device_new(rmidi->card, rmidi->device, SNDRV_SEQ_DEV_ID_MIDISYNTH, 0, &rmidi->seq_dev) >= 0) {
> +			/* Ensure we outlive the sequencer (see snd_rawmidi_dev_seq_free). */
> +			get_device(&rmidi->dev);
> +
>  			rmidi->seq_dev->private_data = rmidi;
>  			rmidi->seq_dev->private_free = snd_rawmidi_dev_seq_free;
>  			sprintf(rmidi->seq_dev->name, "MIDI %d-%d", rmidi->card->number, rmidi->device);
> -- 
> 2.33.0
> 

Re: [PATCH] ALSA: rawmidi: Fix potential UAF from sequencer destruction

[John Keeping]

On Wed, 29 Sep 2021 16:51:47 +0200
Takashi Iwai <tiwai@suse.de> wrote:

> On Wed, 29 Sep 2021 13:36:20 +0200,
> John Keeping wrote:
> > 
> > If the sequencer device outlives the rawmidi device, then
> > snd_rawmidi_dev_seq_free() will run after release_rawmidi_device() has
> > freed the snd_rawmidi structure.
> > 
> > This can easily be reproduced with CONFIG_DEBUG_KOBJECT_RELEASE.
> > 
> > Keep a reference to the rawmidi device until the sequencer has been
> > destroyed in order to avoid this.
> > 
> > Signed-off-by: John Keeping <john@metanate.com>  
> 
> Thanks for the patch.  I wonder, though, how this could be triggered.
> Is this the case where the connected sequencer device is being used
> while the sound card gets released?  Or is it something else?

I'm not sure if it's possible to trigger via the ALSA API; I haven't
found a route that can trigger it, but that doesn't mean there isn't
one :-)

Mostly this is useful to make CONFIG_DEBUG_KOBJECT_RELEASE cleaner.


Regards,
John

> > ---
> >  sound/core/rawmidi.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/sound/core/rawmidi.c b/sound/core/rawmidi.c
> > index 6f30231bdb88..b015f5f69175 100644
> > --- a/sound/core/rawmidi.c
> > +++ b/sound/core/rawmidi.c
> > @@ -1860,6 +1860,7 @@ static void snd_rawmidi_dev_seq_free(struct snd_seq_device *device)
> >  	struct snd_rawmidi *rmidi = device->private_data;
> >  
> >  	rmidi->seq_dev = NULL;
> > +	put_device(&rmidi->dev);
> >  }
> >  #endif
> >  
> > @@ -1936,6 +1937,9 @@ static int snd_rawmidi_dev_register(struct snd_device *device)
> >  #if IS_ENABLED(CONFIG_SND_SEQUENCER)
> >  	if (!rmidi->ops || !rmidi->ops->dev_register) { /* own registration mechanism */
> >  		if (snd_seq_device_new(rmidi->card, rmidi->device, SNDRV_SEQ_DEV_ID_MIDISYNTH, 0, &rmidi->seq_dev) >= 0) {
> > +			/* Ensure we outlive the sequencer (see snd_rawmidi_dev_seq_free). */
> > +			get_device(&rmidi->dev);
> > +
> >  			rmidi->seq_dev->private_data = rmidi;
> >  			rmidi->seq_dev->private_free = snd_rawmidi_dev_seq_free;
> >  			sprintf(rmidi->seq_dev->name, "MIDI %d-%d", rmidi->card->number, rmidi->device);
> > -- 
> > 2.33.0
> >   

Re: [PATCH] ALSA: rawmidi: Fix potential UAF from sequencer destruction

[John Keeping]

On Wed, 29 Sep 2021 16:51:47 +0200
Takashi Iwai <tiwai@suse.de> wrote:

> On Wed, 29 Sep 2021 13:36:20 +0200,
> John Keeping wrote:
> > 
> > If the sequencer device outlives the rawmidi device, then
> > snd_rawmidi_dev_seq_free() will run after release_rawmidi_device() has
> > freed the snd_rawmidi structure.
> > 
> > This can easily be reproduced with CONFIG_DEBUG_KOBJECT_RELEASE.
> > 
> > Keep a reference to the rawmidi device until the sequencer has been
> > destroyed in order to avoid this.
> > 
> > Signed-off-by: John Keeping <john@metanate.com>  
> 
> Thanks for the patch.  I wonder, though, how this could be triggered.
> Is this the case where the connected sequencer device is being used
> while the sound card gets released?  Or is it something else?

I'm not sure if it's possible to trigger via the ALSA API; I haven't
found a route that can trigger it, but that doesn't mean there isn't
one :-)

Mostly this is useful to make CONFIG_DEBUG_KOBJECT_RELEASE cleaner.


Regards,
John

> > ---
> >  sound/core/rawmidi.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/sound/core/rawmidi.c b/sound/core/rawmidi.c
> > index 6f30231bdb88..b015f5f69175 100644
> > --- a/sound/core/rawmidi.c
> > +++ b/sound/core/rawmidi.c
> > @@ -1860,6 +1860,7 @@ static void snd_rawmidi_dev_seq_free(struct snd_seq_device *device)
> >  	struct snd_rawmidi *rmidi = device->private_data;
> >  
> >  	rmidi->seq_dev = NULL;
> > +	put_device(&rmidi->dev);
> >  }
> >  #endif
> >  
> > @@ -1936,6 +1937,9 @@ static int snd_rawmidi_dev_register(struct snd_device *device)
> >  #if IS_ENABLED(CONFIG_SND_SEQUENCER)
> >  	if (!rmidi->ops || !rmidi->ops->dev_register) { /* own registration mechanism */
> >  		if (snd_seq_device_new(rmidi->card, rmidi->device, SNDRV_SEQ_DEV_ID_MIDISYNTH, 0, &rmidi->seq_dev) >= 0) {
> > +			/* Ensure we outlive the sequencer (see snd_rawmidi_dev_seq_free). */
> > +			get_device(&rmidi->dev);
> > +
> >  			rmidi->seq_dev->private_data = rmidi;
> >  			rmidi->seq_dev->private_free = snd_rawmidi_dev_seq_free;
> >  			sprintf(rmidi->seq_dev->name, "MIDI %d-%d", rmidi->card->number, rmidi->device);
> > -- 
> > 2.33.0
> >   

Re: [PATCH] ALSA: rawmidi: Fix potential UAF from sequencer destruction

[Takashi Iwai]

On Wed, 29 Sep 2021 17:17:58 +0200,
John Keeping wrote:
> 
> On Wed, 29 Sep 2021 16:51:47 +0200
> Takashi Iwai <tiwai@suse.de> wrote:
> 
> > On Wed, 29 Sep 2021 13:36:20 +0200,
> > John Keeping wrote:
> > > 
> > > If the sequencer device outlives the rawmidi device, then
> > > snd_rawmidi_dev_seq_free() will run after release_rawmidi_device() has
> > > freed the snd_rawmidi structure.
> > > 
> > > This can easily be reproduced with CONFIG_DEBUG_KOBJECT_RELEASE.
> > > 
> > > Keep a reference to the rawmidi device until the sequencer has been
> > > destroyed in order to avoid this.
> > > 
> > > Signed-off-by: John Keeping <john@metanate.com>  
> > 
> > Thanks for the patch.  I wonder, though, how this could be triggered.
> > Is this the case where the connected sequencer device is being used
> > while the sound card gets released?  Or is it something else?
> 
> I'm not sure if it's possible to trigger via the ALSA API; I haven't
> found a route that can trigger it, but that doesn't mean there isn't
> one :-)
> 
> Mostly this is useful to make CONFIG_DEBUG_KOBJECT_RELEASE cleaner.

Hm, then could you check whether the patch below papers over it
instead?


thanks,

Takashi

--- a/sound/core/seq/seq_ports.c
+++ b/sound/core/seq/seq_ports.c
@@ -415,11 +415,16 @@ static int subscribe_port(struct snd_seq_client *client,
 			grp->count--;
 		}
 	}
-	if (err >= 0 && send_ack && client->type == USER_CLIENT)
+	if (err < 0)
+		return err;
+
+	if (send_ack && client->type == USER_CLIENT)
 		snd_seq_client_notify_subscription(port->addr.client, port->addr.port,
 						   info, SNDRV_SEQ_EVENT_PORT_SUBSCRIBED);
+	else if (client->type == KERNEL_CLIENT)
+		get_device(&client->data.kernel.card->card_dev);
 
-	return err;
+	return 0;
 }
 
 static int unsubscribe_port(struct snd_seq_client *client,
@@ -439,6 +444,8 @@ static int unsubscribe_port(struct snd_seq_client *client,
 		snd_seq_client_notify_subscription(port->addr.client, port->addr.port,
 						   info, SNDRV_SEQ_EVENT_PORT_UNSUBSCRIBED);
 	module_put(port->owner);
+	if (client->type == KERNEL_CLIENT)
+		snd_card_unref(client->data.kernel.card);
 	return err;
 }
 

Re: [PATCH] ALSA: rawmidi: Fix potential UAF from sequencer destruction

[Takashi Iwai]

On Wed, 29 Sep 2021 17:17:58 +0200,
John Keeping wrote:
> 
> On Wed, 29 Sep 2021 16:51:47 +0200
> Takashi Iwai <tiwai@suse.de> wrote:
> 
> > On Wed, 29 Sep 2021 13:36:20 +0200,
> > John Keeping wrote:
> > > 
> > > If the sequencer device outlives the rawmidi device, then
> > > snd_rawmidi_dev_seq_free() will run after release_rawmidi_device() has
> > > freed the snd_rawmidi structure.
> > > 
> > > This can easily be reproduced with CONFIG_DEBUG_KOBJECT_RELEASE.
> > > 
> > > Keep a reference to the rawmidi device until the sequencer has been
> > > destroyed in order to avoid this.
> > > 
> > > Signed-off-by: John Keeping <john@metanate.com>  
> > 
> > Thanks for the patch.  I wonder, though, how this could be triggered.
> > Is this the case where the connected sequencer device is being used
> > while the sound card gets released?  Or is it something else?
> 
> I'm not sure if it's possible to trigger via the ALSA API; I haven't
> found a route that can trigger it, but that doesn't mean there isn't
> one :-)
> 
> Mostly this is useful to make CONFIG_DEBUG_KOBJECT_RELEASE cleaner.

Hm, then could you check whether the patch below papers over it
instead?


thanks,

Takashi

--- a/sound/core/seq/seq_ports.c
+++ b/sound/core/seq/seq_ports.c
@@ -415,11 +415,16 @@ static int subscribe_port(struct snd_seq_client *client,
 			grp->count--;
 		}
 	}
-	if (err >= 0 && send_ack && client->type == USER_CLIENT)
+	if (err < 0)
+		return err;
+
+	if (send_ack && client->type == USER_CLIENT)
 		snd_seq_client_notify_subscription(port->addr.client, port->addr.port,
 						   info, SNDRV_SEQ_EVENT_PORT_SUBSCRIBED);
+	else if (client->type == KERNEL_CLIENT)
+		get_device(&client->data.kernel.card->card_dev);
 
-	return err;
+	return 0;
 }
 
 static int unsubscribe_port(struct snd_seq_client *client,
@@ -439,6 +444,8 @@ static int unsubscribe_port(struct snd_seq_client *client,
 		snd_seq_client_notify_subscription(port->addr.client, port->addr.port,
 						   info, SNDRV_SEQ_EVENT_PORT_UNSUBSCRIBED);
 	module_put(port->owner);
+	if (client->type == KERNEL_CLIENT)
+		snd_card_unref(client->data.kernel.card);
 	return err;
 }
 

Re: [PATCH] ALSA: rawmidi: Fix potential UAF from sequencer destruction

[John Keeping]

On Wed, 29 Sep 2021 17:28:57 +0200
Takashi Iwai <tiwai@suse.de> wrote:

> On Wed, 29 Sep 2021 17:17:58 +0200,
> John Keeping wrote:
> > 
> > On Wed, 29 Sep 2021 16:51:47 +0200
> > Takashi Iwai <tiwai@suse.de> wrote:
> >   
> > > On Wed, 29 Sep 2021 13:36:20 +0200,
> > > John Keeping wrote:  
> > > > 
> > > > If the sequencer device outlives the rawmidi device, then
> > > > snd_rawmidi_dev_seq_free() will run after release_rawmidi_device() has
> > > > freed the snd_rawmidi structure.
> > > > 
> > > > This can easily be reproduced with CONFIG_DEBUG_KOBJECT_RELEASE.
> > > > 
> > > > Keep a reference to the rawmidi device until the sequencer has been
> > > > destroyed in order to avoid this.
> > > > 
> > > > Signed-off-by: John Keeping <john@metanate.com>    
> > > 
> > > Thanks for the patch.  I wonder, though, how this could be triggered.
> > > Is this the case where the connected sequencer device is being used
> > > while the sound card gets released?  Or is it something else?  
> > 
> > I'm not sure if it's possible to trigger via the ALSA API; I haven't
> > found a route that can trigger it, but that doesn't mean there isn't
> > one :-)
> > 
> > Mostly this is useful to make CONFIG_DEBUG_KOBJECT_RELEASE cleaner.  
> 
> Hm, then could you check whether the patch below papers over it
> instead?

No, this patch doesn't solve it.  The issue is that the effect of the
final device_put() is delayed from the time it is called and there is no
way to guarantee the ordering without ensuring the sequencer has been
destroyed before the final reference to the rawmidi device is put.

Both of the functions involved are called from the core
device::release() hook.

I'm using the patch below to easily check that the sequencer has been
freed before the rawmidi data.  This can easily be triggered by
unplugging a USB MIDI device (it's not 100% since the kobject release
delays are random).

-- >8 --
--- a/sound/core/rawmidi.c
+++ b/sound/core/rawmidi.c
@@ -1571,7 +1571,10 @@ static int snd_rawmidi_alloc_substreams(struct snd_rawmidi *rmidi,
 
 static void release_rawmidi_device(struct device *dev)
 {
-       kfree(container_of(dev, struct snd_rawmidi, dev));
+       struct snd_rawmidi *rmidi = container_of(dev, struct snd_rawmidi, dev);
+
+       WARN_ON(rmidi->seq_dev);
+       kfree(rmidi);
 }
 
 /**
-- 8< --

> --- a/sound/core/seq/seq_ports.c
> +++ b/sound/core/seq/seq_ports.c
> @@ -415,11 +415,16 @@ static int subscribe_port(struct snd_seq_client *client,
>  			grp->count--;
>  		}
>  	}
> -	if (err >= 0 && send_ack && client->type == USER_CLIENT)
> +	if (err < 0)
> +		return err;
> +
> +	if (send_ack && client->type == USER_CLIENT)
>  		snd_seq_client_notify_subscription(port->addr.client, port->addr.port,
>  						   info, SNDRV_SEQ_EVENT_PORT_SUBSCRIBED);
> +	else if (client->type == KERNEL_CLIENT)
> +		get_device(&client->data.kernel.card->card_dev);
>  
> -	return err;
> +	return 0;
>  }
>  
>  static int unsubscribe_port(struct snd_seq_client *client,
> @@ -439,6 +444,8 @@ static int unsubscribe_port(struct snd_seq_client *client,
>  		snd_seq_client_notify_subscription(port->addr.client, port->addr.port,
>  						   info, SNDRV_SEQ_EVENT_PORT_UNSUBSCRIBED);
>  	module_put(port->owner);
> +	if (client->type == KERNEL_CLIENT)
> +		snd_card_unref(client->data.kernel.card);
>  	return err;
>  }
>  

Re: [PATCH] ALSA: rawmidi: Fix potential UAF from sequencer destruction

[John Keeping]

On Wed, 29 Sep 2021 17:28:57 +0200
Takashi Iwai <tiwai@suse.de> wrote:

> On Wed, 29 Sep 2021 17:17:58 +0200,
> John Keeping wrote:
> > 
> > On Wed, 29 Sep 2021 16:51:47 +0200
> > Takashi Iwai <tiwai@suse.de> wrote:
> >   
> > > On Wed, 29 Sep 2021 13:36:20 +0200,
> > > John Keeping wrote:  
> > > > 
> > > > If the sequencer device outlives the rawmidi device, then
> > > > snd_rawmidi_dev_seq_free() will run after release_rawmidi_device() has
> > > > freed the snd_rawmidi structure.
> > > > 
> > > > This can easily be reproduced with CONFIG_DEBUG_KOBJECT_RELEASE.
> > > > 
> > > > Keep a reference to the rawmidi device until the sequencer has been
> > > > destroyed in order to avoid this.
> > > > 
> > > > Signed-off-by: John Keeping <john@metanate.com>    
> > > 
> > > Thanks for the patch.  I wonder, though, how this could be triggered.
> > > Is this the case where the connected sequencer device is being used
> > > while the sound card gets released?  Or is it something else?  
> > 
> > I'm not sure if it's possible to trigger via the ALSA API; I haven't
> > found a route that can trigger it, but that doesn't mean there isn't
> > one :-)
> > 
> > Mostly this is useful to make CONFIG_DEBUG_KOBJECT_RELEASE cleaner.  
> 
> Hm, then could you check whether the patch below papers over it
> instead?

No, this patch doesn't solve it.  The issue is that the effect of the
final device_put() is delayed from the time it is called and there is no
way to guarantee the ordering without ensuring the sequencer has been
destroyed before the final reference to the rawmidi device is put.

Both of the functions involved are called from the core
device::release() hook.

I'm using the patch below to easily check that the sequencer has been
freed before the rawmidi data.  This can easily be triggered by
unplugging a USB MIDI device (it's not 100% since the kobject release
delays are random).

-- >8 --
--- a/sound/core/rawmidi.c
+++ b/sound/core/rawmidi.c
@@ -1571,7 +1571,10 @@ static int snd_rawmidi_alloc_substreams(struct snd_rawmidi *rmidi,
 
 static void release_rawmidi_device(struct device *dev)
 {
-       kfree(container_of(dev, struct snd_rawmidi, dev));
+       struct snd_rawmidi *rmidi = container_of(dev, struct snd_rawmidi, dev);
+
+       WARN_ON(rmidi->seq_dev);
+       kfree(rmidi);
 }
 
 /**
-- 8< --

> --- a/sound/core/seq/seq_ports.c
> +++ b/sound/core/seq/seq_ports.c
> @@ -415,11 +415,16 @@ static int subscribe_port(struct snd_seq_client *client,
>  			grp->count--;
>  		}
>  	}
> -	if (err >= 0 && send_ack && client->type == USER_CLIENT)
> +	if (err < 0)
> +		return err;
> +
> +	if (send_ack && client->type == USER_CLIENT)
>  		snd_seq_client_notify_subscription(port->addr.client, port->addr.port,
>  						   info, SNDRV_SEQ_EVENT_PORT_SUBSCRIBED);
> +	else if (client->type == KERNEL_CLIENT)
> +		get_device(&client->data.kernel.card->card_dev);
>  
> -	return err;
> +	return 0;
>  }
>  
>  static int unsubscribe_port(struct snd_seq_client *client,
> @@ -439,6 +444,8 @@ static int unsubscribe_port(struct snd_seq_client *client,
>  		snd_seq_client_notify_subscription(port->addr.client, port->addr.port,
>  						   info, SNDRV_SEQ_EVENT_PORT_UNSUBSCRIBED);
>  	module_put(port->owner);
> +	if (client->type == KERNEL_CLIENT)
> +		snd_card_unref(client->data.kernel.card);
>  	return err;
>  }
>  

Re: [PATCH] ALSA: rawmidi: Fix potential UAF from sequencer destruction

[Takashi Iwai]

On Wed, 29 Sep 2021 18:56:32 +0200,
John Keeping wrote:
> 
> On Wed, 29 Sep 2021 17:28:57 +0200
> Takashi Iwai <tiwai@suse.de> wrote:
> 
> > On Wed, 29 Sep 2021 17:17:58 +0200,
> > John Keeping wrote:
> > > 
> > > On Wed, 29 Sep 2021 16:51:47 +0200
> > > Takashi Iwai <tiwai@suse.de> wrote:
> > >   
> > > > On Wed, 29 Sep 2021 13:36:20 +0200,
> > > > John Keeping wrote:  
> > > > > 
> > > > > If the sequencer device outlives the rawmidi device, then
> > > > > snd_rawmidi_dev_seq_free() will run after release_rawmidi_device() has
> > > > > freed the snd_rawmidi structure.
> > > > > 
> > > > > This can easily be reproduced with CONFIG_DEBUG_KOBJECT_RELEASE.
> > > > > 
> > > > > Keep a reference to the rawmidi device until the sequencer has been
> > > > > destroyed in order to avoid this.
> > > > > 
> > > > > Signed-off-by: John Keeping <john@metanate.com>    
> > > > 
> > > > Thanks for the patch.  I wonder, though, how this could be triggered.
> > > > Is this the case where the connected sequencer device is being used
> > > > while the sound card gets released?  Or is it something else?  
> > > 
> > > I'm not sure if it's possible to trigger via the ALSA API; I haven't
> > > found a route that can trigger it, but that doesn't mean there isn't
> > > one :-)
> > > 
> > > Mostly this is useful to make CONFIG_DEBUG_KOBJECT_RELEASE cleaner.  
> > 
> > Hm, then could you check whether the patch below papers over it
> > instead?
> 
> No, this patch doesn't solve it.  The issue is that the effect of the
> final device_put() is delayed from the time it is called and there is no
> way to guarantee the ordering without ensuring the sequencer has been
> destroyed before the final reference to the rawmidi device is put.
> 
> Both of the functions involved are called from the core
> device::release() hook.
> 
> I'm using the patch below to easily check that the sequencer has been
> freed before the rawmidi data.  This can easily be triggered by
> unplugging a USB MIDI device (it's not 100% since the kobject release
> delays are random).

Hm, it's strange.  I suppose you're *not* using the MIDI device,
right?

The release path for the USB-audio driver is:
  usb_audio_disconnect() ->
    snd_card_free_when_closed() ->
      release_card_device() (via put_device(&card->card_dev)) ->
        snd_card_do_free()

And here in snd_card_do_free(), the snd_device free-callback chains
are called at the beginning (snd_device_free_all()).
As it's executed in a reverse loop, snd_rawmidi_dev_seq_free() shall
be called before snd_rawmidi_dev_free().  Since the final put_device()
for the rawmidi device is called in the latter function, the device
release must not happen before snd_rawmidi_dev_seq_free()...

So I still wonder how the problem could be triggered at all.  Even if
the device object release itself is delayed, it shouldn't matter in
the scenario above (as the snd_device-free-chains are already called
beforehand).


thanks,

Takashi

> 
> -- >8 --
> --- a/sound/core/rawmidi.c
> +++ b/sound/core/rawmidi.c
> @@ -1571,7 +1571,10 @@ static int snd_rawmidi_alloc_substreams(struct snd_rawmidi *rmidi,
>  
>  static void release_rawmidi_device(struct device *dev)
>  {
> -       kfree(container_of(dev, struct snd_rawmidi, dev));
> +       struct snd_rawmidi *rmidi = container_of(dev, struct snd_rawmidi, dev);
> +
> +       WARN_ON(rmidi->seq_dev);
> +       kfree(rmidi);
>  }
>  
>  /**
> -- 8< --
> 
> > --- a/sound/core/seq/seq_ports.c
> > +++ b/sound/core/seq/seq_ports.c
> > @@ -415,11 +415,16 @@ static int subscribe_port(struct snd_seq_client *client,
> >  			grp->count--;
> >  		}
> >  	}
> > -	if (err >= 0 && send_ack && client->type == USER_CLIENT)
> > +	if (err < 0)
> > +		return err;
> > +
> > +	if (send_ack && client->type == USER_CLIENT)
> >  		snd_seq_client_notify_subscription(port->addr.client, port->addr.port,
> >  						   info, SNDRV_SEQ_EVENT_PORT_SUBSCRIBED);
> > +	else if (client->type == KERNEL_CLIENT)
> > +		get_device(&client->data.kernel.card->card_dev);
> >  
> > -	return err;
> > +	return 0;
> >  }
> >  
> >  static int unsubscribe_port(struct snd_seq_client *client,
> > @@ -439,6 +444,8 @@ static int unsubscribe_port(struct snd_seq_client *client,
> >  		snd_seq_client_notify_subscription(port->addr.client, port->addr.port,
> >  						   info, SNDRV_SEQ_EVENT_PORT_UNSUBSCRIBED);
> >  	module_put(port->owner);
> > +	if (client->type == KERNEL_CLIENT)
> > +		snd_card_unref(client->data.kernel.card);
> >  	return err;
> >  }
> >  
> 

Re: [PATCH] ALSA: rawmidi: Fix potential UAF from sequencer destruction

[Takashi Iwai]

On Wed, 29 Sep 2021 18:56:32 +0200,
John Keeping wrote:
> 
> On Wed, 29 Sep 2021 17:28:57 +0200
> Takashi Iwai <tiwai@suse.de> wrote:
> 
> > On Wed, 29 Sep 2021 17:17:58 +0200,
> > John Keeping wrote:
> > > 
> > > On Wed, 29 Sep 2021 16:51:47 +0200
> > > Takashi Iwai <tiwai@suse.de> wrote:
> > >   
> > > > On Wed, 29 Sep 2021 13:36:20 +0200,
> > > > John Keeping wrote:  
> > > > > 
> > > > > If the sequencer device outlives the rawmidi device, then
> > > > > snd_rawmidi_dev_seq_free() will run after release_rawmidi_device() has
> > > > > freed the snd_rawmidi structure.
> > > > > 
> > > > > This can easily be reproduced with CONFIG_DEBUG_KOBJECT_RELEASE.
> > > > > 
> > > > > Keep a reference to the rawmidi device until the sequencer has been
> > > > > destroyed in order to avoid this.
> > > > > 
> > > > > Signed-off-by: John Keeping <john@metanate.com>    
> > > > 
> > > > Thanks for the patch.  I wonder, though, how this could be triggered.
> > > > Is this the case where the connected sequencer device is being used
> > > > while the sound card gets released?  Or is it something else?  
> > > 
> > > I'm not sure if it's possible to trigger via the ALSA API; I haven't
> > > found a route that can trigger it, but that doesn't mean there isn't
> > > one :-)
> > > 
> > > Mostly this is useful to make CONFIG_DEBUG_KOBJECT_RELEASE cleaner.  
> > 
> > Hm, then could you check whether the patch below papers over it
> > instead?
> 
> No, this patch doesn't solve it.  The issue is that the effect of the
> final device_put() is delayed from the time it is called and there is no
> way to guarantee the ordering without ensuring the sequencer has been
> destroyed before the final reference to the rawmidi device is put.
> 
> Both of the functions involved are called from the core
> device::release() hook.
> 
> I'm using the patch below to easily check that the sequencer has been
> freed before the rawmidi data.  This can easily be triggered by
> unplugging a USB MIDI device (it's not 100% since the kobject release
> delays are random).

Hm, it's strange.  I suppose you're *not* using the MIDI device,
right?

The release path for the USB-audio driver is:
  usb_audio_disconnect() ->
    snd_card_free_when_closed() ->
      release_card_device() (via put_device(&card->card_dev)) ->
        snd_card_do_free()

And here in snd_card_do_free(), the snd_device free-callback chains
are called at the beginning (snd_device_free_all()).
As it's executed in a reverse loop, snd_rawmidi_dev_seq_free() shall
be called before snd_rawmidi_dev_free().  Since the final put_device()
for the rawmidi device is called in the latter function, the device
release must not happen before snd_rawmidi_dev_seq_free()...

So I still wonder how the problem could be triggered at all.  Even if
the device object release itself is delayed, it shouldn't matter in
the scenario above (as the snd_device-free-chains are already called
beforehand).


thanks,

Takashi

> 
> -- >8 --
> --- a/sound/core/rawmidi.c
> +++ b/sound/core/rawmidi.c
> @@ -1571,7 +1571,10 @@ static int snd_rawmidi_alloc_substreams(struct snd_rawmidi *rmidi,
>  
>  static void release_rawmidi_device(struct device *dev)
>  {
> -       kfree(container_of(dev, struct snd_rawmidi, dev));
> +       struct snd_rawmidi *rmidi = container_of(dev, struct snd_rawmidi, dev);
> +
> +       WARN_ON(rmidi->seq_dev);
> +       kfree(rmidi);
>  }
>  
>  /**
> -- 8< --
> 
> > --- a/sound/core/seq/seq_ports.c
> > +++ b/sound/core/seq/seq_ports.c
> > @@ -415,11 +415,16 @@ static int subscribe_port(struct snd_seq_client *client,
> >  			grp->count--;
> >  		}
> >  	}
> > -	if (err >= 0 && send_ack && client->type == USER_CLIENT)
> > +	if (err < 0)
> > +		return err;
> > +
> > +	if (send_ack && client->type == USER_CLIENT)
> >  		snd_seq_client_notify_subscription(port->addr.client, port->addr.port,
> >  						   info, SNDRV_SEQ_EVENT_PORT_SUBSCRIBED);
> > +	else if (client->type == KERNEL_CLIENT)
> > +		get_device(&client->data.kernel.card->card_dev);
> >  
> > -	return err;
> > +	return 0;
> >  }
> >  
> >  static int unsubscribe_port(struct snd_seq_client *client,
> > @@ -439,6 +444,8 @@ static int unsubscribe_port(struct snd_seq_client *client,
> >  		snd_seq_client_notify_subscription(port->addr.client, port->addr.port,
> >  						   info, SNDRV_SEQ_EVENT_PORT_UNSUBSCRIBED);
> >  	module_put(port->owner);
> > +	if (client->type == KERNEL_CLIENT)
> > +		snd_card_unref(client->data.kernel.card);
> >  	return err;
> >  }
> >  
> 

Re: [PATCH] ALSA: rawmidi: Fix potential UAF from sequencer destruction

[Takashi Iwai]

On Thu, 30 Sep 2021 08:31:56 +0200,
Takashi Iwai wrote:
> 
> On Wed, 29 Sep 2021 18:56:32 +0200,
> John Keeping wrote:
> > 
> > On Wed, 29 Sep 2021 17:28:57 +0200
> > Takashi Iwai <tiwai@suse.de> wrote:
> > 
> > > On Wed, 29 Sep 2021 17:17:58 +0200,
> > > John Keeping wrote:
> > > > 
> > > > On Wed, 29 Sep 2021 16:51:47 +0200
> > > > Takashi Iwai <tiwai@suse.de> wrote:
> > > >   
> > > > > On Wed, 29 Sep 2021 13:36:20 +0200,
> > > > > John Keeping wrote:  
> > > > > > 
> > > > > > If the sequencer device outlives the rawmidi device, then
> > > > > > snd_rawmidi_dev_seq_free() will run after release_rawmidi_device() has
> > > > > > freed the snd_rawmidi structure.
> > > > > > 
> > > > > > This can easily be reproduced with CONFIG_DEBUG_KOBJECT_RELEASE.
> > > > > > 
> > > > > > Keep a reference to the rawmidi device until the sequencer has been
> > > > > > destroyed in order to avoid this.
> > > > > > 
> > > > > > Signed-off-by: John Keeping <john@metanate.com>    
> > > > > 
> > > > > Thanks for the patch.  I wonder, though, how this could be triggered.
> > > > > Is this the case where the connected sequencer device is being used
> > > > > while the sound card gets released?  Or is it something else?  
> > > > 
> > > > I'm not sure if it's possible to trigger via the ALSA API; I haven't
> > > > found a route that can trigger it, but that doesn't mean there isn't
> > > > one :-)
> > > > 
> > > > Mostly this is useful to make CONFIG_DEBUG_KOBJECT_RELEASE cleaner.  
> > > 
> > > Hm, then could you check whether the patch below papers over it
> > > instead?
> > 
> > No, this patch doesn't solve it.  The issue is that the effect of the
> > final device_put() is delayed from the time it is called and there is no
> > way to guarantee the ordering without ensuring the sequencer has been
> > destroyed before the final reference to the rawmidi device is put.
> > 
> > Both of the functions involved are called from the core
> > device::release() hook.
> > 
> > I'm using the patch below to easily check that the sequencer has been
> > freed before the rawmidi data.  This can easily be triggered by
> > unplugging a USB MIDI device (it's not 100% since the kobject release
> > delays are random).
> 
> Hm, it's strange.  I suppose you're *not* using the MIDI device,
> right?
> 
> The release path for the USB-audio driver is:
>   usb_audio_disconnect() ->
>     snd_card_free_when_closed() ->
>       release_card_device() (via put_device(&card->card_dev)) ->
>         snd_card_do_free()
> 
> And here in snd_card_do_free(), the snd_device free-callback chains
> are called at the beginning (snd_device_free_all()).
> As it's executed in a reverse loop, snd_rawmidi_dev_seq_free() shall
> be called before snd_rawmidi_dev_free().  Since the final put_device()
> for the rawmidi device is called in the latter function, the device
> release must not happen before snd_rawmidi_dev_seq_free()...

Correction: now I finally understood what I misunderstood.
Although the snd_device call chain mentioned above itself is correct,
the snd_rawmidi_dev_seq_free() function isn't called directly from the
snd_device chain, but it's rater the own private_free of
snd_seq_device object.  That is, the call of snd_seq_device
private_free is done in a wrong place; it should be called in the
snd_device call chain instead of the device release.

A fix patch is something like below.  Could you check whether this
fixes the problem?


thanks,

Takashi

--- a/sound/core/seq_device.c
+++ b/sound/core/seq_device.c
@@ -156,6 +156,8 @@ static int snd_seq_device_dev_free(struct snd_device *device)
 	struct snd_seq_device *dev = device->device_data;
 
 	cancel_autoload_drivers();
+	if (dev->private_free)
+		dev->private_free(dev);
 	put_device(&dev->dev);
 	return 0;
 }
@@ -183,11 +185,7 @@ static int snd_seq_device_dev_disconnect(struct snd_device *device)
 
 static void snd_seq_dev_release(struct device *dev)
 {
-	struct snd_seq_device *sdev = to_seq_dev(dev);
-
-	if (sdev->private_free)
-		sdev->private_free(sdev);
-	kfree(sdev);
+	kfree(to_seq_dev(dev));
 }
 
 /*

Re: [PATCH] ALSA: rawmidi: Fix potential UAF from sequencer destruction

[Takashi Iwai]

On Thu, 30 Sep 2021 08:31:56 +0200,
Takashi Iwai wrote:
> 
> On Wed, 29 Sep 2021 18:56:32 +0200,
> John Keeping wrote:
> > 
> > On Wed, 29 Sep 2021 17:28:57 +0200
> > Takashi Iwai <tiwai@suse.de> wrote:
> > 
> > > On Wed, 29 Sep 2021 17:17:58 +0200,
> > > John Keeping wrote:
> > > > 
> > > > On Wed, 29 Sep 2021 16:51:47 +0200
> > > > Takashi Iwai <tiwai@suse.de> wrote:
> > > >   
> > > > > On Wed, 29 Sep 2021 13:36:20 +0200,
> > > > > John Keeping wrote:  
> > > > > > 
> > > > > > If the sequencer device outlives the rawmidi device, then
> > > > > > snd_rawmidi_dev_seq_free() will run after release_rawmidi_device() has
> > > > > > freed the snd_rawmidi structure.
> > > > > > 
> > > > > > This can easily be reproduced with CONFIG_DEBUG_KOBJECT_RELEASE.
> > > > > > 
> > > > > > Keep a reference to the rawmidi device until the sequencer has been
> > > > > > destroyed in order to avoid this.
> > > > > > 
> > > > > > Signed-off-by: John Keeping <john@metanate.com>    
> > > > > 
> > > > > Thanks for the patch.  I wonder, though, how this could be triggered.
> > > > > Is this the case where the connected sequencer device is being used
> > > > > while the sound card gets released?  Or is it something else?  
> > > > 
> > > > I'm not sure if it's possible to trigger via the ALSA API; I haven't
> > > > found a route that can trigger it, but that doesn't mean there isn't
> > > > one :-)
> > > > 
> > > > Mostly this is useful to make CONFIG_DEBUG_KOBJECT_RELEASE cleaner.  
> > > 
> > > Hm, then could you check whether the patch below papers over it
> > > instead?
> > 
> > No, this patch doesn't solve it.  The issue is that the effect of the
> > final device_put() is delayed from the time it is called and there is no
> > way to guarantee the ordering without ensuring the sequencer has been
> > destroyed before the final reference to the rawmidi device is put.
> > 
> > Both of the functions involved are called from the core
> > device::release() hook.
> > 
> > I'm using the patch below to easily check that the sequencer has been
> > freed before the rawmidi data.  This can easily be triggered by
> > unplugging a USB MIDI device (it's not 100% since the kobject release
> > delays are random).
> 
> Hm, it's strange.  I suppose you're *not* using the MIDI device,
> right?
> 
> The release path for the USB-audio driver is:
>   usb_audio_disconnect() ->
>     snd_card_free_when_closed() ->
>       release_card_device() (via put_device(&card->card_dev)) ->
>         snd_card_do_free()
> 
> And here in snd_card_do_free(), the snd_device free-callback chains
> are called at the beginning (snd_device_free_all()).
> As it's executed in a reverse loop, snd_rawmidi_dev_seq_free() shall
> be called before snd_rawmidi_dev_free().  Since the final put_device()
> for the rawmidi device is called in the latter function, the device
> release must not happen before snd_rawmidi_dev_seq_free()...

Correction: now I finally understood what I misunderstood.
Although the snd_device call chain mentioned above itself is correct,
the snd_rawmidi_dev_seq_free() function isn't called directly from the
snd_device chain, but it's rater the own private_free of
snd_seq_device object.  That is, the call of snd_seq_device
private_free is done in a wrong place; it should be called in the
snd_device call chain instead of the device release.

A fix patch is something like below.  Could you check whether this
fixes the problem?


thanks,

Takashi

--- a/sound/core/seq_device.c
+++ b/sound/core/seq_device.c
@@ -156,6 +156,8 @@ static int snd_seq_device_dev_free(struct snd_device *device)
 	struct snd_seq_device *dev = device->device_data;
 
 	cancel_autoload_drivers();
+	if (dev->private_free)
+		dev->private_free(dev);
 	put_device(&dev->dev);
 	return 0;
 }
@@ -183,11 +185,7 @@ static int snd_seq_device_dev_disconnect(struct snd_device *device)
 
 static void snd_seq_dev_release(struct device *dev)
 {
-	struct snd_seq_device *sdev = to_seq_dev(dev);
-
-	if (sdev->private_free)
-		sdev->private_free(sdev);
-	kfree(sdev);
+	kfree(to_seq_dev(dev));
 }
 
 /*

Re: [PATCH] ALSA: rawmidi: Fix potential UAF from sequencer destruction

[John Keeping]

On Thu, 30 Sep 2021 08:55:52 +0200
Takashi Iwai <tiwai@suse.de> wrote:

> On Thu, 30 Sep 2021 08:31:56 +0200,
> Takashi Iwai wrote:
> > 
> > On Wed, 29 Sep 2021 18:56:32 +0200,
> > John Keeping wrote:
> > > 
> > > On Wed, 29 Sep 2021 17:28:57 +0200
> > > Takashi Iwai <tiwai@suse.de> wrote:
> > > 
> > > > On Wed, 29 Sep 2021 17:17:58 +0200,
> > > > John Keeping wrote:
> > > > > 
> > > > > On Wed, 29 Sep 2021 16:51:47 +0200
> > > > > Takashi Iwai <tiwai@suse.de> wrote:
> > > > >   
> > > > > > On Wed, 29 Sep 2021 13:36:20 +0200,
> > > > > > John Keeping wrote:  
> > > > > > > 
> > > > > > > If the sequencer device outlives the rawmidi device, then
> > > > > > > snd_rawmidi_dev_seq_free() will run after release_rawmidi_device() has
> > > > > > > freed the snd_rawmidi structure.
> > > > > > > 
> > > > > > > This can easily be reproduced with CONFIG_DEBUG_KOBJECT_RELEASE.
> > > > > > > 
> > > > > > > Keep a reference to the rawmidi device until the sequencer has been
> > > > > > > destroyed in order to avoid this.
> > > > > > > 
> > > > > > > Signed-off-by: John Keeping <john@metanate.com>    
> > > > > > 
> > > > > > Thanks for the patch.  I wonder, though, how this could be triggered.
> > > > > > Is this the case where the connected sequencer device is being used
> > > > > > while the sound card gets released?  Or is it something else?  
> > > > > 
> > > > > I'm not sure if it's possible to trigger via the ALSA API; I haven't
> > > > > found a route that can trigger it, but that doesn't mean there isn't
> > > > > one :-)
> > > > > 
> > > > > Mostly this is useful to make CONFIG_DEBUG_KOBJECT_RELEASE cleaner.  
> > > > 
> > > > Hm, then could you check whether the patch below papers over it
> > > > instead?
> > > 
> > > No, this patch doesn't solve it.  The issue is that the effect of the
> > > final device_put() is delayed from the time it is called and there is no
> > > way to guarantee the ordering without ensuring the sequencer has been
> > > destroyed before the final reference to the rawmidi device is put.
> > > 
> > > Both of the functions involved are called from the core
> > > device::release() hook.
> > > 
> > > I'm using the patch below to easily check that the sequencer has been
> > > freed before the rawmidi data.  This can easily be triggered by
> > > unplugging a USB MIDI device (it's not 100% since the kobject release
> > > delays are random).
> > 
> > Hm, it's strange.  I suppose you're *not* using the MIDI device,
> > right?
> > 
> > The release path for the USB-audio driver is:
> >   usb_audio_disconnect() ->
> >     snd_card_free_when_closed() ->
> >       release_card_device() (via put_device(&card->card_dev)) ->
> >         snd_card_do_free()
> > 
> > And here in snd_card_do_free(), the snd_device free-callback chains
> > are called at the beginning (snd_device_free_all()).
> > As it's executed in a reverse loop, snd_rawmidi_dev_seq_free() shall
> > be called before snd_rawmidi_dev_free().  Since the final put_device()
> > for the rawmidi device is called in the latter function, the device
> > release must not happen before snd_rawmidi_dev_seq_free()...
> 
> Correction: now I finally understood what I misunderstood.
> Although the snd_device call chain mentioned above itself is correct,
> the snd_rawmidi_dev_seq_free() function isn't called directly from the
> snd_device chain, but it's rater the own private_free of
> snd_seq_device object.  That is, the call of snd_seq_device
> private_free is done in a wrong place; it should be called in the
> snd_device call chain instead of the device release.
> 
> A fix patch is something like below.  Could you check whether this
> fixes the problem?

Yes, this fixes it!


Thanks,
John

> --- a/sound/core/seq_device.c
> +++ b/sound/core/seq_device.c
> @@ -156,6 +156,8 @@ static int snd_seq_device_dev_free(struct snd_device *device)
>  	struct snd_seq_device *dev = device->device_data;
>  
>  	cancel_autoload_drivers();
> +	if (dev->private_free)
> +		dev->private_free(dev);
>  	put_device(&dev->dev);
>  	return 0;
>  }
> @@ -183,11 +185,7 @@ static int snd_seq_device_dev_disconnect(struct snd_device *device)
>  
>  static void snd_seq_dev_release(struct device *dev)
>  {
> -	struct snd_seq_device *sdev = to_seq_dev(dev);
> -
> -	if (sdev->private_free)
> -		sdev->private_free(sdev);
> -	kfree(sdev);
> +	kfree(to_seq_dev(dev));
>  }
>  
>  /*
> 
> 
> 

Re: [PATCH] ALSA: rawmidi: Fix potential UAF from sequencer destruction

[John Keeping]

On Thu, 30 Sep 2021 08:55:52 +0200
Takashi Iwai <tiwai@suse.de> wrote:

> On Thu, 30 Sep 2021 08:31:56 +0200,
> Takashi Iwai wrote:
> > 
> > On Wed, 29 Sep 2021 18:56:32 +0200,
> > John Keeping wrote:
> > > 
> > > On Wed, 29 Sep 2021 17:28:57 +0200
> > > Takashi Iwai <tiwai@suse.de> wrote:
> > > 
> > > > On Wed, 29 Sep 2021 17:17:58 +0200,
> > > > John Keeping wrote:
> > > > > 
> > > > > On Wed, 29 Sep 2021 16:51:47 +0200
> > > > > Takashi Iwai <tiwai@suse.de> wrote:
> > > > >   
> > > > > > On Wed, 29 Sep 2021 13:36:20 +0200,
> > > > > > John Keeping wrote:  
> > > > > > > 
> > > > > > > If the sequencer device outlives the rawmidi device, then
> > > > > > > snd_rawmidi_dev_seq_free() will run after release_rawmidi_device() has
> > > > > > > freed the snd_rawmidi structure.
> > > > > > > 
> > > > > > > This can easily be reproduced with CONFIG_DEBUG_KOBJECT_RELEASE.
> > > > > > > 
> > > > > > > Keep a reference to the rawmidi device until the sequencer has been
> > > > > > > destroyed in order to avoid this.
> > > > > > > 
> > > > > > > Signed-off-by: John Keeping <john@metanate.com>    
> > > > > > 
> > > > > > Thanks for the patch.  I wonder, though, how this could be triggered.
> > > > > > Is this the case where the connected sequencer device is being used
> > > > > > while the sound card gets released?  Or is it something else?  
> > > > > 
> > > > > I'm not sure if it's possible to trigger via the ALSA API; I haven't
> > > > > found a route that can trigger it, but that doesn't mean there isn't
> > > > > one :-)
> > > > > 
> > > > > Mostly this is useful to make CONFIG_DEBUG_KOBJECT_RELEASE cleaner.  
> > > > 
> > > > Hm, then could you check whether the patch below papers over it
> > > > instead?
> > > 
> > > No, this patch doesn't solve it.  The issue is that the effect of the
> > > final device_put() is delayed from the time it is called and there is no
> > > way to guarantee the ordering without ensuring the sequencer has been
> > > destroyed before the final reference to the rawmidi device is put.
> > > 
> > > Both of the functions involved are called from the core
> > > device::release() hook.
> > > 
> > > I'm using the patch below to easily check that the sequencer has been
> > > freed before the rawmidi data.  This can easily be triggered by
> > > unplugging a USB MIDI device (it's not 100% since the kobject release
> > > delays are random).
> > 
> > Hm, it's strange.  I suppose you're *not* using the MIDI device,
> > right?
> > 
> > The release path for the USB-audio driver is:
> >   usb_audio_disconnect() ->
> >     snd_card_free_when_closed() ->
> >       release_card_device() (via put_device(&card->card_dev)) ->
> >         snd_card_do_free()
> > 
> > And here in snd_card_do_free(), the snd_device free-callback chains
> > are called at the beginning (snd_device_free_all()).
> > As it's executed in a reverse loop, snd_rawmidi_dev_seq_free() shall
> > be called before snd_rawmidi_dev_free().  Since the final put_device()
> > for the rawmidi device is called in the latter function, the device
> > release must not happen before snd_rawmidi_dev_seq_free()...
> 
> Correction: now I finally understood what I misunderstood.
> Although the snd_device call chain mentioned above itself is correct,
> the snd_rawmidi_dev_seq_free() function isn't called directly from the
> snd_device chain, but it's rater the own private_free of
> snd_seq_device object.  That is, the call of snd_seq_device
> private_free is done in a wrong place; it should be called in the
> snd_device call chain instead of the device release.
> 
> A fix patch is something like below.  Could you check whether this
> fixes the problem?

Yes, this fixes it!


Thanks,
John

> --- a/sound/core/seq_device.c
> +++ b/sound/core/seq_device.c
> @@ -156,6 +156,8 @@ static int snd_seq_device_dev_free(struct snd_device *device)
>  	struct snd_seq_device *dev = device->device_data;
>  
>  	cancel_autoload_drivers();
> +	if (dev->private_free)
> +		dev->private_free(dev);
>  	put_device(&dev->dev);
>  	return 0;
>  }
> @@ -183,11 +185,7 @@ static int snd_seq_device_dev_disconnect(struct snd_device *device)
>  
>  static void snd_seq_dev_release(struct device *dev)
>  {
> -	struct snd_seq_device *sdev = to_seq_dev(dev);
> -
> -	if (sdev->private_free)
> -		sdev->private_free(sdev);
> -	kfree(sdev);
> +	kfree(to_seq_dev(dev));
>  }
>  
>  /*
> 
> 
> 

Re: [PATCH] ALSA: rawmidi: Fix potential UAF from sequencer destruction

[Takashi Iwai]

On Thu, 30 Sep 2021 12:27:53 +0200,
John Keeping wrote:
> 
> On Thu, 30 Sep 2021 08:55:52 +0200
> Takashi Iwai <tiwai@suse.de> wrote:
> 
> > On Thu, 30 Sep 2021 08:31:56 +0200,
> > Takashi Iwai wrote:
> > > 
> > > On Wed, 29 Sep 2021 18:56:32 +0200,
> > > John Keeping wrote:
> > > > 
> > > > On Wed, 29 Sep 2021 17:28:57 +0200
> > > > Takashi Iwai <tiwai@suse.de> wrote:
> > > > 
> > > > > On Wed, 29 Sep 2021 17:17:58 +0200,
> > > > > John Keeping wrote:
> > > > > > 
> > > > > > On Wed, 29 Sep 2021 16:51:47 +0200
> > > > > > Takashi Iwai <tiwai@suse.de> wrote:
> > > > > >   
> > > > > > > On Wed, 29 Sep 2021 13:36:20 +0200,
> > > > > > > John Keeping wrote:  
> > > > > > > > 
> > > > > > > > If the sequencer device outlives the rawmidi device, then
> > > > > > > > snd_rawmidi_dev_seq_free() will run after release_rawmidi_device() has
> > > > > > > > freed the snd_rawmidi structure.
> > > > > > > > 
> > > > > > > > This can easily be reproduced with CONFIG_DEBUG_KOBJECT_RELEASE.
> > > > > > > > 
> > > > > > > > Keep a reference to the rawmidi device until the sequencer has been
> > > > > > > > destroyed in order to avoid this.
> > > > > > > > 
> > > > > > > > Signed-off-by: John Keeping <john@metanate.com>    
> > > > > > > 
> > > > > > > Thanks for the patch.  I wonder, though, how this could be triggered.
> > > > > > > Is this the case where the connected sequencer device is being used
> > > > > > > while the sound card gets released?  Or is it something else?  
> > > > > > 
> > > > > > I'm not sure if it's possible to trigger via the ALSA API; I haven't
> > > > > > found a route that can trigger it, but that doesn't mean there isn't
> > > > > > one :-)
> > > > > > 
> > > > > > Mostly this is useful to make CONFIG_DEBUG_KOBJECT_RELEASE cleaner.  
> > > > > 
> > > > > Hm, then could you check whether the patch below papers over it
> > > > > instead?
> > > > 
> > > > No, this patch doesn't solve it.  The issue is that the effect of the
> > > > final device_put() is delayed from the time it is called and there is no
> > > > way to guarantee the ordering without ensuring the sequencer has been
> > > > destroyed before the final reference to the rawmidi device is put.
> > > > 
> > > > Both of the functions involved are called from the core
> > > > device::release() hook.
> > > > 
> > > > I'm using the patch below to easily check that the sequencer has been
> > > > freed before the rawmidi data.  This can easily be triggered by
> > > > unplugging a USB MIDI device (it's not 100% since the kobject release
> > > > delays are random).
> > > 
> > > Hm, it's strange.  I suppose you're *not* using the MIDI device,
> > > right?
> > > 
> > > The release path for the USB-audio driver is:
> > >   usb_audio_disconnect() ->
> > >     snd_card_free_when_closed() ->
> > >       release_card_device() (via put_device(&card->card_dev)) ->
> > >         snd_card_do_free()
> > > 
> > > And here in snd_card_do_free(), the snd_device free-callback chains
> > > are called at the beginning (snd_device_free_all()).
> > > As it's executed in a reverse loop, snd_rawmidi_dev_seq_free() shall
> > > be called before snd_rawmidi_dev_free().  Since the final put_device()
> > > for the rawmidi device is called in the latter function, the device
> > > release must not happen before snd_rawmidi_dev_seq_free()...
> > 
> > Correction: now I finally understood what I misunderstood.
> > Although the snd_device call chain mentioned above itself is correct,
> > the snd_rawmidi_dev_seq_free() function isn't called directly from the
> > snd_device chain, but it's rater the own private_free of
> > snd_seq_device object.  That is, the call of snd_seq_device
> > private_free is done in a wrong place; it should be called in the
> > snd_device call chain instead of the device release.
> > 
> > A fix patch is something like below.  Could you check whether this
> > fixes the problem?
> 
> Yes, this fixes it!

Great, I'll submit a proper patch.

Thanks!


Takashi

Re: [PATCH] ALSA: rawmidi: Fix potential UAF from sequencer destruction

[Takashi Iwai]

On Thu, 30 Sep 2021 12:27:53 +0200,
John Keeping wrote:
> 
> On Thu, 30 Sep 2021 08:55:52 +0200
> Takashi Iwai <tiwai@suse.de> wrote:
> 
> > On Thu, 30 Sep 2021 08:31:56 +0200,
> > Takashi Iwai wrote:
> > > 
> > > On Wed, 29 Sep 2021 18:56:32 +0200,
> > > John Keeping wrote:
> > > > 
> > > > On Wed, 29 Sep 2021 17:28:57 +0200
> > > > Takashi Iwai <tiwai@suse.de> wrote:
> > > > 
> > > > > On Wed, 29 Sep 2021 17:17:58 +0200,
> > > > > John Keeping wrote:
> > > > > > 
> > > > > > On Wed, 29 Sep 2021 16:51:47 +0200
> > > > > > Takashi Iwai <tiwai@suse.de> wrote:
> > > > > >   
> > > > > > > On Wed, 29 Sep 2021 13:36:20 +0200,
> > > > > > > John Keeping wrote:  
> > > > > > > > 
> > > > > > > > If the sequencer device outlives the rawmidi device, then
> > > > > > > > snd_rawmidi_dev_seq_free() will run after release_rawmidi_device() has
> > > > > > > > freed the snd_rawmidi structure.
> > > > > > > > 
> > > > > > > > This can easily be reproduced with CONFIG_DEBUG_KOBJECT_RELEASE.
> > > > > > > > 
> > > > > > > > Keep a reference to the rawmidi device until the sequencer has been
> > > > > > > > destroyed in order to avoid this.
> > > > > > > > 
> > > > > > > > Signed-off-by: John Keeping <john@metanate.com>    
> > > > > > > 
> > > > > > > Thanks for the patch.  I wonder, though, how this could be triggered.
> > > > > > > Is this the case where the connected sequencer device is being used
> > > > > > > while the sound card gets released?  Or is it something else?  
> > > > > > 
> > > > > > I'm not sure if it's possible to trigger via the ALSA API; I haven't
> > > > > > found a route that can trigger it, but that doesn't mean there isn't
> > > > > > one :-)
> > > > > > 
> > > > > > Mostly this is useful to make CONFIG_DEBUG_KOBJECT_RELEASE cleaner.  
> > > > > 
> > > > > Hm, then could you check whether the patch below papers over it
> > > > > instead?
> > > > 
> > > > No, this patch doesn't solve it.  The issue is that the effect of the
> > > > final device_put() is delayed from the time it is called and there is no
> > > > way to guarantee the ordering without ensuring the sequencer has been
> > > > destroyed before the final reference to the rawmidi device is put.
> > > > 
> > > > Both of the functions involved are called from the core
> > > > device::release() hook.
> > > > 
> > > > I'm using the patch below to easily check that the sequencer has been
> > > > freed before the rawmidi data.  This can easily be triggered by
> > > > unplugging a USB MIDI device (it's not 100% since the kobject release
> > > > delays are random).
> > > 
> > > Hm, it's strange.  I suppose you're *not* using the MIDI device,
> > > right?
> > > 
> > > The release path for the USB-audio driver is:
> > >   usb_audio_disconnect() ->
> > >     snd_card_free_when_closed() ->
> > >       release_card_device() (via put_device(&card->card_dev)) ->
> > >         snd_card_do_free()
> > > 
> > > And here in snd_card_do_free(), the snd_device free-callback chains
> > > are called at the beginning (snd_device_free_all()).
> > > As it's executed in a reverse loop, snd_rawmidi_dev_seq_free() shall
> > > be called before snd_rawmidi_dev_free().  Since the final put_device()
> > > for the rawmidi device is called in the latter function, the device
> > > release must not happen before snd_rawmidi_dev_seq_free()...
> > 
> > Correction: now I finally understood what I misunderstood.
> > Although the snd_device call chain mentioned above itself is correct,
> > the snd_rawmidi_dev_seq_free() function isn't called directly from the
> > snd_device chain, but it's rater the own private_free of
> > snd_seq_device object.  That is, the call of snd_seq_device
> > private_free is done in a wrong place; it should be called in the
> > snd_device call chain instead of the device release.
> > 
> > A fix patch is something like below.  Could you check whether this
> > fixes the problem?
> 
> Yes, this fixes it!

Great, I'll submit a proper patch.

Thanks!


Takashi