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

["Hu, Jiayu" <jiayu.hu@intel.com>]

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