Re: [dpdk-dev] [PATCH v2] net/bonding: fix potential out of bounds read

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 4/17/2019 10:33 AM, Radu Nicolau wrote:
> Add validation to pointer constructed from the IPv4 header length
> in order to prevent malformed packets from generating a potential
> out of bounds memory read.
> 
> Fixes: 09150784a776 ("net/bonding: burst mode hash calculation")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
> ---
> v2: add fixes lines
> 
>  drivers/net/bonding/rte_eth_bond_pmd.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
> index b0d191d..25dbddc 100644
> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
> @@ -842,6 +842,7 @@ burst_xmit_l34_hash(struct rte_mbuf **buf, uint16_t nb_pkts,
>  
>  	for (i = 0; i < nb_pkts; i++) {
>  		eth_hdr = rte_pktmbuf_mtod(buf[i], struct ether_hdr *);
> +		size_t pkt_end = (size_t)eth_hdr + rte_pktmbuf_pkt_len(buf[i]);

I guess this should be 'data_len' instead.

>  		proto = eth_hdr->ether_type;
>  		vlan_offset = get_vlan_offset(eth_hdr, &proto);
>  		l3hash = 0;
> @@ -865,13 +866,17 @@ burst_xmit_l34_hash(struct rte_mbuf **buf, uint16_t nb_pkts,
>  					tcp_hdr = (struct tcp_hdr *)
>  						((char *)ipv4_hdr +
>  							ip_hdr_offset);
> -					l4hash = HASH_L4_PORTS(tcp_hdr);
> +					if ((size_t)tcp_hdr + sizeof(*tcp_hdr)
> +							< pkt_end)
> +						l4hash = HASH_L4_PORTS(tcp_hdr);
>  				} else if (ipv4_hdr->next_proto_id ==
>  								IPPROTO_UDP) {
>  					udp_hdr = (struct udp_hdr *)
>  						((char *)ipv4_hdr +
>  							ip_hdr_offset);
> -					l4hash = HASH_L4_PORTS(udp_hdr);
> +					if ((size_t)udp_hdr + sizeof(*udp_hdr)
> +							< pkt_end)
> +						l4hash = HASH_L4_PORTS(udp_hdr);
>  				}
>  			}
>  		} else if  (rte_cpu_to_be_16(ETHER_TYPE_IPv6) == proto) {
> 

Other thing is on action to take when malformed packet is detected, right now it
just prevents the out of bound memory access, but calculated hash will be wrong.
What do you think skipping/dropping the packet in that case, if possible?