lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZB/93xJxq/BUqAgG@dragonet>
Date:   Sun, 26 Mar 2023 17:10:07 +0900
From:   "Dae R. Jeong" <threeearcat@...il.com>
To:     socketcan@...tkopp.net, mkl@...gutronix.de, davem@...emloft.net,
        edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
        linux-can@...r.kernel.org, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: WARNING in isotp_tx_timer_handler and WARNING in print_tainted

Hi,

I am curious about the error handling logic in isotp_sendmsg() which
looks a bit unclear to me.

I was looking the `WARNING in isotp_tx_timer_handler` warning [1],
which was firstly addressed by a commit [2] but reoccured even after
the commit.
[1]: https://syzkaller.appspot.com/bug?id=4f492d593461a5e44d76dd9322e179d13191a8ef
[2]: c6adf659a8ba can: isotp: check CAN address family in isotp_bind()

I thought that the warning is caused by the concurrent execution of
two isotp_sendmsg() as described below (I'm not 100% sure though).

CPU1                             CPU2
isotp_sendmsg()                  isotp_sendmsg()
-----                            -----
old_state = so->tx.state; // ISOTP_IDLE

                                 cmpxchg(&so->tx.state, ISTOP_IDLE, ISOTP_SENDING) // success
							     ...
							     so->tx.state = ISTOP_WAIT_FIRST_FC;
							     hrtimer_start(&so->txtimer);

cmpxchg(&so->tx.state, ISTOP_IDLE, ISOTP_SENDING) // failed
// if MSG_DONTWAIT is set in msg->msg_flags or
// a signal is delivered during wait_event_interruptible()
goto err_out;
err_out:
    so->tx.state = old_state; // ISTOP_IDLE

                                 isotp_tx_timer_handler()
								 -----
								 switch (so->tx.state) {
								 default:
								     WARN_ONCE();
								 }

Then, a commit [3] changed the logic of tx timer, and removed the
WARN_ONCE() statement. So I thought that the issue is completely
handled.
[3]: 4f027cba8216 can: isotp: split tx timer into transmission and timeout

But even after [3] is applied, I found a warning that seems related
occurred [4] (in the kernel commit: 478a351ce0d6).
[4]: https://syzkaller.appspot.com/bug?id=11d0e5f6fef53a0ea486bbd07ddd3cba66132150

So I wonder whether the `err_out` logic in isotp_sendmsg() is safe.
For me, it looks like isotp_sendmsg() can change so->tx.state to
ISTOP_IDLE at any time. It may not be a problem if all other locations
are aware of this. Is this an intended behavior?

Thank you in advance.


Best regards,
Dae R. Jeong

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ