netdev - Re: [PATCH net-next 06/13] ipv6: Fix nexthdr for reinjection

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] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 12 May 2016 23:23:04 +0300
From:	Shmulik Ladkani <shmulik.ladkani@...il.com>
To:	Tom Herbert <tom@...bertland.com>
Cc:	<davem@...emloft.net>, <netdev@...r.kernel.org>,
	<kernel-team@...com>
Subject: Re: [PATCH net-next 06/13] ipv6: Fix nexthdr for reinjection

Hi Tom,

On Wed, 11 May 2016 09:47:26 -0700 Tom Herbert <tom@...bertland.com> wrote:
> In ip6_input_finish the protocol handle returns a value greater than
> zero the packet needs to be resubmitted using the returned protocol.
> The returned protocol is being ignored and each time through resubmit
> nexthdr is taken from an offest in the packet. This patch fixes that
> so that nexthdr is taken from return value of the protocol handler.
> 
> Signed-off-by: Tom Herbert <tom@...bertland.com>
> ---
>  net/ipv6/ip6_input.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/net/ipv6/ip6_input.c b/net/ipv6/ip6_input.c
> index 6ed5601..2a0258a 100644
> --- a/net/ipv6/ip6_input.c
> +++ b/net/ipv6/ip6_input.c
> @@ -222,13 +222,14 @@ static int ip6_input_finish(struct net *net, struct sock *sk, struct sk_buff *sk
>  	 */
>  
>  	rcu_read_lock();
> -resubmit:
> +
>  	idev = ip6_dst_idev(skb_dst(skb));
>  	if (!pskb_pull(skb, skb_transport_offset(skb)))
>  		goto discard;
>  	nhoff = IP6CB(skb)->nhoff;
>  	nexthdr = skb_network_header(skb)[nhoff];
>  
> +resubmit:

This has already been attempted in 0243508edd "ipv6: Fix protocol
resubmission" and reverted in 1b0ccfe54a.

It looks that in some genuine extension header handling cases of ipv6
(not related to encapsulation), the original resubmission code REALLY
requires one to re-read IP6CB(skb)->nhoff and refetch the nexthdr.

See ipv6_rthdr_rcv and ipv6_destopt_rcv for example:

Snip from ipv6_rthdr_rcv:

	struct inet6_skb_parm *opt = IP6CB(skb);

		opt->lastopt = opt->srcrt = skb_network_header_len(skb);
		skb->transport_header += (hdr->hdrlen + 1) << 3;
		opt->dst0 = opt->dst1;
		opt->dst1 = 0;
		opt->nhoff = (&hdr->nexthdr) - skb_network_header(skb);
		return 1;

Snip from ipv6_destopt_rcv:

		opt = IP6CB(skb);
#if IS_ENABLED(CONFIG_IPV6_MIP6)
		opt->nhoff = dstbuf;
#else
		opt->nhoff = opt->dst1;
#endif
		return 1;

I think there are two "resubmission" cases:
 1. original ipv6 extension header handling, which seem to require
    nexthdr re-read (after IP6CB(skb)->nhoff re-assigned), as seen above
 2. encapsulation resubmission (e.g. fou)

One suggestion: we may identify the encapsulation case by returning the
negative of the proto number.

Another suggestion: we can take your approach, but execute the nexthdr
re-read within ipv6_rthdr_rcv and ipv6_destopt_rcv (and similar
inet6_protocol.handler functions).

Regards,
Shmulik