From: Alexander Bluhm <bluhm@openbsd.org>
Subject: Re: sysctl(2): unlock ipip_sysctl()
To: Vitaliy Makkoveev <mvs@openbsd.org>
Cc: tech@openbsd.org
Date: Tue, 20 Aug 2024 17:51:12 +0200

On Tue, Aug 20, 2024 at 05:54:03PM +0300, Vitaliy Makkoveev wrote:
> - IPIPCTL_ALLOW - atomically accessed integer;
> - IPIPCTL_STATS - per-CPU counters;
> 
> ok?

ipip_input() calls ipip_input_if().  ipip_allow is read in both
functions without lock which can cause inconsistent behaviour.

ipip_allow = 2
ipip_input() passes packet
ipip_allow = 0
ipip_input_if() does local address spoofing check
This code should only be reached if ipip_allow == 1

You have to load ipip_allow in ipip_input() and pass value or flags
to ipip_input_if().

bluhm

> Index: sys/netinet/in_proto.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet/in_proto.c,v
> diff -u -p -r1.110 in_proto.c
> --- sys/netinet/in_proto.c	20 Aug 2024 07:47:25 -0000	1.110
> +++ sys/netinet/in_proto.c	20 Aug 2024 14:51:59 -0000
> @@ -230,7 +230,7 @@ const struct protosw inetsw[] = {
>    .pr_type	= SOCK_RAW,
>    .pr_domain	= &inetdomain,
>    .pr_protocol	= IPPROTO_IPV4,
> -  .pr_flags	= PR_ATOMIC|PR_ADDR|PR_MPSOCKET,
> +  .pr_flags	= PR_ATOMIC|PR_ADDR|PR_MPSOCKET|PR_MPSYSCTL,
>  #if NGIF > 0
>    .pr_input	= in_gif_input,
>  #else
> Index: sys/netinet/ip_ipip.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet/ip_ipip.c,v
> diff -u -p -r1.102 ip_ipip.c
> --- sys/netinet/ip_ipip.c	17 May 2024 20:44:36 -0000	1.102
> +++ sys/netinet/ip_ipip.c	20 Aug 2024 14:51:59 -0000
> @@ -72,6 +72,11 @@
>  #include <net/pfvar.h>
>  #endif
>  
> +/*
> + * Locks used to protect data:
> + *	a	atomic
> + */
> +
>  #ifdef ENCDEBUG
>  #define DPRINTF(fmt, args...)						\
>  	do {								\
> @@ -87,7 +92,7 @@
>   * We can control the acceptance of IP4 packets by altering the sysctl
>   * net.inet.ipip.allow value.  Zero means drop them, all else is acceptance.
>   */
> -int ipip_allow = 0;
> +int ipip_allow = 0;	/* [a] */
>  
>  struct cpumem *ipipcounters;
>  
> @@ -106,7 +111,8 @@ ipip_input(struct mbuf **mp, int *offp, 
>  	struct ifnet *ifp;
>  
>  	/* If we do not accept IP-in-IP explicitly, drop.  */
> -	if (!ipip_allow && ((*mp)->m_flags & (M_AUTH|M_CONF)) == 0) {
> +	if (atomic_load_int(&ipip_allow) == 0 &&
> +	    ((*mp)->m_flags & (M_AUTH|M_CONF)) == 0) {
>  		DPRINTF("dropped due to policy");
>  		ipipstat_inc(ipips_pdrops);
>  		m_freemp(mp);
> @@ -271,7 +277,8 @@ ipip_input_if(struct mbuf **mp, int *off
>  	}
>  
>  	/* Check for local address spoofing. */
> -	if (!(ifp->if_flags & IFF_LOOPBACK) && ipip_allow != 2) {
> +	if (!(ifp->if_flags & IFF_LOOPBACK) &&
> +	    atomic_load_int(&ipip_allow) != 2) {
>  		struct sockaddr_storage ss;
>  		struct rtentry *rt;
>  
> @@ -584,19 +591,14 @@ int
>  ipip_sysctl(int *name, u_int namelen, void *oldp, size_t *oldlenp, void *newp,
>      size_t newlen)
>  {
> -	int error;
> -
>  	/* All sysctl names at this level are terminal. */
>  	if (namelen != 1)
>  		return (ENOTDIR);
>  
>  	switch (name[0]) {
>  	case IPIPCTL_ALLOW:
> -		NET_LOCK();
> -		error = sysctl_int_bounded(oldp, oldlenp, newp, newlen,
> -		    &ipip_allow, 0, 2);
> -		NET_UNLOCK();
> -		return (error);
> +		return (sysctl_int_bounded(oldp, oldlenp, newp, newlen,
> +		    &ipip_allow, 0, 2));
>  	case IPIPCTL_STATS:
>  		return (ipip_sysctl_ipipstat(oldp, oldlenp, newp));
>  	default: