Re: [PATCH net] ax25: fix use-after-free bugs caused by ax25_ds_del_timer
From: Simon Horman <horms@kernel.org>
Date: 2024-03-27 19:10:29
Also in:
linux-hams, lkml
On Tue, Mar 26, 2024 at 10:25:42PM +0800, Duoming Zhou wrote:
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().
quoted hunk ↗ jump to hunk
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. Also, not strictly related, I think ax25_dev cannot be NULL, so that check could be dropped. But perhaps that is better left alone. 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.