On Thu, Mar 28, 2024 at 01:34:48PM +0800, duoming@zju.edu.cn wrote:

On Wed, 27 Mar 2024 19:10:25 +0000 Simon Horman wrote:

quoted

quoted

When the ax25 device is detaching, the ax25_dev_device_down()
calls ax25_ds_del_timer() to cleanup the slave_timer. When
the timer handler is running, the ax25_ds_del_timer() that
calls del_timer() in it will return directly. As a result,
the use-after-free bugs could happen, one of the scenarios
is shown below:

      (Thread 1)          |      (Thread 2)
                          | ax25_ds_timeout()
ax25_dev_device_down()    |
  ax25_ds_del_timer()     |
    del_timer()           |
  ax25_dev_put() //FREE   |
                          |  ax25_dev-> //USE

In order to mitigate bugs, when the device is detaching, use
timer_shutdown_sync() to stop the timer.

FWIIW, in my reading of things there is another failure mode whereby
ax25_ds_timeout may rearm the timer after the call to del_timer() but
before the call to ax25_dev_put().

I think using timer_shutdown_sync() or del_timer_sync() to replace del_timer()
could prevent the rearm.

I think only timer_shutdown() and timer_shutdown_sync() will prevent a
rearm. But I also think (but am not entirely sure) this is only important
in the ax25_dev_device_down() case (there are others, as you mention
below).

quoted

quoted

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Duoming Zhou <redacted>
---
 net/ax25/ax25_ds_timer.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/net/ax25/ax25_ds_timer.c b/net/ax25/ax25_ds_timer.c
index c4f8adbf814..5624c0d174c 100644
--- a/net/ax25/ax25_ds_timer.c
+++ b/net/ax25/ax25_ds_timer.c

@@ -43,7 +43,12 @@ void ax25_ds_setup_timer(ax25_dev *ax25_dev)
 
 void ax25_ds_del_timer(ax25_dev *ax25_dev)
 {
-	if (ax25_dev)
+	if (!ax25_dev)
+		return;
+
+	if (!ax25_dev->device_up)
+		timer_shutdown_sync(&ax25_dev->dama.slave_timer);
+	else
 		del_timer(&ax25_dev->dama.slave_timer);
 }

I think that a) it is always correct to call timer_shutdown_sync,
and b) ax25_dev->device_up is always true. So a call to
timer_shutdown_sync can simply replace the call to del_timer.

I think timer_shutdown*() is used for the code path to clean up the
driver or detach the device. If timer is shut down by timer_shutdown*(),
it could not be re-armed again unless we reinitialize the timer. The
slave_timer should only be shut down when the ax25 device is detaching or
the driver is removing. And it should not be shut down in other scenarios,
such as called in ax25_ds_state2_machine() or ax25_ds_state3_machine().
So I think calling timer_shutdown_sync() is not always correct.

What's more, the ax25_dev->device_up is not always true. It is set to
false in ax25_kill_by_device().

In a word, the timer_shutdown_sync() could not replace the del_timer()
completely.

Yes, sorry. I missed that ax25_ds_del_timer() is not
only called from ax25_dev_device_down().

I'm actually getting to think that your original patch was correct.
But perhaps a different approach would be to simply call
timer_shutdown_sync() in ax25_dev_device_down(). And leave
ax25_ds_del_timer() alone.

quoted

Zooming out a bit, has removal of ax25 been considered.
I didn't check the logs thoroughly, but I'm not convinced it's been
maintained - other than clean-ups and by-inspection bug fixes - since git
history began.

Best regards,
Duoming Zhou