netdev - Re: [PATCH RFC] ixgbe: ixgbe_atr() must check if network header is available in headlen

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <1476715125.4409.8.camel@intel.com>
Date:   Mon, 17 Oct 2016 14:38:48 +0000
From:   "Duyck, Alexander H" <alexander.h.duyck@...el.com>
To:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "sowmini.varadhan@...cle.com" <sowmini.varadhan@...cle.com>
Subject: Re: [PATCH RFC] ixgbe: ixgbe_atr() must check if network header is
 available in headlen

On Sat, 2016-10-15 at 17:31 -0400, Sowmini Varadhan wrote:
> For some Tx paths (e.g., tpacket_snd()), ixgbe_atr may be
> passed down an sk_buff that has the network and transport
> header in the paged data, so it needs to make sure these
> headers are available in the headlen bytes to calculate the
> l4_proto.
> 
> This patch bails out if the headlen is "too short", and does
> not attempt to call skb_header_pointer() to get the needed
> bytes: the assumption is that the caller should set things
> up properly if the l4_proto based tx steering is desired.
> 
> > Signed-off-by: Sowmini Varadhan <sowmini.varadhan@...cle.com>
> ---
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |    9 +++++++++
>  1 files changed, 9 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index a244d9a..0868de1 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -7632,6 +7632,7 @@ static void ixgbe_atr(struct ixgbe_ring *ring,
> >  	struct sk_buff *skb;
> >  	__be16 vlan_id;
> >  	int l4_proto;
> > +	int min_hdr_size = 0;
>  
> >  	/* if ring doesn't have a interrupt vector, cannot perform ATR */
> >  	if (!q_vector)
> @@ -7650,6 +7651,14 @@ static void ixgbe_atr(struct ixgbe_ring *ring,
>  
> >  	/* snag network header to get L4 type and address */
> >  	skb = first->skb;
> > +	if (first->protocol == htons(ETH_P_IP))
> > +		min_hdr_size = sizeof(struct iphdr) +
> > +			       sizeof(struct tcphdr);
> > +	else if (first->protocol == htons(ETH_P_IPV6))
> > +		min_hdr_size = sizeof(struct ipv6hdr) +
> > +			       sizeof(struct tcphdr);
> > +	if (min_hdr_size && skb_headlen(skb) < ETH_HLEN + min_hdr_size)
> > +		return;
> >  	hdr.network = skb_network_header(skb);
> >  	if (skb->encapsulation &&
> >  	    first->protocol == htons(ETH_P_IP) &&

So this doesn't really cover all the cases necessary.  There end up
being essentially 3 spots where we need to perform checks to verify the
header size.

The first one is inside the checks for skb->encapsulation, ETH_P_IP,
and IPPROTO_UDP.  We could probably just verify that skb_tail_pointer
is greater than skb_transport_header + (8 + 8 + 14), the minimum size
needed to support VxLAN.

The second block where we need to perform this check would be after
this check once the network header has been updated.  There we need to
verify that hdr.network + 40 is less than or equal to skb_tail_pointer.
 That covers both IPv4 w/ TCP or IPv6.

The third check that needs to be performed is to verify that
hdr.network + hlen is greater than or equal to skb_tail_pointer - 20.
That is needed to verify we have enough room for the tcp header data to
be pulled.

- Alex