sound: core: fix device ownership model in card and pcm

Message ID	20230801171928.1460120-1-cujomalainey@chromium.org (mailing list archive)
State	Superseded
Headers	show Return-Path: <alsa-devel-bounces@alsa-project.org> From: cujomalainey@chromium.org To: alsa-devel@alsa-project.org Cc: Curtis Malainey <cujomalainey@chromium.org>, Doug Anderson <dianders@chromium.org>, Jaroslav Kysela <perex@perex.cz>, Takashi Iwai <tiwai@suse.com>, Zheyu Ma <zheyuma97@gmail.com>, Dan Carpenter <error27@gmail.com>, "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>, Clement Lecigne <clecigne@google.com>, Ivan Orlov <ivan.orlov0322@gmail.com>, Greg Kroah-Hartman <gregkh@linuxfoundation.org> Subject: [PATCH] sound: core: fix device ownership model in card and pcm Date: Tue, 1 Aug 2023 10:18:41 -0700 Message-ID: <20230801171928.1460120-1-cujomalainey@chromium.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Message-ID-Hash: O7J2RRDFR2YS4BJKOFRUBMA3S4KD4WV2 Precedence: list Archived-At: <https://mailman.alsa-project.org/hyperkitty/list/alsa-devel@alsa-project.org/message/O7J2RRDFR2YS4BJKOFRUBMA3S4KD4WV2/>
Series	sound: core: fix device ownership model in card and pcm \| expand sound: core: fix device ownership model in card and pcm

Curtis Malainey Aug. 1, 2023, 5:18 p.m. UTC

From: Curtis Malainey <cujomalainey@chromium.org>

The current implementation of how devices are released is valid for
production use cases (root control of memory is handled by card_dev, all
other devices are no-ops).

This model does not work though in a kernel hacking environment where
KASAN and delayed release on kobj is enabled. If the card_dev device is
released before any of the child objects a use-after-free bug is caught
by KASAN as the delayed release still has a reference to the devices
that were freed by the card_dev release. Also both snd_card and snd_pcm
both own two devices internally, so even if they released independently,
the shared struct would result in another use after free.

Solution is to move the child devices into their own memory so they can
be handled independently and released on their own schedule.

Signed-off-by: Curtis Malainey <cujomalainey@chromium.org>
Cc: Doug Anderson <dianders@chromium.org>
---
 include/sound/core.h     |  2 +-
 include/sound/pcm.h      |  2 +-
 sound/core/control.c     | 21 +++++++++++++++------
 sound/core/control_led.c |  4 ++--
 sound/core/pcm.c         | 30 ++++++++++++++++++++----------
 sound/usb/media.c        |  4 ++--
 6 files changed, 41 insertions(+), 22 deletions(-)

kernel test robot Aug. 2, 2023, 3:29 a.m. UTC | #1

Hi,

kernel test robot noticed the following build errors:

[auto build test ERROR on tiwai-sound/for-next]
[also build test ERROR on tiwai-sound/for-linus linus/master v6.5-rc4 next-20230801]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/cujomalainey-chromium-org/sound-core-fix-device-ownership-model-in-card-and-pcm/20230802-012331
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next
patch link:    https://lore.kernel.org/r/20230801171928.1460120-1-cujomalainey%40chromium.org
patch subject: [PATCH] sound: core: fix device ownership model in card and pcm
config: powerpc-randconfig-r014-20230801 (https://download.01.org/0day-ci/archive/20230802/202308021152.c3aRSumS-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
reproduce: (https://download.01.org/0day-ci/archive/20230802/202308021152.c3aRSumS-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202308021152.c3aRSumS-lkp@intel.com/

All errors (new ones prefixed by >>):

>> sound/aoa/soundbus/i2sbus/pcm.c:975:51: error: member reference type 'struct device *' is a pointer; did you mean to use '->'?
     975 |                 dev->pcm->streams[SNDRV_PCM_STREAM_PLAYBACK].dev.parent =
         |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^
         |                                                                 ->
   sound/aoa/soundbus/i2sbus/pcm.c:992:50: error: member reference type 'struct device *' is a pointer; did you mean to use '->'?
     992 |                 dev->pcm->streams[SNDRV_PCM_STREAM_CAPTURE].dev.parent =
         |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^
         |                                                                ->
   2 errors generated.


vim +975 sound/aoa/soundbus/i2sbus/pcm.c

f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   866  
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   867  int
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   868  i2sbus_attach_codec(struct soundbus_dev *dev, struct snd_card *card,
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   869  		    struct codec_info *ci, void *data)
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   870  {
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   871  	int err, in = 0, out = 0;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   872  	struct transfer_info *tmp;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   873  	struct i2sbus_dev *i2sdev = soundbus_dev_to_i2sbus_dev(dev);
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   874  	struct codec_info_item *cii;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   875  
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   876  	if (!dev->pcmname || dev->pcmid == -1) {
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   877  		printk(KERN_ERR "i2sbus: pcm name and id must be set!\n");
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   878  		return -EINVAL;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   879  	}
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   880  
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   881  	list_for_each_entry(cii, &dev->codec_list, list) {
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   882  		if (cii->codec_data == data)
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   883  			return -EALREADY;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   884  	}
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   885  
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   886  	if (!ci->transfers || !ci->transfers->formats
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   887  	    || !ci->transfers->rates || !ci->usable)
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   888  		return -EINVAL;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   889  
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   890  	/* we currently code the i2s transfer on the clock, and support only
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   891  	 * 32 and 64 */
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   892  	if (ci->bus_factor != 32 && ci->bus_factor != 64)
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   893  		return -EINVAL;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   894  
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   895  	/* If you want to fix this, you need to keep track of what transport infos
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   896  	 * are to be used, which codecs they belong to, and then fix all the
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   897  	 * sysclock/busclock stuff above to depend on which is usable */
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   898  	list_for_each_entry(cii, &dev->codec_list, list) {
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   899  		if (cii->codec->sysclock_factor != ci->sysclock_factor) {
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   900  			printk(KERN_DEBUG
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   901  			       "cannot yet handle multiple different sysclocks!\n");
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   902  			return -EINVAL;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   903  		}
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   904  		if (cii->codec->bus_factor != ci->bus_factor) {
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   905  			printk(KERN_DEBUG
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   906  			       "cannot yet handle multiple different bus clocks!\n");
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   907  			return -EINVAL;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   908  		}
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   909  	}
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   910  
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   911  	tmp = ci->transfers;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   912  	while (tmp->formats && tmp->rates) {
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   913  		if (tmp->transfer_in)
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   914  			in = 1;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   915  		else
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   916  			out = 1;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   917  		tmp++;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   918  	}
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   919  
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   920  	cii = kzalloc(sizeof(struct codec_info_item), GFP_KERNEL);
37d122c5768b41 sound/aoa/soundbus/i2sbus/pcm.c        Zhen Lei      2021-06-17   921  	if (!cii)
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   922  		return -ENOMEM;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   923  
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   924  	/* use the private data to point to the codec info */
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   925  	cii->sdev = soundbus_dev_get(dev);
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   926  	cii->codec = ci;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   927  	cii->codec_data = data;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   928  
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   929  	if (!cii->sdev) {
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   930  		printk(KERN_DEBUG
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   931  		       "i2sbus: failed to get soundbus dev reference\n");
d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05   932  		err = -ENODEV;
d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05   933  		goto out_free_cii;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   934  	}
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   935  
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   936  	if (!try_module_get(THIS_MODULE)) {
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   937  		printk(KERN_DEBUG "i2sbus: failed to get module reference!\n");
d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05   938  		err = -EBUSY;
d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05   939  		goto out_put_sdev;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   940  	}
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   941  
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   942  	if (!try_module_get(ci->owner)) {
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   943  		printk(KERN_DEBUG
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   944  		       "i2sbus: failed to get module reference to codec owner!\n");
d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05   945  		err = -EBUSY;
d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05   946  		goto out_put_this_module;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   947  	}
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   948  
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   949  	if (!dev->pcm) {
73e85fe8452b95 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05   950  		err = snd_pcm_new(card, dev->pcmname, dev->pcmid, 0, 0,
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   951  				  &dev->pcm);
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   952  		if (err) {
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   953  			printk(KERN_DEBUG "i2sbus: failed to create pcm\n");
d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05   954  			goto out_put_ci_module;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   955  		}
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   956  	}
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   957  
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   958  	/* ALSA yet again sucks.
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   959  	 * If it is ever fixed, remove this line. See below. */
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   960  	out = in = 1;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   961  
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   962  	if (!i2sdev->out.created && out) {
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   963  		if (dev->pcm->card != card) {
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   964  			/* eh? */
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   965  			printk(KERN_ERR
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   966  			       "Can't attach same bus to different cards!\n");
d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05   967  			err = -EINVAL;
d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05   968  			goto out_put_ci_module;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   969  		}
d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05   970  		err = snd_pcm_new_stream(dev->pcm, SNDRV_PCM_STREAM_PLAYBACK, 1);
d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05   971  		if (err)
d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05   972  			goto out_put_ci_module;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   973  		snd_pcm_set_ops(dev->pcm, SNDRV_PCM_STREAM_PLAYBACK,
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   974  				&i2sbus_playback_ops);
ef46c7af93f98d sound/aoa/soundbus/i2sbus/pcm.c        Takashi Iwai  2015-01-29  @975  		dev->pcm->streams[SNDRV_PCM_STREAM_PLAYBACK].dev.parent =
ef46c7af93f98d sound/aoa/soundbus/i2sbus/pcm.c        Takashi Iwai  2015-01-29   976  			&dev->ofdev.dev;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   977  		i2sdev->out.created = 1;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   978  	}
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   979  
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   980  	if (!i2sdev->in.created && in) {
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   981  		if (dev->pcm->card != card) {
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   982  			printk(KERN_ERR
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   983  			       "Can't attach same bus to different cards!\n");
3d3909ffe57174 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Takashi Iwai  2007-11-23   984  			err = -EINVAL;
d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05   985  			goto out_put_ci_module;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   986  		}
d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05   987  		err = snd_pcm_new_stream(dev->pcm, SNDRV_PCM_STREAM_CAPTURE, 1);
d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05   988  		if (err)
d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05   989  			goto out_put_ci_module;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   990  		snd_pcm_set_ops(dev->pcm, SNDRV_PCM_STREAM_CAPTURE,
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   991  				&i2sbus_record_ops);
ef46c7af93f98d sound/aoa/soundbus/i2sbus/pcm.c        Takashi Iwai  2015-01-29   992  		dev->pcm->streams[SNDRV_PCM_STREAM_CAPTURE].dev.parent =
ef46c7af93f98d sound/aoa/soundbus/i2sbus/pcm.c        Takashi Iwai  2015-01-29   993  			&dev->ofdev.dev;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   994  		i2sdev->in.created = 1;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   995  	}
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   996  
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   997  	/* so we have to register the pcm after adding any substream
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   998  	 * to it because alsa doesn't create the devices for the
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   999  	 * substreams when we add them later.
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21  1000  	 * Therefore, force in and out on both busses (above) and
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21  1001  	 * register the pcm now instead of just after creating it.
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21  1002  	 */
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21  1003  	err = snd_device_register(card, dev->pcm);
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21  1004  	if (err) {
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21  1005  		printk(KERN_ERR "i2sbus: error registering new pcm\n");
d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05  1006  		goto out_put_ci_module;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21  1007  	}
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21  1008  	/* no errors any more, so let's add this to our list */
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21  1009  	list_add(&cii->list, &dev->codec_list);
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21  1010  
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21  1011  	dev->pcm->private_data = i2sdev;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21  1012  	dev->pcm->private_free = i2sbus_private_free;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21  1013  
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21  1014  	/* well, we really should support scatter/gather DMA */
9b2433a9c5b392 sound/aoa/soundbus/i2sbus/pcm.c        Takashi Iwai  2019-12-09  1015  	snd_pcm_set_managed_buffer_all(
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21  1016  		dev->pcm, SNDRV_DMA_TYPE_DEV,
3ca5fc0664ec47 sound/aoa/soundbus/i2sbus/pcm.c        Takashi Iwai  2019-11-05  1017  		&macio_get_pci_dev(i2sdev->macio)->dev,
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21  1018  		64 * 1024, 64 * 1024);
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21  1019  
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21  1020  	return 0;
d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05  1021   out_put_ci_module:
d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05  1022  	module_put(ci->owner);
d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05  1023   out_put_this_module:
d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05  1024  	module_put(THIS_MODULE);
d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05  1025   out_put_sdev:
d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05  1026  	soundbus_dev_put(dev);
d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05  1027   out_free_cii:
d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05  1028  	kfree(cii);
d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05  1029  	return err;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21  1030  }
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21  1031

kernel test robot Aug. 2, 2023, 3:51 a.m. UTC | #2

Hi,

kernel test robot noticed the following build errors:

[auto build test ERROR on tiwai-sound/for-next]
[also build test ERROR on tiwai-sound/for-linus linus/master v6.5-rc4 next-20230801]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/cujomalainey-chromium-org/sound-core-fix-device-ownership-model-in-card-and-pcm/20230802-012331
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next
patch link:    https://lore.kernel.org/r/20230801171928.1460120-1-cujomalainey%40chromium.org
patch subject: [PATCH] sound: core: fix device ownership model in card and pcm
config: powerpc-allmodconfig (https://download.01.org/0day-ci/archive/20230802/202308021146.prrFapWM-lkp@intel.com/config)
compiler: powerpc-linux-gcc (GCC) 12.3.0
reproduce: (https://download.01.org/0day-ci/archive/20230802/202308021146.prrFapWM-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202308021146.prrFapWM-lkp@intel.com/

All errors (new ones prefixed by >>):

   sound/aoa/soundbus/i2sbus/pcm.c: In function 'i2sbus_attach_codec':
>> sound/aoa/soundbus/i2sbus/pcm.c:975:65: error: 'dev->pcm->streams[0].dev' is a pointer; did you mean to use '->'?
     975 |                 dev->pcm->streams[SNDRV_PCM_STREAM_PLAYBACK].dev.parent =
         |                                                                 ^
         |                                                                 ->
   sound/aoa/soundbus/i2sbus/pcm.c:992:64: error: 'dev->pcm->streams[1].dev' is a pointer; did you mean to use '->'?
     992 |                 dev->pcm->streams[SNDRV_PCM_STREAM_CAPTURE].dev.parent =
         |                                                                ^
         |                                                                ->


vim +975 sound/aoa/soundbus/i2sbus/pcm.c

f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   866  
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   867  int
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   868  i2sbus_attach_codec(struct soundbus_dev *dev, struct snd_card *card,
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   869  		    struct codec_info *ci, void *data)
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   870  {
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   871  	int err, in = 0, out = 0;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   872  	struct transfer_info *tmp;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   873  	struct i2sbus_dev *i2sdev = soundbus_dev_to_i2sbus_dev(dev);
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   874  	struct codec_info_item *cii;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   875  
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   876  	if (!dev->pcmname || dev->pcmid == -1) {
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   877  		printk(KERN_ERR "i2sbus: pcm name and id must be set!\n");
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   878  		return -EINVAL;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   879  	}
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   880  
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   881  	list_for_each_entry(cii, &dev->codec_list, list) {
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   882  		if (cii->codec_data == data)
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   883  			return -EALREADY;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   884  	}
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   885  
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   886  	if (!ci->transfers || !ci->transfers->formats
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   887  	    || !ci->transfers->rates || !ci->usable)
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   888  		return -EINVAL;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   889  
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   890  	/* we currently code the i2s transfer on the clock, and support only
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   891  	 * 32 and 64 */
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   892  	if (ci->bus_factor != 32 && ci->bus_factor != 64)
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   893  		return -EINVAL;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   894  
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   895  	/* If you want to fix this, you need to keep track of what transport infos
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   896  	 * are to be used, which codecs they belong to, and then fix all the
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   897  	 * sysclock/busclock stuff above to depend on which is usable */
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   898  	list_for_each_entry(cii, &dev->codec_list, list) {
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   899  		if (cii->codec->sysclock_factor != ci->sysclock_factor) {
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   900  			printk(KERN_DEBUG
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   901  			       "cannot yet handle multiple different sysclocks!\n");
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   902  			return -EINVAL;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   903  		}
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   904  		if (cii->codec->bus_factor != ci->bus_factor) {
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   905  			printk(KERN_DEBUG
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   906  			       "cannot yet handle multiple different bus clocks!\n");
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   907  			return -EINVAL;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   908  		}
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   909  	}
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   910  
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   911  	tmp = ci->transfers;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   912  	while (tmp->formats && tmp->rates) {
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   913  		if (tmp->transfer_in)
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   914  			in = 1;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   915  		else
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   916  			out = 1;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   917  		tmp++;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   918  	}
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   919  
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   920  	cii = kzalloc(sizeof(struct codec_info_item), GFP_KERNEL);
37d122c5768b41 sound/aoa/soundbus/i2sbus/pcm.c        Zhen Lei      2021-06-17   921  	if (!cii)
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   922  		return -ENOMEM;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   923  
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   924  	/* use the private data to point to the codec info */
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   925  	cii->sdev = soundbus_dev_get(dev);
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   926  	cii->codec = ci;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   927  	cii->codec_data = data;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   928  
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   929  	if (!cii->sdev) {
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   930  		printk(KERN_DEBUG
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   931  		       "i2sbus: failed to get soundbus dev reference\n");
d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05   932  		err = -ENODEV;
d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05   933  		goto out_free_cii;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   934  	}
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   935  
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   936  	if (!try_module_get(THIS_MODULE)) {
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   937  		printk(KERN_DEBUG "i2sbus: failed to get module reference!\n");
d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05   938  		err = -EBUSY;
d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05   939  		goto out_put_sdev;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   940  	}
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   941  
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   942  	if (!try_module_get(ci->owner)) {
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   943  		printk(KERN_DEBUG
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   944  		       "i2sbus: failed to get module reference to codec owner!\n");
d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05   945  		err = -EBUSY;
d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05   946  		goto out_put_this_module;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   947  	}
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   948  
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   949  	if (!dev->pcm) {
73e85fe8452b95 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05   950  		err = snd_pcm_new(card, dev->pcmname, dev->pcmid, 0, 0,
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   951  				  &dev->pcm);
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   952  		if (err) {
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   953  			printk(KERN_DEBUG "i2sbus: failed to create pcm\n");
d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05   954  			goto out_put_ci_module;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   955  		}
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   956  	}
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   957  
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   958  	/* ALSA yet again sucks.
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   959  	 * If it is ever fixed, remove this line. See below. */
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   960  	out = in = 1;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   961  
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   962  	if (!i2sdev->out.created && out) {
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   963  		if (dev->pcm->card != card) {
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   964  			/* eh? */
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   965  			printk(KERN_ERR
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   966  			       "Can't attach same bus to different cards!\n");
d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05   967  			err = -EINVAL;
d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05   968  			goto out_put_ci_module;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   969  		}
d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05   970  		err = snd_pcm_new_stream(dev->pcm, SNDRV_PCM_STREAM_PLAYBACK, 1);
d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05   971  		if (err)
d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05   972  			goto out_put_ci_module;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   973  		snd_pcm_set_ops(dev->pcm, SNDRV_PCM_STREAM_PLAYBACK,
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   974  				&i2sbus_playback_ops);
ef46c7af93f98d sound/aoa/soundbus/i2sbus/pcm.c        Takashi Iwai  2015-01-29  @975  		dev->pcm->streams[SNDRV_PCM_STREAM_PLAYBACK].dev.parent =
ef46c7af93f98d sound/aoa/soundbus/i2sbus/pcm.c        Takashi Iwai  2015-01-29   976  			&dev->ofdev.dev;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   977  		i2sdev->out.created = 1;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   978  	}
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   979  
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   980  	if (!i2sdev->in.created && in) {
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   981  		if (dev->pcm->card != card) {
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   982  			printk(KERN_ERR
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   983  			       "Can't attach same bus to different cards!\n");
3d3909ffe57174 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Takashi Iwai  2007-11-23   984  			err = -EINVAL;
d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05   985  			goto out_put_ci_module;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   986  		}
d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05   987  		err = snd_pcm_new_stream(dev->pcm, SNDRV_PCM_STREAM_CAPTURE, 1);
d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05   988  		if (err)
d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05   989  			goto out_put_ci_module;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   990  		snd_pcm_set_ops(dev->pcm, SNDRV_PCM_STREAM_CAPTURE,
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   991  				&i2sbus_record_ops);
ef46c7af93f98d sound/aoa/soundbus/i2sbus/pcm.c        Takashi Iwai  2015-01-29   992  		dev->pcm->streams[SNDRV_PCM_STREAM_CAPTURE].dev.parent =
ef46c7af93f98d sound/aoa/soundbus/i2sbus/pcm.c        Takashi Iwai  2015-01-29   993  			&dev->ofdev.dev;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   994  		i2sdev->in.created = 1;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   995  	}
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   996  
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   997  	/* so we have to register the pcm after adding any substream
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   998  	 * to it because alsa doesn't create the devices for the
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21   999  	 * substreams when we add them later.
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21  1000  	 * Therefore, force in and out on both busses (above) and
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21  1001  	 * register the pcm now instead of just after creating it.
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21  1002  	 */
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21  1003  	err = snd_device_register(card, dev->pcm);
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21  1004  	if (err) {
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21  1005  		printk(KERN_ERR "i2sbus: error registering new pcm\n");
d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05  1006  		goto out_put_ci_module;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21  1007  	}
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21  1008  	/* no errors any more, so let's add this to our list */
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21  1009  	list_add(&cii->list, &dev->codec_list);
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21  1010  
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21  1011  	dev->pcm->private_data = i2sdev;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21  1012  	dev->pcm->private_free = i2sbus_private_free;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21  1013  
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21  1014  	/* well, we really should support scatter/gather DMA */
9b2433a9c5b392 sound/aoa/soundbus/i2sbus/pcm.c        Takashi Iwai  2019-12-09  1015  	snd_pcm_set_managed_buffer_all(
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21  1016  		dev->pcm, SNDRV_DMA_TYPE_DEV,
3ca5fc0664ec47 sound/aoa/soundbus/i2sbus/pcm.c        Takashi Iwai  2019-11-05  1017  		&macio_get_pci_dev(i2sdev->macio)->dev,
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21  1018  		64 * 1024, 64 * 1024);
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21  1019  
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21  1020  	return 0;
d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05  1021   out_put_ci_module:
d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05  1022  	module_put(ci->owner);
d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05  1023   out_put_this_module:
d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05  1024  	module_put(THIS_MODULE);
d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05  1025   out_put_sdev:
d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05  1026  	soundbus_dev_put(dev);
d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05  1027   out_free_cii:
d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05  1028  	kfree(cii);
d595ee7e0162ae sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-10-05  1029  	return err;
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21  1030  }
f3d9478b2ce468 sound/aoa/soundbus/i2sbus/i2sbus-pcm.c Johannes Berg 2006-06-21  1031

Takashi Iwai Aug. 2, 2023, 6:42 a.m. UTC | #3

On Tue, 01 Aug 2023 19:18:41 +0200,
cujomalainey@chromium.org wrote:
> 
> From: Curtis Malainey <cujomalainey@chromium.org>
> 
> The current implementation of how devices are released is valid for
> production use cases (root control of memory is handled by card_dev, all
> other devices are no-ops).
> 
> This model does not work though in a kernel hacking environment where
> KASAN and delayed release on kobj is enabled. If the card_dev device is
> released before any of the child objects a use-after-free bug is caught
> by KASAN as the delayed release still has a reference to the devices
> that were freed by the card_dev release. Also both snd_card and snd_pcm
> both own two devices internally, so even if they released independently,
> the shared struct would result in another use after free.
> 
> Solution is to move the child devices into their own memory so they can
> be handled independently and released on their own schedule.
> 
> Signed-off-by: Curtis Malainey <cujomalainey@chromium.org>
> Cc: Doug Anderson <dianders@chromium.org>

Thanks, it's an interesting bug.

I'm not much against the proposed solution, but still this has to be
carefully evaluated.  So, could you give more details about the bug
itself?  It's related with CONFIG_DEBUG_KOBJECT_RELEASE, right?
In which condition it's triggered and how the UAF looks like?


Takashi

Curtis Malainey Aug. 2, 2023, 5:06 p.m. UTC | #4

On Tue, Aug 1, 2023 at 11:42 PM Takashi Iwai <tiwai@suse.de> wrote:
>
> On Tue, 01 Aug 2023 19:18:41 +0200,
> cujomalainey@chromium.org wrote:
> >
> > From: Curtis Malainey <cujomalainey@chromium.org>
> >
> > The current implementation of how devices are released is valid for
> > production use cases (root control of memory is handled by card_dev, all
> > other devices are no-ops).
> >
> > This model does not work though in a kernel hacking environment where
> > KASAN and delayed release on kobj is enabled. If the card_dev device is
> > released before any of the child objects a use-after-free bug is caught
> > by KASAN as the delayed release still has a reference to the devices
> > that were freed by the card_dev release. Also both snd_card and snd_pcm
> > both own two devices internally, so even if they released independently,
> > the shared struct would result in another use after free.
> >
> > Solution is to move the child devices into their own memory so they can
> > be handled independently and released on their own schedule.
> >
> > Signed-off-by: Curtis Malainey <cujomalainey@chromium.org>
> > Cc: Doug Anderson <dianders@chromium.org>
>
> Thanks, it's an interesting bug.
>
> I'm not much against the proposed solution, but still this has to be
> carefully evaluated.  So, could you give more details about the bug
> itself?  It's related with CONFIG_DEBUG_KOBJECT_RELEASE, right?
> In which condition it's triggered and how the UAF looks like?


Hi Takashi

Here is one of the stack traces we are seeing (for snd_pcm)

[ 1098.876460] ==================================================================
[ 1098.884763] BUG: KASAN: use-after-free in enqueue_timer+0xa0/0x628
[ 1098.884849] Write of size 8 at addr ffffff80cbdc6800 by task kworker/7:4/255
[ 1098.884888]
[ 1098.884909] CPU: 7 PID: 255 Comm: kworker/7:4 Not tainted
5.15.117-lockdep-19614-g05bc12fd18a9 #1
5a757fac993273e9fde5e8de9925ee0cebe8540f
[ 1098.884961] Hardware name: Google Pompom (rev3+) with LTE (DT)
[ 1098.884990] Workqueue: events kobject_delayed_cleanup
[ 1098.885038] Call trace:
[ 1098.885059]  dump_backtrace+0x0/0x4e8
[ 1098.885095]  show_stack+0x34/0x44
[ 1098.885129]  dump_stack_lvl+0xdc/0x11c
[ 1098.885165]  print_address_description+0x30/0x2d8
[ 1098.885203]  kasan_report+0x178/0x1e4
[ 1098.885237]  __asan_report_store8_noabort+0x44/0x50
[ 1098.885276]  enqueue_timer+0xa0/0x628
[ 1098.885309]  internal_add_timer+0xa0/0x254
[ 1098.885346]  __mod_timer+0x588/0xc4c
[ 1098.885378]  add_timer+0x74/0x94
[ 1098.885411]  __queue_delayed_work+0x170/0x208
[ 1098.885447]  queue_delayed_work_on+0x128/0x160
[ 1098.885483]  kobject_put+0x278/0x2cc
[ 1098.885517]  put_device+0x30/0x48
[ 1098.885553]  snd_ctl_dev_free+0xc8/0xe4
[ 1098.885590]  __snd_device_free+0x124/0x1b8
[ 1098.885626]  snd_device_free_all+0x148/0x184
[ 1098.885663]  release_card_device+0x5c/0x180
[ 1098.885698]  device_release+0xd4/0x1b4
[ 1098.885732]  kobject_delayed_cleanup+0x130/0x304
[ 1098.885765]  process_one_work+0x620/0x137c
[ 1098.885802]  worker_thread+0x43c/0xa08
[ 1098.885837]  kthread+0x2e4/0x3a0
[ 1098.885872]  ret_from_fork+0x10/0x20
[ 1098.885907]
[ 1098.885926] Allocated by task 11891:
[ 1098.885953]  kasan_save_stack+0x38/0x68
[ 1098.885992]  __kasan_kmalloc+0x90/0xac
[ 1098.886026]  kmem_cache_alloc_trace+0x258/0x40c
[ 1098.886063]  _snd_pcm_new+0xc4/0x2c8
[ 1098.886098]  snd_pcm_new+0x5c/0x74
[ 1098.886132]  loopback_pcm_new+0xa0/0x20c [snd_aloop]
[ 1098.886194]  loopback_probe+0x210/0x850 [snd_aloop]
[ 1098.886235]  platform_probe+0x150/0x1c8
[ 1098.886273]  really_probe+0x274/0xa20
[ 1098.886308]  __driver_probe_device+0x1b4/0x3b4
[ 1098.886344]  driver_probe_device+0x78/0x1c0
[ 1098.886379]  __device_attach_driver+0x1ac/0x2c8
[ 1098.886414]  bus_for_each_drv+0x11c/0x190
[ 1098.886448]  __device_attach+0x278/0x3c8
[ 1098.886482]  device_initial_probe+0x2c/0x3c
[ 1098.886517]  bus_probe_device+0xbc/0x1c8
[ 1098.886550]  device_add+0x1174/0x1630
[ 1098.886581]  platform_device_add+0x3f8/0x6f8
[ 1098.886617]  platform_device_register_full+0x36c/0x3f8
[ 1098.886656]  0xffffffc0032e3174
[ 1098.886691]  do_one_initcall+0x1e8/0x91c
[ 1098.886727]  do_init_module+0x16c/0x444
[ 1098.886762]  load_module+0x63e0/0x7f8c
[ 1098.886795]  __arm64_sys_finit_module+0x1e4/0x214
[ 1098.886830]  invoke_syscall+0x98/0x278
[ 1098.886862]  el0_svc_common+0x214/0x274
[ 1098.886894]  do_el0_svc+0x9c/0x19c
[ 1098.886926]  el0_svc+0x5c/0xc0
[ 1098.886959]  el0t_64_sync_handler+0x78/0x108
[ 1098.886994]  el0t_64_sync+0x1a4/0x1a8
[ 1098.887027]
[ 1098.887046] Freed by task 255:
[ 1098.887071]  kasan_save_stack+0x38/0x68
[ 1098.887106]  kasan_set_track+0x28/0x3c
[ 1098.887139]  kasan_set_free_info+0x28/0x4c
[ 1098.887174]  ____kasan_slab_free+0x110/0x164
[ 1098.887209]  __kasan_slab_free+0x18/0x28
[ 1098.887242]  kfree+0x200/0x868
[ 1098.887275]  snd_pcm_free+0x88/0x98
[ 1098.887311]  snd_pcm_dev_free+0x48/0x5c
[ 1098.887347]  __snd_device_free+0x124/0x1b8
[ 1098.887384]  snd_device_free_all+0xc8/0x184
[ 1098.887419]  release_card_device+0x5c/0x180
[ 1098.887453]  device_release+0xd4/0x1b4
[ 1098.887486]  kobject_delayed_cleanup+0x130/0x304
[ 1098.887519]  process_one_work+0x620/0x137c
[ 1098.887555]  worker_thread+0x43c/0xa08
[ 1098.887590]  kthread+0x2e4/0x3a0
[ 1098.887623]  ret_from_fork+0x10/0x20

A similar one is generated for snd_card if card_dev.release runs
before ctl_dev.release. This patch is solving the same bug in both
places at once.

You should be able to easily reproduce this if you turn on the following

CONFIG_DEBUG_KOBJECT=y
CONFIG_DEBUG_KOBJECT_RELEASE=y
CONFIG_DEBUG_OBJECTS=y
CONFIG_DEBUG_OBJECTS_TIMERS=y
CONFIG_KASAN=y

Then just unload and reload snd-dummy or snd-aloop in a loop. E.g.

while true; do modprobe snd-dummy; rmmod snd-dummy; done

The issue is that kobj should be able to be released independently of
each other, but since all of them depend on card_dev for memory
release and sometimes they even share the same allocation, it breaks
this rule.

There is still another latent bug hiding in the system as well that I
am working on today related to devm memory release of snd_card running
before card_dev.release

It will reproduce with the same setup even with the above patch
applied, so just an FYI if you spot it.

[ 4972.098280] kobject: 'snd_dummy' (000000006c6d3069):
kobject_release, parent 000000009bf07dfe (delayed 2000)
[ 4972.098278] CPU: 9 PID: 16058 Comm: kworker/9:0 Tainted: G     U
         6.5.0-rc3-00236-gd54aad9920bd-dirty #18
a69e57e648f1e29627a189036c9fd8083c170225
[ 4972.098294] Hardware name: Google Anahera/Anahera, BIOS
Google_Anahera.14505.315.0 12/02/2022
[ 4972.098302] Workqueue: events kobject_delayed_cleanup
[ 4972.098317] Call Trace:
[ 4972.098324]  <TASK>
[ 4972.098330]  dump_stack_lvl+0x91/0xd0
[ 4972.098344]  print_report+0x15b/0x4d0
[ 4972.098356]  ? __virt_addr_valid+0xbb/0x130
[ 4972.098369]  ? kasan_addr_to_slab+0x11/0x80
[ 4972.098381]  ? release_card_device+0x86/0xd0
[ 4972.098392]  kasan_report+0xb1/0xf0
[ 4972.098403]  ? release_card_device+0x86/0xd0
[ 4972.098415]  ? _raw_spin_unlock_irqrestore+0x1b/0x40
[ 4972.098427]  release_card_device+0x86/0xd0
[ 4972.098438]  device_release+0x66/0x110
[ 4972.098449]  kobject_delayed_cleanup+0x133/0x140
[ 4972.098462]  process_one_work+0x3e3/0x680
[ 4972.098474]  worker_thread+0x487/0x6d0
[ 4972.098487]  ? __pfx_worker_thread+0x10/0x10
[ 4972.098497]  kthread+0x199/0x1c0
[ 4972.098508]  ? __pfx_worker_thread+0x10/0x10
[ 4972.098518]  ? __pfx_kthread+0x10/0x10
[ 4972.098528]  ret_from_fork+0x3c/0x60
[ 4972.098540]  ? __pfx_kthread+0x10/0x10
[ 4972.098552]  ret_from_fork_asm+0x1b/0x30
[ 4972.098563] RIP: 0000:0x0
[ 4972.098577] Code: Unable to access opcode bytes at
0xffffffffffffffd6.
[ 4972.098584] RSP: 0000:0000000000000000 EFLAGS: 00000000 ORIG_RAX:
0000000000000000
[ 4972.098596] RAX: 0000000000000000 RBX: 0000000000000000 RCX:
0000000000000000
[ 4972.098604] RDX: 0000000000000000 RSI: 0000000000000000 RDI:
0000000000000000
[ 4972.098612] RBP: 0000000000000000 R08: 0000000000000000 R09:
0000000000000000
[ 4972.098620] R10: 0000000000000000 R11: 0000000000000000 R12:
0000000000000000
[ 4972.098622] kobject: 'drivers' (00000000d71d1f56): kobject_release,
parent 000000007062c760 (delayed 4000)
[ 4972.098631] R13: 0000000000000000 R14: 0000000000000000 R15:
0000000000000000
[ 4972.098641] kobject: 'holders' (0000000091718821): kobject_release,
parent 000000007062c760 (delayed 2000)
[ 4972.098641]  </TASK>

Curtis

>
>
>
> Takashi

Dan Carpenter Aug. 3, 2023, 9:49 a.m. UTC | #5

Oh, hm.  I read my email out of order.  This answers the questions I
had.  Hopefully we can include some of this into the commit message.

regards,
dan carpenter

Takashi Iwai Aug. 3, 2023, 1:06 p.m. UTC | #6

On Wed, 02 Aug 2023 19:06:05 +0200,
Curtis Malainey wrote:
> 
> On Tue, Aug 1, 2023 at 11:42 PM Takashi Iwai <tiwai@suse.de> wrote:
> >
> > On Tue, 01 Aug 2023 19:18:41 +0200,
> > cujomalainey@chromium.org wrote:
> > >
> > > From: Curtis Malainey <cujomalainey@chromium.org>
> > >
> > > The current implementation of how devices are released is valid for
> > > production use cases (root control of memory is handled by card_dev, all
> > > other devices are no-ops).
> > >
> > > This model does not work though in a kernel hacking environment where
> > > KASAN and delayed release on kobj is enabled. If the card_dev device is
> > > released before any of the child objects a use-after-free bug is caught
> > > by KASAN as the delayed release still has a reference to the devices
> > > that were freed by the card_dev release. Also both snd_card and snd_pcm
> > > both own two devices internally, so even if they released independently,
> > > the shared struct would result in another use after free.
> > >
> > > Solution is to move the child devices into their own memory so they can
> > > be handled independently and released on their own schedule.
> > >
> > > Signed-off-by: Curtis Malainey <cujomalainey@chromium.org>
> > > Cc: Doug Anderson <dianders@chromium.org>
> >
> > Thanks, it's an interesting bug.
> >
> > I'm not much against the proposed solution, but still this has to be
> > carefully evaluated.  So, could you give more details about the bug
> > itself?  It's related with CONFIG_DEBUG_KOBJECT_RELEASE, right?
> > In which condition it's triggered and how the UAF looks like?
> 
> 
> Hi Takashi
> 
> Here is one of the stack traces we are seeing (for snd_pcm)
> 
> [ 1098.876460] ==================================================================
> [ 1098.884763] BUG: KASAN: use-after-free in enqueue_timer+0xa0/0x628
> [ 1098.884849] Write of size 8 at addr ffffff80cbdc6800 by task kworker/7:4/255
> [ 1098.884888]
> [ 1098.884909] CPU: 7 PID: 255 Comm: kworker/7:4 Not tainted
> 5.15.117-lockdep-19614-g05bc12fd18a9 #1
> 5a757fac993273e9fde5e8de9925ee0cebe8540f
> [ 1098.884961] Hardware name: Google Pompom (rev3+) with LTE (DT)
> [ 1098.884990] Workqueue: events kobject_delayed_cleanup
> [ 1098.885038] Call trace:
> [ 1098.885059]  dump_backtrace+0x0/0x4e8
> [ 1098.885095]  show_stack+0x34/0x44
> [ 1098.885129]  dump_stack_lvl+0xdc/0x11c
> [ 1098.885165]  print_address_description+0x30/0x2d8
> [ 1098.885203]  kasan_report+0x178/0x1e4
> [ 1098.885237]  __asan_report_store8_noabort+0x44/0x50
> [ 1098.885276]  enqueue_timer+0xa0/0x628
> [ 1098.885309]  internal_add_timer+0xa0/0x254
> [ 1098.885346]  __mod_timer+0x588/0xc4c
> [ 1098.885378]  add_timer+0x74/0x94
> [ 1098.885411]  __queue_delayed_work+0x170/0x208
> [ 1098.885447]  queue_delayed_work_on+0x128/0x160
> [ 1098.885483]  kobject_put+0x278/0x2cc
> [ 1098.885517]  put_device+0x30/0x48
> [ 1098.885553]  snd_ctl_dev_free+0xc8/0xe4
> [ 1098.885590]  __snd_device_free+0x124/0x1b8
> [ 1098.885626]  snd_device_free_all+0x148/0x184
> [ 1098.885663]  release_card_device+0x5c/0x180
> [ 1098.885698]  device_release+0xd4/0x1b4
> [ 1098.885732]  kobject_delayed_cleanup+0x130/0x304
> [ 1098.885765]  process_one_work+0x620/0x137c
> [ 1098.885802]  worker_thread+0x43c/0xa08
> [ 1098.885837]  kthread+0x2e4/0x3a0
> [ 1098.885872]  ret_from_fork+0x10/0x20
> [ 1098.885907]
> [ 1098.885926] Allocated by task 11891:
> [ 1098.885953]  kasan_save_stack+0x38/0x68
> [ 1098.885992]  __kasan_kmalloc+0x90/0xac
> [ 1098.886026]  kmem_cache_alloc_trace+0x258/0x40c
> [ 1098.886063]  _snd_pcm_new+0xc4/0x2c8
> [ 1098.886098]  snd_pcm_new+0x5c/0x74
> [ 1098.886132]  loopback_pcm_new+0xa0/0x20c [snd_aloop]
> [ 1098.886194]  loopback_probe+0x210/0x850 [snd_aloop]
> [ 1098.886235]  platform_probe+0x150/0x1c8
> [ 1098.886273]  really_probe+0x274/0xa20
> [ 1098.886308]  __driver_probe_device+0x1b4/0x3b4
> [ 1098.886344]  driver_probe_device+0x78/0x1c0
> [ 1098.886379]  __device_attach_driver+0x1ac/0x2c8
> [ 1098.886414]  bus_for_each_drv+0x11c/0x190
> [ 1098.886448]  __device_attach+0x278/0x3c8
> [ 1098.886482]  device_initial_probe+0x2c/0x3c
> [ 1098.886517]  bus_probe_device+0xbc/0x1c8
> [ 1098.886550]  device_add+0x1174/0x1630
> [ 1098.886581]  platform_device_add+0x3f8/0x6f8
> [ 1098.886617]  platform_device_register_full+0x36c/0x3f8
> [ 1098.886656]  0xffffffc0032e3174
> [ 1098.886691]  do_one_initcall+0x1e8/0x91c
> [ 1098.886727]  do_init_module+0x16c/0x444
> [ 1098.886762]  load_module+0x63e0/0x7f8c
> [ 1098.886795]  __arm64_sys_finit_module+0x1e4/0x214
> [ 1098.886830]  invoke_syscall+0x98/0x278
> [ 1098.886862]  el0_svc_common+0x214/0x274
> [ 1098.886894]  do_el0_svc+0x9c/0x19c
> [ 1098.886926]  el0_svc+0x5c/0xc0
> [ 1098.886959]  el0t_64_sync_handler+0x78/0x108
> [ 1098.886994]  el0t_64_sync+0x1a4/0x1a8
> [ 1098.887027]
> [ 1098.887046] Freed by task 255:
> [ 1098.887071]  kasan_save_stack+0x38/0x68
> [ 1098.887106]  kasan_set_track+0x28/0x3c
> [ 1098.887139]  kasan_set_free_info+0x28/0x4c
> [ 1098.887174]  ____kasan_slab_free+0x110/0x164
> [ 1098.887209]  __kasan_slab_free+0x18/0x28
> [ 1098.887242]  kfree+0x200/0x868
> [ 1098.887275]  snd_pcm_free+0x88/0x98
> [ 1098.887311]  snd_pcm_dev_free+0x48/0x5c
> [ 1098.887347]  __snd_device_free+0x124/0x1b8
> [ 1098.887384]  snd_device_free_all+0xc8/0x184
> [ 1098.887419]  release_card_device+0x5c/0x180
> [ 1098.887453]  device_release+0xd4/0x1b4
> [ 1098.887486]  kobject_delayed_cleanup+0x130/0x304
> [ 1098.887519]  process_one_work+0x620/0x137c
> [ 1098.887555]  worker_thread+0x43c/0xa08
> [ 1098.887590]  kthread+0x2e4/0x3a0
> [ 1098.887623]  ret_from_fork+0x10/0x20
> 
> A similar one is generated for snd_card if card_dev.release runs
> before ctl_dev.release. This patch is solving the same bug in both
> places at once.

Hmm.  It's still puzzling, and I'm not 100% sure whether your analysis
is right.  The above shows it's freed by task 255, while the task
hitting UAF itself is 255.  It might be rather the combination of
devres and delayed releases.

With CONFIG_DEBUG_KOBJECT_RELEASE, the kernel should show the info of
each delayed release kobject.  Could you give them together with the
Oops, so that we can see the flow how it hits UAF?


thanks,

Takashi

> 
> You should be able to easily reproduce this if you turn on the following
> 
> CONFIG_DEBUG_KOBJECT=y
> CONFIG_DEBUG_KOBJECT_RELEASE=y
> CONFIG_DEBUG_OBJECTS=y
> CONFIG_DEBUG_OBJECTS_TIMERS=y
> CONFIG_KASAN=y
> 
> Then just unload and reload snd-dummy or snd-aloop in a loop. E.g.
> 
> while true; do modprobe snd-dummy; rmmod snd-dummy; done
> 
> The issue is that kobj should be able to be released independently of
> each other, but since all of them depend on card_dev for memory
> release and sometimes they even share the same allocation, it breaks
> this rule.
> 
> There is still another latent bug hiding in the system as well that I
> am working on today related to devm memory release of snd_card running
> before card_dev.release
> 
> It will reproduce with the same setup even with the above patch
> applied, so just an FYI if you spot it.
> 
> [ 4972.098280] kobject: 'snd_dummy' (000000006c6d3069):
> kobject_release, parent 000000009bf07dfe (delayed 2000)
> [ 4972.098278] CPU: 9 PID: 16058 Comm: kworker/9:0 Tainted: G     U
>          6.5.0-rc3-00236-gd54aad9920bd-dirty #18
> a69e57e648f1e29627a189036c9fd8083c170225
> [ 4972.098294] Hardware name: Google Anahera/Anahera, BIOS
> Google_Anahera.14505.315.0 12/02/2022
> [ 4972.098302] Workqueue: events kobject_delayed_cleanup
> [ 4972.098317] Call Trace:
> [ 4972.098324]  <TASK>
> [ 4972.098330]  dump_stack_lvl+0x91/0xd0
> [ 4972.098344]  print_report+0x15b/0x4d0
> [ 4972.098356]  ? __virt_addr_valid+0xbb/0x130
> [ 4972.098369]  ? kasan_addr_to_slab+0x11/0x80
> [ 4972.098381]  ? release_card_device+0x86/0xd0
> [ 4972.098392]  kasan_report+0xb1/0xf0
> [ 4972.098403]  ? release_card_device+0x86/0xd0
> [ 4972.098415]  ? _raw_spin_unlock_irqrestore+0x1b/0x40
> [ 4972.098427]  release_card_device+0x86/0xd0
> [ 4972.098438]  device_release+0x66/0x110
> [ 4972.098449]  kobject_delayed_cleanup+0x133/0x140
> [ 4972.098462]  process_one_work+0x3e3/0x680
> [ 4972.098474]  worker_thread+0x487/0x6d0
> [ 4972.098487]  ? __pfx_worker_thread+0x10/0x10
> [ 4972.098497]  kthread+0x199/0x1c0
> [ 4972.098508]  ? __pfx_worker_thread+0x10/0x10
> [ 4972.098518]  ? __pfx_kthread+0x10/0x10
> [ 4972.098528]  ret_from_fork+0x3c/0x60
> [ 4972.098540]  ? __pfx_kthread+0x10/0x10
> [ 4972.098552]  ret_from_fork_asm+0x1b/0x30
> [ 4972.098563] RIP: 0000:0x0
> [ 4972.098577] Code: Unable to access opcode bytes at
> 0xffffffffffffffd6.
> [ 4972.098584] RSP: 0000:0000000000000000 EFLAGS: 00000000 ORIG_RAX:
> 0000000000000000
> [ 4972.098596] RAX: 0000000000000000 RBX: 0000000000000000 RCX:
> 0000000000000000
> [ 4972.098604] RDX: 0000000000000000 RSI: 0000000000000000 RDI:
> 0000000000000000
> [ 4972.098612] RBP: 0000000000000000 R08: 0000000000000000 R09:
> 0000000000000000
> [ 4972.098620] R10: 0000000000000000 R11: 0000000000000000 R12:
> 0000000000000000
> [ 4972.098622] kobject: 'drivers' (00000000d71d1f56): kobject_release,
> parent 000000007062c760 (delayed 4000)
> [ 4972.098631] R13: 0000000000000000 R14: 0000000000000000 R15:
> 0000000000000000
> [ 4972.098641] kobject: 'holders' (0000000091718821): kobject_release,
> parent 000000007062c760 (delayed 2000)
> [ 4972.098641]  </TASK>
> 
> Curtis
> 
> >
> >
> >
> > Takashi
>

Takashi Iwai Aug. 3, 2023, 3:35 p.m. UTC | #7

On Thu, 03 Aug 2023 15:06:19 +0200,
Takashi Iwai wrote:
> 
> On Wed, 02 Aug 2023 19:06:05 +0200,
> Curtis Malainey wrote:
> > 
> > On Tue, Aug 1, 2023 at 11:42 PM Takashi Iwai <tiwai@suse.de> wrote:
> > >
> > > On Tue, 01 Aug 2023 19:18:41 +0200,
> > > cujomalainey@chromium.org wrote:
> > > >
> > > > From: Curtis Malainey <cujomalainey@chromium.org>
> > > >
> > > > The current implementation of how devices are released is valid for
> > > > production use cases (root control of memory is handled by card_dev, all
> > > > other devices are no-ops).
> > > >
> > > > This model does not work though in a kernel hacking environment where
> > > > KASAN and delayed release on kobj is enabled. If the card_dev device is
> > > > released before any of the child objects a use-after-free bug is caught
> > > > by KASAN as the delayed release still has a reference to the devices
> > > > that were freed by the card_dev release. Also both snd_card and snd_pcm
> > > > both own two devices internally, so even if they released independently,
> > > > the shared struct would result in another use after free.
> > > >
> > > > Solution is to move the child devices into their own memory so they can
> > > > be handled independently and released on their own schedule.
> > > >
> > > > Signed-off-by: Curtis Malainey <cujomalainey@chromium.org>
> > > > Cc: Doug Anderson <dianders@chromium.org>
> > >
> > > Thanks, it's an interesting bug.
> > >
> > > I'm not much against the proposed solution, but still this has to be
> > > carefully evaluated.  So, could you give more details about the bug
> > > itself?  It's related with CONFIG_DEBUG_KOBJECT_RELEASE, right?
> > > In which condition it's triggered and how the UAF looks like?
> > 
> > 
> > Hi Takashi
> > 
> > Here is one of the stack traces we are seeing (for snd_pcm)
> > 
> > [ 1098.876460] ==================================================================
> > [ 1098.884763] BUG: KASAN: use-after-free in enqueue_timer+0xa0/0x628
> > [ 1098.884849] Write of size 8 at addr ffffff80cbdc6800 by task kworker/7:4/255
> > [ 1098.884888]
> > [ 1098.884909] CPU: 7 PID: 255 Comm: kworker/7:4 Not tainted
> > 5.15.117-lockdep-19614-g05bc12fd18a9 #1
> > 5a757fac993273e9fde5e8de9925ee0cebe8540f
> > [ 1098.884961] Hardware name: Google Pompom (rev3+) with LTE (DT)
> > [ 1098.884990] Workqueue: events kobject_delayed_cleanup
> > [ 1098.885038] Call trace:
> > [ 1098.885059]  dump_backtrace+0x0/0x4e8
> > [ 1098.885095]  show_stack+0x34/0x44
> > [ 1098.885129]  dump_stack_lvl+0xdc/0x11c
> > [ 1098.885165]  print_address_description+0x30/0x2d8
> > [ 1098.885203]  kasan_report+0x178/0x1e4
> > [ 1098.885237]  __asan_report_store8_noabort+0x44/0x50
> > [ 1098.885276]  enqueue_timer+0xa0/0x628
> > [ 1098.885309]  internal_add_timer+0xa0/0x254
> > [ 1098.885346]  __mod_timer+0x588/0xc4c
> > [ 1098.885378]  add_timer+0x74/0x94
> > [ 1098.885411]  __queue_delayed_work+0x170/0x208
> > [ 1098.885447]  queue_delayed_work_on+0x128/0x160
> > [ 1098.885483]  kobject_put+0x278/0x2cc
> > [ 1098.885517]  put_device+0x30/0x48
> > [ 1098.885553]  snd_ctl_dev_free+0xc8/0xe4
> > [ 1098.885590]  __snd_device_free+0x124/0x1b8
> > [ 1098.885626]  snd_device_free_all+0x148/0x184
> > [ 1098.885663]  release_card_device+0x5c/0x180
> > [ 1098.885698]  device_release+0xd4/0x1b4
> > [ 1098.885732]  kobject_delayed_cleanup+0x130/0x304
> > [ 1098.885765]  process_one_work+0x620/0x137c
> > [ 1098.885802]  worker_thread+0x43c/0xa08
> > [ 1098.885837]  kthread+0x2e4/0x3a0
> > [ 1098.885872]  ret_from_fork+0x10/0x20
> > [ 1098.885907]
> > [ 1098.885926] Allocated by task 11891:
> > [ 1098.885953]  kasan_save_stack+0x38/0x68
> > [ 1098.885992]  __kasan_kmalloc+0x90/0xac
> > [ 1098.886026]  kmem_cache_alloc_trace+0x258/0x40c
> > [ 1098.886063]  _snd_pcm_new+0xc4/0x2c8
> > [ 1098.886098]  snd_pcm_new+0x5c/0x74
> > [ 1098.886132]  loopback_pcm_new+0xa0/0x20c [snd_aloop]
> > [ 1098.886194]  loopback_probe+0x210/0x850 [snd_aloop]
> > [ 1098.886235]  platform_probe+0x150/0x1c8
> > [ 1098.886273]  really_probe+0x274/0xa20
> > [ 1098.886308]  __driver_probe_device+0x1b4/0x3b4
> > [ 1098.886344]  driver_probe_device+0x78/0x1c0
> > [ 1098.886379]  __device_attach_driver+0x1ac/0x2c8
> > [ 1098.886414]  bus_for_each_drv+0x11c/0x190
> > [ 1098.886448]  __device_attach+0x278/0x3c8
> > [ 1098.886482]  device_initial_probe+0x2c/0x3c
> > [ 1098.886517]  bus_probe_device+0xbc/0x1c8
> > [ 1098.886550]  device_add+0x1174/0x1630
> > [ 1098.886581]  platform_device_add+0x3f8/0x6f8
> > [ 1098.886617]  platform_device_register_full+0x36c/0x3f8
> > [ 1098.886656]  0xffffffc0032e3174
> > [ 1098.886691]  do_one_initcall+0x1e8/0x91c
> > [ 1098.886727]  do_init_module+0x16c/0x444
> > [ 1098.886762]  load_module+0x63e0/0x7f8c
> > [ 1098.886795]  __arm64_sys_finit_module+0x1e4/0x214
> > [ 1098.886830]  invoke_syscall+0x98/0x278
> > [ 1098.886862]  el0_svc_common+0x214/0x274
> > [ 1098.886894]  do_el0_svc+0x9c/0x19c
> > [ 1098.886926]  el0_svc+0x5c/0xc0
> > [ 1098.886959]  el0t_64_sync_handler+0x78/0x108
> > [ 1098.886994]  el0t_64_sync+0x1a4/0x1a8
> > [ 1098.887027]
> > [ 1098.887046] Freed by task 255:
> > [ 1098.887071]  kasan_save_stack+0x38/0x68
> > [ 1098.887106]  kasan_set_track+0x28/0x3c
> > [ 1098.887139]  kasan_set_free_info+0x28/0x4c
> > [ 1098.887174]  ____kasan_slab_free+0x110/0x164
> > [ 1098.887209]  __kasan_slab_free+0x18/0x28
> > [ 1098.887242]  kfree+0x200/0x868
> > [ 1098.887275]  snd_pcm_free+0x88/0x98
> > [ 1098.887311]  snd_pcm_dev_free+0x48/0x5c
> > [ 1098.887347]  __snd_device_free+0x124/0x1b8
> > [ 1098.887384]  snd_device_free_all+0xc8/0x184
> > [ 1098.887419]  release_card_device+0x5c/0x180
> > [ 1098.887453]  device_release+0xd4/0x1b4
> > [ 1098.887486]  kobject_delayed_cleanup+0x130/0x304
> > [ 1098.887519]  process_one_work+0x620/0x137c
> > [ 1098.887555]  worker_thread+0x43c/0xa08
> > [ 1098.887590]  kthread+0x2e4/0x3a0
> > [ 1098.887623]  ret_from_fork+0x10/0x20
> > 
> > A similar one is generated for snd_card if card_dev.release runs
> > before ctl_dev.release. This patch is solving the same bug in both
> > places at once.
> 
> Hmm.  It's still puzzling, and I'm not 100% sure whether your analysis
> is right.  The above shows it's freed by task 255, while the task
> hitting UAF itself is 255.  It might be rather the combination of
> devres and delayed releases.
> 
> With CONFIG_DEBUG_KOBJECT_RELEASE, the kernel should show the info of
> each delayed release kobject.  Could you give them together with the
> Oops, so that we can see the flow how it hits UAF?

Now I slowly understand what's happening.  Essentially, it's because
the *embedded* struct device is released asynchronously from the
ALSA's resource management (via dev_free ops).  The objects may be
already released via card's device release (that calls
snd_device_free_all()), while the release of each struct device that
was already triggered beforehand can be delayed due to the debug
option.

You code change looks mostly fine, but as far as I see,
compress_offload also needs a similar change.  Meanwhile, rawmidi and
hwdep do release the object via device release properly, so those are
fine.  Ditto for sequencer.  And timer is only a global device, hence
it must be fine.

And, I believe we need a bit more detailed patch description.  So,
could you improve the patch description, split the change to each
component (control, pcm and compress_offload), and resubmit?

Also, maybe it's worth to change snd_device_initialize() to take the
release (optional) argument, too, instead of setting it explicitly
afterwards at each place.  Already majority of callers will set own
release callbacks.  This can be done at one more additional patch at
last.


thanks,

Takashi

Curtis Malainey Aug. 3, 2023, 11:39 p.m. UTC | #8

On Thu, Aug 3, 2023 at 8:35 AM Takashi Iwai <tiwai@suse.de> wrote:
>
> On Thu, 03 Aug 2023 15:06:19 +0200,
> Takashi Iwai wrote:
> >
> > On Wed, 02 Aug 2023 19:06:05 +0200,
> > Curtis Malainey wrote:
> > >
> > > On Tue, Aug 1, 2023 at 11:42 PM Takashi Iwai <tiwai@suse.de> wrote:
> > > >
> > > > On Tue, 01 Aug 2023 19:18:41 +0200,
> > > > cujomalainey@chromium.org wrote:
> > > > >
> > > > > From: Curtis Malainey <cujomalainey@chromium.org>
> > > > >
> > > > > The current implementation of how devices are released is valid for
> > > > > production use cases (root control of memory is handled by card_dev, all
> > > > > other devices are no-ops).
> > > > >
> > > > > This model does not work though in a kernel hacking environment where
> > > > > KASAN and delayed release on kobj is enabled. If the card_dev device is
> > > > > released before any of the child objects a use-after-free bug is caught
> > > > > by KASAN as the delayed release still has a reference to the devices
> > > > > that were freed by the card_dev release. Also both snd_card and snd_pcm
> > > > > both own two devices internally, so even if they released independently,
> > > > > the shared struct would result in another use after free.
> > > > >
> > > > > Solution is to move the child devices into their own memory so they can
> > > > > be handled independently and released on their own schedule.
> > > > >
> > > > > Signed-off-by: Curtis Malainey <cujomalainey@chromium.org>
> > > > > Cc: Doug Anderson <dianders@chromium.org>
> > > >
> > > > Thanks, it's an interesting bug.
> > > >
> > > > I'm not much against the proposed solution, but still this has to be
> > > > carefully evaluated.  So, could you give more details about the bug
> > > > itself?  It's related with CONFIG_DEBUG_KOBJECT_RELEASE, right?
> > > > In which condition it's triggered and how the UAF looks like?
> > >
> > >
> > > Hi Takashi
> > >
> > > Here is one of the stack traces we are seeing (for snd_pcm)
> > >
> > > [ 1098.876460] ==================================================================
> > > [ 1098.884763] BUG: KASAN: use-after-free in enqueue_timer+0xa0/0x628
> > > [ 1098.884849] Write of size 8 at addr ffffff80cbdc6800 by task kworker/7:4/255
> > > [ 1098.884888]
> > > [ 1098.884909] CPU: 7 PID: 255 Comm: kworker/7:4 Not tainted
> > > 5.15.117-lockdep-19614-g05bc12fd18a9 #1
> > > 5a757fac993273e9fde5e8de9925ee0cebe8540f
> > > [ 1098.884961] Hardware name: Google Pompom (rev3+) with LTE (DT)
> > > [ 1098.884990] Workqueue: events kobject_delayed_cleanup
> > > [ 1098.885038] Call trace:
> > > [ 1098.885059]  dump_backtrace+0x0/0x4e8
> > > [ 1098.885095]  show_stack+0x34/0x44
> > > [ 1098.885129]  dump_stack_lvl+0xdc/0x11c
> > > [ 1098.885165]  print_address_description+0x30/0x2d8
> > > [ 1098.885203]  kasan_report+0x178/0x1e4
> > > [ 1098.885237]  __asan_report_store8_noabort+0x44/0x50
> > > [ 1098.885276]  enqueue_timer+0xa0/0x628
> > > [ 1098.885309]  internal_add_timer+0xa0/0x254
> > > [ 1098.885346]  __mod_timer+0x588/0xc4c
> > > [ 1098.885378]  add_timer+0x74/0x94
> > > [ 1098.885411]  __queue_delayed_work+0x170/0x208
> > > [ 1098.885447]  queue_delayed_work_on+0x128/0x160
> > > [ 1098.885483]  kobject_put+0x278/0x2cc
> > > [ 1098.885517]  put_device+0x30/0x48
> > > [ 1098.885553]  snd_ctl_dev_free+0xc8/0xe4
> > > [ 1098.885590]  __snd_device_free+0x124/0x1b8
> > > [ 1098.885626]  snd_device_free_all+0x148/0x184
> > > [ 1098.885663]  release_card_device+0x5c/0x180
> > > [ 1098.885698]  device_release+0xd4/0x1b4
> > > [ 1098.885732]  kobject_delayed_cleanup+0x130/0x304
> > > [ 1098.885765]  process_one_work+0x620/0x137c
> > > [ 1098.885802]  worker_thread+0x43c/0xa08
> > > [ 1098.885837]  kthread+0x2e4/0x3a0
> > > [ 1098.885872]  ret_from_fork+0x10/0x20
> > > [ 1098.885907]
> > > [ 1098.885926] Allocated by task 11891:
> > > [ 1098.885953]  kasan_save_stack+0x38/0x68
> > > [ 1098.885992]  __kasan_kmalloc+0x90/0xac
> > > [ 1098.886026]  kmem_cache_alloc_trace+0x258/0x40c
> > > [ 1098.886063]  _snd_pcm_new+0xc4/0x2c8
> > > [ 1098.886098]  snd_pcm_new+0x5c/0x74
> > > [ 1098.886132]  loopback_pcm_new+0xa0/0x20c [snd_aloop]
> > > [ 1098.886194]  loopback_probe+0x210/0x850 [snd_aloop]
> > > [ 1098.886235]  platform_probe+0x150/0x1c8
> > > [ 1098.886273]  really_probe+0x274/0xa20
> > > [ 1098.886308]  __driver_probe_device+0x1b4/0x3b4
> > > [ 1098.886344]  driver_probe_device+0x78/0x1c0
> > > [ 1098.886379]  __device_attach_driver+0x1ac/0x2c8
> > > [ 1098.886414]  bus_for_each_drv+0x11c/0x190
> > > [ 1098.886448]  __device_attach+0x278/0x3c8
> > > [ 1098.886482]  device_initial_probe+0x2c/0x3c
> > > [ 1098.886517]  bus_probe_device+0xbc/0x1c8
> > > [ 1098.886550]  device_add+0x1174/0x1630
> > > [ 1098.886581]  platform_device_add+0x3f8/0x6f8
> > > [ 1098.886617]  platform_device_register_full+0x36c/0x3f8
> > > [ 1098.886656]  0xffffffc0032e3174
> > > [ 1098.886691]  do_one_initcall+0x1e8/0x91c
> > > [ 1098.886727]  do_init_module+0x16c/0x444
> > > [ 1098.886762]  load_module+0x63e0/0x7f8c
> > > [ 1098.886795]  __arm64_sys_finit_module+0x1e4/0x214
> > > [ 1098.886830]  invoke_syscall+0x98/0x278
> > > [ 1098.886862]  el0_svc_common+0x214/0x274
> > > [ 1098.886894]  do_el0_svc+0x9c/0x19c
> > > [ 1098.886926]  el0_svc+0x5c/0xc0
> > > [ 1098.886959]  el0t_64_sync_handler+0x78/0x108
> > > [ 1098.886994]  el0t_64_sync+0x1a4/0x1a8
> > > [ 1098.887027]
> > > [ 1098.887046] Freed by task 255:
> > > [ 1098.887071]  kasan_save_stack+0x38/0x68
> > > [ 1098.887106]  kasan_set_track+0x28/0x3c
> > > [ 1098.887139]  kasan_set_free_info+0x28/0x4c
> > > [ 1098.887174]  ____kasan_slab_free+0x110/0x164
> > > [ 1098.887209]  __kasan_slab_free+0x18/0x28
> > > [ 1098.887242]  kfree+0x200/0x868
> > > [ 1098.887275]  snd_pcm_free+0x88/0x98
> > > [ 1098.887311]  snd_pcm_dev_free+0x48/0x5c
> > > [ 1098.887347]  __snd_device_free+0x124/0x1b8
> > > [ 1098.887384]  snd_device_free_all+0xc8/0x184
> > > [ 1098.887419]  release_card_device+0x5c/0x180
> > > [ 1098.887453]  device_release+0xd4/0x1b4
> > > [ 1098.887486]  kobject_delayed_cleanup+0x130/0x304
> > > [ 1098.887519]  process_one_work+0x620/0x137c
> > > [ 1098.887555]  worker_thread+0x43c/0xa08
> > > [ 1098.887590]  kthread+0x2e4/0x3a0
> > > [ 1098.887623]  ret_from_fork+0x10/0x20
> > >
> > > A similar one is generated for snd_card if card_dev.release runs
> > > before ctl_dev.release. This patch is solving the same bug in both
> > > places at once.
> >
> > Hmm.  It's still puzzling, and I'm not 100% sure whether your analysis
> > is right.  The above shows it's freed by task 255, while the task
> > hitting UAF itself is 255.  It might be rather the combination of
> > devres and delayed releases.
> >
> > With CONFIG_DEBUG_KOBJECT_RELEASE, the kernel should show the info of
> > each delayed release kobject.  Could you give them together with the
> > Oops, so that we can see the flow how it hits UAF?
>
> Now I slowly understand what's happening.  Essentially, it's because
> the *embedded* struct device is released asynchronously from the
> ALSA's resource management (via dev_free ops).  The objects may be
> already released via card's device release (that calls
> snd_device_free_all()), while the release of each struct device that
> was already triggered beforehand can be delayed due to the debug
> option.

Ah that actually is the second bug I am still working on and not this
one. This patch fixes a bug in both the devm and non-devm case.

If you look at snd_card_init it installs release_card_device as the
release function for the struct device card_dev.

snd_card also has another struct device embedded in it, ctl_dev.

release_card_device calls snd_card_do_free which at the end releases
the snd_card struct.

The problem here is that in the kernel hacking situation, the release
function on the devices are not always called inplace (i.e. assuming
proper referencing counting behaviour). So with a bit of RNG, you hit
a case where card_dev runs release_card_device before ctl_dev, or even
before the PCM devices which results in the release function operating
on a freed pointer.

The other sign this is an issue is that we have 2 struct devices in
the same memory allocation (both in card and pcm), therefore they
cannot properly own their release.

>
> You code change looks mostly fine, but as far as I see,
> compress_offload also needs a similar change.  Meanwhile, rawmidi and
> hwdep do release the object via device release properly, so those are
> fine.  Ditto for sequencer.  And timer is only a global device, hence
> it must be fine.

Yes I noticed this too, will draft versions for them once I have the
snd_card devm version solved. So far though only snd_card has
reproduced the devm/release race.

>
> And, I believe we need a bit more detailed patch description.  So,
> could you improve the patch description, split the change to each
> component (control, pcm and compress_offload), and resubmit?

I can definitely update this to contain a bit more detail. That being
said, given the confusion about which bug this solves, do you still
want me to split this up? The bug is not resolved without both PCM and
card change for this bug which is hit in the delayed release, but they
are for two different paths reported by KASAN.

>
> Also, maybe it's worth to change snd_device_initialize() to take the
> release (optional) argument, too, instead of setting it explicitly
> afterwards at each place.  Already majority of callers will set own
> release callbacks.  This can be done at one more additional patch at
> last.

Sounds like a good idea, definitely an option once we get the
underlying issue solved.

Curtis

>
>
> thanks,
>
> Takashi

Takashi Iwai Aug. 4, 2023, 8:58 a.m. UTC | #9

On Fri, 04 Aug 2023 01:39:30 +0200,
Curtis Malainey wrote:
> > Now I slowly understand what's happening.  Essentially, it's because
> > the *embedded* struct device is released asynchronously from the
> > ALSA's resource management (via dev_free ops).  The objects may be
> > already released via card's device release (that calls
> > snd_device_free_all()), while the release of each struct device that
> > was already triggered beforehand can be delayed due to the debug
> > option.
> 
> Ah that actually is the second bug I am still working on and not this
> one. This patch fixes a bug in both the devm and non-devm case.
> 
> If you look at snd_card_init it installs release_card_device as the
> release function for the struct device card_dev.
> 
> snd_card also has another struct device embedded in it, ctl_dev.
> 
> release_card_device calls snd_card_do_free which at the end releases
> the snd_card struct.
> 
> The problem here is that in the kernel hacking situation, the release
> function on the devices are not always called inplace (i.e. assuming
> proper referencing counting behaviour). So with a bit of RNG, you hit
> a case where card_dev runs release_card_device before ctl_dev, or even
> before the PCM devices which results in the release function operating
> on a freed pointer.
> 
> The other sign this is an issue is that we have 2 struct devices in
> the same memory allocation (both in card and pcm), therefore they
> cannot properly own their release.

Right, that's why those two aren't coded like rawmidi and hwdep.

> > You code change looks mostly fine, but as far as I see,
> > compress_offload also needs a similar change.  Meanwhile, rawmidi and
> > hwdep do release the object via device release properly, so those are
> > fine.  Ditto for sequencer.  And timer is only a global device, hence
> > it must be fine.
> 
> Yes I noticed this too, will draft versions for them once I have the
> snd_card devm version solved. So far though only snd_card has
> reproduced the devm/release race.
> 
> >
> > And, I believe we need a bit more detailed patch description.  So,
> > could you improve the patch description, split the change to each
> > component (control, pcm and compress_offload), and resubmit?
> 
> I can definitely update this to contain a bit more detail. That being
> said, given the confusion about which bug this solves, do you still
> want me to split this up? The bug is not resolved without both PCM and
> card change for this bug which is hit in the delayed release, but they
> are for two different paths reported by KASAN.

It's rather for making changes easier to read.  The change for each
component seems completely individual and can be applied
independently.


> > Also, maybe it's worth to change snd_device_initialize() to take the
> > release (optional) argument, too, instead of setting it explicitly
> > afterwards at each place.  Already majority of callers will set own
> > release callbacks.  This can be done at one more additional patch at
> > last.
> 
> Sounds like a good idea, definitely an option once we get the
> underlying issue solved.

Now I've been reconsidering the problem, and thought of another
possible workaround.  We may add the card's refcount control for the
device init and release, so that we delay the actual resource free.
The basic idea is to take card->card_ref at the device init and unref
it at the actual device release callback.  Then the snd_card_free()
call is held until all those refcounted devices are released.

Below is a PoC patch (yes, this can be split, too ;)
A quick test on VM seems OK, but needs more reviews and tests.

It contains somewhat ugly conditional put_device() at the dev_free
ops.  We can make those a bit saner with some helpers later, too.

BTW, this approach may avoid another potential problem by the delayed
release; if we finish the driver remove without waiting for the actual
device releases, it may hit a code (the piece of the device release
callback) of the already removed module, and it's not guaranteed to be
present.
I'm not sure whether this really happens, but it's another thing to be
considered.


thanks,

Takashi

---
diff --git a/include/sound/core.h b/include/sound/core.h
index f6e0dd648b80..00c514a80a4a 100644
--- a/include/sound/core.h
+++ b/include/sound/core.h
@@ -239,7 +239,10 @@ extern struct dentry *sound_debugfs_root;
 
 void snd_request_card(int card);
 
-void snd_device_initialize(struct device *dev, struct snd_card *card);
+void __snd_device_initialize(struct device *dev, struct snd_card *card,
+			     bool with_ref);
+#define snd_device_initialize(dev, card) \
+	__snd_device_initialize(dev, card, false)
 
 int snd_register_device(int type, struct snd_card *card, int dev,
 			const struct file_operations *f_ops,
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index 30f73097447b..a29398cc9d27 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -1085,6 +1085,7 @@ static int snd_compress_dev_disconnect(struct snd_device *device)
 
 	compr = device->device_data;
 	snd_unregister_device(&compr->dev);
+	put_device(&compr->dev);
 	return 0;
 }
 
@@ -1158,7 +1159,8 @@ static int snd_compress_dev_free(struct snd_device *device)
 
 	compr = device->device_data;
 	snd_compress_proc_done(compr);
-	put_device(&compr->dev);
+	if (device->state != SNDRV_DEV_DISCONNECTED)
+		put_device(&compr->dev);
 	return 0;
 }
 
@@ -1189,7 +1191,7 @@ int snd_compress_new(struct snd_card *card, int device,
 
 	snd_compress_set_id(compr, id);
 
-	snd_device_initialize(&compr->dev, card);
+	__snd_device_initialize(&compr->dev, card, true);
 	dev_set_name(&compr->dev, "comprC%iD%i", card->number, device);
 
 	ret = snd_device_new(card, SNDRV_DEV_COMPRESS, compr, &ops);
diff --git a/sound/core/control.c b/sound/core/control.c
index 8386b53acdcd..bea501f20c4b 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -2351,7 +2351,9 @@ static int snd_ctl_dev_disconnect(struct snd_device *device)
 	up_read(&snd_ctl_layer_rwsem);
 	up_read(&card->controls_rwsem);
 
-	return snd_unregister_device(&card->ctl_dev);
+	snd_unregister_device(&card->ctl_dev);
+	put_device(&card->ctl_dev);
+	return 0;
 }
 
 /*
@@ -2373,7 +2375,8 @@ static int snd_ctl_dev_free(struct snd_device *device)
 	xa_destroy(&card->ctl_hash);
 #endif
 	up_write(&card->controls_rwsem);
-	put_device(&card->ctl_dev);
+	if (device->state != SNDRV_DEV_DISCONNECTED)
+		put_device(&card->ctl_dev);
 	return 0;
 }
 
@@ -2395,7 +2398,7 @@ int snd_ctl_create(struct snd_card *card)
 	if (snd_BUG_ON(card->number < 0 || card->number >= SNDRV_CARDS))
 		return -ENXIO;
 
-	snd_device_initialize(&card->ctl_dev, card);
+	__snd_device_initialize(&card->ctl_dev, card, true);
 	dev_set_name(&card->ctl_dev, "controlC%d", card->number);
 
 	err = snd_device_new(card, SNDRV_DEV_CONTROL, card, &ops);
diff --git a/sound/core/init.c b/sound/core/init.c
index baef2688d0cf..d122ff60c463 100644
--- a/sound/core/init.c
+++ b/sound/core/init.c
@@ -117,6 +117,10 @@ static int get_slot_from_bitmask(int mask, int (*check)(struct module *, int),
  */
 static void default_release(struct device *dev)
 {
+	struct snd_card *card = dev_get_drvdata(dev);
+
+	if (card)
+		snd_card_unref(card);
 }
 
 /**
@@ -124,15 +128,20 @@ static void default_release(struct device *dev)
  * @dev: device to initialize
  * @card: card to assign, optional
  */
-void snd_device_initialize(struct device *dev, struct snd_card *card)
+void __snd_device_initialize(struct device *dev, struct snd_card *card,
+			     bool with_ref)
 {
 	device_initialize(dev);
 	if (card)
 		dev->parent = &card->card_dev;
 	dev->class = &sound_class;
 	dev->release = default_release;
+	if (with_ref) {
+		dev_set_drvdata(dev, card);
+		get_device(&card->card_dev);
+	}
 }
-EXPORT_SYMBOL_GPL(snd_device_initialize);
+EXPORT_SYMBOL_GPL(__snd_device_initialize);
 
 static int snd_card_init(struct snd_card *card, struct device *parent,
 			 int idx, const char *xid, struct module *module,
diff --git a/sound/core/pcm.c b/sound/core/pcm.c
index 9d95e3731123..ccad084a359f 100644
--- a/sound/core/pcm.c
+++ b/sound/core/pcm.c
@@ -30,7 +30,7 @@ static DEFINE_MUTEX(register_mutex);
 static LIST_HEAD(snd_pcm_notify_list);
 #endif
 
-static int snd_pcm_free(struct snd_pcm *pcm);
+static int snd_pcm_free(struct snd_pcm *pcm, bool do_put);
 static int snd_pcm_dev_free(struct snd_device *device);
 static int snd_pcm_dev_register(struct snd_device *device);
 static int snd_pcm_dev_disconnect(struct snd_device *device);
@@ -650,7 +650,7 @@ int snd_pcm_new_stream(struct snd_pcm *pcm, int stream, int substream_count)
 	if (!substream_count)
 		return 0;
 
-	snd_device_initialize(&pstr->dev, pcm->card);
+	__snd_device_initialize(&pstr->dev, pcm->card, true);
 	pstr->dev.groups = pcm_dev_attr_groups;
 	pstr->dev.type = &pcm_dev_type;
 	dev_set_name(&pstr->dev, "pcmC%iD%i%c", pcm->card->number, pcm->device,
@@ -752,7 +752,7 @@ static int _snd_pcm_new(struct snd_card *card, const char *id, int device,
 	return 0;
 
 free_pcm:
-	snd_pcm_free(pcm);
+	snd_pcm_free(pcm, true);
 	return err;
 }
 
@@ -821,7 +821,7 @@ static void free_chmap(struct snd_pcm_str *pstr)
 	}
 }
 
-static void snd_pcm_free_stream(struct snd_pcm_str * pstr)
+static void snd_pcm_free_stream(struct snd_pcm_str * pstr, bool do_put)
 {
 	struct snd_pcm_substream *substream, *substream_next;
 #if IS_ENABLED(CONFIG_SND_PCM_OSS)
@@ -846,7 +846,7 @@ static void snd_pcm_free_stream(struct snd_pcm_str * pstr)
 	}
 #endif
 	free_chmap(pstr);
-	if (pstr->substream_count)
+	if (pstr->substream_count && do_put)
 		put_device(&pstr->dev);
 }
 
@@ -861,7 +861,7 @@ static void snd_pcm_free_stream(struct snd_pcm_str * pstr)
 #define pcm_call_notify(pcm, call) do {} while (0)
 #endif
 
-static int snd_pcm_free(struct snd_pcm *pcm)
+static int snd_pcm_free(struct snd_pcm *pcm, bool do_put)
 {
 	if (!pcm)
 		return 0;
@@ -870,8 +870,8 @@ static int snd_pcm_free(struct snd_pcm *pcm)
 	if (pcm->private_free)
 		pcm->private_free(pcm);
 	snd_pcm_lib_preallocate_free_for_all(pcm);
-	snd_pcm_free_stream(&pcm->streams[SNDRV_PCM_STREAM_PLAYBACK]);
-	snd_pcm_free_stream(&pcm->streams[SNDRV_PCM_STREAM_CAPTURE]);
+	snd_pcm_free_stream(&pcm->streams[SNDRV_PCM_STREAM_PLAYBACK], do_put);
+	snd_pcm_free_stream(&pcm->streams[SNDRV_PCM_STREAM_CAPTURE], do_put);
 	kfree(pcm);
 	return 0;
 }
@@ -879,7 +879,7 @@ static int snd_pcm_free(struct snd_pcm *pcm)
 static int snd_pcm_dev_free(struct snd_device *device)
 {
 	struct snd_pcm *pcm = device->device_data;
-	return snd_pcm_free(pcm);
+	return snd_pcm_free(pcm, device->state != SNDRV_DEV_DISCONNECTED);
 }
 
 int snd_pcm_attach_substream(struct snd_pcm *pcm, int stream,
@@ -1125,7 +1125,10 @@ static int snd_pcm_dev_disconnect(struct snd_device *device)
 
 	pcm_call_notify(pcm, n_disconnect);
 	for (cidx = 0; cidx < 2; cidx++) {
+		if (!pcm->streams[cidx].substream_count)
+			continue;
 		snd_unregister_device(&pcm->streams[cidx].dev);
+		put_device(&pcm->streams[cidx].dev);
 		free_chmap(&pcm->streams[cidx]);
 	}
 	mutex_unlock(&pcm->open_mutex);

Curtis Malainey Aug. 4, 2023, 7:17 p.m. UTC | #10

On Fri, Aug 4, 2023 at 1:58 AM Takashi Iwai <tiwai@suse.de> wrote:
>
> Now I've been reconsidering the problem, and thought of another
> possible workaround.  We may add the card's refcount control for the
> device init and release, so that we delay the actual resource free.
> The basic idea is to take card->card_ref at the device init and unref
> it at the actual device release callback.  Then the snd_card_free()
> call is held until all those refcounted devices are released.
>
> Below is a PoC patch (yes, this can be split, too ;)
> A quick test on VM seems OK, but needs more reviews and tests.
>
> It contains somewhat ugly conditional put_device() at the dev_free
> ops.  We can make those a bit saner with some helpers later, too.
>
> BTW, this approach may avoid another potential problem by the delayed
> release; if we finish the driver remove without waiting for the actual
> device releases, it may hit a code (the piece of the device release
> callback) of the already removed module, and it's not guaranteed to be
> present.
> I'm not sure whether this really happens, but it's another thing to be
> considered.
>
>
> thanks,
>
> Takashi
>
> ---
> diff --git a/include/sound/core.h b/include/sound/core.h
> index f6e0dd648b80..00c514a80a4a 100644
> --- a/include/sound/core.h
> +++ b/include/sound/core.h

Unfortunately it doesn't hold up in my testing, hit the devm vs device
race bug after a little over 30min of hammering snd-dummy (on top of
your for-next branch)

[ 2214.013410] kobject: 'BAT0' (000000006eb2300b): kobject_uevent_env
[ 2214.013433] kobject: 'BAT0' (000000006eb2300b): fill_kobj_path:
path = '/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:01/PNP0C09:00/PNP0C0A:00/power_supply/BAT0'
[ 2214.142604] kobject: 'card1' (00000000f5621ef1): kobject_cleanup,
parent 0000000000000000
[ 2214.142627] kobject: 'card1' (00000000f5621ef1): calling ktype release
[ 2214.145618] kobject: 'snd_dummy.0' (00000000965e7bc3): kobject_uevent_env
[ 2214.145648] kobject: 'snd_dummy.0' (00000000965e7bc3):
fill_kobj_path: path = '/devices/platform/snd_dummy.0'
[ 2214.145773] kobject: 'snd_dummy.0' (00000000965e7bc3): kobject_uevent_env
[ 2214.145793] kobject: 'snd_dummy.0' (00000000965e7bc3):
fill_kobj_path: path = '/devices/platform/snd_dummy.0'
[ 2214.145867] kobject: 'snd_dummy.0' (00000000965e7bc3):
kobject_release, parent 0000000000000000 (delayed 4000)
[ 2214.145941] kobject: 'snd_dummy' (000000003e14c027):
kobject_release, parent 0000000092654d15 (delayed 2000)
[ 2214.146355] ==================================================================
[ 2214.146363] kobject: 'drivers' (000000007b7f6032): kobject_release,
parent 000000009dc04f8f (delayed 2000)
[ 2214.146364] BUG: KASAN: slab-use-after-free in release_card_device+0x86/0xd0
[ 2214.146384] kobject: 'holders' (000000000b4379d6): kobject_release,
parent 000000009dc04f8f (delayed 2000)
[ 2214.146384] Read of size 1 at addr ffff888184a0c949 by task kworker/9:2/1012

[ 2214.146403] CPU: 9 PID: 1012 Comm: kworker/9:2 Tainted: G     U  W
        6.5.0-rc3-00286-gb6697501cf3e-dirty #19
27c5c3da575c6eb45fc95c08db2c659f33df80d3
[ 2214.146417] Hardware name: Google Anahera/Anahera, BIOS
Google_Anahera.14505.315.0 12/02/2022
[ 2214.146425] Workqueue: events kobject_delayed_cleanup
[ 2214.146439] Call Trace:
[ 2214.146446]  <TASK>
[ 2214.146447] kobject: 'notes' (00000000f7709af3): kobject_release,
parent 000000009dc04f8f (delayed 1000)
[ 2214.146452]  dump_stack_lvl+0x91/0xd0
[ 2214.146466]  print_report+0x15b/0x4d0
[ 2214.146478]  ? __virt_addr_valid+0xbb/0x130
[ 2214.146491]  ? kasan_addr_to_slab+0x11/0x80
[ 2214.146504]  ? release_card_device+0x86/0xd0
[ 2214.146516]  kasan_report+0xb1/0xf0
[ 2214.146526]  ? release_card_device+0x86/0xd0
[ 2214.146539]  ? _raw_spin_unlock_irqrestore+0x1b/0x40
[ 2214.146552]  release_card_device+0x86/0xd0
[ 2214.146565]  device_release+0x66/0x110
[ 2214.146576]  kobject_delayed_cleanup+0x133/0x140
[ 2214.146587]  process_one_work+0x3e3/0x680
[ 2214.146600]  worker_thread+0x487/0x6d0
[ 2214.146613]  ? __pfx_worker_thread+0x10/0x10
[ 2214.146624]  kthread+0x199/0x1c0
[ 2214.146634]  ? __pfx_worker_thread+0x10/0x10
[ 2214.146644]  ? __pfx_kthread+0x10/0x10
[ 2214.146655]  ret_from_fork+0x3c/0x60
[ 2214.146667]  ? __pfx_kthread+0x10/0x10
[ 2214.146677]  ret_from_fork_asm+0x1b/0x30
[ 2214.146689] RIP: 0000:0x0
[ 2214.146703] Code: Unable to access opcode bytes at 0xffffffffffffffd6.
[ 2214.146710] RSP: 0000:0000000000000000 EFLAGS: 00000000 ORIG_RAX:
0000000000000000
[ 2214.146723] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
[ 2214.146731] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
[ 2214.146739] RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000
[ 2214.146747] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
[ 2214.146760] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
[ 2214.146769]  </TASK>

[ 2214.146779] Allocated by task 12068:
[ 2214.146785]  kasan_set_track+0x4f/0x80
[ 2214.146797]  __kasan_kmalloc+0x92/0xb0
[ 2214.146804]  __kmalloc_node_track_caller+0x72/0x140
[ 2214.146815]  __devres_alloc_node+0x50/0xc0
[ 2214.146825]  snd_devm_card_new+0x5a/0xe0
[ 2214.146835]  snd_dummy_probe+0x91/0x660 [snd_dummy]
[ 2214.146852]  platform_probe+0xb5/0xf0
[ 2214.146861]  really_probe+0x177/0x3c0
[ 2214.146871]  __driver_probe_device+0xec/0x1b0
[ 2214.146881]  driver_probe_device+0x4f/0xd0
[ 2214.146890]  __device_attach_driver+0xd0/0xf0
[ 2214.146900]  bus_for_each_drv+0xc6/0x120
[ 2214.146909]  __device_attach+0x15c/0x210
[ 2214.146918]  bus_probe_device+0x5b/0xe0
[ 2214.146926]  device_add+0x462/0x6d0
[ 2214.146936]  platform_device_add+0x27a/0x360
[ 2214.146945]  platform_device_register_full+0x1e7/0x1f0
[ 2214.146954]  0xffffffffc0ec0118
[ 2214.146961]  do_one_initcall+0x153/0x370
[ 2214.146970]  do_init_module+0x126/0x350
[ 2214.146979]  __se_sys_finit_module+0x2d7/0x460
[ 2214.146988]  do_syscall_64+0x4c/0xa0
[ 2214.147000]  entry_SYSCALL_64_after_hwframe+0x6e/0xd8

[ 2214.147028] Freed by task 12072:
[ 2214.147038]  kasan_set_track+0x4f/0x80
[ 2214.147054]  kasan_save_free_info+0x2c/0x50
[ 2214.147069]  ____kasan_slab_free+0x12c/0x180
[ 2214.147086]  __kmem_cache_free+0x104/0x2a0
[ 2214.147103]  release_nodes+0x69/0x80
[ 2214.147119]  devres_release_all+0xbe/0x100
[ 2214.147135]  device_unbind_cleanup+0x14/0xd0
[ 2214.147152]  device_release_driver_internal+0x12c/0x180
[ 2214.147169]  bus_remove_device+0x158/0x190
[ 2214.147183]  device_del+0x2dd/0x490
[ 2214.147198]  platform_device_del+0x38/0xf0
[ 2214.147214]  platform_device_unregister+0x16/0x40
[ 2214.147229]  snd_dummy_unregister_all+0x26/0x50 [snd_dummy]
[ 2214.147256]  __se_sys_delete_module+0x25a/0x380
[ 2214.147273]  do_syscall_64+0x4c/0xa0
[ 2214.147288]  entry_SYSCALL_64_after_hwframe+0x6e/0xd8

[ 2214.147311] Last potentially related work creation:
[ 2214.147320]  kasan_save_stack+0x3e/0x60
[ 2214.147335]  __kasan_record_aux_stack+0xaf/0xc0
[ 2214.147350]  insert_work+0x36/0x160
[ 2214.147365]  __queue_work+0x54d/0x5c0
[ 2214.147380]  call_timer_fn+0x9f/0x190
[ 2214.147394]  __run_timers+0x427/0x4c0
[ 2214.147408]  run_timer_softirq+0x21/0x40
[ 2214.147421]  __do_softirq+0x164/0x344

[ 2214.147443] Second to last potentially related work creation:
[ 2214.147451]  kasan_save_stack+0x3e/0x60
[ 2214.147466]  __kasan_record_aux_stack+0xaf/0xc0
[ 2214.147480]  insert_work+0x36/0x160
[ 2214.147495]  __queue_work+0x54d/0x5c0
[ 2214.147509]  call_timer_fn+0x9f/0x190
[ 2214.147522]  __run_timers+0x427/0x4c0
[ 2214.147535]  run_timer_softirq+0x21/0x40

<snipped due to grep context being set at 100 lines>

I was talking with Stephen Boyd about this bug and his recommendation
was to keep snd_card always out of devm and just allocate a pointer in
devm to snd_card to puppet it as if it was managed via devm and just
reference count rather than release the card so that its always
handled through device->release. What do you think? This would go
alongside the current patch proposed.

Takashi Iwai Aug. 5, 2023, 8:09 a.m. UTC | #11

On Fri, 04 Aug 2023 21:17:56 +0200,
Curtis Malainey wrote:
> 
> On Fri, Aug 4, 2023 at 1:58 AM Takashi Iwai <tiwai@suse.de> wrote:
> >
> > Now I've been reconsidering the problem, and thought of another
> > possible workaround.  We may add the card's refcount control for the
> > device init and release, so that we delay the actual resource free.
> > The basic idea is to take card->card_ref at the device init and unref
> > it at the actual device release callback.  Then the snd_card_free()
> > call is held until all those refcounted devices are released.
> >
> > Below is a PoC patch (yes, this can be split, too ;)
> > A quick test on VM seems OK, but needs more reviews and tests.
> >
> > It contains somewhat ugly conditional put_device() at the dev_free
> > ops.  We can make those a bit saner with some helpers later, too.
> >
> > BTW, this approach may avoid another potential problem by the delayed
> > release; if we finish the driver remove without waiting for the actual
> > device releases, it may hit a code (the piece of the device release
> > callback) of the already removed module, and it's not guaranteed to be
> > present.
> > I'm not sure whether this really happens, but it's another thing to be
> > considered.
> >
> >
> > thanks,
> >
> > Takashi
> >
> > ---
> > diff --git a/include/sound/core.h b/include/sound/core.h
> > index f6e0dd648b80..00c514a80a4a 100644
> > --- a/include/sound/core.h
> > +++ b/include/sound/core.h
> 
> Unfortunately it doesn't hold up in my testing, hit the devm vs device
> race bug after a little over 30min of hammering snd-dummy (on top of
> your for-next branch)
(snip)
> I was talking with Stephen Boyd about this bug and his recommendation
> was to keep snd_card always out of devm and just allocate a pointer in
> devm to snd_card to puppet it as if it was managed via devm and just
> reference count rather than release the card so that its always
> handled through device->release. What do you think? This would go
> alongside the current patch proposed.

Indeed, that's another issue about devres vs delayed kobj release.
A quick fix would be something like below, I suppose.
(note: totally untested)


thanks,

Takashi

--- a/sound/core/init.c
+++ b/sound/core/init.c
@@ -188,7 +188,10 @@ EXPORT_SYMBOL(snd_card_new);
 
 static void __snd_card_release(struct device *dev, void *data)
 {
-	snd_card_free(data);
+	struct snd_card **card_p = data;
+
+	if (card_p)
+		snd_card_free(*card_p);
 }
 
 /**
@@ -221,21 +224,25 @@ int snd_devm_card_new(struct device *parent, int idx, const char *xid,
 		      struct snd_card **card_ret)
 {
 	struct snd_card *card;
+	struct snd_card **card_devres;
 	int err;
 
 	*card_ret = NULL;
-	card = devres_alloc(__snd_card_release, sizeof(*card) + extra_size,
-			    GFP_KERNEL);
+	card_devres = devres_alloc(__snd_card_release, sizeof(void *), GFP_KERNEL);
+	if (!card_devres)
+		return -ENOMEM;
+	devres_add(parent, card_devres);
+
+	card = kzalloc(sizeof(*card) + extra_size, GFP_KERNEL);
 	if (!card)
 		return -ENOMEM;
+
 	card->managed = true;
 	err = snd_card_init(card, parent, idx, xid, module, extra_size);
-	if (err < 0) {
-		devres_free(card); /* in managed mode, we need to free manually */
+	if (err < 0)
 		return err;
-	}
 
-	devres_add(parent, card);
+	*card_devres = card;
 	*card_ret = card;
 	return 0;
 }
@@ -295,8 +302,7 @@ static int snd_card_init(struct snd_card *card, struct device *parent,
 		mutex_unlock(&snd_card_mutex);
 		dev_err(parent, "cannot find the slot for index %d (range 0-%i), error: %d\n",
 			 idx, snd_ecards_limit - 1, err);
-		if (!card->managed)
-			kfree(card); /* manually free here, as no destructor called */
+		kfree(card); /* manually free here, as no destructor called */
 		return err;
 	}
 	set_bit(idx, snd_cards_lock);		/* lock it */
@@ -592,8 +598,7 @@ static int snd_card_do_free(struct snd_card *card)
 #endif
 	if (card->release_completion)
 		complete(card->release_completion);
-	if (!card->managed)
-		kfree(card);
+	kfree(card);
 	return 0;
 }

Takashi Iwai Aug. 6, 2023, 6:32 p.m. UTC | #12

On Sat, 05 Aug 2023 10:09:58 +0200,
Takashi Iwai wrote:
> 
> On Fri, 04 Aug 2023 21:17:56 +0200,
> Curtis Malainey wrote:
> > 
> > On Fri, Aug 4, 2023 at 1:58 AM Takashi Iwai <tiwai@suse.de> wrote:
> > >
> > > Now I've been reconsidering the problem, and thought of another
> > > possible workaround.  We may add the card's refcount control for the
> > > device init and release, so that we delay the actual resource free.
> > > The basic idea is to take card->card_ref at the device init and unref
> > > it at the actual device release callback.  Then the snd_card_free()
> > > call is held until all those refcounted devices are released.
> > >
> > > Below is a PoC patch (yes, this can be split, too ;)
> > > A quick test on VM seems OK, but needs more reviews and tests.
> > >
> > > It contains somewhat ugly conditional put_device() at the dev_free
> > > ops.  We can make those a bit saner with some helpers later, too.
> > >
> > > BTW, this approach may avoid another potential problem by the delayed
> > > release; if we finish the driver remove without waiting for the actual
> > > device releases, it may hit a code (the piece of the device release
> > > callback) of the already removed module, and it's not guaranteed to be
> > > present.
> > > I'm not sure whether this really happens, but it's another thing to be
> > > considered.
> > >
> > >
> > > thanks,
> > >
> > > Takashi
> > >
> > > ---
> > > diff --git a/include/sound/core.h b/include/sound/core.h
> > > index f6e0dd648b80..00c514a80a4a 100644
> > > --- a/include/sound/core.h
> > > +++ b/include/sound/core.h
> > 
> > Unfortunately it doesn't hold up in my testing, hit the devm vs device
> > race bug after a little over 30min of hammering snd-dummy (on top of
> > your for-next branch)
> (snip)
> > I was talking with Stephen Boyd about this bug and his recommendation
> > was to keep snd_card always out of devm and just allocate a pointer in
> > devm to snd_card to puppet it as if it was managed via devm and just
> > reference count rather than release the card so that its always
> > handled through device->release. What do you think? This would go
> > alongside the current patch proposed.
> 
> Indeed, that's another issue about devres vs delayed kobj release.
> A quick fix would be something like below, I suppose.
> (note: totally untested)

Scratch it.  It's still obviously broken.
Will cook more proper patches later.


Takashi

Takashi Iwai Aug. 7, 2023, 1:43 p.m. UTC | #13

On Sun, 06 Aug 2023 20:32:06 +0200,
Takashi Iwai wrote:
> 
> On Sat, 05 Aug 2023 10:09:58 +0200,
> Takashi Iwai wrote:
> > 
> > On Fri, 04 Aug 2023 21:17:56 +0200,
> > Curtis Malainey wrote:
> > > 
> > > On Fri, Aug 4, 2023 at 1:58 AM Takashi Iwai <tiwai@suse.de> wrote:
> > > >
> > > > Now I've been reconsidering the problem, and thought of another
> > > > possible workaround.  We may add the card's refcount control for the
> > > > device init and release, so that we delay the actual resource free.
> > > > The basic idea is to take card->card_ref at the device init and unref
> > > > it at the actual device release callback.  Then the snd_card_free()
> > > > call is held until all those refcounted devices are released.
> > > >
> > > > Below is a PoC patch (yes, this can be split, too ;)
> > > > A quick test on VM seems OK, but needs more reviews and tests.
> > > >
> > > > It contains somewhat ugly conditional put_device() at the dev_free
> > > > ops.  We can make those a bit saner with some helpers later, too.
> > > >
> > > > BTW, this approach may avoid another potential problem by the delayed
> > > > release; if we finish the driver remove without waiting for the actual
> > > > device releases, it may hit a code (the piece of the device release
> > > > callback) of the already removed module, and it's not guaranteed to be
> > > > present.
> > > > I'm not sure whether this really happens, but it's another thing to be
> > > > considered.
> > > >
> > > >
> > > > thanks,
> > > >
> > > > Takashi
> > > >
> > > > ---
> > > > diff --git a/include/sound/core.h b/include/sound/core.h
> > > > index f6e0dd648b80..00c514a80a4a 100644
> > > > --- a/include/sound/core.h
> > > > +++ b/include/sound/core.h
> > > 
> > > Unfortunately it doesn't hold up in my testing, hit the devm vs device
> > > race bug after a little over 30min of hammering snd-dummy (on top of
> > > your for-next branch)
> > (snip)
> > > I was talking with Stephen Boyd about this bug and his recommendation
> > > was to keep snd_card always out of devm and just allocate a pointer in
> > > devm to snd_card to puppet it as if it was managed via devm and just
> > > reference count rather than release the card so that its always
> > > handled through device->release. What do you think? This would go
> > > alongside the current patch proposed.
> > 
> > Indeed, that's another issue about devres vs delayed kobj release.
> > A quick fix would be something like below, I suppose.
> > (note: totally untested)
> 
> Scratch it.  It's still obviously broken.
> Will cook more proper patches later.

Now RFC patchset is ready.
I'll submit for testing in another thread.


Takashi

sound: core: fix device ownership model in card and pcm

Commit Message

Comments

Patch