[syzbot] [can?] WARNING: refcount bug in j1939_session_put

[syzbot] [can?] WARNING: refcount bug in j1939_session_put

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit:    743ff02152bc ethtool: Don't check for NULL info in prepare..
git tree:       net-next
console+strace: https://syzkaller.appspot.com/x/log.txt?x=146ac8d3980000
kernel config:  https://syzkaller.appspot.com/x/.config?x=5efb917b1462a973
dashboard link: https://syzkaller.appspot.com/bug?extid=ad601904231505ad6617
compiler:       Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=131875c9980000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=1741f94b980000

Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/69cb8d5cd046/disk-743ff021.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/8f52c95a23c5/vmlinux-743ff021.xz
kernel image: https://storage.googleapis.com/syzbot-assets/93f2f40e650b/bzImage-743ff021.xz

The issue was bisected to:

commit c9c0ee5f20c593faf289fa8850c3ed84031dd12a
Author: Breno Leitao <leitao@debian.org>
Date:   Mon Jul 29 10:47:40 2024 +0000

    net: skbuff: Skip early return in skb_unref when debugging

bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=16b7066d980000
final oops:     https://syzkaller.appspot.com/x/report.txt?x=15b7066d980000
console output: https://syzkaller.appspot.com/x/log.txt?x=11b7066d980000

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+ad601904231505ad6617@syzkaller.appspotmail.com
Fixes: c9c0ee5f20c5 ("net: skbuff: Skip early return in skb_unref when debugging")

------------[ cut here ]------------
refcount_t: underflow; use-after-free.
WARNING: CPU: 0 PID: 5233 at lib/refcount.c:28 refcount_warn_saturate+0x15a/0x1d0 lib/refcount.c:28
Modules linked in:
CPU: 0 UID: 0 PID: 5233 Comm: syz-executor339 Not tainted 6.10.0-syzkaller-12610-g743ff02152bc #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 06/27/2024
RIP: 0010:refcount_warn_saturate+0x15a/0x1d0 lib/refcount.c:28
Code: 00 17 40 8c e8 67 97 a5 fc 90 0f 0b 90 90 eb 99 e8 1b 89 e3 fc c6 05 76 7d 31 0b 01 90 48 c7 c7 60 17 40 8c e8 47 97 a5 fc 90 <0f> 0b 90 90 e9 76 ff ff ff e8 f8 88 e3 fc c6 05 50 7d 31 0b 01 90
RSP: 0018:ffffc900000076a0 EFLAGS: 00010246
RAX: adacdd6de1fa9d00 RBX: ffff88802c47c224 RCX: ffff8880234fda00
RDX: 0000000080000101 RSI: 0000000000000000 RDI: 0000000000000000
RBP: 0000000000000003 R08: ffffffff81559432 R09: 1ffff1101724519a
R10: dffffc0000000000 R11: ffffed101724519b R12: ffff88807d01a468
R13: ffff88802c47c224 R14: 1ffff1100fa03498 R15: ffff88807d01a400
FS:  0000555569709380(0000) GS:ffff8880b9200000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000020000000 CR3: 000000001eb56000 CR4: 00000000003506f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
 <IRQ>
 kfree_skb_reason include/linux/skbuff.h:1260 [inline]
 kfree_skb include/linux/skbuff.h:1269 [inline]
 j1939_session_destroy net/can/j1939/transport.c:282 [inline]
 __j1939_session_release net/can/j1939/transport.c:294 [inline]
 kref_put include/linux/kref.h:65 [inline]
 j1939_session_put+0x1e7/0x440 net/can/j1939/transport.c:299
 j1939_tp_cmd_recv net/can/j1939/transport.c:2113 [inline]
 j1939_tp_recv+0x7fe/0x1050 net/can/j1939/transport.c:2161
 j1939_can_recv+0x732/0xb20 net/can/j1939/main.c:108
 deliver net/can/af_can.c:572 [inline]
 can_rcv_filter+0x359/0x7f0 net/can/af_can.c:606
 can_receive+0x31c/0x470 net/can/af_can.c:663
 can_rcv+0x144/0x260 net/can/af_can.c:687
 __netif_receive_skb_one_core net/core/dev.c:5660 [inline]
 __netif_receive_skb+0x2e0/0x650 net/core/dev.c:5774
 process_backlog+0x662/0x15b0 net/core/dev.c:6107
 __napi_poll+0xcb/0x490 net/core/dev.c:6771
 napi_poll net/core/dev.c:6840 [inline]
 net_rx_action+0x89b/0x1240 net/core/dev.c:6962
 handle_softirqs+0x2c4/0x970 kernel/softirq.c:554
 __do_softirq kernel/softirq.c:588 [inline]
 invoke_softirq kernel/softirq.c:428 [inline]
 __irq_exit_rcu+0xf4/0x1c0 kernel/softirq.c:637
 irq_exit_rcu+0x9/0x30 kernel/softirq.c:649
 instr_sysvec_apic_timer_interrupt arch/x86/kernel/apic/apic.c:1043 [inline]
 sysvec_apic_timer_interrupt+0xa6/0xc0 arch/x86/kernel/apic/apic.c:1043
 </IRQ>
 <TASK>
 asm_sysvec_apic_timer_interrupt+0x1a/0x20 arch/x86/include/asm/idtentry.h:702
RIP: 0010:__raw_spin_unlock_irqrestore include/linux/spinlock_api_smp.h:152 [inline]
RIP: 0010:_raw_spin_unlock_irqrestore+0xd8/0x140 kernel/locking/spinlock.c:194
Code: 9c 8f 44 24 20 42 80 3c 23 00 74 08 4c 89 f7 e8 0e 9c 3b f6 f6 44 24 21 02 75 52 41 f7 c7 00 02 00 00 74 01 fb bf 01 00 00 00 <e8> 63 c1 a3 f5 65 8b 05 64 b7 44 74 85 c0 74 43 48 c7 04 24 0e 36
RSP: 0018:ffffc900035378c0 EFLAGS: 00000206
RAX: adacdd6de1fa9d00 RBX: 1ffff920006a6f1c RCX: ffffffff81701f3a
RDX: dffffc0000000000 RSI: ffffffff8bead5a0 RDI: 0000000000000001
RBP: ffffc90003537950 R08: ffffffff9351e8e7 R09: 1ffffffff26a3d1c
R10: dffffc0000000000 R11: fffffbfff26a3d1d R12: dffffc0000000000
R13: 1ffff920006a6f18 R14: ffffc900035378e0 R15: 0000000000000246
 j1939_sk_send_loop net/can/j1939/socket.c:1164 [inline]
 j1939_sk_sendmsg+0xe01/0x14c0 net/can/j1939/socket.c:1277
 sock_sendmsg_nosec net/socket.c:730 [inline]
 __sock_sendmsg+0x221/0x270 net/socket.c:745
 ____sys_sendmsg+0x525/0x7d0 net/socket.c:2597
 ___sys_sendmsg net/socket.c:2651 [inline]
 __sys_sendmsg+0x2b0/0x3a0 net/socket.c:2680
 do_syscall_x64 arch/x86/entry/common.c:52 [inline]
 do_syscall_64+0xf3/0x230 arch/x86/entry/common.c:83
 entry_SYSCALL_64_after_hwframe+0x77/0x7f
RIP: 0033:0x7fd558d90e09
Code: 28 00 00 00 75 05 48 83 c4 28 c3 e8 01 1a 00 00 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 b8 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007ffe518919c8 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
RAX: ffffffffffffffda RBX: 0000000000000003 RCX: 00007fd558d90e09
RDX: 0000000000000000 RSI: 0000000020000280 RDI: 0000000000000003
RBP: 00000000000f4240 R08: 0000000000000000 R09: 0000000000000001
R10: 0000000000000000 R11: 0000000000000246 R12: 00007ffe51891a20
R13: 00007fd558dde406 R14: 0000000000000003 R15: 00007ffe51891a00
 </TASK>
----------------
Code disassembly (best guess):
   0:	9c                   	pushf
   1:	8f 44 24 20          	pop    0x20(%rsp)
   5:	42 80 3c 23 00       	cmpb   $0x0,(%rbx,%r12,1)
   a:	74 08                	je     0x14
   c:	4c 89 f7             	mov    %r14,%rdi
   f:	e8 0e 9c 3b f6       	call   0xf63b9c22
  14:	f6 44 24 21 02       	testb  $0x2,0x21(%rsp)
  19:	75 52                	jne    0x6d
  1b:	41 f7 c7 00 02 00 00 	test   $0x200,%r15d
  22:	74 01                	je     0x25
  24:	fb                   	sti
  25:	bf 01 00 00 00       	mov    $0x1,%edi
* 2a:	e8 63 c1 a3 f5       	call   0xf5a3c192 <-- trapping instruction
  2f:	65 8b 05 64 b7 44 74 	mov    %gs:0x7444b764(%rip),%eax        # 0x7444b79a
  36:	85 c0                	test   %eax,%eax
  38:	74 43                	je     0x7d
  3a:	48                   	rex.W
  3b:	c7                   	.byte 0xc7
  3c:	04 24                	add    $0x24,%al
  3e:	0e                   	(bad)
  3f:	36                   	ss


---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.

syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
For information about bisection process see: https://goo.gl/tpsmEJ#bisection

If the report is already addressed, let syzbot know by replying with:
#syz fix: exact-commit-title

If you want syzbot to run the reproducer, reply with:
#syz test: git://repo/address.git branch-or-commit-hash
If you attach or paste a git patch, syzbot will apply it before testing.

If you want to overwrite report's subsystems, reply with:
#syz set subsystems: new-subsystem
(See the list of subsystem names on the web dashboard)

If the report is a duplicate of another one, reply with:
#syz dup: exact-subject-of-another-report

If you want to undo deduplication, reply with:
#syz undup

Re: [syzbot] [can?] WARNING: refcount bug in j1939_session_put

[Edward Adam Davis]

Fixes: c9c0ee5f20c5 ("net: skbuff: Skip early return in skb_unref when debugging")

Root cause: In commit c9c0ee5f20c5, There are following rules:
In debug builds (CONFIG_DEBUG_NET set), the reference count is always  decremented, even when it's 1

This rule will cause the reference count to be 0 after calling skc_unref,
which will affect the release of skb.

The solution I have proposed is:
Before releasing the SKB during session destroy, check the CONFIG_DEBUG_NET
and skb_unref return values to avoid reference count errors caused by a 
reference count of 0 when releasing the SKB.

#syz test: net-next 743ff02152bc

diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
index 4be73de5033c..50d96015c125 100644
--- a/net/can/j1939/transport.c
+++ b/net/can/j1939/transport.c
@@ -278,7 +278,8 @@ static void j1939_session_destroy(struct j1939_session *session)
 
 	while ((skb = skb_dequeue(&session->skb_queue)) != NULL) {
 		/* drop ref taken in j1939_session_skb_queue() */
-		skb_unref(skb);
+		if (skb_unref(skb) && IS_ENABLED(CONFIG_DEBUG_NET))
+			skb_get(skb);
 		kfree_skb(skb);
 	}
 	__j1939_session_drop(session);

Re: [syzbot] [can?] WARNING: refcount bug in j1939_session_put

[syzbot]

Hello,

syzbot has tested the proposed patch but the reproducer is still triggering an issue:
WARNING: refcount bug in j1939_session_put

------------[ cut here ]------------
refcount_t: addition on 0; use-after-free.
WARNING: CPU: 0 PID: 6069 at lib/refcount.c:25 refcount_warn_saturate+0x13a/0x1d0 lib/refcount.c:25
Modules linked in:
CPU: 0 UID: 0 PID: 6069 Comm: syz.0.15 Not tainted 6.10.0-syzkaller-12610-g743ff02152bc-dirty #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 06/27/2024
RIP: 0010:refcount_warn_saturate+0x13a/0x1d0 lib/refcount.c:25
Code: a0 16 40 8c e8 87 97 a5 fc 90 0f 0b 90 90 eb b9 e8 3b 89 e3 fc c6 05 95 7d 31 0b 01 90 48 c7 c7 00 17 40 8c e8 67 97 a5 fc 90 <0f> 0b 90 90 eb 99 e8 1b 89 e3 fc c6 05 76 7d 31 0b 01 90 48 c7 c7
RSP: 0018:ffffc90000007698 EFLAGS: 00010246
RAX: f96573282d1dbd00 RBX: ffff88801d361864 RCX: ffff88802c26bc00
RDX: 0000000000000101 RSI: 0000000000000000 RDI: 0000000000000000
RBP: 0000000000000002 R08: ffffffff81559432 R09: fffffbfff1cb9f88
R10: dffffc0000000000 R11: fffffbfff1cb9f88 R12: ffff88802afb7468
R13: ffff88801d361864 R14: ffff888066d74000 R15: ffff88802afb7400
FS:  00007fa08b7526c0(0000) GS:ffff8880b9200000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 000000c0010e6000 CR3: 000000006730e000 CR4: 00000000003506f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
 <IRQ>
 __j1939_session_release net/can/j1939/transport.c:295 [inline]
 kref_put include/linux/kref.h:65 [inline]
 j1939_session_put+0x26f/0x4a0 net/can/j1939/transport.c:300
 j1939_tp_cmd_recv net/can/j1939/transport.c:2114 [inline]
 j1939_tp_recv+0x7fe/0x1050 net/can/j1939/transport.c:2162
 j1939_can_recv+0x732/0xb20 net/can/j1939/main.c:108
 deliver net/can/af_can.c:572 [inline]
 can_rcv_filter+0x359/0x7f0 net/can/af_can.c:606
 can_receive+0x31c/0x470 net/can/af_can.c:663
 can_rcv+0x144/0x260 net/can/af_can.c:687
 __netif_receive_skb_one_core net/core/dev.c:5660 [inline]
 __netif_receive_skb+0x2e0/0x650 net/core/dev.c:5774
 process_backlog+0x662/0x15b0 net/core/dev.c:6107
 __napi_poll+0xcb/0x490 net/core/dev.c:6771
 napi_poll net/core/dev.c:6840 [inline]
 net_rx_action+0x89b/0x1240 net/core/dev.c:6962
 handle_softirqs+0x2c4/0x970 kernel/softirq.c:554
 __do_softirq kernel/softirq.c:588 [inline]
 invoke_softirq kernel/softirq.c:428 [inline]
 __irq_exit_rcu+0xf4/0x1c0 kernel/softirq.c:637
 irq_exit_rcu+0x9/0x30 kernel/softirq.c:649
 instr_sysvec_apic_timer_interrupt arch/x86/kernel/apic/apic.c:1043 [inline]
 sysvec_apic_timer_interrupt+0xa6/0xc0 arch/x86/kernel/apic/apic.c:1043
 </IRQ>
 <TASK>
 asm_sysvec_apic_timer_interrupt+0x1a/0x20 arch/x86/include/asm/idtentry.h:702
RIP: 0010:__raw_spin_unlock_irqrestore include/linux/spinlock_api_smp.h:152 [inline]
RIP: 0010:_raw_spin_unlock_irqrestore+0xd8/0x140 kernel/locking/spinlock.c:194
Code: 9c 8f 44 24 20 42 80 3c 23 00 74 08 4c 89 f7 e8 0e 9c 3b f6 f6 44 24 21 02 75 52 41 f7 c7 00 02 00 00 74 01 fb bf 01 00 00 00 <e8> 63 c1 a3 f5 65 8b 05 64 b7 44 74 85 c0 74 43 48 c7 04 24 0e 36
RSP: 0018:ffffc900033cf8c0 EFLAGS: 00000206
RAX: f96573282d1dbd00 RBX: 1ffff92000679f1c RCX: ffffffff81701f3a
RDX: dffffc0000000000 RSI: ffffffff8bead5a0 RDI: 0000000000000001
RBP: ffffc900033cf950 R08: ffffffff9351e917 R09: 1ffffffff26a3d22
R10: dffffc0000000000 R11: fffffbfff26a3d23 R12: dffffc0000000000
R13: 1ffff92000679f18 R14: ffffc900033cf8e0 R15: 0000000000000246
 j1939_sk_send_loop net/can/j1939/socket.c:1164 [inline]
 j1939_sk_sendmsg+0xe01/0x14c0 net/can/j1939/socket.c:1277
 sock_sendmsg_nosec net/socket.c:730 [inline]
 __sock_sendmsg+0x221/0x270 net/socket.c:745
 ____sys_sendmsg+0x525/0x7d0 net/socket.c:2597
 ___sys_sendmsg net/socket.c:2651 [inline]
 __sys_sendmsg+0x2b0/0x3a0 net/socket.c:2680
 do_syscall_x64 arch/x86/entry/common.c:52 [inline]
 do_syscall_64+0xf3/0x230 arch/x86/entry/common.c:83
 entry_SYSCALL_64_after_hwframe+0x77/0x7f
RIP: 0033:0x7fa08a9773b9
Code: ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 40 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 a8 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007fa08b752048 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
RAX: ffffffffffffffda RBX: 00007fa08ab05f80 RCX: 00007fa08a9773b9
RDX: 0000000000000000 RSI: 0000000020000280 RDI: 0000000000000003
RBP: 00007fa08a9e48e6 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
R13: 000000000000000b R14: 00007fa08ab05f80 R15: 00007fff3eaa0cf8
 </TASK>
----------------
Code disassembly (best guess):
   0:	9c                   	pushf
   1:	8f 44 24 20          	pop    0x20(%rsp)
   5:	42 80 3c 23 00       	cmpb   $0x0,(%rbx,%r12,1)
   a:	74 08                	je     0x14
   c:	4c 89 f7             	mov    %r14,%rdi
   f:	e8 0e 9c 3b f6       	call   0xf63b9c22
  14:	f6 44 24 21 02       	testb  $0x2,0x21(%rsp)
  19:	75 52                	jne    0x6d
  1b:	41 f7 c7 00 02 00 00 	test   $0x200,%r15d
  22:	74 01                	je     0x25
  24:	fb                   	sti
  25:	bf 01 00 00 00       	mov    $0x1,%edi
* 2a:	e8 63 c1 a3 f5       	call   0xf5a3c192 <-- trapping instruction
  2f:	65 8b 05 64 b7 44 74 	mov    %gs:0x7444b764(%rip),%eax        # 0x7444b79a
  36:	85 c0                	test   %eax,%eax
  38:	74 43                	je     0x7d
  3a:	48                   	rex.W
  3b:	c7                   	.byte 0xc7
  3c:	04 24                	add    $0x24,%al
  3e:	0e                   	(bad)
  3f:	36                   	ss


Tested on:

commit:         743ff021 ethtool: Don't check for NULL info in prepare..
git tree:       git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git
console output: https://syzkaller.appspot.com/x/log.txt?x=11568613980000
kernel config:  https://syzkaller.appspot.com/x/.config?x=5efb917b1462a973
dashboard link: https://syzkaller.appspot.com/bug?extid=ad601904231505ad6617
compiler:       Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40
patch:          https://syzkaller.appspot.com/x/patch.diff?x=1386c28d980000

Re: [syzbot] [can?] WARNING: refcount bug in j1939_session_put

[Breno Leitao]

Hello Edward,

On Wed, Aug 07, 2024 at 09:42:40AM +0800, Edward Adam Davis wrote:
> Fixes: c9c0ee5f20c5 ("net: skbuff: Skip early return in skb_unref when debugging")
> 
> Root cause: In commit c9c0ee5f20c5, There are following rules:
> In debug builds (CONFIG_DEBUG_NET set), the reference count is always  decremented, even when it's 1

That is the goal, to pick problems like the one reported here. I.e, the
reference shouldn't be negative. If that is the case, it means that
there is a bug, and the skb is being unreferenced more than what it
needs to.

> This rule will cause the reference count to be 0 after calling skc_unref,
> which will affect the release of skb.
> 
> The solution I have proposed is:
> Before releasing the SKB during session destroy, check the CONFIG_DEBUG_NET
> and skb_unref return values to avoid reference count errors caused by a 
> reference count of 0 when releasing the SKB.

I am not sure this is the best approach. I would sugest finding where
the skb is being unreferenced first, so, it doesn't need to be
unreferenced again.

This suggestion is basically working around the findings.

Thanks for looking at this problem.
--breno

[PATCH net-next] can: j1939: fix uaf in j1939_session_destroy

[Edward Adam Davis]

The root cause of this problem is when both of the following conditions
are met simultaneously:
[1] Introduced commit c9c0ee5f20c5, There are following rules:
In debug builds (CONFIG_DEBUG_NET set), the reference count is always
decremented, even when it's 1.

[2] When executing sendmsg, the newly created session did not increase the
skb reference count, only added skb to the session's skb_queue.

The solution is:
When creating a new session, do not add the skb to the skb_queue.
Instead, when using skb, uniformly use j1939_session_skb_queue to add
the skb to the queue and increase the skb reference count through it.

Fixes: c9c0ee5f20c5 ("net: skbuff: Skip early return in skb_unref when debugging")
Reported-and-tested-by: syzbot+ad601904231505ad6617@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=ad601904231505ad6617
Signed-off-by: Edward Adam Davis <eadavis@qq.com>
---
 net/can/j1939/socket.c    | 7 ++++---
 net/can/j1939/transport.c | 2 +-
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/net/can/j1939/socket.c b/net/can/j1939/socket.c
index 305dd72c844c..ec78bee1bfa6 100644
--- a/net/can/j1939/socket.c
+++ b/net/can/j1939/socket.c
@@ -1170,10 +1170,11 @@ static int j1939_sk_send_loop(struct j1939_priv *priv,  struct sock *sk,
 					break;
 				}
 			}
-		} else {
-			skcb->offset = session->total_queued_size;
-			j1939_session_skb_queue(session, skb);
 		}
+		/* Session is ready, add it to skb queue and increase ref count.
+		 */
+		skcb->offset = session->total_queued_size;
+		j1939_session_skb_queue(session, skb);
 
 		todo_size -= segment_size;
 		session->total_queued_size += segment_size;
diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
index 4be73de5033c..dd503bc3adb5 100644
--- a/net/can/j1939/transport.c
+++ b/net/can/j1939/transport.c
@@ -1505,7 +1505,6 @@ static struct j1939_session *j1939_session_new(struct j1939_priv *priv,
 	session->state = J1939_SESSION_NEW;
 
 	skb_queue_head_init(&session->skb_queue);
-	skb_queue_tail(&session->skb_queue, skb);
 
 	skcb = j1939_skb_to_cb(skb);
 	memcpy(&session->skcb, skcb, sizeof(session->skcb));
@@ -1548,6 +1547,7 @@ j1939_session *j1939_session_fresh_new(struct j1939_priv *priv,
 		kfree_skb(skb);
 		return NULL;
 	}
+	j1939_session_skb_queue(session, skb);
 
 	/* alloc data area */
 	skb_put(skb, size);
-- 
2.43.0

Re: [PATCH net-next] can: j1939: fix uaf in j1939_session_destroy

[Jakub Kicinski]

On Wed,  7 Aug 2024 20:35:47 +0800 Edward Adam Davis wrote:
> Fixes: c9c0ee5f20c5 ("net: skbuff: Skip early return in skb_unref when debugging")

Definitely not where the _bug_ was added, as Breno said.
It is kinda tempting to annotate somehow that this commit helped catch
the bug, tho. Not sure how.

Re: [syzbot] [can?] WARNING: refcount bug in j1939_session_put

[Edward Adam Davis]

On Wed, 7 Aug 2024 01:02:28 -0700, Breno Leitao wrote:
> > Fixes: c9c0ee5f20c5 ("net: skbuff: Skip early return in skb_unref when debugging")
> >
> > Root cause: In commit c9c0ee5f20c5, There are following rules:
> > In debug builds (CONFIG_DEBUG_NET set), the reference count is always  decremented, even when it's 1
> 
> That is the goal, to pick problems like the one reported here. I.e, the
> reference shouldn't be negative. If that is the case, it means that
> there is a bug, and the skb is being unreferenced more than what it
> needs to.
Got it, I will remove the Fixes tag.
> 
> > This rule will cause the reference count to be 0 after calling skc_unref,
> > which will affect the release of skb.
> >
> > The solution I have proposed is:
> > Before releasing the SKB during session destroy, check the CONFIG_DEBUG_NET
> > and skb_unref return values to avoid reference count errors caused by a
> > reference count of 0 when releasing the SKB.
> 
> I am not sure this is the best approach. I would sugest finding where
> the skb is being unreferenced first, so, it doesn't need to be
> unreferenced again.
> 
> This suggestion is basically working around the findings.

BR,
--
Edward

[PATCH net-next V2] can: j1939: fix uaf warning in j1939_session_destroy

[Edward Adam Davis]

The root cause of this problem is when both of the following conditions
are met simultaneously:
[1] Introduced commit c9c0ee5f20c5, There are following rules:
In debug builds (CONFIG_DEBUG_NET set), the reference count is always
decremented, even when it's 1.

[2] When executing sendmsg, the newly created session did not increase the
skb reference count, only added skb to the session's skb_queue.

The solution is:
When creating a new session, do not add the skb to the skb_queue.
Instead, when using skb, uniformly use j1939_session_skb_queue to add
the skb to the queue and increase the skb reference count through it.

Reported-and-tested-by: syzbot+ad601904231505ad6617@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=ad601904231505ad6617
Signed-off-by: Edward Adam Davis <eadavis@qq.com>
---
 net/can/j1939/socket.c    | 7 ++++---
 net/can/j1939/transport.c | 2 +-
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/net/can/j1939/socket.c b/net/can/j1939/socket.c
index 305dd72c844c..ec78bee1bfa6 100644
--- a/net/can/j1939/socket.c
+++ b/net/can/j1939/socket.c
@@ -1170,10 +1170,11 @@ static int j1939_sk_send_loop(struct j1939_priv *priv,  struct sock *sk,
 					break;
 				}
 			}
-		} else {
-			skcb->offset = session->total_queued_size;
-			j1939_session_skb_queue(session, skb);
 		}
+		/* Session is ready, add it to skb queue and increase ref count.
+		 */
+		skcb->offset = session->total_queued_size;
+		j1939_session_skb_queue(session, skb);
 
 		todo_size -= segment_size;
 		session->total_queued_size += segment_size;
diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
index 4be73de5033c..dd503bc3adb5 100644
--- a/net/can/j1939/transport.c
+++ b/net/can/j1939/transport.c
@@ -1505,7 +1505,6 @@ static struct j1939_session *j1939_session_new(struct j1939_priv *priv,
 	session->state = J1939_SESSION_NEW;
 
 	skb_queue_head_init(&session->skb_queue);
-	skb_queue_tail(&session->skb_queue, skb);
 
 	skcb = j1939_skb_to_cb(skb);
 	memcpy(&session->skcb, skcb, sizeof(session->skcb));
@@ -1548,6 +1547,7 @@ j1939_session *j1939_session_fresh_new(struct j1939_priv *priv,
 		kfree_skb(skb);
 		return NULL;
 	}
+	j1939_session_skb_queue(session, skb);
 
 	/* alloc data area */
 	skb_put(skb, size);
-- 
2.43.0

Re: [PATCH net-next V2] can: j1939: fix uaf warning in j1939_session_destroy

[Oleksij Rempel]

Hi Edward,

On Thu, Aug 08, 2024 at 07:08:49AM +0800, Edward Adam Davis wrote:
> The root cause of this problem is when both of the following conditions
> are met simultaneously:
> [1] Introduced commit c9c0ee5f20c5, There are following rules:
> In debug builds (CONFIG_DEBUG_NET set), the reference count is always
> decremented, even when it's 1.
> 
> [2] When executing sendmsg, the newly created session did not increase the
> skb reference count, only added skb to the session's skb_queue.
> 
> The solution is:
> When creating a new session, do not add the skb to the skb_queue.
> Instead, when using skb, uniformly use j1939_session_skb_queue to add
> the skb to the queue and increase the skb reference count through it.
> 
> Reported-and-tested-by: syzbot+ad601904231505ad6617@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=ad601904231505ad6617
> Signed-off-by: Edward Adam Davis <eadavis@qq.com>

This patch breaks j1939.
The issue can be reproduced by running following commands:
git clone git@github.com:linux-can/can-tests.git
cd can-tests/j1939/
ip link add type vcan
ip l s dev vcan0 up
./run_all.sh vcan0 vcan0


Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net-next V2] can: j1939: fix uaf warning in j1939_session_destroy

[Edward Adam Davis]

On Thu, 8 Aug 2024 09:49:18 +0200, Oleksij Rempel wrote:
> > the skb to the queue and increase the skb reference count through it.
> > 
> > Reported-and-tested-by: syzbot+ad601904231505ad6617@syzkaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=ad601904231505ad6617
> > Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> 
> This patch breaks j1939.
> The issue can be reproduced by running following commands:
I tried to reproduce the problem using the following command, but was 
unsuccessful. Prompt me to install j1939cat and j1939acd, and there are
some other errors.

Can you share the logs from when you reproduced the problem?
> git clone git@github.com:linux-can/can-tests.git
> cd can-tests/j1939/
> ip link add type vcan
> ip l s dev vcan0 up
> ./run_all.sh vcan0 vcan0

BR,

--
Edward

Re: [PATCH net-next V2] can: j1939: fix uaf warning in j1939_session_destroy

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1157 bytes --]

On 08.08.2024 19:07:55, Edward Adam Davis wrote:
> On Thu, 8 Aug 2024 09:49:18 +0200, Oleksij Rempel wrote:
> > > the skb to the queue and increase the skb reference count through it.
> > > 
> > > Reported-and-tested-by: syzbot+ad601904231505ad6617@syzkaller.appspotmail.com
> > > Closes: https://syzkaller.appspot.com/bug?extid=ad601904231505ad6617
> > > Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> > 
> > This patch breaks j1939.
> > The issue can be reproduced by running following commands:
> I tried to reproduce the problem using the following command, but was 
> unsuccessful. Prompt me to install j1939cat and j1939acd, and there are
> some other errors.

The j1939 commands are part of the can-utils package. On Debian
compatible systems it's "sudo apt install can-utils". Or you can build
them from source: https://github.com/linux-can/can-utils

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH net-next V2] can: j1939: fix uaf warning in j1939_session_destroy

[Sabyrzhan Tasbolatov]

On Thu, 8 Aug 2024 19:07:55 +0800, Edward Adam Davis wrote:
> On Thu, 8 Aug 2024 09:49:18 +0200, Oleksij Rempel wrote:
> > > the skb to the queue and increase the skb reference count through it.
> > > 
> > > Reported-and-tested-by: syzbot+ad601904231505ad6617@syzkaller.appspotmail.com
> > > Closes: https://syzkaller.appspot.com/bug?extid=ad601904231505ad6617
> > > Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> > 
> > This patch breaks j1939.
> > The issue can be reproduced by running following commands:
> I tried to reproduce the problem using the following command, but was 
> unsuccessful. Prompt me to install j1939cat and j1939acd, and there are
> some other errors.
> 
> Can you share the logs from when you reproduced the problem?

Hello,

Here is the log of can-tests/j1939/run_all.sh:

# ip link add type vcan
# ip l s dev vcan0 up
# ./run_all.sh vcan0 vcan0
##############################################
run: j1939_ac_100k_dual_can.sh
generate random data for the test
1+0 records in
1+0 records out
102400 bytes (102 kB, 100 KiB) copied, 0.00191192 s, 53.6 MB/s
start j1939acd and j1939cat on vcan0
8321
8323
start j1939acd and j1939cat on vcan0
[  132.211317][ T8326] vcan0: tx drop: invalid sa for name 0x0000000011223340
j1939cat: j1939cat_send_one: transfer error: -99: Cannot assign requested address

It fails here:
https://github.com/linux-can/can-tests/blob/master/j1939/j1939_ac_100k_dual_can.sh#L70

The error message is printed in this condition:
https://elixir.bootlin.com/linux/v6.12-rc2/source/net/can/j1939/address-claim.c#L104-L108

I've applied your patch on the current 6.12.0-rc2 and the syzkaller C repro
doesn't trigger WARNING uaf, refcount anymore though.

== Offtopic:
I wonder if can-tests/j1939 should be refactored from shell to C tests in the
same linux-can/can-tests repository (or even migrate to KUnit tests)
to improve debugging, test coverage. I'd like to understand which syscalls
and params are used j1939cat and j1939acd utils -- currently, tracing with
strace and trace-cmd (ftrace).

> > git clone git@github.com:linux-can/can-tests.git
> > cd can-tests/j1939/
> > ip link add type vcan
> > ip l s dev vcan0 up
> > ./run_all.sh vcan0 vcan0

Re: [PATCH net-next V2] can: j1939: fix uaf warning in j1939_session_destroy

[Oleksij Rempel]

Hi Sabyrzhan,

On Fri, Oct 11, 2024 at 06:41:24PM +0500, Sabyrzhan Tasbolatov wrote:
> On Thu, 8 Aug 2024 19:07:55 +0800, Edward Adam Davis wrote:
> > On Thu, 8 Aug 2024 09:49:18 +0200, Oleksij Rempel wrote:
> > > > the skb to the queue and increase the skb reference count through it.
> > > > 
> > > > Reported-and-tested-by: syzbot+ad601904231505ad6617@syzkaller.appspotmail.com
> > > > Closes: https://syzkaller.appspot.com/bug?extid=ad601904231505ad6617
> > > > Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> > > 
> > > This patch breaks j1939.
> > > The issue can be reproduced by running following commands:
> > I tried to reproduce the problem using the following command, but was 
> > unsuccessful. Prompt me to install j1939cat and j1939acd, and there are
> > some other errors.
> > 
> > Can you share the logs from when you reproduced the problem?
 
ah, i was on vacation and it went under my radar, sorry :(

> Hello,
> 
> Here is the log of can-tests/j1939/run_all.sh:
> 
> # ip link add type vcan
> # ip l s dev vcan0 up
> # ./run_all.sh vcan0 vcan0
> ##############################################
> run: j1939_ac_100k_dual_can.sh
> generate random data for the test
> 1+0 records in
> 1+0 records out
> 102400 bytes (102 kB, 100 KiB) copied, 0.00191192 s, 53.6 MB/s
> start j1939acd and j1939cat on vcan0
> 8321
> 8323
> start j1939acd and j1939cat on vcan0
> [  132.211317][ T8326] vcan0: tx drop: invalid sa for name 0x0000000011223340
> j1939cat: j1939cat_send_one: transfer error: -99: Cannot assign requested address
> 
> It fails here:
> https://github.com/linux-can/can-tests/blob/master/j1939/j1939_ac_100k_dual_can.sh#L70

I assume it is just secondary fail, it probably failed on address claim
stage in j1939acd, so the j1939cat was not able to start transfer due to
missing (not claimed) address.

> The error message is printed in this condition:
> https://elixir.bootlin.com/linux/v6.12-rc2/source/net/can/j1939/address-claim.c#L104-L108
> 
> I've applied your patch on the current 6.12.0-rc2 and the syzkaller C repro
> doesn't trigger WARNING uaf, refcount anymore though.

Yes, because transfer protocol is broken now. 

> == Offtopic:
> I wonder if can-tests/j1939 should be refactored from shell to C tests in the
> same linux-can/can-tests repository (or even migrate to KUnit tests)
> to improve debugging, test coverage. I'd like to understand which syscalls
> and params are used j1939cat and j1939acd utils -- currently, tracing with
> strace and trace-cmd (ftrace).

I have nothing against it, some of them I implemented in C:
https://github.com/linux-can/can-tests/blob/master/j1939/tst-j1939-ac.c#L1160

Right now I do not have enough time to port it, but I can support anyone
who is willing to do it.

Best Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |