Re: [dpdk-dev] [PATCH] gro: add missing invalid packet checks

["Morten Brørup" <mb@smartsharesystems.com>]

> -----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?