[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 6/7 v5] Packet abstraction used by VMWARE networ
From: |
Dmitry Fleytman |
Subject: |
Re: [Qemu-devel] [PATCH 6/7 v5] Packet abstraction used by VMWARE network devices |
Date: |
Tue, 17 Apr 2012 14:45:00 +0300 |
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