Hello, Anthony Thanks for you comments, see inline.
On Mon, Apr 16, 2012 at 11:06 PM, Anthony Liguori <anth...@codemonkey.ws> wrote: > On 03/18/2012 04:27 AM, Dmitry Fleytman wrote: >> >> Signed-off-by: Dmitry Fleytman<dmi...@daynix.com> >> Signed-off-by: Yan Vugenfirer<y...@daynix.com> >> --- >> 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<dmi...@daynix.com> >> + * Tamir Shomer<tam...@daynix.com> >> + * Yan Vugenfirer<y...@daynix.com> >> + * >> + * 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
