[v2] 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>
---
v2:
Allow empty L4 payload for cksum offload.
---
lib/librte_vhost/virtio_net.c | 49 +++++++++++++++++++++++++++++++++++++------
1 file changed, 43 insertions(+), 6 deletions(-)
Comments
On Mon, Mar 15, 2021 at 4:52 PM Xiao Wang <xiao.w.wang@intel.com> wrote:
>
> 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>
> ---
> v2:
> Allow empty L4 payload for cksum offload.
> ---
> 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..53a8ff2898 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;
On principle, the check should happen before calling rte_pktmbuf_mtod,
like what rte_pktmbuf_read does.
Looking at the rest of the patch, does this helper function only
handle mono segment mbufs?
My reading of copy_desc_to_mbuf() was that it could generate multi
segments mbufs...
[snip]
> 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;
> + }
... so here, comparing l3 length to only the first segment length
(data_len) would be invalid.
If this helper must deal with multi segments, why not use rte_pktmbuf_read?
This function returns access to mbuf data after checking offset and
length are contiguous, else copy the needed data in a passed buffer.
> *l4_hdr = (char *)l3_hdr + m->l3_len;
> m->ol_flags |= PKT_TX_IPV4;
> + data_len -= m->l3_len;
> break;
> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Monday, March 15, 2021 4:17 PM
> To: Wang, Xiao W <xiao.w.wang@intel.com>
> Cc: Xia, Chenbo <chenbo.xia@intel.com>; Maxime Coquelin <maxime.coquelin@redhat.com>; Liu, Yong <yong.liu@intel.com>; dev
> <dev@dpdk.org>; Ananyev, Konstantin <konstantin.ananyev@intel.com>; dpdk stable <stable@dpdk.org>
> Subject: Re: [dpdk-stable] [PATCH v2] vhost: add header check in dequeue offload
>
> On Mon, Mar 15, 2021 at 4:52 PM Xiao Wang <xiao.w.wang@intel.com> wrote:
> >
> > 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>
> > ---
> > v2:
> > Allow empty L4 payload for cksum offload.
> > ---
> > 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..53a8ff2898 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;
>
> On principle, the check should happen before calling rte_pktmbuf_mtod,
> like what rte_pktmbuf_read does.
>
> Looking at the rest of the patch, does this helper function only
> handle mono segment mbufs?
> My reading of copy_desc_to_mbuf() was that it could generate multi
> segments mbufs...
>
>
> [snip]
>
> > 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;
> > + }
>
> ... so here, comparing l3 length to only the first segment length
> (data_len) would be invalid.
>
> If this helper must deal with multi segments, why not use rte_pktmbuf_read?
> This function returns access to mbuf data after checking offset and
> length are contiguous, else copy the needed data in a passed buffer.
From my understanding, yes multi-seg is allowed, but an expectation
Is that at least packet header (l2/l3/l4?) will always reside in first segment.
>
>
> > *l4_hdr = (char *)l3_hdr + m->l3_len;
> > m->ol_flags |= PKT_TX_IPV4;
> > + data_len -= m->l3_len;
> > break;
>
>
> --
> David Marchand
Hi,
Comments inline.
BRs,
Xiao
> -----Original Message-----
> From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Sent: Tuesday, March 16, 2021 2:53 AM
> To: David Marchand <david.marchand@redhat.com>; Wang, Xiao W
> <xiao.w.wang@intel.com>
> Cc: Xia, Chenbo <chenbo.xia@intel.com>; Maxime Coquelin
> <maxime.coquelin@redhat.com>; Liu, Yong <yong.liu@intel.com>; dev
> <dev@dpdk.org>; dpdk stable <stable@dpdk.org>
> Subject: RE: [dpdk-stable] [PATCH v2] vhost: add header check in dequeue
> offload
>
>
>
> > -----Original Message-----
> > From: David Marchand <david.marchand@redhat.com>
> > Sent: Monday, March 15, 2021 4:17 PM
> > To: Wang, Xiao W <xiao.w.wang@intel.com>
> > Cc: Xia, Chenbo <chenbo.xia@intel.com>; Maxime Coquelin
> <maxime.coquelin@redhat.com>; Liu, Yong <yong.liu@intel.com>; dev
> > <dev@dpdk.org>; Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> dpdk stable <stable@dpdk.org>
> > Subject: Re: [dpdk-stable] [PATCH v2] vhost: add header check in dequeue
> offload
> >
> > On Mon, Mar 15, 2021 at 4:52 PM Xiao Wang <xiao.w.wang@intel.com>
> wrote:
> > >
> > > 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>
> > > ---
> > > v2:
> > > Allow empty L4 payload for cksum offload.
> > > ---
> > > 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..53a8ff2898 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;
> >
> > On principle, the check should happen before calling rte_pktmbuf_mtod,
> > like what rte_pktmbuf_read does.
Yes, I agree. Will fix it in v3.
> >
> > Looking at the rest of the patch, does this helper function only
> > handle mono segment mbufs?
> > My reading of copy_desc_to_mbuf() was that it could generate multi
> > segments mbufs...
copy_desc_to_mbuf() could generate multi seg mbufs, and the whole packet would be copied into these multi-mbufs when packet size is larger than mbuf's capacity.
Anyway, one mbuf's capacity is big enough for holding the L2/L3/L4 header.
> >
> >
> > [snip]
> >
> > > 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;
> > > + }
> >
> > ... so here, comparing l3 length to only the first segment length
> > (data_len) would be invalid.
> >
> > If this helper must deal with multi segments, why not use
> rte_pktmbuf_read?
> > This function returns access to mbuf data after checking offset and
> > length are contiguous, else copy the needed data in a passed buffer.
>
> From my understanding, yes multi-seg is allowed, but an expectation
> Is that at least packet header (l2/l3/l4?) will always reside in first segment.
Yeah, I think so.
Thanks for all the comments,
-Xiao
>
> >
> >
> > > *l4_hdr = (char *)l3_hdr + m->l3_len;
> > > m->ol_flags |= PKT_TX_IPV4;
> > > + data_len -= m->l3_len;
> > > break;
> >
> >
> > --
> > David Marchand
@@ -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);