On Tue, May 26, 2020 at 07:39:01PM +1000, Matt Dunwoodie wrote:
> Hi tech,
>
> After some feedback and comments, we've addressed the concerns, and
> fixed a few things from our side too. Overall the structure is familiar
> with no major changes, so any prior readings mostly carry over.
>
> This is a quick summary of the changes. Detailed changes can be found
> on the out-of-tree repo or we can elaborate if more detail is needed.
>
> * Allow non-root to read WireGuard network configuration of the
> interface.
> - This allows users to help debug issues without
> escalating to root.
>
> * Removal of wgconf option from ifconfig.
> - This option was not mature enough for inclusion into base. Upon
> reflection, this behaviour probably needs further consideration
> about how to integrate into ifconfig.
>
> * Fix ioctl interface.
> - The linked list setup was fragile, complicated, and broken, so
> we've gone with something much simpler, and user easy to program
> for, using a boring ioctl pattern that's been tried successfully
> elsewhere already.
>
> * Limit the padding of the packet to the MTU.
> - Fits the protocol spec and prevents unneccessary fragmentation.
>
> * Align handshake/identity locks and timer semantics with spec and other
> implementations.
> - This avoids any subtle edge cases that may occur.
>
> Here are the changes that shouldn't need any explanation:
>
> * Updated documentation based on feedback from tech and others.
> * Maintain order of peers when adding/deleting .
> * Rename noise_alloc to noise_upcall.
> * Remove wg_timers_fn indirection.
>
> Minor formatting changes may also be present.
>
> Once someone picks this up, we're more than happy to help integrate
> this and maintain it in-tree.
>
> Signed
> Matt Dunwoodie
Hi Matt,
just repeating what I commented yesterday for the new diff to make sure
it isn't overlooked.
> +int
> +wg_ioctl_get(struct wg_softc *sc, struct wg_data_io *data)
> +{
> + struct wg_interface_io *iface_p, iface_o;
> + struct wg_peer_io *peer_p, peer_o;
> + struct wg_aip_io *aip_p;
> +
> + struct wg_peer *peer;
> + struct wg_aip *aip;
> +
> + size_t size, peer_count, aip_count;
> + int ret = 0, is_suser = suser(curproc) == 0;
> +
> + size = sizeof(struct wg_interface_io);
> + if (data->wgd_size < size && !is_suser)
> + goto ret_size;
> +
> + iface_p = data->wgd_interface;
> + bzero(&iface_o, sizeof(iface_o));
> +
> + rw_enter_read(&sc->sc_lock);
> +
> + if (sc->sc_udp_port != 0) {
> + iface_o.i_port = ntohs(sc->sc_udp_port);
> + iface_o.i_flags |= WG_INTERFACE_HAS_PORT;
> + }
> +
> + if (sc->sc_udp_rtable != 0) {
> + iface_o.i_rtable = sc->sc_udp_rtable;
> + iface_o.i_flags |= WG_INTERFACE_HAS_RTABLE;
> + }
> +
> + if (!is_suser)
> + goto out;
> +
> + if (noise_local_keys(&sc->sc_local, iface_o.i_public,
> + iface_o.i_private) == 0) {
> + iface_o.i_flags |= WG_INTERFACE_HAS_PUBLIC;
> + iface_o.i_flags |= WG_INTERFACE_HAS_PRIVATE;
> + }
> +
> + if (((SIZE_MAX - size) / sizeof(struct wg_peer_io)) < sc->sc_peer_num)
> + goto error;
> + size += sizeof(struct wg_peer_io) * sc->sc_peer_num;
> + if (((SIZE_MAX - size) / sizeof(struct wg_aip_io)) < sc->sc_aip_num)
> + goto error;
I still think those two should return an error. 'goto error' is misleading as
it doesn't actually set ret != 0. 'error' should probably be renamed
to 'cleanup' to avoid confusion and ret should be set to ERANGE .
> + size += sizeof(struct wg_aip_io) * sc->sc_aip_num;
> + if (data->wgd_size < size)
> + goto error;
> +
> + peer_count = 0;
> + peer_p = &iface_p->i_peers[0];
> + WG_PEERS_FOREACH(peer, sc) {
> + bzero(&peer_o, sizeof(peer_o));
> + peer_o.p_flags = WG_PEER_HAS_PUBLIC;
> + peer_o.p_protocol_version = 1;
> +
> + if (noise_remote_keys(&peer->p_remote, peer_o.p_public,
> + peer_o.p_psk) == 0)
> + peer_o.p_flags |= WG_PEER_HAS_PSK;
> +
> + if (wg_timers_get_persistent_keepalive(&peer->p_timers,
> + &peer_o.p_pka) == 0)
> + peer_o.p_flags |= WG_PEER_HAS_PKA;
> +
> + if (wg_peer_get_sockaddr(peer, &peer_o.p_sa) == 0)
> + peer_o.p_flags |= WG_PEER_HAS_ENDPOINT;
> +
> + mtx_enter(&peer->p_counters_mtx);
> + peer_o.p_txbytes = peer->p_counters_tx;
> + peer_o.p_rxbytes = peer->p_counters_rx;
> + mtx_leave(&peer->p_counters_mtx);
> +
> + wg_timers_get_last_handshake(&peer->p_timers,
> + &peer_o.p_last_handshake);
> +
> + aip_count = 0;
> + aip_p = &peer_p->p_aips[0];
> + LIST_FOREACH(aip, &peer->p_aip, a_entry) {
> + if ((ret = copyout(&aip->a_data, aip_p,
> sizeof(*aip_p))) != 0)
> + goto error;
> + aip_p++;
> + aip_count++;
> + }
> + peer_o.p_aips_count = aip_count;
> +
> + if ((ret = copyout(&peer_o, peer_p, sizeof(peer_o))) != 0)
> + goto error;
> +
> + peer_p = (struct wg_peer_io *)aip_p;
> + peer_count++;
> + }
> + iface_o.i_peers_count = peer_count;
> +
> +out:
> + if ((ret = copyout(&iface_o, iface_p, sizeof(iface_o))) != 0)
> + goto error;
> +error:
> + rw_exit_read(&sc->sc_lock);
> + explicit_bzero(&iface_o, sizeof(iface_o));
> + explicit_bzero(&peer_o, sizeof(peer_o));
> +ret_size:
> + data->wgd_size = size;
> + return ret;
> +}
