I'll make a comment that I am quite unhappy about this ioctl
methodology. I don't like void *'s and varying sizes.
I would be much happier if this was a fixed structure, filled with
known objects.
It looks fragile.
Tobias Heider <tobias.hei...@stusta.de> wrote:
> 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;
> > +}
> > +
>
