| Message ID | 20210918092819.156291-1-william.xuanziyang@huawei.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] can: isotp: add result check for wait_event_interruptible() | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 6 of 6 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | warning | WARNING: line length of 85 exceeds 80 columns |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
> Using wait_event_interruptible() to wait for complete transmission, > but do not check the result of wait_event_interruptible() which can > be interrupted. It will result in tx buffer has multiple accessers > and the later process interferes with the previous process. > > Following is one of the problems reported by syzbot. > > ============================================================= > WARNING: CPU: 0 PID: 0 at net/can/isotp.c:840 isotp_tx_timer_handler+0x2e0/0x4c0 > CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.13.0-rc7+ #68 > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.13.0-1ubuntu1 04/01/2014 > RIP: 0010:isotp_tx_timer_handler+0x2e0/0x4c0 > Call Trace: > <IRQ> > ? isotp_setsockopt+0x390/0x390 > __hrtimer_run_queues+0xb8/0x610 > hrtimer_run_softirq+0x91/0xd0 > ? rcu_read_lock_sched_held+0x4d/0x80 > __do_softirq+0xe8/0x553 > irq_exit_rcu+0xf8/0x100 > sysvec_apic_timer_interrupt+0x9e/0xc0 > </IRQ> > asm_sysvec_apic_timer_interrupt+0x12/0x20 > > Add result check for wait_event_interruptible() in isotp_sendmsg() > to avoid multiple accessers for tx buffer. > > Reported-by: syzbot+78bab6958a614b0c80b9@syzkaller.appspotmail.com > Fixes: e057dd3fc20f ("can: add ISO 15765-2:2016 transport protocol") > Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com> Hi Oliver, I send a new patch with this problem, ignore this patch please. Thank you!
On 27.09.21 03:39, Ziyang Xuan (William) wrote: >> Using wait_event_interruptible() to wait for complete transmission, >> but do not check the result of wait_event_interruptible() which can >> be interrupted. It will result in tx buffer has multiple accessers >> and the later process interferes with the previous process. >> >> Following is one of the problems reported by syzbot. >> >> ============================================================= >> WARNING: CPU: 0 PID: 0 at net/can/isotp.c:840 isotp_tx_timer_handler+0x2e0/0x4c0 >> CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.13.0-rc7+ #68 >> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.13.0-1ubuntu1 04/01/2014 >> RIP: 0010:isotp_tx_timer_handler+0x2e0/0x4c0 >> Call Trace: >> <IRQ> >> ? isotp_setsockopt+0x390/0x390 >> __hrtimer_run_queues+0xb8/0x610 >> hrtimer_run_softirq+0x91/0xd0 >> ? rcu_read_lock_sched_held+0x4d/0x80 >> __do_softirq+0xe8/0x553 >> irq_exit_rcu+0xf8/0x100 >> sysvec_apic_timer_interrupt+0x9e/0xc0 >> </IRQ> >> asm_sysvec_apic_timer_interrupt+0x12/0x20 >> >> Add result check for wait_event_interruptible() in isotp_sendmsg() >> to avoid multiple accessers for tx buffer. >> >> Reported-by: syzbot+78bab6958a614b0c80b9@syzkaller.appspotmail.com >> Fixes: e057dd3fc20f ("can: add ISO 15765-2:2016 transport protocol") >> Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com> > > Hi Oliver, > I send a new patch with this problem, ignore this patch please. > > Thank you! > I was still pondering on this patch ;-) But I'll take a look on the update now. Thanks & best regards, Oliver
diff --git a/net/can/isotp.c b/net/can/isotp.c index caaa532ece94..2ac29c2b2ca6 100644 --- a/net/can/isotp.c +++ b/net/can/isotp.c @@ -865,7 +865,9 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size) return -EAGAIN; /* wait for complete transmission of current pdu */ - wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE); + err = wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE); + if (err) + return err; } if (!size || size > MAX_MSG_LENGTH)
Using wait_event_interruptible() to wait for complete transmission, but do not check the result of wait_event_interruptible() which can be interrupted. It will result in tx buffer has multiple accessers and the later process interferes with the previous process. Following is one of the problems reported by syzbot. ============================================================= WARNING: CPU: 0 PID: 0 at net/can/isotp.c:840 isotp_tx_timer_handler+0x2e0/0x4c0 CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.13.0-rc7+ #68 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.13.0-1ubuntu1 04/01/2014 RIP: 0010:isotp_tx_timer_handler+0x2e0/0x4c0 Call Trace: <IRQ> ? isotp_setsockopt+0x390/0x390 __hrtimer_run_queues+0xb8/0x610 hrtimer_run_softirq+0x91/0xd0 ? rcu_read_lock_sched_held+0x4d/0x80 __do_softirq+0xe8/0x553 irq_exit_rcu+0xf8/0x100 sysvec_apic_timer_interrupt+0x9e/0xc0 </IRQ> asm_sysvec_apic_timer_interrupt+0x12/0x20 Add result check for wait_event_interruptible() in isotp_sendmsg() to avoid multiple accessers for tx buffer. Reported-by: syzbot+78bab6958a614b0c80b9@syzkaller.appspotmail.com Fixes: e057dd3fc20f ("can: add ISO 15765-2:2016 transport protocol") Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com> --- net/can/isotp.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)