Re: [PATCH] ALSA: hda: Do not unset preset when cleaning up codec

[Takashi Iwai <tiwai@suse.de>]

On Tue, 17 Jan 2023 16:47:34 +0100,
Cezary Rojewski wrote:
> 
> Several functions that take part in codec's initialization and removal
> are re-used by ASoC codec drivers implementations. Drivers mimic the
> behavior of hda_codec_driver_probe/remove() found in
> sound/pci/hda/hda_bind.c with their component->probe/remove() instead.
> 
> One of the reasons for that is the expectation of
> snd_hda_codec_device_new() to receive a valid struct snd_card pointer
> what cannot be fulfilled on ASoC side until a card is attempted to be
> bound and its component probing is triggered.
> 
> As ASoC sound card may be unbound without codec device being actually
> removed from the system, unsetting ->preset in 
> snd_hda_codec_cleanup_for_unbind() interferes with module unload -> load
> scenario causing null-ptr-deref. Preset is assigned only once, during
> device/driver matching whereas ASoC codec driver's module reloading may
> occur several times throughout the lifetime of an audio stack.
> 
> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
> ---
> 
> This is a continuation of a discussion that begun in the middle of 2022
> [1] and was part of a larger series addressing several HDAudio topics.
> 
> Single rmmod on ASoC's codec driver module is enough to cause a panic.
> Given our results, no regression shows up with modprobe/rmmod on
> snd_hda_intel side with this patch applied.

I think one possible regression by this change would be the case you
reload another codec driver.  With keeping codec->preset, it's still
thought as if already matched, and a wrong one could be used.

And, this would be nothing but a leak of the possibly freed address.
After hda_codec_driver_remove(), card->preset may point to an already
freed address.

So, just removing isn't right.  It has to be cleared somewhere
instead, e.g. in hda_bind.c.

But, one thing I'm still concerned is that your comment about the call
without the card binding.  Do you mean that the
snd_hda_codec_cleanup_for_unbind() may be called even if codec->card
isn't set?


thanks,

Takashi