netdev - Re: [PATCH]IP: Send an ICMP "Fragment Reassembly Timeout" message when enabling connection track

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]

Message-ID: <4B5844F2.30104@trash.net>
Date:	Thu, 21 Jan 2010 13:13:38 +0100
From:	Patrick McHardy <kaber@...sh.net>
To:	Shan Wei <shanwei@...fujitsu.com>
CC:	David Miller <davem@...emloft.net>, kuznet@....inr.ac.ru,
	pekkas@...core.fi, jmorris@...ei.org, yoshfuji@...ux-ipv6.org,
	eric.dumazet@...il.com, david@...e-labs.org, jorge@...2.net,
	opurdila@...acom.com,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	netfilter-devel@...r.kernel.org
Subject: Re: [PATCH]IP: Send  an ICMP "Fragment Reassembly Timeout" message
 when enabling connection track

Shan Wei wrote:
> No matter whether connection track is enabled, an end host should send 
> an ICMPv4 "Fragment Reassembly Timeout" message when defrag timeout. 
> The reasons are following two points:  
> 
> 1. RFC 792 says:
>    >>>> >> > >   If a host reassembling a fragmented datagram cannot complete the
>    >>>> >> > >   reassembly due to missing fragments within its time limit it
>    >>>> >> > >   discards the datagram, and it may send a time exceeded message.
>    >>>> >> > > 
>    >>>> >> > >   If fragment zero is not available then no time exceeded need be
>    >>>> >> > >   sent at all.
>    >>>> >> > > 
>    >>>> >> > > Read more: http://www.faqs.org/rfcs/rfc792.html#ixzz0aOXRD7Wp
> 
> 2. Patrick McHardy also agrees with this opinion. :-) 
>    About the discussion of this opinion, refer to http://patchwork.ozlabs.org/patch/41649
> 
> The patch fixed the problem like this:
> When enabling connection track, fragments are received at PRE_ROUTING HOOK.
> If they are failed to reassemble, ip_expire() will be called. 
> Before sending an ICMP "Fragment Reassembly Timeout" message, 
> the patch searches router table to get the destination entry only for host type.
> 
> The patch has been tested on both host type and route type.

Looks good. One comment below:

> @@ -205,13 +207,38 @@ static void ip_expire(unsigned long arg)
>  	if ((qp->q.last_in & INET_FRAG_FIRST_IN) && qp->q.fragments != NULL) {
>  		struct sk_buff *head = qp->q.fragments;
>  
> -		/* Send an ICMP "Fragment Reassembly Timeout" message. */
>  		rcu_read_lock();
>  		head->dev = dev_get_by_index_rcu(net, qp->iif);
> -		if (head->dev)
> -			icmp_send(head, ICMP_TIME_EXCEEDED, ICMP_EXC_FRAGTIME, 0);
> -		rcu_read_unlock();
> +		if (!head->dev)
> +			goto out_rcu_unlock;
> +
> +		/*
> +		 * Only search router table for the head fragment,
> +		 * when defraging timeout at PRE_ROUTING HOOK.
> +		 */
> +		if (qp->user == IP_DEFRAG_CONNTRACK_IN && !skb_dst(head)) {
> +			const struct iphdr *iph = ip_hdr(head);
> +			int err = ip_route_input(head, iph->daddr, iph->saddr,
> +						 iph->tos, head->dev);
> +			if (unlikely(err))
> +				goto out_rcu_unlock;
> +
> +			/*
> +			 * Only an end host needs to send an ICMP
> +			 * "Fragment Reassembly Timeout" message, per RFC792.
> +			 */
> +			if (skb_rtable(head)->rt_type != RTN_LOCAL) {
> +				skb_dst_drop(head);

Is manually dropping the dst entry necessary here? It will get released
by the fragment destructor anyways if I'm not mistaken.

> +				goto out_rcu_unlock;
> +			}
> +		}
> +
> +		/* Send an ICMP "Fragment Reassembly Timeout" message. */
> +		icmp_send(head, ICMP_TIME_EXCEEDED, ICMP_EXC_FRAGTIME, 0);
>  	}
> +
> +out_rcu_unlock:
> +	rcu_read_unlock();
>  out:
>  	spin_unlock(&qp->q.lock);
>  	ipq_put(qp);
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html