gro: add missing invalid packet checks
Checks
Commit Message
Currently, GRO library doesn't check if input packets have
invalid headers. The packets with invalid headers will also
be processed by GRO.
However, GRO shouldn't process invalid packets. This patch adds
missing invalid packet checks.
Fixes: 0d2cbe59b719 ("lib/gro: support TCP/IPv4")
Fixes: 9e0b9d2ec0f4 ("gro: support VxLAN GRO")
Cc: stable@dpdk.org
Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
---
lib/librte_gro/gro_tcp4.c | 10 ++++++++++
lib/librte_gro/gro_tcp4.h | 4 ++++
lib/librte_gro/gro_vxlan_tcp4.c | 13 +++++++++++++
3 files changed, 27 insertions(+)
Comments
On Tue, 8 Jan 2019 14:08:45 +0800
Jiayu Hu <jiayu.hu@intel.com> wrote:
> + /*
> + * Don't process the packet whose Ethernet, IPv4 and TCP header
> + * lengths are invalid. In addition, if the IPv4 header contains
> + * Options, the packet shouldn't be processed.
> + */
> + if (unlikely(ILLEGAL_ETHER_HDRLEN(pkt->l2_len) ||
> + ILLEGAL_IPV4_HDRLEN(pkt->l3_len) ||
> + ILLEGAL_TCP_HDRLEN(pkt->l4_len)))
> + return -1;
I like it when code is as picky as possible when doing optimizations because
it reduces possible security riskg.
To me this looks more confusing and not as careful as doing it like:
if (unlikely(pkt->l2_len != ETHER_HDR_LEN))
return -1;
eth_hdr = rte_pktmbuf_mtod(pkt, struct ether_hdr *);
ipv4_hdr = (struct ipv4_hdr *)((char *)eth_hdr + ETHER_HDR_LEN);
if (pkt->l3_len != (ipv4->version_ihl & IPV4_HDR_IHL_MASK) << 4)
return -1;
if (pkt->l4_len < sizeof(struct tcp_hdr))
return -1;
You should also check for TCP options as well.
And IPv6 has same issues.
> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Tuesday, January 8, 2019 2:32 PM
> To: Hu, Jiayu <jiayu.hu@intel.com>
> Cc: dev@dpdk.org; Bie, Tiwei <tiwei.bie@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>; stable@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] gro: add missing invalid packet checks
>
> On Tue, 8 Jan 2019 14:08:45 +0800
> Jiayu Hu <jiayu.hu@intel.com> wrote:
>
> > + /*
> > + * Don't process the packet whose Ethernet, IPv4 and TCP header
> > + * lengths are invalid. In addition, if the IPv4 header contains
> > + * Options, the packet shouldn't be processed.
> > + */
> > + if (unlikely(ILLEGAL_ETHER_HDRLEN(pkt->l2_len) ||
> > + ILLEGAL_IPV4_HDRLEN(pkt->l3_len) ||
> > + ILLEGAL_TCP_HDRLEN(pkt->l4_len)))
> > + return -1;
In the GRO design, we assume applications give correct
MBUF->l2_len/.. for input packets of GRO. Specifically, GRO
library assumes applications will set values to MBUF->l2_len/...
and guarantee the values are the same as the values in the packet
headers. The reason for this assumption is to process header faster.
This is also why I want to add this assumption in the programmer
guide.
The above code is to forbid GRO to process invalid packets, which
have invalid packet header lengths, like TCP header length is less than
20 bytes.
>
> I like it when code is as picky as possible when doing optimizations because
> it reduces possible security riskg.
>
> To me this looks more confusing and not as careful as doing it like:
>
> if (unlikely(pkt->l2_len != ETHER_HDR_LEN))
> return -1;
> eth_hdr = rte_pktmbuf_mtod(pkt, struct ether_hdr *);
> ipv4_hdr = (struct ipv4_hdr *)((char *)eth_hdr + ETHER_HDR_LEN);
>
> if (pkt->l3_len != (ipv4->version_ihl & IPV4_HDR_IHL_MASK) << 4)
> return -1;
>
> if (pkt->l4_len < sizeof(struct tcp_hdr))
> return -1;
>
> You should also check for TCP options as well.
There are two ways to get ether, ipv4 and tcp headers:
1). Use MBUF->l2_len/l3_len...;
2). Parse packet and ignore MBUF->l2_len/....
If we follow the choice 1, we don't need to parse packet and
don't need to check if values of MBUF->l2_len/... are correct,
since we assume applications will set correct values. If we follow
the choice 2, we don't need to care about the values of MBUF->l2_len/...
I am a little confused about your code, since it parses packet and
checks if the values of MBUF->l2_len/... are correct. If we don't use
MBUF->l2_len/... to get ether/ipv4/tcp headers, why should we check
the values of MBUF->l2_len/...?
Thanks,
Jiayu
>
> And IPv6 has same issues.
>
>
> > -----Original Message-----
> > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > Sent: Tuesday, January 8, 2019 2:32 PM
> > To: Hu, Jiayu <jiayu.hu@intel.com>
> > Cc: dev@dpdk.org; Bie, Tiwei <tiwei.bie@intel.com>; Richardson, Bruce
> > <bruce.richardson@intel.com>; stable@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH] gro: add missing invalid packet checks
> >
> > On Tue, 8 Jan 2019 14:08:45 +0800
> > Jiayu Hu <jiayu.hu@intel.com> wrote:
> >
> > > + /*
> > > + * Don't process the packet whose Ethernet, IPv4 and TCP header
> > > + * lengths are invalid. In addition, if the IPv4 header contains
> > > + * Options, the packet shouldn't be processed.
> > > + */
> > > + if (unlikely(ILLEGAL_ETHER_HDRLEN(pkt->l2_len) ||
> > > + ILLEGAL_IPV4_HDRLEN(pkt->l3_len) ||
> > > + ILLEGAL_TCP_HDRLEN(pkt->l4_len)))
> > > + return -1;
>
> In the GRO design, we assume applications give correct
> MBUF->l2_len/.. for input packets of GRO. Specifically, GRO
> library assumes applications will set values to MBUF->l2_len/...
> and guarantee the values are the same as the values in the packet
> headers. The reason for this assumption is to process header faster.
> This is also why I want to add this assumption in the programmer
> guide.
>
> The above code is to forbid GRO to process invalid packets, which
> have invalid packet header lengths, like TCP header length is less than
> 20 bytes.
>
> >
> > I like it when code is as picky as possible when doing optimizations because
> > it reduces possible security riskg.
> >
> > To me this looks more confusing and not as careful as doing it like:
> >
> > if (unlikely(pkt->l2_len != ETHER_HDR_LEN))
> > return -1;
> > eth_hdr = rte_pktmbuf_mtod(pkt, struct ether_hdr *);
> > ipv4_hdr = (struct ipv4_hdr *)((char *)eth_hdr + ETHER_HDR_LEN);
> >
> > if (pkt->l3_len != (ipv4->version_ihl & IPV4_HDR_IHL_MASK) << 4)
> > return -1;
> >
> > if (pkt->l4_len < sizeof(struct tcp_hdr))
> > return -1;
> >
> > You should also check for TCP options as well.
>
> There are two ways to get ether, ipv4 and tcp headers:
> 1). Use MBUF->l2_len/l3_len...;
> 2). Parse packet and ignore MBUF->l2_len/....
>
> If we follow the choice 1, we don't need to parse packet and
> don't need to check if values of MBUF->l2_len/... are correct,
> since we assume applications will set correct values. If we follow
> the choice 2, we don't need to care about the values of MBUF->l2_len/...
>
> I am a little confused about your code, since it parses packet and
> checks if the values of MBUF->l2_len/... are correct. If we don't use
> MBUF->l2_len/... to get ether/ipv4/tcp headers, why should we check
> the values of MBUF->l2_len/...?
>
Agree that we don't need both.
My preference would be to stick with 1).
In many cases user would have already determined l2/l3/l4 len
by this stage.
Konstantin
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev,
> Konstantin
> Sent: Tuesday, January 8, 2019 11:39 AM
> To: Hu, Jiayu; Stephen Hemminger
> Cc: dev@dpdk.org; Bie, Tiwei; Richardson, Bruce; stable@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] gro: add missing invalid packet checks
>
>
>
> >
> >
> > > -----Original Message-----
> > > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > > Sent: Tuesday, January 8, 2019 2:32 PM
> > > To: Hu, Jiayu <jiayu.hu@intel.com>
> > > Cc: dev@dpdk.org; Bie, Tiwei <tiwei.bie@intel.com>; Richardson,
> Bruce
> > > <bruce.richardson@intel.com>; stable@dpdk.org
> > > Subject: Re: [dpdk-dev] [PATCH] gro: add missing invalid packet
> checks
> > >
> > > On Tue, 8 Jan 2019 14:08:45 +0800
> > > Jiayu Hu <jiayu.hu@intel.com> wrote:
> > >
> > > > + /*
> > > > + * Don't process the packet whose Ethernet, IPv4 and TCP
> header
> > > > + * lengths are invalid. In addition, if the IPv4 header
> contains
> > > > + * Options, the packet shouldn't be processed.
> > > > + */
> > > > + if (unlikely(ILLEGAL_ETHER_HDRLEN(pkt->l2_len) ||
> > > > + ILLEGAL_IPV4_HDRLEN(pkt->l3_len) ||
> > > > + ILLEGAL_TCP_HDRLEN(pkt->l4_len)))
> > > > + return -1;
> >
> > In the GRO design, we assume applications give correct
> > MBUF->l2_len/.. for input packets of GRO. Specifically, GRO
> > library assumes applications will set values to MBUF->l2_len/...
> > and guarantee the values are the same as the values in the packet
> > headers. The reason for this assumption is to process header faster.
> > This is also why I want to add this assumption in the programmer
> > guide.
+1 to more detailed documentation about assumptions and preconditions.
> >
> > The above code is to forbid GRO to process invalid packets, which
> > have invalid packet header lengths, like TCP header length is less
> than
> > 20 bytes.
> >
> > >
> > > I like it when code is as picky as possible when doing
> optimizations because
> > > it reduces possible security riskg.
> > >
> > > To me this looks more confusing and not as careful as doing it
> like:
> > >
> > > if (unlikely(pkt->l2_len != ETHER_HDR_LEN))
> > > return -1;
> > > eth_hdr = rte_pktmbuf_mtod(pkt, struct ether_hdr *);
> > > ipv4_hdr = (struct ipv4_hdr *)((char *)eth_hdr + ETHER_HDR_LEN);
> > >
> > > if (pkt->l3_len != (ipv4->version_ihl & IPV4_HDR_IHL_MASK) << 4)
> > > return -1;
> > >
> > > if (pkt->l4_len < sizeof(struct tcp_hdr))
> > > return -1;
> > >
> > > You should also check for TCP options as well.
> >
> > There are two ways to get ether, ipv4 and tcp headers:
> > 1). Use MBUF->l2_len/l3_len...;
> > 2). Parse packet and ignore MBUF->l2_len/....
> >
> > If we follow the choice 1, we don't need to parse packet and
> > don't need to check if values of MBUF->l2_len/... are correct,
> > since we assume applications will set correct values. If we follow
> > the choice 2, we don't need to care about the values of MBUF-
> >l2_len/...
> >
> > I am a little confused about your code, since it parses packet and
> > checks if the values of MBUF->l2_len/... are correct. If we don't use
> > MBUF->l2_len/... to get ether/ipv4/tcp headers, why should we check
> > the values of MBUF->l2_len/...?
> >
>
> Agree that we don't need both.
> My preference would be to stick with 1).
> In many cases user would have already determined l2/l3/l4 len
> by this stage.
> Konstantin
Do we have a generic packet header validation library? Otherwise, that would perhaps be a better path. Such a library could probably use some of the flags from the PMD to determine how much to validate in software.
And if it is a documented precondition of the GRO library that m->l2_len/l3_len... must be set and sensible, perhaps an RTE_ASSERT() could be considered instead of gracefully returning -1?
> -----Original Message-----
> From: Morten Brørup [mailto:mb@smartsharesystems.com]
> Sent: Tuesday, January 8, 2019 7:34 PM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Hu, Jiayu
> <jiayu.hu@intel.com>; Stephen Hemminger
> <stephen@networkplumber.org>
> Cc: dev@dpdk.org; Bie, Tiwei <tiwei.bie@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>; stable@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH] gro: add missing invalid packet checks
>
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev,
> > Konstantin
> > Sent: Tuesday, January 8, 2019 11:39 AM
> > To: Hu, Jiayu; Stephen Hemminger
> > Cc: dev@dpdk.org; Bie, Tiwei; Richardson, Bruce; stable@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH] gro: add missing invalid packet checks
> >
> >
> >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > > > Sent: Tuesday, January 8, 2019 2:32 PM
> > > > To: Hu, Jiayu <jiayu.hu@intel.com>
> > > > Cc: dev@dpdk.org; Bie, Tiwei <tiwei.bie@intel.com>; Richardson,
> > Bruce
> > > > <bruce.richardson@intel.com>; stable@dpdk.org
> > > > Subject: Re: [dpdk-dev] [PATCH] gro: add missing invalid packet
> > checks
> > > >
> > > > On Tue, 8 Jan 2019 14:08:45 +0800
> > > > Jiayu Hu <jiayu.hu@intel.com> wrote:
> > > >
> > > > > + /*
> > > > > + * Don't process the packet whose Ethernet, IPv4 and TCP
> > header
> > > > > + * lengths are invalid. In addition, if the IPv4 header
> > contains
> > > > > + * Options, the packet shouldn't be processed.
> > > > > + */
> > > > > + if (unlikely(ILLEGAL_ETHER_HDRLEN(pkt->l2_len) ||
> > > > > + ILLEGAL_IPV4_HDRLEN(pkt->l3_len) ||
> > > > > + ILLEGAL_TCP_HDRLEN(pkt->l4_len)))
> > > > > + return -1;
> > >
> > > In the GRO design, we assume applications give correct
> > > MBUF->l2_len/.. for input packets of GRO. Specifically, GRO
> > > library assumes applications will set values to MBUF->l2_len/...
> > > and guarantee the values are the same as the values in the packet
> > > headers. The reason for this assumption is to process header faster.
>
> > > This is also why I want to add this assumption in the programmer
> > > guide.
>
> +1 to more detailed documentation about assumptions and preconditions.
>
>
> > >
> > > The above code is to forbid GRO to process invalid packets, which
> > > have invalid packet header lengths, like TCP header length is less
> > than
> > > 20 bytes.
> > >
> > > >
> > > > I like it when code is as picky as possible when doing
> > optimizations because
> > > > it reduces possible security riskg.
> > > >
> > > > To me this looks more confusing and not as careful as doing it
> > like:
> > > >
> > > > if (unlikely(pkt->l2_len != ETHER_HDR_LEN))
> > > > return -1;
> > > > eth_hdr = rte_pktmbuf_mtod(pkt, struct ether_hdr *);
> > > > ipv4_hdr = (struct ipv4_hdr *)((char *)eth_hdr + ETHER_HDR_LEN);
> > > >
> > > > if (pkt->l3_len != (ipv4->version_ihl & IPV4_HDR_IHL_MASK) << 4)
> > > > return -1;
> > > >
> > > > if (pkt->l4_len < sizeof(struct tcp_hdr))
> > > > return -1;
> > > >
> > > > You should also check for TCP options as well.
> > >
> > > There are two ways to get ether, ipv4 and tcp headers:
> > > 1). Use MBUF->l2_len/l3_len...;
> > > 2). Parse packet and ignore MBUF->l2_len/....
> > >
> > > If we follow the choice 1, we don't need to parse packet and
> > > don't need to check if values of MBUF->l2_len/... are correct,
> > > since we assume applications will set correct values. If we follow
> > > the choice 2, we don't need to care about the values of MBUF-
> > >l2_len/...
> > >
> > > I am a little confused about your code, since it parses packet and
> > > checks if the values of MBUF->l2_len/... are correct. If we don't use
> > > MBUF->l2_len/... to get ether/ipv4/tcp headers, why should we check
> > > the values of MBUF->l2_len/...?
> > >
> >
> > Agree that we don't need both.
> > My preference would be to stick with 1).
> > In many cases user would have already determined l2/l3/l4 len
> > by this stage.
> > Konstantin
>
> Do we have a generic packet header validation library? Otherwise, that
> would perhaps be a better path. Such a library could probably use some of
> the flags from the PMD to determine how much to validate in software.
As far as I know, we don't have the library to check if the values of MBUF->l2_len/...
are valid or not.
>
> And if it is a documented precondition of the GRO library that m-
> >l2_len/l3_len... must be set and sensible, perhaps an RTE_ASSERT() could
> be considered instead of gracefully returning -1?
Comparing with terminating the application by RTE_ASSERT(), I think
not processing the invalid packet would be a better choice.
Thanks,
Jiayu
> -----Original Message-----
> From: Morten Brørup [mailto:mb@smartsharesystems.com]
> Sent: Tuesday, January 8, 2019 11:34 AM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Hu, Jiayu <jiayu.hu@intel.com>; Stephen Hemminger
> <stephen@networkplumber.org>
> Cc: dev@dpdk.org; Bie, Tiwei <tiwei.bie@intel.com>; Richardson, Bruce <bruce.richardson@intel.com>; stable@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH] gro: add missing invalid packet checks
>
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev,
> > Konstantin
> > Sent: Tuesday, January 8, 2019 11:39 AM
> > To: Hu, Jiayu; Stephen Hemminger
> > Cc: dev@dpdk.org; Bie, Tiwei; Richardson, Bruce; stable@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH] gro: add missing invalid packet checks
> >
> >
> >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > > > Sent: Tuesday, January 8, 2019 2:32 PM
> > > > To: Hu, Jiayu <jiayu.hu@intel.com>
> > > > Cc: dev@dpdk.org; Bie, Tiwei <tiwei.bie@intel.com>; Richardson,
> > Bruce
> > > > <bruce.richardson@intel.com>; stable@dpdk.org
> > > > Subject: Re: [dpdk-dev] [PATCH] gro: add missing invalid packet
> > checks
> > > >
> > > > On Tue, 8 Jan 2019 14:08:45 +0800
> > > > Jiayu Hu <jiayu.hu@intel.com> wrote:
> > > >
> > > > > + /*
> > > > > + * Don't process the packet whose Ethernet, IPv4 and TCP
> > header
> > > > > + * lengths are invalid. In addition, if the IPv4 header
> > contains
> > > > > + * Options, the packet shouldn't be processed.
> > > > > + */
> > > > > + if (unlikely(ILLEGAL_ETHER_HDRLEN(pkt->l2_len) ||
> > > > > + ILLEGAL_IPV4_HDRLEN(pkt->l3_len) ||
> > > > > + ILLEGAL_TCP_HDRLEN(pkt->l4_len)))
> > > > > + return -1;
> > >
> > > In the GRO design, we assume applications give correct
> > > MBUF->l2_len/.. for input packets of GRO. Specifically, GRO
> > > library assumes applications will set values to MBUF->l2_len/...
> > > and guarantee the values are the same as the values in the packet
> > > headers. The reason for this assumption is to process header faster.
>
> > > This is also why I want to add this assumption in the programmer
> > > guide.
>
> +1 to more detailed documentation about assumptions and preconditions.
>
>
> > >
> > > The above code is to forbid GRO to process invalid packets, which
> > > have invalid packet header lengths, like TCP header length is less
> > than
> > > 20 bytes.
> > >
> > > >
> > > > I like it when code is as picky as possible when doing
> > optimizations because
> > > > it reduces possible security riskg.
> > > >
> > > > To me this looks more confusing and not as careful as doing it
> > like:
> > > >
> > > > if (unlikely(pkt->l2_len != ETHER_HDR_LEN))
> > > > return -1;
> > > > eth_hdr = rte_pktmbuf_mtod(pkt, struct ether_hdr *);
> > > > ipv4_hdr = (struct ipv4_hdr *)((char *)eth_hdr + ETHER_HDR_LEN);
> > > >
> > > > if (pkt->l3_len != (ipv4->version_ihl & IPV4_HDR_IHL_MASK) << 4)
> > > > return -1;
> > > >
> > > > if (pkt->l4_len < sizeof(struct tcp_hdr))
> > > > return -1;
> > > >
> > > > You should also check for TCP options as well.
> > >
> > > There are two ways to get ether, ipv4 and tcp headers:
> > > 1). Use MBUF->l2_len/l3_len...;
> > > 2). Parse packet and ignore MBUF->l2_len/....
> > >
> > > If we follow the choice 1, we don't need to parse packet and
> > > don't need to check if values of MBUF->l2_len/... are correct,
> > > since we assume applications will set correct values. If we follow
> > > the choice 2, we don't need to care about the values of MBUF-
> > >l2_len/...
> > >
> > > I am a little confused about your code, since it parses packet and
> > > checks if the values of MBUF->l2_len/... are correct. If we don't use
> > > MBUF->l2_len/... to get ether/ipv4/tcp headers, why should we check
> > > the values of MBUF->l2_len/...?
> > >
> >
> > Agree that we don't need both.
> > My preference would be to stick with 1).
> > In many cases user would have already determined l2/l3/l4 len
> > by this stage.
> > Konstantin
>
> Do we have a generic packet header validation library? Otherwise, that would perhaps be a better path. Such a library could probably use
> some of the flags from the PMD to determine how much to validate in software.
AFAIK - we don't have a generic header parsing library.
Yes, it would be good to have such ability, but I think that's out of scope for that patch.
BTW, volunteers are welcome :)
>
> And if it is a documented precondition of the GRO library that m->l2_len/l3_len... must be set and sensible, perhaps an RTE_ASSERT() could
> be considered instead of gracefully returning -1?
I suppose that's too extreme.
What's wrong with checking input parameters and return an error if they are invalid?
Konstantin
> From: Ananyev, Konstantin [mailto:konstantin.ananyev@intel.com]
> > From: Morten Brørup [mailto:mb@smartsharesystems.com]
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev,
> > > Konstantin
> > > > > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > > > >
> > > > > On Tue, 8 Jan 2019 14:08:45 +0800
> > > > > Jiayu Hu <jiayu.hu@intel.com> wrote:
> > > > >
> > > > > > + /*
> > > > > > + * Don't process the packet whose Ethernet, IPv4 and TCP
> > > header
> > > > > > + * lengths are invalid. In addition, if the IPv4 header
> > > contains
> > > > > > + * Options, the packet shouldn't be processed.
> > > > > > + */
> > > > > > + if (unlikely(ILLEGAL_ETHER_HDRLEN(pkt->l2_len) ||
> > > > > > + ILLEGAL_IPV4_HDRLEN(pkt->l3_len) ||
> > > > > > + ILLEGAL_TCP_HDRLEN(pkt->l4_len)))
> > > > > > + return -1;
> > > >
> > > > In the GRO design, we assume applications give correct
> > > > MBUF->l2_len/.. for input packets of GRO. Specifically, GRO
> > > > library assumes applications will set values to MBUF->l2_len/...
> > > > and guarantee the values are the same as the values in the packet
> > > > headers. The reason for this assumption is to process header
> faster.
> >
> > > > This is also why I want to add this assumption in the programmer
> > > > guide.
> >
> > +1 to more detailed documentation about assumptions and
> preconditions.
> >
> >
> > > >
> > > > The above code is to forbid GRO to process invalid packets, which
> > > > have invalid packet header lengths, like TCP header length is
> less
> > > than
> > > > 20 bytes.
> > > >
> > > > >
> > > > > I like it when code is as picky as possible when doing
> > > optimizations because
> > > > > it reduces possible security riskg.
> > > > >
> > > > > To me this looks more confusing and not as careful as doing it
> > > like:
> > > > >
> > > > > if (unlikely(pkt->l2_len != ETHER_HDR_LEN))
> > > > > return -1;
> > > > > eth_hdr = rte_pktmbuf_mtod(pkt, struct ether_hdr *);
> > > > > ipv4_hdr = (struct ipv4_hdr *)((char *)eth_hdr +
> ETHER_HDR_LEN);
> > > > >
> > > > > if (pkt->l3_len != (ipv4->version_ihl & IPV4_HDR_IHL_MASK)
> << 4)
> > > > > return -1;
> > > > >
> > > > > if (pkt->l4_len < sizeof(struct tcp_hdr))
> > > > > return -1;
> > > > >
> > > > > You should also check for TCP options as well.
> > > >
> > > > There are two ways to get ether, ipv4 and tcp headers:
> > > > 1). Use MBUF->l2_len/l3_len...;
> > > > 2). Parse packet and ignore MBUF->l2_len/....
> > > >
> > > > If we follow the choice 1, we don't need to parse packet and
> > > > don't need to check if values of MBUF->l2_len/... are correct,
> > > > since we assume applications will set correct values. If we
> follow
> > > > the choice 2, we don't need to care about the values of MBUF-
> > > >l2_len/...
> > > >
> > > > I am a little confused about your code, since it parses packet
> and
> > > > checks if the values of MBUF->l2_len/... are correct. If we don't
> use
> > > > MBUF->l2_len/... to get ether/ipv4/tcp headers, why should we
> check
> > > > the values of MBUF->l2_len/...?
> > > >
> > >
> > > Agree that we don't need both.
> > > My preference would be to stick with 1).
> > > In many cases user would have already determined l2/l3/l4 len
> > > by this stage.
> > > Konstantin
> >
> > Do we have a generic packet header validation library? Otherwise,
> that would perhaps be a better path. Such a library could probably use
> > some of the flags from the PMD to determine how much to validate in
> software.
>
> AFAIK - we don't have a generic header parsing library.
> Yes, it would be good to have such ability, but I think that's out of
> scope for that patch.
> BTW, volunteers are welcome :)
>
> >
> > And if it is a documented precondition of the GRO library that m-
> >l2_len/l3_len... must be set and sensible, perhaps an RTE_ASSERT()
> could
> > be considered instead of gracefully returning -1?
>
> I suppose that's too extreme.
> What's wrong with checking input parameters and return an error if they
> are invalid?
> Konstantin
>
It is extreme, and it was partly meant as a provocation to think deeper about it: Do we really need to follow Postel's law (https://en.wikipedia.org/wiki/Robustness_principle) in functions where we are in full control ourselves?
Here's what's wrong with it: If each function a packet passes through has to parse the packet headers and validate the mbuf parameters from scratch, it will have an unnecessary performance cost. It should be done only once in the fast path, and subsequent functions should be able to rely on the result of that.
Generally, we should be able to rely on assumptions/preconditions. And when a function relies on something, it is a good thing to describe such preconditions in the function's documentation.
From a high level perspective, DPDK Core should not be a bunch of completely independent libraries, but a consistent library where its functions can rely on preconditions and postconditions of its other functions and their intended use in the fast path.
Regarding documentation, it might also be worth mentioning that the m->l2_len/... fields are described as "fields to support TX offloads" in rte_mbuf.h. So when RX offload features - such as GRO - rely on these fields, perhaps the description in rte_mbuf.h should be updated accordingly.
Med venlig hilsen / kind regards
- Morten Brørup
> -----Original Message-----
> From: Morten Brørup [mailto:mb@smartsharesystems.com]
> Sent: Tuesday, January 8, 2019 10:50 PM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Hu, Jiayu
> <jiayu.hu@intel.com>; Stephen Hemminger
> <stephen@networkplumber.org>
> Cc: dev@dpdk.org; Bie, Tiwei <tiwei.bie@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>; stable@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH] gro: add missing invalid packet checks
>
> > From: Ananyev, Konstantin [mailto:konstantin.ananyev@intel.com]
> > > From: Morten Brørup [mailto:mb@smartsharesystems.com]
> > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev,
> > > > Konstantin
> > > > > > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > > > > >
> > > > > > On Tue, 8 Jan 2019 14:08:45 +0800
> > > > > > Jiayu Hu <jiayu.hu@intel.com> wrote:
> > > > > >
> > > > > > > + /*
> > > > > > > + * Don't process the packet whose Ethernet, IPv4 and TCP
> > > > header
> > > > > > > + * lengths are invalid. In addition, if the IPv4 header
> > > > contains
> > > > > > > + * Options, the packet shouldn't be processed.
> > > > > > > + */
> > > > > > > + if (unlikely(ILLEGAL_ETHER_HDRLEN(pkt->l2_len) ||
> > > > > > > + ILLEGAL_IPV4_HDRLEN(pkt->l3_len) ||
> > > > > > > + ILLEGAL_TCP_HDRLEN(pkt->l4_len)))
> > > > > > > + return -1;
> > > > >
> > > > > In the GRO design, we assume applications give correct
> > > > > MBUF->l2_len/.. for input packets of GRO. Specifically, GRO
> > > > > library assumes applications will set values to MBUF->l2_len/...
> > > > > and guarantee the values are the same as the values in the packet
> > > > > headers. The reason for this assumption is to process header
> > faster.
> > >
> > > > > This is also why I want to add this assumption in the programmer
> > > > > guide.
> > >
> > > +1 to more detailed documentation about assumptions and
> > preconditions.
> > >
> > >
> > > > >
> > > > > The above code is to forbid GRO to process invalid packets, which
> > > > > have invalid packet header lengths, like TCP header length is
> > less
> > > > than
> > > > > 20 bytes.
> > > > >
> > > > > >
> > > > > > I like it when code is as picky as possible when doing
> > > > optimizations because
> > > > > > it reduces possible security riskg.
> > > > > >
> > > > > > To me this looks more confusing and not as careful as doing it
> > > > like:
> > > > > >
> > > > > > if (unlikely(pkt->l2_len != ETHER_HDR_LEN))
> > > > > > return -1;
> > > > > > eth_hdr = rte_pktmbuf_mtod(pkt, struct ether_hdr *);
> > > > > > ipv4_hdr = (struct ipv4_hdr *)((char *)eth_hdr +
> > ETHER_HDR_LEN);
> > > > > >
> > > > > > if (pkt->l3_len != (ipv4->version_ihl & IPV4_HDR_IHL_MASK)
> > << 4)
> > > > > > return -1;
> > > > > >
> > > > > > if (pkt->l4_len < sizeof(struct tcp_hdr))
> > > > > > return -1;
> > > > > >
> > > > > > You should also check for TCP options as well.
> > > > >
> > > > > There are two ways to get ether, ipv4 and tcp headers:
> > > > > 1). Use MBUF->l2_len/l3_len...;
> > > > > 2). Parse packet and ignore MBUF->l2_len/....
> > > > >
> > > > > If we follow the choice 1, we don't need to parse packet and
> > > > > don't need to check if values of MBUF->l2_len/... are correct,
> > > > > since we assume applications will set correct values. If we
> > follow
> > > > > the choice 2, we don't need to care about the values of MBUF-
> > > > >l2_len/...
> > > > >
> > > > > I am a little confused about your code, since it parses packet
> > and
> > > > > checks if the values of MBUF->l2_len/... are correct. If we don't
> > use
> > > > > MBUF->l2_len/... to get ether/ipv4/tcp headers, why should we
> > check
> > > > > the values of MBUF->l2_len/...?
> > > > >
> > > >
> > > > Agree that we don't need both.
> > > > My preference would be to stick with 1).
> > > > In many cases user would have already determined l2/l3/l4 len
> > > > by this stage.
> > > > Konstantin
> > >
> > > Do we have a generic packet header validation library? Otherwise,
> > that would perhaps be a better path. Such a library could probably use
> > > some of the flags from the PMD to determine how much to validate in
> > software.
> >
> > AFAIK - we don't have a generic header parsing library.
> > Yes, it would be good to have such ability, but I think that's out of
> > scope for that patch.
> > BTW, volunteers are welcome :)
> >
> > >
> > > And if it is a documented precondition of the GRO library that m-
> > >l2_len/l3_len... must be set and sensible, perhaps an RTE_ASSERT()
> > could
> > > be considered instead of gracefully returning -1?
> >
> > I suppose that's too extreme.
> > What's wrong with checking input parameters and return an error if they
> > are invalid?
> > Konstantin
> >
> It is extreme, and it was partly meant as a provocation to think deeper
> about it: Do we really need to follow Postel's law
> (https://en.wikipedia.org/wiki/Robustness_principle) in functions where we
> are in full control ourselves?
>
> Here's what's wrong with it: If each function a packet passes through has to
> parse the packet headers and validate the mbuf parameters from scratch, it
> will have an unnecessary performance cost. It should be done only once in
> the fast path, and subsequent functions should be able to rely on the result
> of that.
>
> Generally, we should be able to rely on assumptions/preconditions. And
> when a function relies on something, it is a good thing to describe such
> preconditions in the function's documentation.
>
> From a high level perspective, DPDK Core should not be a bunch of
> completely independent libraries, but a consistent library where its
> functions can rely on preconditions and postconditions of its other
> functions and their intended use in the fast path.
Good point.
>
> Regarding documentation, it might also be worth mentioning that the m-
> >l2_len/... fields are described as "fields to support TX offloads" in
> rte_mbuf.h. So when RX offload features - such as GRO - rely on these fields,
> perhaps the description in rte_mbuf.h should be updated accordingly.
Yes, both IP reassembly and GRO use those fields. We need to update the description.
Thanks,
Jiayu
>
>
> Med venlig hilsen / kind regards
> - Morten Brørup
@@ -208,6 +208,16 @@ gro_tcp4_reassemble(struct rte_mbuf *pkt,
int cmp;
uint8_t find;
+ /*
+ * Don't process the packet whose Ethernet, IPv4 and TCP header
+ * lengths are invalid. In addition, if the IPv4 header contains
+ * Options, the packet shouldn't be processed.
+ */
+ if (unlikely(ILLEGAL_ETHER_HDRLEN(pkt->l2_len) ||
+ ILLEGAL_IPV4_HDRLEN(pkt->l3_len) ||
+ ILLEGAL_TCP_HDRLEN(pkt->l4_len)))
+ return -1;
+
eth_hdr = rte_pktmbuf_mtod(pkt, struct ether_hdr *);
ipv4_hdr = (struct ipv4_hdr *)((char *)eth_hdr + pkt->l2_len);
tcp_hdr = (struct tcp_hdr *)((char *)ipv4_hdr + pkt->l3_len);
@@ -17,6 +17,10 @@
*/
#define MAX_IPV4_PKT_LENGTH UINT16_MAX
+#define ILLEGAL_ETHER_HDRLEN(len) ((len) < ETHER_HDR_LEN)
+#define ILLEGAL_IPV4_HDRLEN(len) ((len) != 20)
+#define ILLEGAL_TCP_HDRLEN(len) ((len) < 20 || (len) > 60)
+
/* Header fields representing a TCP/IPv4 flow */
struct tcp4_flow_key {
struct ether_addr eth_saddr;
@@ -306,6 +306,19 @@ gro_vxlan_tcp4_reassemble(struct rte_mbuf *pkt,
uint16_t hdr_len;
uint8_t find;
+ /*
+ * Don't process the packet whose outer Ethernet, outer IPv4,
+ * inner Ethernet, inner IPv4 and inner TCP header lengths
+ * are invalid. In addition, if the outer or inner IPv4 header
+ * contains Options, the packet shouldn't be processed.
+ */
+ if (unlikely(ILLEGAL_ETHER_HDRLEN(pkt->outer_l2_len) ||
+ ILLEGAL_IPV4_HDRLEN(pkt->outer_l3_len) ||
+ ILLEGAL_ETHER_HDRLEN(pkt->l2_len) ||
+ ILLEGAL_IPV4_HDRLEN(pkt->l3_len) ||
+ ILLEGAL_TCP_HDRLEN(pkt->l4_len)))
+ return -1;
+
outer_eth_hdr = rte_pktmbuf_mtod(pkt, struct ether_hdr *);
outer_ipv4_hdr = (struct ipv4_hdr *)((char *)outer_eth_hdr +
pkt->outer_l2_len);