On 30 Jan 2020, at 16:34, Gleb Smirnoff wrote:

On Tue, Jan 28, 2020 at 10:44:25PM +0000, Kristof Provost wrote:
K> Author: kp
K> Date: Tue Jan 28 22:44:24 2020
K> New Revision: 357233
K> URL: https://svnweb.freebsd.org/changeset/base/357233
K>
K> Log:
K>   epair: Do not abuse params to register the second interface
K>
K> if_epair used the 'params' argument to pass a pointer to the b interface
K>   through if_clone_create().
K> This pointer can be controlled by userspace, which means it could be abused to
K>   trigger a panic. While this requires PRIV_NET_IFCREATE
K> privileges those are assigned to vnet jails, which means that vnet jails
K>   could panic the system.
K>
K>   Reported by:    Ilja Van Sprundel <ivansprun...@ioactive.com>
...
K> Modified: head/sys/net/if_clone.h
K> ==============================================================================
K> --- head/sys/net/if_clone.h       Tue Jan 28 21:46:59 2020        (r357232)
K> +++ head/sys/net/if_clone.h       Tue Jan 28 22:44:24 2020        (r357233)
K> @@ -79,7 +79,8 @@ int     if_clone_list(struct if_clonereq *);
K>  struct if_clone *if_clone_findifc(struct ifnet *);
K>  void     if_clone_addgroup(struct ifnet *, struct if_clone *);
K>
K> -/* The below interface used only by epair(4). */
K> +/* The below interfaces are used only by epair(4). */
K> +void     if_clone_addif(struct if_clone *, struct ifnet *);
K>  int      if_clone_destroyif(struct if_clone *, struct ifnet *);

IMHO, makes sense to move all these declaration into if_epair.c itself.

Yeah, that does make sense.

One minor issue is that it turns out that if_clone_destroyif() isn’t just used by if_epair, but also by the wifi code.

How does this look?

        diff --git a/sys/net/if_clone.c b/sys/net/if_clone.c
        index acc392ead16..452605b0464 100644
        --- a/sys/net/if_clone.c
        +++ b/sys/net/if_clone.c
@@ -106,6 +106,9 @@ static int ifc_simple_match(struct if_clone *, const char *); static int ifc_simple_create(struct if_clone *, char *, size_t, caddr_t);
         static int     ifc_simple_destroy(struct if_clone *, struct ifnet *);

        +/* The below interface is used only by epair(4). */
        +void           if_clone_addif(struct if_clone *, struct ifnet *);
        +
         static struct mtx if_cloners_mtx;
MTX_SYSINIT(if_cloners_lock, &if_cloners_mtx, "if_cloners lock", MTX_DEF);
         VNET_DEFINE_STATIC(int, if_cloners_count);
        diff --git a/sys/net/if_clone.h b/sys/net/if_clone.h
        index ed7d6f4d02d..c1ddf89c72d 100644
        --- a/sys/net/if_clone.h
        +++ b/sys/net/if_clone.h
        @@ -79,8 +79,7 @@ int   if_clone_list(struct if_clonereq *);
         struct if_clone *if_clone_findifc(struct ifnet *);
         void   if_clone_addgroup(struct ifnet *, struct if_clone *);

        -/* The below interfaces are used only by epair(4). */
        -void   if_clone_addif(struct if_clone *, struct ifnet *);
        +/* The below interface is  used only by epair(4) and ieee80211. */
         int    if_clone_destroyif(struct if_clone *, struct ifnet *);

         #endif /* _KERNEL */
        diff --git a/sys/net/if_epair.c b/sys/net/if_epair.c
        index 376bdbe9117..7eff03b840f 100644
        --- a/sys/net/if_epair.c
        +++ b/sys/net/if_epair.c
@@ -94,6 +94,9 @@ SYSCTL_INT(_net_link_epair, OID_AUTO, epair_debug, CTLFLAG_RW,
         #define        DPRINTF(fmt, arg...)
         #endif

        +/* if_clone private function, just for us. */
        +extern void if_clone_addif(struct if_clone *, struct ifnet *);
        +
         static void epair_nh_sintr(struct mbuf *);
static struct mbuf *epair_nh_m2cpuid(struct mbuf *, uintptr_t, u_int *);
         static void epair_nh_drainedcpu(u_int);

Best regards,
Kristof
_______________________________________________
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Reply via email to