From: Claudio Jeker <cjeker@diehard.n-r-g.com>
Subject: Re: neuter tun(4)/tap(4) ioctls that change interface flags
To: David Gwynne <david@gwynne.id.au>
Cc: tech@openbsd.org
Date: Wed, 9 Oct 2024 08:25:08 +0200

On Wed, Oct 09, 2024 at 03:34:06PM +1000, David Gwynne wrote:
> once upon a time there was just tun(4) and it handled both layer 3 (ipv4
> and ipv6) and ethernet. flipping interface type around at runtime was a
> recipe for disaster, so we've been progressively locking this down
> as time goes by splitting ethernet support out into the tap(4)
> interface. tun(4) is set up as a p2p interface with the right interface
> flags for p2p, and tap(4) is set up as an ethernet interface with the
> right interface flags for ethernet.
> 
> we locked down the ability for userland to reconfigure the interface
> type of tun/tap interfaces at runtime, but we still allow the flags to
> change. this diff removes that ability. it still allows the TUNSIFMODE
> and TUNSIFINFO ioctls, but you have to specify the interface flags
> appropriate for the interface type.
> 
> it also removes the ability to set IFF_UP using these ioctls.
> 
> nothing in base uses them, so if there's going to be any fallout it's
> from ports. eg, openvpn does TUNGIFINFO, sets IFF_MULTICAST, and then
> TUNSIFINFO to apply it. because IFF_MULTICAST is set for both tun and
> tap, this is just a waste of cpu time more than anything else, but is
> otherwise handled by this diff. other software may be cleverer^Wdumber
> though.
> 
> thoughts? ok?

I'm partially or even fully responsible for this. I'm all for it to go
away. 

Now this probably needs to go in soon so we can shake out the
cleverer^Wdumber ports. Since that will take some time.

OK claudio@
 
> Index: if_tun.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if_tun.c,v
> diff -u -p -r1.240 if_tun.c
> --- if_tun.c	23 Dec 2023 10:52:54 -0000	1.240
> +++ if_tun.c	8 Oct 2024 12:09:25 -0000
> @@ -101,8 +101,8 @@ int	tundebug = TUN_DEBUG;
>  #define TUNDEBUG(a)	/* (tundebug? printf a : 0) */
>  #endif
>  
> -/* Only these IFF flags are changeable by TUNSIFINFO */
> -#define TUN_IFF_FLAGS (IFF_UP|IFF_POINTOPOINT|IFF_MULTICAST|IFF_BROADCAST)
> +/* Pretend that these IFF flags are changeable by TUNSIFINFO */
> +#define TUN_IFF_FLAGS (IFF_POINTOPOINT|IFF_MULTICAST|IFF_BROADCAST)
>  
>  void	tunattach(int);
>  
> @@ -709,17 +709,18 @@ tun_dev_ioctl(dev_t dev, u_long cmd, voi
>  			error = EINVAL;
>  			break;
>  		}
> +		if (tunp->flags != (sc->sc_if.if_flags & TUN_IFF_FLAGS)) {
> +			error = EINVAL;
> +			break;
> +		}
>  		sc->sc_if.if_mtu = tunp->mtu;
> -		sc->sc_if.if_flags =
> -		    (tunp->flags & TUN_IFF_FLAGS) |
> -		    (sc->sc_if.if_flags & ~TUN_IFF_FLAGS);
>  		sc->sc_if.if_baudrate = tunp->baudrate;
>  		break;
>  	case TUNGIFINFO:
>  		tunp = (struct tuninfo *)data;
>  		tunp->mtu = sc->sc_if.if_mtu;
>  		tunp->type = sc->sc_if.if_type;
> -		tunp->flags = sc->sc_if.if_flags;
> +		tunp->flags = sc->sc_if.if_flags & TUN_IFF_FLAGS;
>  		tunp->baudrate = sc->sc_if.if_baudrate;
>  		break;
>  #ifdef TUN_DEBUG
> @@ -731,13 +732,7 @@ tun_dev_ioctl(dev_t dev, u_long cmd, voi
>  		break;
>  #endif
>  	case TUNSIFMODE:
> -		switch (*(int *)data & (IFF_POINTOPOINT|IFF_BROADCAST)) {
> -		case IFF_POINTOPOINT:
> -		case IFF_BROADCAST:
> -			sc->sc_if.if_flags &= ~TUN_IFF_FLAGS;
> -			sc->sc_if.if_flags |= *(int *)data & TUN_IFF_FLAGS;
> -			break;
> -		default:
> +		if (*(int *)data != (sc->sc_if.if_flags & TUN_IFF_FLAGS)) {
>  			error = EINVAL;
>  			break;
>  		}
> 

-- 
:wq Claudio