Re: [Qemu-devel] [PATCH 6/7 v5] Packet abstraction used by VMWARE network devices

[Anthony Liguori]

On 03/18/2012 04:27 AM, Dmitry Fleytman wrote:
> Signed-off-by: Dmitry Fleytman<address@hidden>
> Signed-off-by: Yan Vugenfirer<address@hidden>
> ---
>   hw/vmxnet_pkt.c | 1243 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   hw/vmxnet_pkt.h |  479 +++++++++++++++++++++
>   2 files changed, 1722 insertions(+), 0 deletions(-)
>   create mode 100644 hw/vmxnet_pkt.c
>   create mode 100644 hw/vmxnet_pkt.h
>
> diff --git a/hw/vmxnet_pkt.c b/hw/vmxnet_pkt.c
> new file mode 100644
> index 0000000..5fe2672
> --- /dev/null
> +++ b/hw/vmxnet_pkt.c
> @@ -0,0 +1,1243 @@
> +/*
> + * QEMU VMWARE VMXNET* paravirtual NICs - packets abstractions
> + *
> + * Copyright (c) 2012 Ravello Systems LTD (http://ravellosystems.com)
> + *
> + * Developed by Daynix Computing LTD (http://www.daynix.com)
> + *
> + * Authors:
> + * Dmitry Fleytman<address@hidden>
> + * Tamir Shomer<address@hidden>
> + * Yan Vugenfirer<address@hidden>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + *
> + */
> +
> +#include "vmxnet_pkt.h"
> +#include "vmxnet_utils.h"
> +#include "iov.h"
> +
> +#include "net/checksum.h"
> +
> +/*=============================================================================
> + *=============================================================================
> + *
> + *                            TX CODE
> + *
> + *=============================================================================
> + *===========================================================================*/
> +
> +enum {
> +    VMXNET_TX_PKT_VHDR_FRAG = 0,
> +    VMXNET_TX_PKT_L2HDR_FRAG,
> +    VMXNET_TX_PKT_L3HDR_FRAG,
> +    VMXNET_TX_PKT_PL_START_FRAG
> +};
> +
> +/* TX packet private context */
> +typedef struct _Vmxnet_TxPkt {
> +    struct virtio_net_hdr virt_hdr;
> +    bool has_virt_hdr;
> +
> +    struct iovec *vec;
> +
> +    uint8_t l2_hdr[ETH_MAX_L2_HDR_LEN];
> +    uint8_t l3_hdr[ETH_MAX_L3_HDR_LEN];
> +
> +    uint32_t payload_len;
> +
> +    uint32_t payload_frags;
> +    uint32_t max_payload_frags;
> +
> +    uint16_t hdr_len;
> +    eth_pkt_types_e packet_type;
> +    uint16_t l3_proto;
> +} Vmxnet_TxPkt;
> +
> +/******************************************************************************
> + *
> + * Function: vmxnet_tx_pkt_init
> + *
> + * Desc: Init function for tx packet functionality.
> + *
> + * Params:  (OUT) pkt - private handle.
> + *          (IN) max_frags - max tx ip fragments.
> + *          (IN) has_virt_hdr - device uses virtio header.
> + *
> + * Return:  0 on success, -1 on error
> + *
> + * Scope: Global
> + *
> + *****************************************************************************/

I applaud the use of comments but I don't think it's necessary to duplicate this 
in the .c and .h file.  We also are using GtkDoc as our comment format these days.

> +int vmxnet_tx_pkt_init(Vmxnet_TxPkt_h *pkt, uint32_t max_frags,
> +    bool has_virt_hdr)
> +{
> +    int rc = 0;
> +
> +    Vmxnet_TxPkt *p = g_malloc(sizeof *p);
> +    if (!p) {
> +        rc = -1;
> +        goto Exit;
> +    }


g_malloc cannot return NULL.

> +
> +    memset(p, 0, sizeof *p);

g_malloc0 will memset for you.

> +
> +    p->vec = g_malloc((sizeof *p->vec) *
> +        (max_frags + VMXNET_TX_PKT_PL_START_FRAG));
> +    if (!p->vec) {
> +        rc = -1;
> +        goto Exit;
> +    }
> +
> +    p->max_payload_frags = max_frags;
> +    p->has_virt_hdr = has_virt_hdr;
> +    p->vec[VMXNET_TX_PKT_VHDR_FRAG].iov_base =&p->virt_hdr;
> +    p->vec[VMXNET_TX_PKT_VHDR_FRAG].iov_len =
> +        p->has_virt_hdr ? sizeof p->virt_hdr : 0;
> +    p->vec[VMXNET_TX_PKT_L2HDR_FRAG].iov_base =&p->l2_hdr;
> +    p->vec[VMXNET_TX_PKT_L3HDR_FRAG].iov_base =&p->l3_hdr;
> +
> +    *pkt = p;
> +
> +Exit:

labels should be all lower case.

> +    if (rc) {
> +        vmxnet_tx_pkt_uninit(p);
> +    }
> +    return rc;
> +}
> +
> +/******************************************************************************
> + *
> + * Function: vmxnet_tx_pkt_uninit
> + *
> + * Desc: Clean all tx packet resources.
> + *
> + * Params:  (IN) pkt - private handle.
> + *
> + * Return:  nothing
> + *
> + * Scope: Global
> + *
> + *****************************************************************************/
> +void vmxnet_tx_pkt_uninit(Vmxnet_TxPkt_h pkt)
> +{
> +    Vmxnet_TxPkt *p = (Vmxnet_TxPkt *)pkt;
> +
> +    if (p) {
> +        if (p->vec) {
> +            g_free(p->vec);
> +        }
> +
> +        g_free(p);
> +    }
> +}
> +
> +/******************************************************************************
> + *
> + * Function: vmxnet_tx_pkt_update_ip_checksums
> + *
> + * Desc: fix ip header fields and calculate checksums needed.
> + *
> + * Params:  (IN) pkt - private handle.
> + *
> + * Return:  Nothing.
> + *
> + * Scope: Global
> + *
> + *****************************************************************************/
> +void vmxnet_tx_pkt_update_ip_checksums(Vmxnet_TxPkt_h pkt)
> +{
> +    uint16_t csum;
> +    Vmxnet_TxPkt *p = (Vmxnet_TxPkt *)pkt;
> +    assert(p);
> +    uint8_t gso_type = p->virt_hdr.gso_type&  ~VIRTIO_NET_HDR_GSO_ECN;
> +    struct ip_header *ip_hdr;
> +    target_phys_addr_t payload = (target_phys_addr_t)
> +        (uint64_t) p->vec[VMXNET_TX_PKT_PL_START_FRAG].iov_base;
> +
> +    if (VIRTIO_NET_HDR_GSO_TCPV4 != gso_type&&
> +        VIRTIO_NET_HDR_GSO_UDP != gso_type) {
> +        return;
> +    }
> +
> +    ip_hdr = p->vec[VMXNET_TX_PKT_L3HDR_FRAG].iov_base;
> +
> +    if (p->payload_len + p->vec[VMXNET_TX_PKT_L3HDR_FRAG].iov_len>
> +        ETH_MAX_IP_DGRAM_LEN) {
> +        return;
> +    }
> +
> +    ip_hdr->ip_len = cpu_to_be16(p->payload_len +
> +        p->vec[VMXNET_TX_PKT_L3HDR_FRAG].iov_len);
> +
> +    /* Calculate IP header checksum                    */
> +    ip_hdr->ip_sum = 0;
> +    csum = net_raw_checksum((uint8_t *)ip_hdr,
> +        p->vec[VMXNET_TX_PKT_L3HDR_FRAG].iov_len);
> +    ip_hdr->ip_sum = cpu_to_be16(csum);
> +
> +    /* Calculate IP pseudo header checksum             */
> +    csum = cpu_to_be16(eth_calc_pseudo_hdr_csum(ip_hdr, p->payload_len));
> +    cpu_physical_memory_write(payload + p->virt_hdr.csum_offset,
> +&csum, sizeof(csum));
> +}
> +
> +/******************************************************************************
> + *
> + * Function: vmxnet_tx_pkt_get_l4_proto
> + *
> + * Desc: get l4 protocol.
> + *
> + * Params:  (IN) p - module context
> + *
> + * Return: l4 protocol
> + *
> + * Scope: Local
> + *
> + *****************************************************************************/
> +static uint8_t vmxnet_tx_pkt_get_l4_proto(Vmxnet_TxPkt *p)
> +{
> +    struct ip_header *ip_hdr;
> +    struct ip6_header *ip6_hdr;
> +
> +    if (!p->vec[VMXNET_TX_PKT_L3HDR_FRAG].iov_len) {
> +        return 0;
> +    }
> +
> +    if (ETH_P_IP == p->l3_proto) {
> +        ip_hdr = p->vec[VMXNET_TX_PKT_L3HDR_FRAG].iov_base;
> +        return ip_hdr->ip_p;
> +    } else if (p->l3_proto == ETH_P_IPV6) {
> +        ip6_hdr = p->vec[VMXNET_TX_PKT_L3HDR_FRAG].iov_base;
> +        return ip6_hdr->ip6_nxt;
> +    }
> +
> +    return 0;
> +}
> +
> +/******************************************************************************
> + *
> + * Function: vmxnet_tx_pkt_calculate_hdr_len
> + *
> + * Desc: store l2 and l3 headers length.
> + *
> + * Params:  (IN) p - module context
> + *
> + * Return: nothing
> + *
> + * Scope: Local
> + *
> + *****************************************************************************/
> +static void vmxnet_tx_pkt_calculate_hdr_len(Vmxnet_TxPkt *p)
> +{
> +    p->hdr_len = p->vec[VMXNET_TX_PKT_L2HDR_FRAG].iov_len +
> +        p->vec[VMXNET_TX_PKT_L3HDR_FRAG].iov_len;
> +}
> +
> +/******************************************************************************
> + *
> + * Function: vmxnet_tx_pkt_prepare
> + *
> + * Desc: populates headers and parses them to gether some metadata for later
> + *       use.
> + *
> + * Params:  (IN) pkt - private handle.
> + *          (IN) pa - physical address of tx data
> + *          (IN) len - length of data
> + *
> + * Return:  number of bytes populated by the function.
> + *
> + * Scope: Global
> + *
> + *****************************************************************************/
> +size_t vmxnet_tx_pkt_prepare(Vmxnet_TxPkt_h pkt, target_phys_addr_t pa,
> +    size_t len)
> +{
> +    Vmxnet_TxPkt *p = (Vmxnet_TxPkt *)pkt;
> +    /* some pointers that my lines stay nice and short. */
> +    void *l2_iov_base = NULL, *l3_iov_base = NULL;
> +    size_t *l2_iov_len = NULL, *l3_iov_len = NULL;
> +    assert(p);
> +
> +    l2_iov_base = p->vec[VMXNET_TX_PKT_L2HDR_FRAG].iov_base;
> +    l2_iov_len =&p->vec[VMXNET_TX_PKT_L2HDR_FRAG].iov_len;
> +    l3_iov_base = p->vec[VMXNET_TX_PKT_L3HDR_FRAG].iov_base;
> +    l3_iov_len =&p->vec[VMXNET_TX_PKT_L3HDR_FRAG].iov_len;
> +
> +    cpu_physical_memory_read(pa, l2_iov_base, ETH_MAX_L2_HDR_LEN);

As best as I can tell from looking through the patches, this is a buffer 
overflow.

I don't think you validate that VMXNET_TX_PKT_L2HDR_FRAG's length is >= 
ETH_MAX_L2_HDR_LEN but you still read it unconditionally.

> +    *l2_iov_len = eth_get_l2_hdr_length(l2_iov_base);
> +
> +    p->l3_proto = eth_get_l3_proto(l2_iov_base, *l2_iov_len);
> +
> +    switch (p->l3_proto) {
> +    case ETH_P_IP:
> +        assert(len>= *l2_iov_len + sizeof(struct ip_header));
> +
> +        cpu_physical_memory_read(pa + *l2_iov_len, l3_iov_base,
> +            sizeof(struct ip_header));

While you check len here, I think it's still possible for l3_iov_len to be 
smaller than sizeof(struct ip_header).

Regards,

Anthony Liguori