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

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

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.
>
>   BUG: unable to handle page fault for address: fffffffffffffff8
>   Oops: Oops: 0000 [#1] SMP NOPTI
>   CPU: 5 UID: 0 PID: 34491 Comm: irq/165-AudioDS Kdump: loaded Tainted: G           OE       7.3.0-rc6-ibt+ #2 PREEMPT(lazy)
>   RIP: 0010:sdw_intel_thread+0x3c/0x70 [soundwire_intel]
>   Call Trace:
>    <TASK>
>    hda_dsp_interrupt_thread+0x97/0x320 [snd_sof_intel_hda_generic]
>    irq_thread_fn+0x23/0x60
>    irq_thread+0xc7/0x190
>    kthread+0xe5/0x120
>    </TASK>
>
> --Richard
>
> ---
> diff --git a/drivers/soundwire/intel_init.c b/drivers/soundwire/intel_init.c
> index a7437cd42028..66a08dad7281 100644
> --- a/drivers/soundwire/intel_init.c
> +++ b/drivers/soundwire/intel_init.c
> @@ -155,6 +155,8 @@ irqreturn_t sdw_intel_thread(int irq, void *dev_id)
>  	struct sdw_intel_ctx *ctx = dev_id;
>  	struct sdw_intel_link_res *link;
>
> +	msleep(5000);
> +
>  	mutex_lock(&ctx->link_lock);
>  	list_for_each_entry(link, &ctx->link_list, list)
>  		sdw_cdns_irq(irq, link->cdns);
> @@ -393,6 +395,7 @@ void sdw_intel_exit(struct sdw_intel_ctx *ctx)
>  	sdw_intel_cleanup(ctx);
>  	kfree(ctx->peripherals);
>  	kfree(ctx->ldev);
> +	memset(ctx, 0, sizeof(*ctx));
>  	kfree(ctx);
>  }
>  EXPORT_SYMBOL_NS(sdw_intel_exit, "SOUNDWIRE_INTEL_INIT");

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
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.

Thanks,
Charles