Hi Matt,
i finally found some time to look at your diff and it looks pretty good
to me so far. I have a few question about the SIOCGWG ioctl.
> +void
> +wg_status(void)
> +{
> + size_t i, j, size = 0;
> + struct timespec now;
> + char hbuf[NI_MAXHOST], sbuf[NI_MAXSERV];
> + char key[WG_BASE64_KEY_LEN + 1];
> + struct wg_data_io wgdata;
> + struct wg_interface_io *iface;
> +
> + strlcpy(wgdata.wgd_name, ifname, sizeof(wgdata.wgd_name));
> + wgdata.wgd_size = 0;
> + wgdata.wgd_mem = NULL;
> +
> + if (ioctl(sock, SIOCGWG, (caddr_t)&wgdata) == -1 &&
> + (errno == ENOTTY || errno == EPERM))
> + return;
> +
> + while (size < wgdata.wgd_size) {
> + size = wgdata.wgd_size;
> + wgdata.wgd_mem = realloc(wgdata.wgd_mem, size);
> + if (ioctl(sock, SIOCGWG, (caddr_t)&wgdata) == -1)
> + err(1, "SIOCGWG");
> + }
As I understand it you call the IOCTL at least twice. The first time
you pass in 'wgd_size == 0'. The IOCTL handler calculates the required size
and returns it in wgd_size. Then you pass in a realloced wgdata until the
ioctl returns 0 and the size has not changed.
> +
> + iface = wgdata.wgd_mem;
> +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, aip_o;
> +
> + struct wg_peer *peer;
> + struct wg_aip *aip;
> +
> + size_t size, i;
> + void *mem;
> + int ret = 0;
> +
> + rw_enter_read(&sc->sc_lock);
> +
> + size = sizeof(struct wg_interface_io);
> + 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;
> + size += sizeof(struct wg_aip_io) * sc->sc_aip_num;
> +
> + if (data->wgd_size < size)
> + goto error;
The (data->wgd_size < size) is for the loop in ifconfig. If the input size
was too small you 'goto error', which is an unfortunate name because it
is not actually an error but sets 'wgd_size' to the required 'size' and
returns 0. Maybe 'cleanup' would be a better name.
What I don't understand is why the two previous checks that make sure the
result fits into SIZE_MAX 'goto error' without setting ret != 0 (thus breaking
the loop in ifconfig without data actually being written).
It looks a bit like the error got lost in refactoring.
ERANGE would probably be a good choice:
34 ERANGE Result too large. A result of the function was too large to fit
in the available space (perhaps exceeded precision).
> +
> +error:
> + rw_exit_read(&sc->sc_lock);
> + explicit_bzero(&iface_o, sizeof(iface_o));
> + explicit_bzero(&peer_o, sizeof(peer_o));
> + data->wgd_size = size;
> + return ret;
> +}
> +
