Re: [PATCH v2 3/3] soundwire: intel_auxdevice: Don't disable IRQs before removing children

[Charles Keepax <ckeepax@opensource.cirrus.com>]

On Fri, Oct 09, 2026 at 09:23:17AM +0000, Richard Patel wrote:
> On Fri, Oct 09, 2026 at 10:03:25AM +0100, Charles Keepax wrote:
> > On Thu, Oct 08, 2026 at 11:11:32PM +0000, Richard Patel wrote:
> > > On Thu, Oct 08, 2026 at 01:41:06PM +0100, Charles Keepax wrote:
> > > > On Mon, Oct 05, 2026 at 02:11:47PM +0100, Charles Keepax wrote:
> > > > > On Mon, Oct 05, 2026 at 11:32:05AM +0100, Charles Keepax wrote:
> > > > > > On Sun, Oct 04, 2026 at 12:29:58PM +0000, Richard Patel wrote:
> > > > > > > On Fri, Sep 25, 2026 at 04:42:16PM +0100, Charles Keepax wrote:
> > > > Ok found some time to look at this properly I think this is all
> > > > fine. sdw_intel_exit() first calls sdw_intel_cleanup() which will
> > > > eventually call sdw_cdns_enable_interrupt(..., false), which
> > > > should disable the SoundWire IRQs. Then sdw_intel_exit() frees
> > > > the ctx, whilst at that point whilst the IRQ is still registered
> > > > one should no longer be able to see soundwire IRQs, so you shouldn't
> > > > get a dereferencing of ctx.
> > >
> > > On my Galaxy Book6, I was able to get a ctx UAF with your v2 patch set
> > > by adding a sleep.
> > Hmm... yeah, I guess the masking ensures a new IRQ can't come in
> > but nothing ensures a currently running IRQ is synchronised in.
> > Well assuming the masking does actually prevent an IRQ coming in.
> > 
> > That is a little awkward, normally freeing the IRQ would
> > synchronise it but as the "IRQ" here is done as a pile of
> > callbacks that doesn't happen. We could do a manual sync on the
> > IRQ but that feels like a bit of a layering violation, since
> > the actually IRQ is several layers away in another part of the
> > code. We could add some flags/completions such that we can wait
> > for the current IRQ to finish but feels a bit like adding code
> > that shouldn't exist. I think the correct solution is probably
> > to switch the handling over to the IRQ framework.
> > 
> > I am going to go for the theory this is not directly a problem
> > with this series since the problem exists unchanged before and
> > after the series. So lets not block this stuff on it, but I will
> 
> Yep, sounds good :-) Thanks again for the fixes. 
> 
> > try to find time to start porting more of the handling over to
> > the IRQ framework, or happy to help review if you would rather
> > take a run at it.
> 
> I was going to defer kfree(ctx) via RCU, what do you think?

My slight concern would be it is tackling the UAF directly, but
really the problem here is the IRQ handler is still running after
the link device has been destroyed. It seems quite likely you can
end up with other problems, which we may have to add other
mitigations for later. So doing something to ensure the IRQ
thread has completed (or at least the SoundWire part there of) in
intel_link_remove after the IRQ is disabled feels more robust.

However, that said if the changes are small and neat then it
certainly improves the current situation, so I am not totally
against the idea.

Thanks,
Charles