linux-kernel - Re: [PATCH] soundwire: intel_auxdevice: Don't disable IRQs before removing children

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <Z2WwdxuPpnNZUBrC@opensource.cirrus.com>
Date: Fri, 20 Dec 2024 17:59:19 +0000
From: Charles Keepax <ckeepax@...nsource.cirrus.com>
To: Pierre-Louis Bossart <pierre-louis.bossart@...ux.dev>
CC: Richard Fitzgerald <rf@...nsource.cirrus.com>, <vkoul@...nel.org>,
        <peter.ujfalusi@...ux.intel.com>, <yung-chuan.liao@...ux.intel.com>,
        <sanyog.r.kale@...el.com>, <linux-sound@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <patches@...nsource.cirrus.com>
Subject: Re: [PATCH] soundwire: intel_auxdevice: Don't disable IRQs before
 removing children

On Thu, Dec 19, 2024 at 10:27:53AM +0000, Charles Keepax wrote:
> On Wed, Dec 18, 2024 at 04:40:22PM -0500, Pierre-Louis Bossart wrote:
> > Having looked at the code in more details, I think there are other issues,
> > see e.g. this part of the code called from snd_bus_master_delete().
> > 
> > static int sdw_delete_slave(struct device *dev, void *data)
> > {
> > 	struct sdw_slave *slave = dev_to_sdw_dev(dev);
> > 	struct sdw_bus *bus = slave->bus;
> > 
> > 	pm_runtime_disable(dev);
> > 
> > 	sdw_slave_debugfs_exit(slave);
> > 
> > 	mutex_lock(&bus->bus_lock);
> > 
> > 	if (slave->dev_num) { /* clear dev_num if assigned */
> > 		clear_bit(slave->dev_num, bus->assigned);
> > 		if (bus->ops && bus->ops->put_device_num)
> > 			bus->ops->put_device_num(bus, slave);
> > 	}
> > 
> > So at this point an interaction with the device is not longer possible, even
> > if the Cadence interrupts are kept active, since there's no valid device
> > number to use...
> > 
> > 	list_del_init(&slave->node);
> > 	mutex_unlock(&bus->bus_lock);
> > 
> > ... but this is where the .remove() will take place.
> > 
> > 	device_unregister(dev);
> > 	return 0;
> > }
> > 
> > What am I missing?
> 
> Hmm... yes that is a good spot, I will investigate that further.
> Certainly I do see these operations happening in the wrong order
> but it doesn't seem to cause the register transactions to fail.
> Most likely it is best to reverse these two, it makes sense
> to not clear the device number until we are finished with the
> device, but would be good to understand what is going on first.

Ok, so yeah nothing on the read/write path checks assigned. So yeah
it has marked the device as not being assigned but that doesn't
actually stop anything from communicating with the device, the
dev_num is still the same one the hardware has. Assigned is only
checked when handling slave status events.

So I think your point is valid this code should also be changed
although the only way it can currently go wrong is if a new device
registered on the bus at exactly this moment and was assigned the
dev_num of the old device.

Likely will be the new year before I get the time to think
through the details.

Thanks,
Charles