Messages in this thread | | | Date | Wed, 27 Mar 2024 19:10:25 +0000 | From | Simon Horman <> | Subject | Re: [PATCH net] ax25: fix use-after-free bugs caused by ax25_ds_del_timer |
| |
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().
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") > Signed-off-by: Duoming Zhou <duoming@zju.edu.cn> > --- > 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.
| |