vhost: add header check in dequeue offload
Checks
Commit Message
When parsing the virtio net header and packet header for dequeue offload,
we need to perform sanity check on the packet header to ensure:
- No out-of-boundary memory access.
- The packet header and virtio_net header are valid and aligned.
Fixes: d0cf91303d73 ("vhost: add Tx offload capabilities")
Cc: stable@dpdk.org
Signed-off-by: Xiao Wang <xiao.w.wang@intel.com>
---
lib/librte_vhost/virtio_net.c | 49 +++++++++++++++++++++++++++++++++++++------
1 file changed, 43 insertions(+), 6 deletions(-)
Comments
>
> When parsing the virtio net header and packet header for dequeue offload,
> we need to perform sanity check on the packet header to ensure:
> - No out-of-boundary memory access.
> - The packet header and virtio_net header are valid and aligned.
>
> Fixes: d0cf91303d73 ("vhost: add Tx offload capabilities")
> Cc: stable@dpdk.org
>
> Signed-off-by: Xiao Wang <xiao.w.wang@intel.com>
> ---
> lib/librte_vhost/virtio_net.c | 49 +++++++++++++++++++++++++++++++++++++------
> 1 file changed, 43 insertions(+), 6 deletions(-)
>
> diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c
> index 583bf379c6..0fba0053a3 100644
> --- a/lib/librte_vhost/virtio_net.c
> +++ b/lib/librte_vhost/virtio_net.c
> @@ -1821,44 +1821,64 @@ virtio_net_with_host_offload(struct virtio_net *dev)
> return false;
> }
>
> -static void
> -parse_ethernet(struct rte_mbuf *m, uint16_t *l4_proto, void **l4_hdr)
> +static int
> +parse_ethernet(struct rte_mbuf *m, uint16_t *l4_proto, void **l4_hdr,
> + uint16_t *len)
> {
> struct rte_ipv4_hdr *ipv4_hdr;
> struct rte_ipv6_hdr *ipv6_hdr;
> void *l3_hdr = NULL;
> struct rte_ether_hdr *eth_hdr;
> uint16_t ethertype;
> + uint16_t data_len = m->data_len;
>
> eth_hdr = rte_pktmbuf_mtod(m, struct rte_ether_hdr *);
>
> + if (data_len <= sizeof(struct rte_ether_hdr))
> + return -EINVAL;
> +
> m->l2_len = sizeof(struct rte_ether_hdr);
> ethertype = rte_be_to_cpu_16(eth_hdr->ether_type);
> + data_len -= sizeof(struct rte_ether_hdr);
>
> if (ethertype == RTE_ETHER_TYPE_VLAN) {
> + if (data_len <= sizeof(struct rte_vlan_hdr))
> + return -EINVAL;
> +
> struct rte_vlan_hdr *vlan_hdr =
> (struct rte_vlan_hdr *)(eth_hdr + 1);
>
> m->l2_len += sizeof(struct rte_vlan_hdr);
> ethertype = rte_be_to_cpu_16(vlan_hdr->eth_proto);
> + data_len -= sizeof(struct rte_vlan_hdr);
> }
>
> l3_hdr = (char *)eth_hdr + m->l2_len;
>
> switch (ethertype) {
> case RTE_ETHER_TYPE_IPV4:
> + if (data_len <= sizeof(struct rte_ipv4_hdr))
> + return -EINVAL;
> ipv4_hdr = l3_hdr;
> *l4_proto = ipv4_hdr->next_proto_id;
> m->l3_len = rte_ipv4_hdr_len(ipv4_hdr);
> + if (data_len <= m->l3_len) {
> + m->l3_len = 0;
> + return -EINVAL;
> + }
> *l4_hdr = (char *)l3_hdr + m->l3_len;
> m->ol_flags |= PKT_TX_IPV4;
> + data_len -= m->l3_len;
> break;
> case RTE_ETHER_TYPE_IPV6:
> + if (data_len <= sizeof(struct rte_ipv6_hdr))
> + return -EINVAL;
> ipv6_hdr = l3_hdr;
> *l4_proto = ipv6_hdr->proto;
> m->l3_len = sizeof(struct rte_ipv6_hdr);
> *l4_hdr = (char *)l3_hdr + m->l3_len;
> m->ol_flags |= PKT_TX_IPV6;
> + data_len -= m->l3_len;
> break;
> default:
> m->l3_len = 0;
> @@ -1866,6 +1886,9 @@ parse_ethernet(struct rte_mbuf *m, uint16_t *l4_proto, void **l4_hdr)
> *l4_hdr = NULL;
> break;
> }
> +
> + *len = data_len;
> + return 0;
> }
>
> static __rte_always_inline void
> @@ -1874,24 +1897,30 @@ vhost_dequeue_offload(struct virtio_net_hdr *hdr, struct rte_mbuf *m)
> uint16_t l4_proto = 0;
> void *l4_hdr = NULL;
> struct rte_tcp_hdr *tcp_hdr = NULL;
> + uint16_t len = 0;
>
> if (hdr->flags == 0 && hdr->gso_type == VIRTIO_NET_HDR_GSO_NONE)
> return;
>
> - parse_ethernet(m, &l4_proto, &l4_hdr);
> + if (parse_ethernet(m, &l4_proto, &l4_hdr, &len) < 0)
> + return;
> +
> if (hdr->flags == VIRTIO_NET_HDR_F_NEEDS_CSUM) {
> if (hdr->csum_start == (m->l2_len + m->l3_len)) {
> switch (hdr->csum_offset) {
> case (offsetof(struct rte_tcp_hdr, cksum)):
> - if (l4_proto == IPPROTO_TCP)
> + if (l4_proto == IPPROTO_TCP &&
> + len > sizeof(struct rte_tcp_hdr))
Shouldn't it be '>=' here?
> m->ol_flags |= PKT_TX_TCP_CKSUM;
> break;
> case (offsetof(struct rte_udp_hdr, dgram_cksum)):
> - if (l4_proto == IPPROTO_UDP)
> + if (l4_proto == IPPROTO_UDP &&
> + len > sizeof(struct rte_udp_hdr))
> m->ol_flags |= PKT_TX_UDP_CKSUM;
> break;
> case (offsetof(struct rte_sctp_hdr, cksum)):
> - if (l4_proto == IPPROTO_SCTP)
> + if (l4_proto == IPPROTO_SCTP &&
> + len > sizeof(struct rte_sctp_hdr))
> m->ol_flags |= PKT_TX_SCTP_CKSUM;
> break;
> default:
> @@ -1904,12 +1933,20 @@ vhost_dequeue_offload(struct virtio_net_hdr *hdr, struct rte_mbuf *m)
> switch (hdr->gso_type & ~VIRTIO_NET_HDR_GSO_ECN) {
> case VIRTIO_NET_HDR_GSO_TCPV4:
> case VIRTIO_NET_HDR_GSO_TCPV6:
> + if (l4_proto != IPPROTO_TCP ||
> + len <= sizeof(struct rte_tcp_hdr))
> + break;
> tcp_hdr = l4_hdr;
> + if (len <= (tcp_hdr->data_off & 0xf0) >> 2)
> + break;
> m->ol_flags |= PKT_TX_TCP_SEG;
> m->tso_segsz = hdr->gso_size;
> m->l4_len = (tcp_hdr->data_off & 0xf0) >> 2;
> break;
> case VIRTIO_NET_HDR_GSO_UDP:
> + if (l4_proto != IPPROTO_UDP ||
> + len <= sizeof(struct rte_udp_hdr))
> + break;
> m->ol_flags |= PKT_TX_UDP_SEG;
> m->tso_segsz = hdr->gso_size;
> m->l4_len = sizeof(struct rte_udp_hdr);
> --
> 2.15.1
Hi Konstantin,
Comments inline.
BRs,
Xiao
> -----Original Message-----
> From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Sent: Thursday, March 11, 2021 6:38 PM
> To: Wang, Xiao W <xiao.w.wang@intel.com>; Xia, Chenbo
> <chenbo.xia@intel.com>; maxime.coquelin@redhat.com
> Cc: Liu, Yong <yong.liu@intel.com>; dev@dpdk.org; Wang, Xiao W
> <xiao.w.wang@intel.com>; stable@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH] vhost: add header check in dequeue
> offload
>
>
>
> >
> > When parsing the virtio net header and packet header for dequeue
> offload,
> > we need to perform sanity check on the packet header to ensure:
> > - No out-of-boundary memory access.
> > - The packet header and virtio_net header are valid and aligned.
> >
> > Fixes: d0cf91303d73 ("vhost: add Tx offload capabilities")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Xiao Wang <xiao.w.wang@intel.com>
> > ---
> > lib/librte_vhost/virtio_net.c | 49
> +++++++++++++++++++++++++++++++++++++------
> > 1 file changed, 43 insertions(+), 6 deletions(-)
> >
> > diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c
> > index 583bf379c6..0fba0053a3 100644
> > --- a/lib/librte_vhost/virtio_net.c
> > +++ b/lib/librte_vhost/virtio_net.c
> > @@ -1821,44 +1821,64 @@ virtio_net_with_host_offload(struct
> virtio_net *dev)
> > return false;
> > }
> >
> > -static void
> > -parse_ethernet(struct rte_mbuf *m, uint16_t *l4_proto, void **l4_hdr)
> > +static int
> > +parse_ethernet(struct rte_mbuf *m, uint16_t *l4_proto, void **l4_hdr,
> > + uint16_t *len)
> > {
> > struct rte_ipv4_hdr *ipv4_hdr;
> > struct rte_ipv6_hdr *ipv6_hdr;
> > void *l3_hdr = NULL;
> > struct rte_ether_hdr *eth_hdr;
> > uint16_t ethertype;
> > + uint16_t data_len = m->data_len;
> >
> > eth_hdr = rte_pktmbuf_mtod(m, struct rte_ether_hdr *);
> >
> > + if (data_len <= sizeof(struct rte_ether_hdr))
> > + return -EINVAL;
> > +
> > m->l2_len = sizeof(struct rte_ether_hdr);
> > ethertype = rte_be_to_cpu_16(eth_hdr->ether_type);
> > + data_len -= sizeof(struct rte_ether_hdr);
> >
> > if (ethertype == RTE_ETHER_TYPE_VLAN) {
> > + if (data_len <= sizeof(struct rte_vlan_hdr))
> > + return -EINVAL;
> > +
> > struct rte_vlan_hdr *vlan_hdr =
> > (struct rte_vlan_hdr *)(eth_hdr + 1);
> >
> > m->l2_len += sizeof(struct rte_vlan_hdr);
> > ethertype = rte_be_to_cpu_16(vlan_hdr->eth_proto);
> > + data_len -= sizeof(struct rte_vlan_hdr);
> > }
> >
> > l3_hdr = (char *)eth_hdr + m->l2_len;
> >
> > switch (ethertype) {
> > case RTE_ETHER_TYPE_IPV4:
> > + if (data_len <= sizeof(struct rte_ipv4_hdr))
> > + return -EINVAL;
> > ipv4_hdr = l3_hdr;
> > *l4_proto = ipv4_hdr->next_proto_id;
> > m->l3_len = rte_ipv4_hdr_len(ipv4_hdr);
> > + if (data_len <= m->l3_len) {
> > + m->l3_len = 0;
> > + return -EINVAL;
> > + }
> > *l4_hdr = (char *)l3_hdr + m->l3_len;
> > m->ol_flags |= PKT_TX_IPV4;
> > + data_len -= m->l3_len;
> > break;
> > case RTE_ETHER_TYPE_IPV6:
> > + if (data_len <= sizeof(struct rte_ipv6_hdr))
> > + return -EINVAL;
> > ipv6_hdr = l3_hdr;
> > *l4_proto = ipv6_hdr->proto;
> > m->l3_len = sizeof(struct rte_ipv6_hdr);
> > *l4_hdr = (char *)l3_hdr + m->l3_len;
> > m->ol_flags |= PKT_TX_IPV6;
> > + data_len -= m->l3_len;
> > break;
> > default:
> > m->l3_len = 0;
> > @@ -1866,6 +1886,9 @@ parse_ethernet(struct rte_mbuf *m, uint16_t
> *l4_proto, void **l4_hdr)
> > *l4_hdr = NULL;
> > break;
> > }
> > +
> > + *len = data_len;
> > + return 0;
> > }
> >
> > static __rte_always_inline void
> > @@ -1874,24 +1897,30 @@ vhost_dequeue_offload(struct virtio_net_hdr
> *hdr, struct rte_mbuf *m)
> > uint16_t l4_proto = 0;
> > void *l4_hdr = NULL;
> > struct rte_tcp_hdr *tcp_hdr = NULL;
> > + uint16_t len = 0;
> >
> > if (hdr->flags == 0 && hdr->gso_type ==
> VIRTIO_NET_HDR_GSO_NONE)
> > return;
> >
> > - parse_ethernet(m, &l4_proto, &l4_hdr);
> > + if (parse_ethernet(m, &l4_proto, &l4_hdr, &len) < 0)
> > + return;
> > +
> > if (hdr->flags == VIRTIO_NET_HDR_F_NEEDS_CSUM) {
> > if (hdr->csum_start == (m->l2_len + m->l3_len)) {
> > switch (hdr->csum_offset) {
> > case (offsetof(struct rte_tcp_hdr, cksum)):
> > - if (l4_proto == IPPROTO_TCP)
> > + if (l4_proto == IPPROTO_TCP &&
> > + len > sizeof(struct rte_tcp_hdr))
>
> Shouldn't it be '>=' here?
You are right, there're packets with no payload. I just did an experiment where I send a tcp packet with only headers from guest using scapy and vhost side get 54 Bytes (14+20+20), there's no padding.
I would fix this issue also for udp and sctp.
Thanks for the comment.
-Xiao
@@ -1821,44 +1821,64 @@ virtio_net_with_host_offload(struct virtio_net *dev)
return false;
}
-static void
-parse_ethernet(struct rte_mbuf *m, uint16_t *l4_proto, void **l4_hdr)
+static int
+parse_ethernet(struct rte_mbuf *m, uint16_t *l4_proto, void **l4_hdr,
+ uint16_t *len)
{
struct rte_ipv4_hdr *ipv4_hdr;
struct rte_ipv6_hdr *ipv6_hdr;
void *l3_hdr = NULL;
struct rte_ether_hdr *eth_hdr;
uint16_t ethertype;
+ uint16_t data_len = m->data_len;
eth_hdr = rte_pktmbuf_mtod(m, struct rte_ether_hdr *);
+ if (data_len <= sizeof(struct rte_ether_hdr))
+ return -EINVAL;
+
m->l2_len = sizeof(struct rte_ether_hdr);
ethertype = rte_be_to_cpu_16(eth_hdr->ether_type);
+ data_len -= sizeof(struct rte_ether_hdr);
if (ethertype == RTE_ETHER_TYPE_VLAN) {
+ if (data_len <= sizeof(struct rte_vlan_hdr))
+ return -EINVAL;
+
struct rte_vlan_hdr *vlan_hdr =
(struct rte_vlan_hdr *)(eth_hdr + 1);
m->l2_len += sizeof(struct rte_vlan_hdr);
ethertype = rte_be_to_cpu_16(vlan_hdr->eth_proto);
+ data_len -= sizeof(struct rte_vlan_hdr);
}
l3_hdr = (char *)eth_hdr + m->l2_len;
switch (ethertype) {
case RTE_ETHER_TYPE_IPV4:
+ if (data_len <= sizeof(struct rte_ipv4_hdr))
+ return -EINVAL;
ipv4_hdr = l3_hdr;
*l4_proto = ipv4_hdr->next_proto_id;
m->l3_len = rte_ipv4_hdr_len(ipv4_hdr);
+ if (data_len <= m->l3_len) {
+ m->l3_len = 0;
+ return -EINVAL;
+ }
*l4_hdr = (char *)l3_hdr + m->l3_len;
m->ol_flags |= PKT_TX_IPV4;
+ data_len -= m->l3_len;
break;
case RTE_ETHER_TYPE_IPV6:
+ if (data_len <= sizeof(struct rte_ipv6_hdr))
+ return -EINVAL;
ipv6_hdr = l3_hdr;
*l4_proto = ipv6_hdr->proto;
m->l3_len = sizeof(struct rte_ipv6_hdr);
*l4_hdr = (char *)l3_hdr + m->l3_len;
m->ol_flags |= PKT_TX_IPV6;
+ data_len -= m->l3_len;
break;
default:
m->l3_len = 0;
@@ -1866,6 +1886,9 @@ parse_ethernet(struct rte_mbuf *m, uint16_t *l4_proto, void **l4_hdr)
*l4_hdr = NULL;
break;
}
+
+ *len = data_len;
+ return 0;
}
static __rte_always_inline void
@@ -1874,24 +1897,30 @@ vhost_dequeue_offload(struct virtio_net_hdr *hdr, struct rte_mbuf *m)
uint16_t l4_proto = 0;
void *l4_hdr = NULL;
struct rte_tcp_hdr *tcp_hdr = NULL;
+ uint16_t len = 0;
if (hdr->flags == 0 && hdr->gso_type == VIRTIO_NET_HDR_GSO_NONE)
return;
- parse_ethernet(m, &l4_proto, &l4_hdr);
+ if (parse_ethernet(m, &l4_proto, &l4_hdr, &len) < 0)
+ return;
+
if (hdr->flags == VIRTIO_NET_HDR_F_NEEDS_CSUM) {
if (hdr->csum_start == (m->l2_len + m->l3_len)) {
switch (hdr->csum_offset) {
case (offsetof(struct rte_tcp_hdr, cksum)):
- if (l4_proto == IPPROTO_TCP)
+ if (l4_proto == IPPROTO_TCP &&
+ len > sizeof(struct rte_tcp_hdr))
m->ol_flags |= PKT_TX_TCP_CKSUM;
break;
case (offsetof(struct rte_udp_hdr, dgram_cksum)):
- if (l4_proto == IPPROTO_UDP)
+ if (l4_proto == IPPROTO_UDP &&
+ len > sizeof(struct rte_udp_hdr))
m->ol_flags |= PKT_TX_UDP_CKSUM;
break;
case (offsetof(struct rte_sctp_hdr, cksum)):
- if (l4_proto == IPPROTO_SCTP)
+ if (l4_proto == IPPROTO_SCTP &&
+ len > sizeof(struct rte_sctp_hdr))
m->ol_flags |= PKT_TX_SCTP_CKSUM;
break;
default:
@@ -1904,12 +1933,20 @@ vhost_dequeue_offload(struct virtio_net_hdr *hdr, struct rte_mbuf *m)
switch (hdr->gso_type & ~VIRTIO_NET_HDR_GSO_ECN) {
case VIRTIO_NET_HDR_GSO_TCPV4:
case VIRTIO_NET_HDR_GSO_TCPV6:
+ if (l4_proto != IPPROTO_TCP ||
+ len <= sizeof(struct rte_tcp_hdr))
+ break;
tcp_hdr = l4_hdr;
+ if (len <= (tcp_hdr->data_off & 0xf0) >> 2)
+ break;
m->ol_flags |= PKT_TX_TCP_SEG;
m->tso_segsz = hdr->gso_size;
m->l4_len = (tcp_hdr->data_off & 0xf0) >> 2;
break;
case VIRTIO_NET_HDR_GSO_UDP:
+ if (l4_proto != IPPROTO_UDP ||
+ len <= sizeof(struct rte_udp_hdr))
+ break;
m->ol_flags |= PKT_TX_UDP_SEG;
m->tso_segsz = hdr->gso_size;
m->l4_len = sizeof(struct rte_udp_hdr);