Messages in this thread | | | Date | Wed, 10 Nov 2021 18:05:25 -0800 | From | Jakub Kicinski <> | Subject | Re: [PATCH v1 2/2] hamradio: defer 6pack kfree after unregister_netdev |
| |
On Mon, 8 Nov 2021 18:37:59 +0800 Lin Ma wrote: > There is a possible race condition (use-after-free) like below > > (USE) | (FREE) > dev_queue_xmit | > __dev_queue_xmit | > __dev_xmit_skb | > sch_direct_xmit | ... > xmit_one | > netdev_start_xmit | tty_ldisc_kill > __netdev_start_xmit | 6pack_close > sp_xmit | kfree > sp_encaps | > | > > According to the patch "defer ax25 kfree after unregister_netdev", this
This is the previous patch of the series? Maybe call it "previous patch"?
> patch reorder the kfree after the unregister_netdev to avoid the possible > UAF as the unregister_netdev() is well synchronized and won't return if > there is a running routine. > > Signed-off-by: Lin Ma <linma@zju.edu.cn>
> diff --git a/drivers/net/hamradio/6pack.c b/drivers/net/hamradio/6pack.c > index 49f10053a794..bfdf89e54752 100644 > --- a/drivers/net/hamradio/6pack.c > +++ b/drivers/net/hamradio/6pack.c > @@ -672,11 +672,13 @@ static void sixpack_close(struct tty_struct *tty) > del_timer_sync(&sp->tx_t); > del_timer_sync(&sp->resync_t); > > - /* Free all 6pack frame buffers. */ > + unregister_netdev(sp->dev); > + > + /* Free all 6pack frame buffers after unreg. */ > kfree(sp->rbuff); > kfree(sp->xbuff); > > - unregister_netdev(sp->dev); > + free_netdev(sp->dev);
You should mention in the commit message why you think it's safe to add free_netdev() which wasn't here before...
This driver seems to be setting:
dev->needs_free_netdev = true;
so unregister_netdev() will free the netdev automatically.
That said I don't see a reason why this driver needs the automatic cleanup.
You can either remove that setting and then you can call free_netdev() like you do, or you need to move the cleanup to dev->priv_destructor.
> } > > /* Perform I/O control on an active 6pack channel. */
| |