New: The usb/cdc-acm driver uses memory after it is freed

https://bugzilla.kernel.org/show_bug.cgi?id=220052

            Bug ID: 220052
           Summary: The usb/cdc-acm driver uses memory after it is freed
           Product: Drivers
           Version: 2.5
          Hardware: All
                OS: Linux
            Status: NEW
          Severity: normal
          Priority: P3
         Component: USB
          Assignee: [email protected]
          Reporter: [email protected]
        Regression: No

Created attachment 308013
--> https://bugzilla.kernel.org/attachment.cgi?id=308013&action=edit
A patch file that solves the kernel panic.

In the cdc-acm driver, memory is used after it is freed. The code is in drivers/usb/class/cdc-acm.c, in the acm_softint function. The problem manifests when a usb connection is disconnected by unplugging a cable and can result in a kernel panic. The panic is not always seen but only when the freed memory is claimed and used by an other process, somewhere between the for loop and the acm_submit_read_urbs.

A solution is attached in the form of a patch where 2 lines of code are exchanged with 2 other lines. The patch is based on kernel version linux-6.15-rc3.

The kernel panic is shown below.

cdc_acm 1-1.3:1.2: urb 0 failed submission with -2

Unable to handle kernel NULL pointer dereference at virtual address 00000030

Mem abort info:
Exception class = DABT (current EL), IL = 32 bits SET = 0, FnV = 0
EA = 0, S1PTW = 0
Data abort info:
ISV = 0, ISS = 0x00000004
CM = 0, WnR = 0
user pgtable: 4k pages, 48-bit VAs, pgd = ffff8000bbab6000 [0000000000000030] *pgd=0000000000000000

Internal error: Oops: 96000004 [#1] PREEMPT SMP
Modules linked in: 8021q garp stp mrp ax88179_178a usbnet crc32_ce crct10dif_ce
galcore(O)
CPU: 0 PID: 1740 Comm: kworker/0:3 Tainted: G           O   
4.14.98-imx_4.14.98_2.0.0_ga+g5d6cbeafb80c #1
Hardware name: Freescale i.MX8MQ EVK (DT)
Workqueue: events acm_softint
task: ffff8000baa12880 task.stack: ffff00000d448000
PC is at usb_autopm_put_interface+0x14/0x48
LR is at acm_softint+0xb8/0xd8
pc : [<ffff0000088e3abc>] lr : [<ffff0000089480b8>] pstate: 40000145

sp : ffff00000d44bda0
x29: ffff00000d44bda0 x28: 0000000000000000
x27: 0000000000000000 x26: ffff000009222ec0
x25: ffff0000080e7760 x24: ffff8000b6386788
x23: 0000000000000000 x22: ffff8000bff60580
x21: ffff8000b6385f88 x20: ffff8000b6386000
x19: ffff8000b6386788 x18: 0000000000000001
x17: 0000ffffb6667098 x16: ffff000008261920
x15: ffff000009752000 x14: 00000000fffffff0
x13: ffff0000098e5150 x12: ffff000009738df8
x11: ffff000008663128 x10: ffff0000098e4000
x9 : 0000000000000006 x8 : 6d6275732064656c
x7 : 6961662030206272 x6 : 0000000000000514
x5 : 0000000000000000 x4 : 0000000000000000
x3 : 0000000000000330 x2 : 00000000fffefa67
x1 : ffff000009736000 x0 : 0000000000000000
Process kworker/0:3 (pid: 1740, stack limit = 0xffff00000d448000)

Call trace:
Exception stack(0xffff00000d44bc60 to 0xffff00000d44bda0)

bc60: 0000000000000000 ffff000009736000 00000000fffefa67 0000000000000330
bc80: 0000000000000000 0000000000000000 0000000000000514 6961662030206272
bca0: 6d6275732064656c 0000000000000006 ffff0000098e4000 ffff000008663128
bcc0: ffff000009738df8 ffff0000098e5150 00000000fffffff0 ffff000009752000
bce0: ffff000008261920 0000ffffb6667098 0000000000000001 ffff8000b6386788
bd00: ffff8000b6386000 ffff8000b6385f88 ffff8000bff60580 0000000000000000
bd20: ffff8000b6386788 ffff0000080e7760 ffff000009222ec0 0000000000000000
bd40: 0000000000000000 ffff00000d44bda0 ffff0000089480b8 ffff00000d44bda0
bd60: ffff0000088e3abc 0000000040000145 00000000014000c0 ffff8000bff60580
bd80: ffffffffffffffff ffff0000089480b0 ffff00000d44bda0 ffff0000088e3abc

[<ffff0000088e3abc>] usb_autopm_put_interface+0x14/0x48
[<ffff0000089480b8>] acm_softint+0xb8/0xd8
[<ffff0000080e75ec>] process_one_work+0x1d4/0x348
[<ffff0000080e77a8>] worker_thread+0x48/0x470
[<ffff0000080edaac>] kthread+0x12c/0x130
[<ffff000008084ed8>] ret_from_fork+0x10/0x18
Code: f0007281 910cc003 910003fd f9454022 (f9401801) 
---[ end trace 1b12fec59341c199 ]---

You may reply to this email to add a comment.

You are receiving this mail because:
You are watching the assignee of the bug.

Please submit this properly through email so that we can review and apply it if it is correct.

thanks,

greg k-h

https://bugzilla.kernel.org/show_bug.cgi?id=220052

--- Comment #1 from [email protected] ---

Please submit this properly through email so that we can review and apply it if it is correct.

thanks,

greg k-h

You may reply to this email to add a comment.

You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.kernel.org/show_bug.cgi?id=220052

--- Comment #2 from Ben ([email protected]) ---
One of our customers complained about a kernel panic that occurs now and than after disconnecting a usb-cable. However, most of the disconnects are without a panic. After some investigation the panic shows up between 1 and 100 disconnect attempts. From the panic output can be seen that the acm_softint is one of the last functions on the stack . This function is first calling a usb_kill_urb that frees the urb memory but 2 lines further the allocated urb memory is still used in the acm_submit_read_urbs->acm_submit_read_urb.

Exchanging the 2 lines as indicated in the patch solves the problem completely and the kernel panic is not seen anymore. This is proved by 7000 'virtual' disconnects.

You may reply to this email to add a comment.

You are receiving this mail because:
You are watching the assignee of the bug.

One of our customers complained about a kernel panic that occurs now and than after disconnecting a usb-cable. However, most of the disconnects are without a panic. After some investigation the panic shows up between 1 and 100 disconnect attempts. From the panic output can be seen that the acm_softint is one of the last functions on the stack . This function is first calling a usb_kill_urb that frees the urb memory but 2 lines further the allocated urb memory is still used in the acm_submit_read_urbs->acm_submit_read_urb.

Exchanging the 2 lines as indicated in the patch solves the problem completely and the kernel panic is not seen anymore. This is proved by 7000 'virtual' disconnects.

https://bugzilla.kernel.org/show_bug.cgi?id=220052

--- Comment #3 from Ben ([email protected]) ---
One of our customers complained about a kernel panic that occurs now and than after disconnecting a usb-cable. However, most of the disconnects are without a panic. After some investigation the panic shows up between 1 and 100 disconnect attempts. From the panic output can be seen that the acm_softint is one of the last functions on the stack . This function is first calling a usb_kill_urb that frees the urb memory but 2 lines further the allocated urb memory is still used in the acm_submit_read_urbs->acm_submit_read_urb.

Exchanging the 2 lines as indicated in the patch solves the problem completely and the kernel panic is not seen anymore. This is proved by 7000 'virtual' disconnects.

You may reply to this email to add a comment.

You are receiving this mail because:
You are watching the assignee of the bug.

Yes, it solves your particular problem.

Now, with your patch, you first call
acm_submit_read_urbs() -> acm_submit_read_urb():

res = usb_submit_urb(acm->read_urbs[index], mem_flags);

And then you do

+ for (i = 0; i < acm->rx_buflimit; i++)
+ usb_kill_urb(acm->read_urbs[i]);

In other words, you undo what you just did.

Your diagnosis of the issue may be good, but the fix is _not_. The purpose of acm_softint() is to

1. kill outstanding IO
2. error handling with usb_clear_halt()
3. restart the outstanding IO

You cannot reverse steps 1 and 3 and expect that to work. The issue must be one of refcounting.

Sorry
Oliver

https://bugzilla.kernel.org/show_bug.cgi?id=220052

--- Comment #4 from [email protected] ---

Yes, it solves your particular problem.

Now, with your patch, you first call
acm_submit_read_urbs() -> acm_submit_read_urb():

res = usb_submit_urb(acm->read_urbs[index], mem_flags);

And then you do

+ for (i = 0; i < acm->rx_buflimit; i++)
+ usb_kill_urb(acm->read_urbs[i]);

In other words, you undo what you just did.

Your diagnosis of the issue may be good, but the fix is _not_. The purpose of acm_softint() is to

1. kill outstanding IO
2. error handling with usb_clear_halt()
3. restart the outstanding IO

You cannot reverse steps 1 and 3 and expect that to work. The issue must be one of refcounting.

Sorry
Oliver

You may reply to this email to add a comment.

You are receiving this mail because:
You are watching the assignee of the bug.