[PATCH] slimbus: messaging: hold lock until transaction is completed

[PATCH] slimbus: messaging: hold lock until transaction is completed

[Changyul Lee]

slim_msg_response() drops lock after idr_find() and dereferences txn.
If the requester times out in the meantime, it frees the TID and
returns, so the handler writes the reply into msg->rbuf and calls
complete() on txn->comp, both of which have already benn freed.

Keep lock held until complete(txn->comp) finishes, so a requester
in slim_free_txn_tid() and cannot return while txn is still in use.
Call idr_remove() because slim_free_txn_tid() takes the same lock.

Fixes: afbdcc7c384b ("slimbus: Add messaging APIs to slimbus framework")

Signed-off-by: Changyul Lee <lcy8047@gmail.com>
---
 drivers/slimbus/messaging.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/slimbus/messaging.c b/drivers/slimbus/messaging.c
index e2dbe4a66b70..aa98f5480ba4 100644
--- a/drivers/slimbus/messaging.c
+++ b/drivers/slimbus/messaging.c
@@ -29,22 +29,24 @@ void slim_msg_response(struct slim_controller *ctrl, u8 *reply, u8 tid, u8 len)
 
 	spin_lock_irqsave(&ctrl->txn_lock, flags);
 	txn = idr_find(&ctrl->tid_idr, tid);
-	spin_unlock_irqrestore(&ctrl->txn_lock, flags);
-
-	if (txn == NULL)
+	if (txn == NULL) {
+		spin_unlock_irqrestore(&ctrl->txn_lock, flags);
 		return;
+	}
 
 	msg = txn->msg;
 	if (msg == NULL || msg->rbuf == NULL) {
+		spin_unlock_irqrestore(&ctrl->txn_lock, flags);
 		dev_err(ctrl->dev, "Got response to invalid TID:%d, len:%d\n",
 				tid, len);
 		return;
 	}
 
-	slim_free_txn_tid(ctrl, txn);
+	idr_remove(&ctrl->tid_idr, txn->tid);
 	memcpy(msg->rbuf, reply, len);
 	if (txn->comp)
 		complete(txn->comp);
+	spin_unlock_irqrestore(&ctrl->txn_lock, flags);
 
 	/* Remove runtime-pm vote now that response was received for TID txn */
 	pm_runtime_mark_last_busy(ctrl->dev);
-- 
2.43.0

Re: [PATCH] slimbus: messaging: hold lock until transaction is completed

[Greg Kroah-Hartman]

On Fri, Sep 11, 2026 at 12:59:11AM -0400, Changyul Lee wrote:
> slim_msg_response() drops lock after idr_find() and dereferences txn.
> If the requester times out in the meantime, it frees the TID and
> returns, so the handler writes the reply into msg->rbuf and calls
> complete() on txn->comp, both of which have already benn freed.
> 
> Keep lock held until complete(txn->comp) finishes, so a requester
> in slim_free_txn_tid() and cannot return while txn is still in use.
> Call idr_remove() because slim_free_txn_tid() takes the same lock.
> 
> Fixes: afbdcc7c384b ("slimbus: Add messaging APIs to slimbus framework")
> 
> Signed-off-by: Changyul Lee <lcy8047@gmail.com>

No blank line between these please.

And how was this found and tested?

thanks,

greg k-h

Re: [PATCH] slimbus: messaging: hold lock until transaction is completed

[Changyul Lee]

On Fri, Sep 11, 2026 at 08:00:28AM +0200, Greg Kroah-Hartman wrote:
> On Fri, Sep 11, 2026 at 12:59:11AM -0400, Changyul Lee wrote:
> > Fixes: afbdcc7c384b ("slimbus: Add messaging APIs to slimbus framework")
> >
> > Signed-off-by: Changyul Lee <lcy8047@gmail.com>
>
> No blank line between these please.

Will fix in v2.

> And how was this found and tested?

Sorry for the delayed reply. I wanted to reproduce the issue on real
hardware in addition to QEMU, and obtaining the board took longer than
expected.

I found the issue using a static analysis tool I developed. The tool
reported that slim_msg_response() accesses txn after releasing
ctrl->txn_lock.

I also used LLM assistants to help verify the report. I reviewed their
output and ran the tests below myself.

The race window is small because slim_msg_response() accesses txn only
a few instructions after releasing the lock. However, the bug can occur
when a response arrives just before the request times out. The request
can then return while slim_msg_response() is still accessing txn.

I tested this in QEMU and on a DragonBoard 845c. Both tests use a
test-only delay to widen the race window.

1) QEMU

I first reproduced the issue in QEMU on v7.3 (c03c1a14507d), x86_64,
with 2 CPUs, CONFIG_KASAN=y, CONFIG_KASAN_INLINE=y, and
CONFIG_KASAN_STACK=y.

QEMU has no SLIMbus controller, so a test module registers a dummy
controller using slim_register_controller(). The controller's
->xfer_msg() only records the TID. A kthread on CPU 0 calls
slim_readb() and waits for the response. A kthread on CPU 1 calls
slim_msg_response() with the recorded TID.

A response arriving after the request has timed out does not trigger
the bug because the timeout path has already removed the TID, and
idr_find() returns NULL. The response must instead find the transaction
before the timeout path removes it, but access txn after the request
has returned.

The request timeout is txn->rl + HZ milliseconds. With HZ=1000, it is
about 1000 ms. To reproduce the race, I added a 1200 ms delay after
releasing txn_lock, between idr_find() and the access to txn.

Without the fix patch, slim_readb() returns -110 first. Then
slim_msg_response() accesses txn from the stack frame of the returned
request, and KASAN reports a use-after-free.

  BUG: KASAN: use-after-free in slim_msg_response+0x29e/0x2b0 [slimbus]
  Read of size 8 at addr ffff88800340fd20 by task slim091-resp/72
  Call Trace:
   slim_msg_response+0x29e/0x2b0 [slimbus]
   resp_fn+0x9d/0xc0 [slim091_test]
   kthread+0x32a/0x410
   ret_from_fork+0x365/0x570

With the fix patch, there is no KASAN report or oops. The timeout
path waits for txn_lock in slim_free_txn_tid(), so slim_readb()
returns -110 only after slim_msg_response() has called complete()
and released the lock.

2) DragonBoard 845c

Since the QEMU test uses a dummy controller, I also reproduced the issue
on a DragonBoard 845c with a Qualcomm SLIMbus controller (qcom-ngd-ctrl)
and a WCD9340 codec.

The board runs v7.3 on arm64 with CONFIG_HZ=250, so the request timeout
is about 250 ms. I added mdelay(500) after releasing txn_lock following
a idr_find() in the unpatched kernel. A module parameter controls the
delay after boot.

I enabled the delay and triggered read requests through regmap debugfs
by reading a WCD9340 register.

  echo 1 > /sys/module/slimbus/parameters/test_delay
  echo 1 > /sys/kernel/debug/regmap/217:250:1:0/cache_bypass
  dd if=/sys/kernel/debug/regmap/217:250:1:0/registers of=/dev/null bs=9 count=1

On kernel A (without the fix patch, with KASAN), the read crashed in
memcpy(). The msg pointer was read from the stack frame of the returned
request, so msg->rbuf contained an invalid address.

  Unable to handle kernel paging request at virtual address 17ffff3dd2800001
  CM = 0, WnR = 1, TnD = 0, TagAccess = 0
  pc : __pi_memcpy_generic+0x7c/0x22c
  Call trace:
   __pi_memcpy_generic+0x7c/0x22c (P)
   slim_msg_response+0x118/0x2a0
   qcom_slim_ngd_rx_msgq_cb+0x16c/0x56c [slim_qcom_ngd_ctrl]
   vchan_complete+0x400/0x5e8

On kernel B (without the fix patch, without KASAN), the read also
crashed. The txn->msg pointer was read from the stack frame of the
returned request, and its value was 1. The kernel then crashed while
accessing msg->rbuf at address 0x9 (1 + 8).

  Unable to handle kernel NULL pointer dereference at virtual address
0000000000000009
  pc : slim_msg_response+0x74/0x140
  Call trace:
   slim_msg_response+0x74/0x140 (P)
   qcom_slim_ngd_rx_msgq_cb+0x8c/0x1a8 [slim_qcom_ngd_ctrl]
   vchan_complete+0x1f8/0x228
   tasklet_action_common+0x128/0x180
  Kernel panic - not syncing: Oops: Fatal exception in interrupt

On kernel C (with the fix patch and KASAN, using the same test delay),
I ran 7 reads followed by about 170 seconds of continuous reads, with
196 timeouts in total. There was no oops or KASAN report in
slim_msg_response(), and "Got response to invalid TID" did not appear.

I should have included the discovery and testing details in the
original submission. I'll include them in future reports and respond
to review comments more promptly.

I will send v2 with the blank line removed, a summary of the
reproduction and testing results in the commit message, and an
Assisted-by: LLM tag. I'll include the detailed test results below
the --- separator.

Thanks,
Changyul


2026년 10월 22일 (목) 오후 1:05, Changyul Lee <lcy8047@gmail.com>님이 작성:
>
> On Fri, Sep 11, 2026 at 08:00:28AM +0200, Greg Kroah-Hartman wrote:
> > On Fri, Sep 11, 2026 at 12:59:11AM -0400, Changyul Lee wrote:
> > > Fixes: afbdcc7c384b ("slimbus: Add messaging APIs to slimbus framework")
> > >
> > > Signed-off-by: Changyul Lee <lcy8047@gmail.com>
> >
> > No blank line between these please.
>
> Will fix in v2.
>
> > And how was this found and tested?
>
> Sorry for the delayed reply. I wanted to reproduce the issue on real
> hardware in addition to QEMU, and obtaining the board took longer than
> expected.
>
> I found the issue using a static analysis tool I developed. The tool
> reported that slim_msg_response() accesses txn after releasing
> ctrl->txn_lock.
>
> I also used LLM assistants to help verify the report. I reviewed their
> output and ran the tests below myself.
>
> The race window is small because slim_msg_response() accesses txn only
> a few instructions after releasing the lock. However, the bug can occur
> when a response arrives just before the request times out. The request
> can then return while slim_msg_response() is still accessing txn.
>
> I tested this in QEMU and on a DragonBoard 845c. Both tests use a
> test-only delay to widen the race window.
>
> 1) QEMU
>
> I first reproduced the issue in QEMU on v7.3 (c03c1a14507d), x86_64,
> with 2 CPUs, CONFIG_KASAN=y, CONFIG_KASAN_INLINE=y, and
> CONFIG_KASAN_STACK=y.
>
> QEMU has no SLIMbus controller, so a test module registers a dummy
> controller using slim_register_controller(). The controller's
> ->xfer_msg() only records the TID. A kthread on CPU 0 calls
> slim_readb() and waits for the response. A kthread on CPU 1 calls
> slim_msg_response() with the recorded TID.
>
> A response arriving after the request has timed out does not trigger
> the bug because the timeout path has already removed the TID, and
> idr_find() returns NULL. The response must instead find the transaction
> before the timeout path removes it, but access txn after the request
> has returned.
>
> The request timeout is txn->rl + HZ milliseconds. With HZ=1000, it is
> about 1000 ms. To reproduce the race, I added a 1200 ms delay after
> releasing txn_lock, between idr_find() and the access to txn.
>
> Without the fix patch, slim_readb() returns -110 first. Then
> slim_msg_response() accesses txn from the stack frame of the returned
> request, and KASAN reports a use-after-free.
>
>   BUG: KASAN: use-after-free in slim_msg_response+0x29e/0x2b0 [slimbus]
>   Read of size 8 at addr ffff88800340fd20 by task slim091-resp/72
>   Call Trace:
>    slim_msg_response+0x29e/0x2b0 [slimbus]
>    resp_fn+0x9d/0xc0 [slim091_test]
>    kthread+0x32a/0x410
>    ret_from_fork+0x365/0x570
>
> With the fix patch, there is no KASAN report or oops. The timeout
> path waits for txn_lock in slim_free_txn_tid(), so slim_readb()
> returns -110 only after slim_msg_response() has called complete()
> and released the lock.
>
> 2) DragonBoard 845c
>
> Since the QEMU test uses a dummy controller, I also reproduced the issue
> on a DragonBoard 845c with a Qualcomm SLIMbus controller (qcom-ngd-ctrl)
> and a WCD9340 codec.
>
> The board runs v7.3 on arm64 with CONFIG_HZ=250, so the request timeout
> is about 250 ms. I added mdelay(500) after releasing txn_lock following
> a idr_find() in the unpatched kernel. A module parameter controls the
> delay after boot.
>
> I enabled the delay and triggered read requests through regmap debugfs
> by reading a WCD9340 register.
>
>   echo 1 > /sys/module/slimbus/parameters/test_delay
>   echo 1 > /sys/kernel/debug/regmap/217:250:1:0/cache_bypass
>   dd if=/sys/kernel/debug/regmap/217:250:1:0/registers of=/dev/null bs=9 count=1
>
> On kernel A (without the fix patch, with KASAN), the read crashed in
> memcpy(). The msg pointer was read from the stack frame of the returned
> request, so msg->rbuf contained an invalid address.
>
>   Unable to handle kernel paging request at virtual address 17ffff3dd2800001
>   CM = 0, WnR = 1, TnD = 0, TagAccess = 0
>   pc : __pi_memcpy_generic+0x7c/0x22c
>   Call trace:
>    __pi_memcpy_generic+0x7c/0x22c (P)
>    slim_msg_response+0x118/0x2a0
>    qcom_slim_ngd_rx_msgq_cb+0x16c/0x56c [slim_qcom_ngd_ctrl]
>    vchan_complete+0x400/0x5e8
>
> On kernel B (without the fix patch, without KASAN), the read also
> crashed. The txn->msg pointer was read from the stack frame of the
> returned request, and its value was 1. The kernel then crashed while
> accessing msg->rbuf at address 0x9 (1 + 8).
>
>   Unable to handle kernel NULL pointer dereference at virtual address 0000000000000009
>   pc : slim_msg_response+0x74/0x140
>   Call trace:
>    slim_msg_response+0x74/0x140 (P)
>    qcom_slim_ngd_rx_msgq_cb+0x8c/0x1a8 [slim_qcom_ngd_ctrl]
>    vchan_complete+0x1f8/0x228
>    tasklet_action_common+0x128/0x180
>   Kernel panic - not syncing: Oops: Fatal exception in interrupt
>
> On kernel C (with the fix patch and KASAN, using the same test delay),
> I ran 7 reads followed by about 170 seconds of continuous reads, with
> 196 timeouts in total. There was no oops or KASAN report in
> slim_msg_response(), and "Got response to invalid TID" did not appear.
>
> I should have included the discovery and testing details in the
> original submission. I'll include them in future reports and respond
> to review comments more promptly.
>
> I will send v2 with the blank line removed, a summary of the
> reproduction and testing results in the commit message, and an
> Assisted-by: LLM tag. I'll include the detailed test results below
> the --- separator.
>
> Thanks,
> Changyul
>
> 2026년 9월 11일 (금) 오전 2:02, Greg Kroah-Hartman <gregkh@linuxfoundation.org>님이 작성:
>>
>> On Fri, Sep 11, 2026 at 12:59:11AM -0400, Changyul Lee wrote:
>> > slim_msg_response() drops lock after idr_find() and dereferences txn.
>> > If the requester times out in the meantime, it frees the TID and
>> > returns, so the handler writes the reply into msg->rbuf and calls
>> > complete() on txn->comp, both of which have already benn freed.
>> >
>> > Keep lock held until complete(txn->comp) finishes, so a requester
>> > in slim_free_txn_tid() and cannot return while txn is still in use.
>> > Call idr_remove() because slim_free_txn_tid() takes the same lock.
>> >
>> > Fixes: afbdcc7c384b ("slimbus: Add messaging APIs to slimbus framework")
>> >
>> > Signed-off-by: Changyul Lee <lcy8047@gmail.com>
>>
>> No blank line between these please.
>>
>> And how was this found and tested?
>>
>> thanks,
>>
>> greg k-h