> On 23 Jun 2020, at 10:00, Jason A. Donenfeld <ja...@zx2c4.com> wrote:
>
> 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);
> + }
> +
You call ifioctl_unlocked() while you holding lock. I guess it’s should be
named ifioctl_locked().
> + 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
>
