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

[Dmitry Fleytman]

Hello, Anthony

Thanks for you comments, see inline.

On Mon, Apr 16, 2012 at 11:06 PM, Anthony Liguori <address@hidden> wrote:
> 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.

Good point. Will be fixed in the next submission.

>
>
>> +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.

Thanks, fixed.

>
>
>> +
>> +    memset(p, 0, sizeof *p);
>
>
> g_malloc0 will memset for you.


Also fixed.

>
>> +
>> +    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.


Fixed.

>
>> +    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.
>

There is no buffer overflow here because we allocate exactly
ETH_MAX_L2_HDR_LEN bytes for target buffer.

>
>> +    *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).
>

Also, the target buffer is allocated by our code and has enough space
to fit IP header

> Regards,
>
> Anthony Liguori