| Message ID | 855641b37699b6ff501c4bae8370d26f59da9c81.1643343397.git.duoming@zju.edu.cn (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | d01ffb9eee4af165d83b08dd73ebdf9fe94a519b |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ax25: fix NPD and UAF bugs when detaching ax25 device | expand |
On Fri, Jan 28, 2022 at 12:47:16PM +0800, Duoming Zhou wrote: > diff --git a/include/net/ax25.h b/include/net/ax25.h > index 526e4958919..50b417df622 100644 > --- a/include/net/ax25.h > +++ b/include/net/ax25.h > @@ -239,6 +239,7 @@ typedef struct ax25_dev { > #if defined(CONFIG_AX25_DAMA_SLAVE) || defined(CONFIG_AX25_DAMA_MASTER) > ax25_dama_info dama; > #endif > + refcount_t refcount; > } ax25_dev; > > typedef struct ax25_cb { > @@ -293,6 +294,15 @@ static __inline__ void ax25_cb_put(ax25_cb *ax25) > } > } > > +#define ax25_dev_hold(__ax25_dev) \ > + refcount_inc(&((__ax25_dev)->refcount)) Make this an inline function. > + > +static __inline__ void ax25_dev_put(ax25_dev *ax25_dev) Please run checkpatch.pl --strict on your patches. s/__inline__/inline/ > +{ > + if (refcount_dec_and_test(&ax25_dev->refcount)) { > + kfree(ax25_dev); > + } Delete the extra curly braces. > +} > static inline __be16 ax25_type_trans(struct sk_buff *skb, struct net_device *dev) > { > skb->dev = dev; > diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c > index 44a8730c26a..32f61978ff2 100644 > --- a/net/ax25/af_ax25.c > +++ b/net/ax25/af_ax25.c > @@ -91,6 +91,7 @@ static void ax25_kill_by_device(struct net_device *dev) > spin_unlock_bh(&ax25_list_lock); > lock_sock(sk); > s->ax25_dev = NULL; > + ax25_dev_put(ax25_dev); > release_sock(sk); > ax25_disconnect(s, ENETUNREACH); > spin_lock_bh(&ax25_list_lock); > @@ -439,6 +440,7 @@ static int ax25_ctl_ioctl(const unsigned int cmd, void __user *arg) > } > > out_put: > + ax25_dev_put(ax25_dev); The ax25_ctl_ioctl() has a ton of reference leak paths now. Almost every return -ESOMETHING needs to be fixed. > ax25_cb_put(ax25); > return ret; > > diff --git a/net/ax25/ax25_dev.c b/net/ax25/ax25_dev.c > index 256fadb94df..770b787fb7b 100644 > --- a/net/ax25/ax25_dev.c > +++ b/net/ax25/ax25_dev.c > @@ -37,6 +37,7 @@ ax25_dev *ax25_addr_ax25dev(ax25_address *addr) > for (ax25_dev = ax25_dev_list; ax25_dev != NULL; ax25_dev = ax25_dev->next) > if (ax25cmp(addr, (const ax25_address *)ax25_dev->dev->dev_addr) == 0) { > res = ax25_dev; > + ax25_dev_hold(ax25_dev); > } > spin_unlock_bh(&ax25_dev_lock); > > @@ -56,6 +57,7 @@ void ax25_dev_device_up(struct net_device *dev) > return; > } > > + refcount_set(&ax25_dev->refcount, 1); > dev->ax25_ptr = ax25_dev; > ax25_dev->dev = dev; > dev_hold_track(dev, &ax25_dev->dev_tracker, GFP_ATOMIC); > @@ -83,6 +85,7 @@ void ax25_dev_device_up(struct net_device *dev) > spin_lock_bh(&ax25_dev_lock); > ax25_dev->next = ax25_dev_list; > ax25_dev_list = ax25_dev; > + ax25_dev_hold(ax25_dev); > spin_unlock_bh(&ax25_dev_lock); > > ax25_register_dev_sysctl(ax25_dev); > @@ -112,20 +115,22 @@ void ax25_dev_device_down(struct net_device *dev) > > if ((s = ax25_dev_list) == ax25_dev) { > ax25_dev_list = s->next; > + ax25_dev_put(ax25_dev); Do we not have to call ax25_dev_hold(s->next)? > spin_unlock_bh(&ax25_dev_lock); > dev->ax25_ptr = NULL; > dev_put_track(dev, &ax25_dev->dev_tracker); > - kfree(ax25_dev); > + ax25_dev_put(ax25_dev); > return; > } > > while (s != NULL && s->next != NULL) { > if (s->next == ax25_dev) { > s->next = ax25_dev->next; > + ax25_dev_put(ax25_dev); ax25_dev_hold(ax25_dev->next)? > spin_unlock_bh(&ax25_dev_lock); > dev->ax25_ptr = NULL; > dev_put_track(dev, &ax25_dev->dev_tracker); > - kfree(ax25_dev); > + ax25_dev_put(ax25_dev); > return; > } > > @@ -133,6 +138,7 @@ void ax25_dev_device_down(struct net_device *dev) > } > spin_unlock_bh(&ax25_dev_lock); > dev->ax25_ptr = NULL; > + ax25_dev_put(ax25_dev); > } > > int ax25_fwd_ioctl(unsigned int cmd, struct ax25_fwd_struct *fwd) > @@ -149,6 +155,7 @@ int ax25_fwd_ioctl(unsigned int cmd, struct ax25_fwd_struct *fwd) > if (ax25_dev->forward != NULL) > return -EINVAL; Every return -ERROR; in this function leaks reference counts. This one should drop the reference for both fwd_dev and ax25_dev. > ax25_dev->forward = fwd_dev->dev; > + ax25_dev_put(fwd_dev); > break; > > case SIOCAX25DELFWD: regards, dan carpenter
On Fri, Jan 28, 2022 at 12:47:16PM +0800, Duoming Zhou wrote: > If we dereference ax25_dev after we call kfree(ax25_dev) in > ax25_dev_device_down(), it will lead to concurrency UAF bugs. > There are eight syscall functions suffer from UAF bugs, include > ax25_bind(), ax25_release(), ax25_connect(), ax25_ioctl(), > ax25_getname(), ax25_sendmsg(), ax25_getsockopt() and > ax25_info_show(). > > One of the concurrency UAF can be shown as below: > > (USE) | (FREE) > | ax25_device_event > | ax25_dev_device_down > ax25_bind | ... > ... | kfree(ax25_dev) > ax25_fillin_cb() | ... > ax25_fillin_cb_from_dev() | > ... | > > The root cause of UAF bugs is that kfree(ax25_dev) in > ax25_dev_device_down() is not protected by any locks. > When ax25_dev, which there are still pointers point to, > is released, the concurrency UAF bug will happen. > > This patch introduces refcount into ax25_dev in order to > guarantee that there are no pointers point to it when ax25_dev > is released. > > Signed-off-by: Duoming Zhou <duoming@zju.edu.cn> I pointed out a few bugs in my previous email. I've had more time to look at it now. Basically you just want to audit all the calls sites which call ax25_dev_ax25dev() and make sure all the error paths decrement. Most of them are buggy. I'm testing a new Smatch check which is supposed to detect these sorts of bugs. I think the refcount in ax25_bind() needs a matching decrement. Where is that? I don't know networking well enough to know the answer to this... > @@ -112,20 +115,22 @@ void ax25_dev_device_down(struct net_device *dev) > > if ((s = ax25_dev_list) == ax25_dev) { > ax25_dev_list = s->next; > + ax25_dev_put(ax25_dev); It would be more readable to do ax25_dev_put(ax25_dev_list). It's weird to put ax25_dev here and then a couple lines later > spin_unlock_bh(&ax25_dev_lock); > dev->ax25_ptr = NULL; > dev_put_track(dev, &ax25_dev->dev_tracker); > - kfree(ax25_dev); > + ax25_dev_put(ax25_dev); Here > return; > } > > while (s != NULL && s->next != NULL) { > if (s->next == ax25_dev) { > s->next = ax25_dev->next; > + ax25_dev_put(ax25_dev); Same. > spin_unlock_bh(&ax25_dev_lock); > dev->ax25_ptr = NULL; > dev_put_track(dev, &ax25_dev->dev_tracker); > - kfree(ax25_dev); > + ax25_dev_put(ax25_dev); > return; > } > regards, dan carpenter
Thank you very much for your time and pointing out problems in my patch. The decrement of ax25_bind() is in ax25_kill_by_device(). If we don't call ax25_bind() before ax25_kill_by_device(), the ax25_list will be empty and ax25_dev_put() in ax25_kill_by_device() will not execute. > @@ -91,6 +91,7 @@ static void ax25_kill_by_device(struct net_device *dev) > spin_unlock_bh(&ax25_list_lock); > lock_sock(sk); > s->ax25_dev = NULL; > + ax25_dev_put(ax25_dev); > release_sock(sk); > ax25_disconnect(s, ENETUNREACH); > spin_lock_bh(&ax25_list_lock); I will send the improved patch as soon as possible. Best wishes, Duoming Zhou
Thank you very much for your time and pointing out problems in my patch. Another two questions you asked is shown below: > @@ -112,20 +115,22 @@ void ax25_dev_device_down(struct net_device *dev) > > if ((s = ax25_dev_list) == ax25_dev) { > ax25_dev_list = s->next; > + ax25_dev_put(ax25_dev); Do we not have to call ax25_dev_hold(s->next)? > spin_unlock_bh(&ax25_dev_lock); > dev->ax25_ptr = NULL; > dev_put_track(dev, &ax25_dev->dev_tracker); > - kfree(ax25_dev); > + ax25_dev_put(ax25_dev); > return; > } > > while (s != NULL && s->next != NULL) { > if (s->next == ax25_dev) { > s->next = ax25_dev->next; > + ax25_dev_put(ax25_dev); ax25_dev_hold(ax25_dev->next)? Answer: We don't have to call ax25_dev_hold(s->next) or ax25_dev_hold(ax25_dev->next) in ax25_dev_device_down() because we have already increased the refcount when we insert ax25_dev into the linked list in ax25_dev_device_up(). > @@ -83,6 +85,7 @@ void ax25_dev_device_up(struct net_device *dev) > spin_lock_bh(&ax25_dev_lock); > ax25_dev->next = ax25_dev_list; > ax25_dev_list = ax25_dev; > + ax25_dev_hold(ax25_dev); > spin_unlock_bh(&ax25_dev_lock); Best wishes, Duoming Zhou
diff --git a/include/net/ax25.h b/include/net/ax25.h index 526e4958919..50b417df622 100644 --- a/include/net/ax25.h +++ b/include/net/ax25.h @@ -239,6 +239,7 @@ typedef struct ax25_dev { #if defined(CONFIG_AX25_DAMA_SLAVE) || defined(CONFIG_AX25_DAMA_MASTER) ax25_dama_info dama; #endif + refcount_t refcount; } ax25_dev; typedef struct ax25_cb { @@ -293,6 +294,15 @@ static __inline__ void ax25_cb_put(ax25_cb *ax25) } } +#define ax25_dev_hold(__ax25_dev) \ + refcount_inc(&((__ax25_dev)->refcount)) + +static __inline__ void ax25_dev_put(ax25_dev *ax25_dev) +{ + if (refcount_dec_and_test(&ax25_dev->refcount)) { + kfree(ax25_dev); + } +} static inline __be16 ax25_type_trans(struct sk_buff *skb, struct net_device *dev) { skb->dev = dev; diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c index 44a8730c26a..32f61978ff2 100644 --- a/net/ax25/af_ax25.c +++ b/net/ax25/af_ax25.c @@ -91,6 +91,7 @@ static void ax25_kill_by_device(struct net_device *dev) spin_unlock_bh(&ax25_list_lock); lock_sock(sk); s->ax25_dev = NULL; + ax25_dev_put(ax25_dev); release_sock(sk); ax25_disconnect(s, ENETUNREACH); spin_lock_bh(&ax25_list_lock); @@ -439,6 +440,7 @@ static int ax25_ctl_ioctl(const unsigned int cmd, void __user *arg) } out_put: + ax25_dev_put(ax25_dev); ax25_cb_put(ax25); return ret; diff --git a/net/ax25/ax25_dev.c b/net/ax25/ax25_dev.c index 256fadb94df..770b787fb7b 100644 --- a/net/ax25/ax25_dev.c +++ b/net/ax25/ax25_dev.c @@ -37,6 +37,7 @@ ax25_dev *ax25_addr_ax25dev(ax25_address *addr) for (ax25_dev = ax25_dev_list; ax25_dev != NULL; ax25_dev = ax25_dev->next) if (ax25cmp(addr, (const ax25_address *)ax25_dev->dev->dev_addr) == 0) { res = ax25_dev; + ax25_dev_hold(ax25_dev); } spin_unlock_bh(&ax25_dev_lock); @@ -56,6 +57,7 @@ void ax25_dev_device_up(struct net_device *dev) return; } + refcount_set(&ax25_dev->refcount, 1); dev->ax25_ptr = ax25_dev; ax25_dev->dev = dev; dev_hold_track(dev, &ax25_dev->dev_tracker, GFP_ATOMIC); @@ -83,6 +85,7 @@ void ax25_dev_device_up(struct net_device *dev) spin_lock_bh(&ax25_dev_lock); ax25_dev->next = ax25_dev_list; ax25_dev_list = ax25_dev; + ax25_dev_hold(ax25_dev); spin_unlock_bh(&ax25_dev_lock); ax25_register_dev_sysctl(ax25_dev); @@ -112,20 +115,22 @@ void ax25_dev_device_down(struct net_device *dev) if ((s = ax25_dev_list) == ax25_dev) { ax25_dev_list = s->next; + ax25_dev_put(ax25_dev); spin_unlock_bh(&ax25_dev_lock); dev->ax25_ptr = NULL; dev_put_track(dev, &ax25_dev->dev_tracker); - kfree(ax25_dev); + ax25_dev_put(ax25_dev); return; } while (s != NULL && s->next != NULL) { if (s->next == ax25_dev) { s->next = ax25_dev->next; + ax25_dev_put(ax25_dev); spin_unlock_bh(&ax25_dev_lock); dev->ax25_ptr = NULL; dev_put_track(dev, &ax25_dev->dev_tracker); - kfree(ax25_dev); + ax25_dev_put(ax25_dev); return; } @@ -133,6 +138,7 @@ void ax25_dev_device_down(struct net_device *dev) } spin_unlock_bh(&ax25_dev_lock); dev->ax25_ptr = NULL; + ax25_dev_put(ax25_dev); } int ax25_fwd_ioctl(unsigned int cmd, struct ax25_fwd_struct *fwd) @@ -149,6 +155,7 @@ int ax25_fwd_ioctl(unsigned int cmd, struct ax25_fwd_struct *fwd) if (ax25_dev->forward != NULL) return -EINVAL; ax25_dev->forward = fwd_dev->dev; + ax25_dev_put(fwd_dev); break; case SIOCAX25DELFWD: @@ -161,6 +168,7 @@ int ax25_fwd_ioctl(unsigned int cmd, struct ax25_fwd_struct *fwd) return -EINVAL; } + ax25_dev_put(ax25_dev); return 0; } diff --git a/net/ax25/ax25_route.c b/net/ax25/ax25_route.c index d0b2e094bd5..1e32693833e 100644 --- a/net/ax25/ax25_route.c +++ b/net/ax25/ax25_route.c @@ -116,6 +116,7 @@ static int __must_check ax25_rt_add(struct ax25_routes_struct *route) ax25_rt->dev = ax25_dev->dev; ax25_rt->digipeat = NULL; ax25_rt->ip_mode = ' '; + ax25_dev_put(ax25_dev); if (route->digi_count != 0) { if ((ax25_rt->digipeat = kmalloc(sizeof(ax25_digi), GFP_ATOMIC)) == NULL) { write_unlock_bh(&ax25_route_lock); @@ -172,6 +173,7 @@ static int ax25_rt_del(struct ax25_routes_struct *route) } } } + ax25_dev_put(ax25_dev); write_unlock_bh(&ax25_route_lock); return 0; @@ -214,6 +216,7 @@ static int ax25_rt_opt(struct ax25_route_opt_struct *rt_option) } out: + ax25_dev_put(ax25_dev); write_unlock_bh(&ax25_route_lock); return err; }
If we dereference ax25_dev after we call kfree(ax25_dev) in ax25_dev_device_down(), it will lead to concurrency UAF bugs. There are eight syscall functions suffer from UAF bugs, include ax25_bind(), ax25_release(), ax25_connect(), ax25_ioctl(), ax25_getname(), ax25_sendmsg(), ax25_getsockopt() and ax25_info_show(). One of the concurrency UAF can be shown as below: (USE) | (FREE) | ax25_device_event | ax25_dev_device_down ax25_bind | ... ... | kfree(ax25_dev) ax25_fillin_cb() | ... ax25_fillin_cb_from_dev() | ... | The root cause of UAF bugs is that kfree(ax25_dev) in ax25_dev_device_down() is not protected by any locks. When ax25_dev, which there are still pointers point to, is released, the concurrency UAF bug will happen. This patch introduces refcount into ax25_dev in order to guarantee that there are no pointers point to it when ax25_dev is released. Signed-off-by: Duoming Zhou <duoming@zju.edu.cn> --- include/net/ax25.h | 10 ++++++++++ net/ax25/af_ax25.c | 2 ++ net/ax25/ax25_dev.c | 12 ++++++++++-- net/ax25/ax25_route.c | 3 +++ 4 files changed, 25 insertions(+), 2 deletions(-)