You can crash a system by running something like:

    for i in 1 2 3; do while true; do ifconfig bridge0 create& ifconfig bridge0 
destroy& done& done

This works with every type of interface I've tried. It appears that
if_clone_destroy and if_clone_create race with other ioctls, which
causes a variety of different UaFs or just general logic errors.

One common root cause appears to be that most ifioctl functions use
ifunit() to find an interface by name, which traverses if_list. Writes
to if_list are protected by a lock, but reads are apparently
unprotected. There's also the question of the life time of the object
returned from ifunit(). Most things that access &ifnet's if_list are
done without locking, and even if those accesses were to be locked, the
lock would be released before the object is no longer used, causing the
UaF in that case as well.

This patch fixes the issue by making if_clone_{create,destroy} exclusive
with all other ifioctls.
---
 sys/net/if.c | 36 +++++++++++++++++++++++++++++++++++-
 1 file changed, 35 insertions(+), 1 deletion(-)

diff --git sys/net/if.c sys/net/if.c
index fb2f86f4a7c..9eedea83d32 100644
--- sys/net/if.c
+++ sys/net/if.c
@@ -246,6 +246,11 @@ struct task if_input_task_locked = 
TASK_INITIALIZER(if_netisr, NULL);
  */
 struct rwlock netlock = RWLOCK_INITIALIZER("netlock");
 
+/*
+ * Protect modifications to objects owned by ifnet's if_list
+ */
+struct rwlock iflist_obj_lock = RWLOCK_INITIALIZER("iflist_obj_lock");
+
 /*
  * Network interface utility routines.
  */
@@ -1905,7 +1910,7 @@ if_setrdomain(struct ifnet *ifp, int rdomain)
  * Interface ioctls.
  */
 int
-ifioctl(struct socket *so, u_long cmd, caddr_t data, struct proc *p)
+ifioctl_unlocked(struct socket *so, u_long cmd, caddr_t data, struct proc *p)
 {
        struct ifnet *ifp;
        struct ifreq *ifr = (struct ifreq *)data;
@@ -2432,6 +2437,35 @@ ifioctl_get(u_long cmd, caddr_t data)
        return (error);
 }
 
+int
+ifioctl(struct socket *so, u_long cmd, caddr_t data, struct proc *p)
+{
+       int ret;
+
+       switch (cmd) {
+       case SIOCIFCREATE:
+       case SIOCIFDESTROY:
+               rw_enter_write(&iflist_obj_lock);
+               break;
+       default:
+               rw_enter_read(&iflist_obj_lock);
+       }
+
+       ret = ifioctl_unlocked(so, cmd, data, p);
+
+       switch (cmd) {
+       case SIOCIFCREATE:
+       case SIOCIFDESTROY:
+               rw_exit_write(&iflist_obj_lock);
+               break;
+       default:
+               rw_exit_read(&iflist_obj_lock);
+       }
+
+       return (ret);
+}
+
+
 static int
 if_sffpage_check(const caddr_t data)
 {
-- 
2.27.0

Reply via email to