On Tue, 26 May 2020 13:28:22 +0200
Tobias Heider <[email protected]> wrote:
> Hi Matt,
>
> just repeating what I commented yesterday for the new diff to make
> sure it isn't overlooked.
Thank you for repeating it, I didn't get around to addressing it before
the new diff.
> > +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 .
I'll elaborate on this a bit. These goto errors are just checks for
integer overflows on size. Upon further consideration it probably
doesn't make sense if we are using a size_t. Therefore it makes sense
to remove these two checks.
> > + size += sizeof(struct wg_aip_io) * sc->sc_aip_num;
> > + if (data->wgd_size < size)
> > + goto error;
This has been changed to "goto unlock_and_ret_size;". I think Jason
touched on it, but the problem is that if we return ERANGE, then
wg_data_io does not get copied back to userspace and the caller cannot
read the returned size. Therefore we need to return 0 to indicate no
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;
> > +}
>
