Messages in this thread Patch in this message | | | Date | Tue, 15 Mar 2022 13:26:57 +0300 | From | Dan Carpenter <> | Subject | Re: [PATCH net V4 1/2] ax25: Fix refcount leaks caused by ax25_cb_del() |
| |
On Tue, Mar 15, 2022 at 09:54:03AM +0800, Duoming Zhou wrote: > The previous commit d01ffb9eee4a ("ax25: add refcount in ax25_dev to > avoid UAF bugs") and commit feef318c855a ("ax25: fix UAF bugs of > net_device caused by rebinding operation") increase the refcounts of > ax25_dev and net_device in ax25_bind() and decrease the matching refcounts > in ax25_kill_by_device() in order to prevent UAF bugs, but there are > reference count leaks. > > The root cause of refcount leaks is shown below: > > (Thread 1) | (Thread 2) > ax25_bind() | > ... | > ax25_addr_ax25dev() | > ax25_dev_hold() //(1) | > ... | > dev_hold_track() //(2) | > ... | ax25_destroy_socket() > | ax25_cb_del() > | ... > | hlist_del_init() //(3) > | > | > (Thread 3) | > ax25_kill_by_device() | > ... | > ax25_for_each(s, &ax25_list) { | > if (s->ax25_dev == ax25_dev) //(4) | > ... | > > Firstly, we use ax25_bind() to increase the refcount of ax25_dev in > position (1) and increase the refcount of net_device in position (2). > Then, we use ax25_cb_del() invoked by ax25_destroy_socket() to delete > ax25_cb in hlist in position (3) before calling ax25_kill_by_device(). > Finally, the decrements of refcounts in ax25_kill_by_device() will not > be executed, because no s->ax25_dev equals to ax25_dev in position (4). > > This patch adds decrements of refcounts in ax25_release() and use > lock_sock() to do synchronization. If refcounts decrease in ax25_release(), > the decrements of refcounts in ax25_kill_by_device() will not be > executed and vice versa. > > Fixes: d01ffb9eee4a ("ax25: add refcount in ax25_dev to avoid UAF bugs") > Fixes: 87563a043cef ("ax25: fix reference count leaks of ax25_dev") > Fixes: feef318c855a ("ax25: fix UAF bugs of net_device caused by rebinding operation") > Reported-by: Thomas Osterried <thomas@osterried.de> > Signed-off-by: Duoming Zhou <duoming@zju.edu.cn> > --- > Changes in V4: > - Add decrements of refcounts in ax25_release() instead of using any additional variables.
I'm happy that this is simpler. I'm not super happy about the if (sk->sk_wq) check. That seems like a fragile side-effect condition instead of something deliberate. But I don't know networking so maybe this is something which we can rely on.
When you sent the earlier patch then I asked if the devices in ax25_kill_by_device() were always bound and if we could just use a local variable instead of something tied to the ax25_dev struct. I still wonder about that. In other words, could we just do this?
regards, dan carpenter
diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c index 6bd097180772..4af9d9a939c6 100644 --- a/net/ax25/af_ax25.c +++ b/net/ax25/af_ax25.c @@ -78,6 +78,7 @@ static void ax25_kill_by_device(struct net_device *dev) ax25_dev *ax25_dev; ax25_cb *s; struct sock *sk; + bool found = false; if ((ax25_dev = ax25_dev_ax25dev(dev)) == NULL) return; @@ -86,6 +87,7 @@ static void ax25_kill_by_device(struct net_device *dev) again: ax25_for_each(s, &ax25_list) { if (s->ax25_dev == ax25_dev) { + found = true; sk = s->sk; if (!sk) { spin_unlock_bh(&ax25_list_lock); @@ -115,6 +117,11 @@ static void ax25_kill_by_device(struct net_device *dev) } } spin_unlock_bh(&ax25_list_lock); + + if (!found) { + dev_put_track(ax25_dev->dev, &ax25_dev->dev_tracker); + ax25_dev_put(ax25_dev); + } } /*
| |