On Tue, 26 May 2020 13:28:22 +0200
Tobias Heider <tobias.hei...@stusta.de> 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;
> > +}  
> 

Reply via email to