Re: [Qemu-devel] [PATCH v8] net: L2TPv3 transport

[Stefan Hajnoczi]

On Sun, Apr 06, 2014 at 03:22:16PM +0100, address@hidden wrote:
> +int net_init_l2tpv3(const NetClientOptions *opts,
> +                    const char *name,
> +                    NetClientState *peer)
> +{
> +
> +
> +    const NetdevL2TPv3Options *l2tpv3;
> +    NetL2TPV3State *s;
> +    NetClientState *nc;
> +    int fd = -1, gairet;
> +    struct addrinfo hints;
> +    struct addrinfo *result = NULL;
> +    char *srcport, *dstport;
> +
> +    nc = qemu_new_net_client(&net_l2tpv3_info, peer, "l2tpv3", name);
> +
> +    s = DO_UPCAST(NetL2TPV3State, nc, nc);
> +
> +    s->queue_head = 0;
> +    s->queue_tail = 0;
> +    s->header_mismatch = false;
> +
> +    assert(opts->kind == NET_CLIENT_OPTIONS_KIND_L2TPV3);
> +    l2tpv3 = opts->l2tpv3;
> +
> +    if (l2tpv3->has_ipv6 && l2tpv3->ipv6) {
> +        s->ipv6 = l2tpv3->ipv6;
> +    } else {
> +        s->ipv6 = false;
> +    }
> +
> +    if ((l2tpv3->has_offset) && (l2tpv3->offset > 256)) {
> +        error_report("l2tpv3_open : offset must be less than 256 bytes");
> +        goto outerr;
> +    }
> +
> +    if (l2tpv3->has_rxcookie || l2tpv3->has_txcookie) {
> +        if (l2tpv3->has_rxcookie && l2tpv3->has_txcookie) {
> +            s->cookie = true;
> +        } else {
> +            goto outerr;
> +        }
> +    } else {
> +        s->cookie = false;
> +    }
> +
> +    if (l2tpv3->has_cookie64 || l2tpv3->cookie64) {
> +        s->cookie_is_64  = true;
> +    } else {
> +        s->cookie_is_64  = false;
> +    }
> +
> +    if (l2tpv3->has_udp && l2tpv3->udp) {
> +        s->udp = true;
> +        if (!(l2tpv3->has_srcport && l2tpv3->has_dstport)) {
> +            error_report("l2tpv3_open : need both src and dst port for udp");
> +            goto outerr;
> +        } else {
> +            srcport = l2tpv3->srcport;
> +            dstport = l2tpv3->dstport;
> +        }
> +    } else {
> +        s->udp = false;
> +        srcport = NULL;
> +        dstport = NULL;
> +    }
> +
> +
> +    s->offset = 4;
> +    s->session_offset = 0;
> +    s->cookie_offset = 4;
> +    s->counter_offset = 4;
> +
> +    s->tx_session = l2tpv3->txsession;
> +    if (l2tpv3->has_rxsession) {
> +        s->rx_session = l2tpv3->rxsession;
> +    } else {
> +        s->rx_session = s->tx_session;
> +    }
> +
> +    if (s->cookie) {
> +        s->rx_cookie = l2tpv3->rxcookie;
> +        s->tx_cookie = l2tpv3->txcookie;
> +        if (s->cookie_is_64 == true) {
> +            /* 64 bit cookie */
> +            s->offset += 8;
> +            s->counter_offset += 8;
> +        } else {
> +            /* 32 bit cookie */
> +            s->offset += 4;
> +            s->counter_offset += 4;
> +        }
> +    }
> +
> +    memset(&hints, 0, sizeof(hints));
> +
> +    if (s->ipv6) {
> +        hints.ai_family = AF_INET6;
> +    } else {
> +        hints.ai_family = AF_INET;
> +    }
> +    if (s->udp) {
> +        hints.ai_socktype = SOCK_DGRAM;
> +        hints.ai_protocol = 0;
> +        s->offset += 4;
> +        s->counter_offset += 4;
> +        s->session_offset += 4;
> +        s->cookie_offset += 4;
> +    } else {
> +        hints.ai_socktype = SOCK_RAW;
> +        hints.ai_protocol = IPPROTO_L2TP;
> +    }
> +
> +    gairet = getaddrinfo(l2tpv3->src, srcport, &hints, &result);
> +
> +    if ((gairet != 0) || (result == NULL)) {
> +        error_report(
> +            "l2tpv3_open : could not resolve src, errno = %s",
> +            gai_strerror(gairet)
> +        );
> +        goto outerr;
> +    }
> +    fd = socket(result->ai_family, result->ai_socktype, result->ai_protocol);
> +    if (fd == -1) {
> +        fd = -errno;
> +        error_report("l2tpv3_open : socket creation failed, errno = %d", 
> -fd);
> +        freeaddrinfo(result);
> +        goto outerr;
> +    }
> +    if (bind(fd, (struct sockaddr *) result->ai_addr, result->ai_addrlen)) {
> +        error_report("l2tpv3_open :  could not bind socket err=%i", errno);
> +        goto outerr;
> +    }
> +
> +    freeaddrinfo(result);

It's a little risky to do this without:
result = NULL;

If we take the goto outerr below (for example, if getaddrinfo() doesn't
assign NULL before returning an error value) or the code is changed in
the future, then we'd get a double-free in the outerr code path.

> +
> +    memset(&hints, 0, sizeof(hints));
> +
> +    if (s->ipv6) {
> +        hints.ai_family = AF_INET6;
> +    } else {
> +        hints.ai_family = AF_INET;
> +    }
> +    if (s->udp) {
> +        hints.ai_socktype = SOCK_DGRAM;
> +        hints.ai_protocol = 0;
> +    } else {
> +        hints.ai_socktype = SOCK_RAW;
> +        hints.ai_protocol = IPPROTO_L2TP;
> +    }
> +
> +    gairet = getaddrinfo(l2tpv3->dst, dstport, &hints, &result);
> +    if ((gairet != 0) || (result == NULL)) {
> +        error_report(
> +            "l2tpv3_open : could not resolve dst, error = %s",
> +            gai_strerror(gairet)
> +        );
> +        goto outerr;
> +    }
> +
> +    s->dgram_dst = g_malloc(sizeof(struct sockaddr_storage));
> +    memset(s->dgram_dst, '\0' , sizeof(struct sockaddr_storage));
> +    memcpy(s->dgram_dst, result->ai_addr, result->ai_addrlen);
> +    s->dst_size = result->ai_addrlen;
> +
> +    freeaddrinfo(result);

Same here.  The assumption is that nothing below will goto outerr.  If
this gets changed in the future it's very easy to forget that we now
need to set result to NULL to prevent double-free.

Please add result = NULL here.

> +
> +    if (l2tpv3->has_counter && l2tpv3->counter) {
> +        s->has_counter = true;
> +        s->offset += 4;
> +    } else {
> +        s->has_counter = false;
> +    }
> +
> +    if (l2tpv3->has_pincounter && l2tpv3->pincounter) {
> +        s->has_counter = true;  /* pin counter implies that there is counter 
> */
> +        s->pin_counter = true;
> +    } else {
> +        s->pin_counter = false;
> +    }
> +
> +    if (l2tpv3->has_offset) {
> +        /* extra offset */
> +        s->offset += l2tpv3->offset;
> +    }
> +
> +    if ((s->ipv6) || (s->udp)) {
> +        s->header_size = s->offset;
> +    } else {
> +        s->header_size = s->offset + sizeof(struct iphdr);
> +    }
> +
> +    s->msgvec = build_l2tpv3_vector(s, MAX_L2TPV3_MSGCNT);
> +    s->vec = g_malloc(sizeof(struct iovec) * MAX_L2TPV3_IOVCNT);
> +    s->header_buf = g_malloc(s->header_size);
> +
> +    qemu_set_nonblock(fd);
> +
> +    s->fd = fd;
> +    s->counter = 0;
> +
> +    l2tpv3_read_poll(s, true);
> +
> +    snprintf(s->nc.info_str, sizeof(s->nc.info_str),
> +             "l2tpv3: connected");
> +    return 0;
> +outerr:
> +    qemu_del_net_client(nc);
> +    if (fd > 0) {
> +        close(fd);
> +    }
> +    if (result) {
> +        freeaddrinfo(result);
> +    }
> +    return -1;
> +}