ax25: add refcount in ax25_dev to avoid UAF bugs

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>
Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/include/net/ax25.h b/include/net/ax25.h
index 526e49589197909b459de068222c4a9cf76050ba..50b417df6221158d2f1a2d566f90f352f4aede8d 100644 (file)
--- 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 44a8730c26accee8c155ab66d409a3de59b4b9fb..32f61978ff29df8a8d5ca40e6d839042f790daff 100644 (file)
--- a/net/ax25/af_ax25.c
+++ b/net/ax25/af_ax25.c
@@ -91,6 +91,7 @@ again:
                        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 256fadb94df3125ddc2f5ed5f8b1c91d6e243546..770b787fb7bbdd1f306f833ce3f1ceff5a8f58d9 100644 (file)
--- 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 d0b2e094bd552f8fa6d767c02a87a9bf317dd709..1e32693833e57397e6fa59fbde7399ff055b20d6 100644 (file)
--- 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;
 }