[RFC] sctp/tcp: Question -- ICMPv4 length check (not) redundant?

[RFC] sctp/tcp: Question -- ICMPv4 length check (not) redundant?

[Gerrit Renker]

I wonder if the two pieces of code as marked by the patch below still
serve a purpose. 

>From grepping through the net/ code it seems they are both redundant.

This is on the basis that

 * the v6 counterparts of these handlers ({tcp,sctp}_v6_err) do not have
   this payload-length check;

 * icmp_unreach() in net/ipv4/icmp.c already has this test:

	/* Checkin full IP header plus 8 bytes of protocol to
	 * avoid additional coding at protocol handlers.
	 */
        if (!pskb_may_pull(skb, iph->ihl * 4 + 8))
                goto out;
 
 * a similar test is implemented by icmpv6_notify() in net/ipv6/icmp.c;

 * this type of test seems to be absent in all of the following handlers:
   - tcp_v6_err()		- sctp_v6_err() 
   - __udp4_lib_err()		- __udp6_lib_err()
   - ah4_err()			- ah6_err()
   - esp4_err()			- esp6_err()
   - xfrm_tunnel_err()		- xfrm6_tunnel_err()
   - tunnel4_err()		- tunnel6_err()		- tunnel64_err()
   - ipip_err()			- ipip6_err()
   - ip4ip6_err()		- ip6ip6_err() 
   - ipcomp4_err()		- ipcomp6_err()

---------------------------------------------------------------------------
 The following two are the exceptions in question.
 The question is whether there is a reason for the code marked below.
---------------------------------------------------------------------------

--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -344,11 +344,6 @@ void tcp_v4_err(struct sk_buff *skb, u32 info)
 	int err;
 	struct net *net = dev_net(skb->dev);
 
-	if (skb->len < (iph->ihl << 2) + 8) {
-		ICMP_INC_STATS_BH(net, ICMP_MIB_INERRORS);
-		return;
-	}
-
 	sk = inet_lookup(net, &tcp_hashinfo, iph->daddr, th->dest,
 			iph->saddr, th->source, inet_iif(skb));
 	if (!sk) {
--- a/net/sctp/input.c
+++ b/net/sctp/input.c
@@ -534,11 +534,6 @@ void sctp_v4_err(struct sk_buff *skb, __u32 info)
 	sk_buff_data_t saveip, savesctp;
 	int err;
 
-	if (skb->len < ihlen + 8) {
-		ICMP_INC_STATS_BH(&init_net, ICMP_MIB_INERRORS);
-		return;
-	}
-
 	/* Fix up skb to look at the embedded net header. */
 	saveip = skb->network_header;
 	savesctp = skb->transport_header;

Re: [RFC] sctp/tcp: Question -- ICMPv4 length check (not) redundant?

[Vlad Yasevich]

Gerrit Renker wrote:
> I wonder if the two pieces of code as marked by the patch below still
> serve a purpose. 
> 
> From grepping through the net/ code it seems they are both redundant.
> 
> This is on the basis that
> 
>  * the v6 counterparts of these handlers ({tcp,sctp}_v6_err) do not have
>    this payload-length check;
> 
>  * icmp_unreach() in net/ipv4/icmp.c already has this test:
> 
> 	/* Checkin full IP header plus 8 bytes of protocol to
> 	 * avoid additional coding at protocol handlers.
> 	 */
>         if (!pskb_may_pull(skb, iph->ihl * 4 + 8))
>                 goto out;
>  
>  * a similar test is implemented by icmpv6_notify() in net/ipv6/icmp.c;
> 
>  * this type of test seems to be absent in all of the following handlers:
>    - tcp_v6_err()		- sctp_v6_err() 
>    - __udp4_lib_err()		- __udp6_lib_err()
>    - ah4_err()			- ah6_err()
>    - esp4_err()			- esp6_err()
>    - xfrm_tunnel_err()		- xfrm6_tunnel_err()
>    - tunnel4_err()		- tunnel6_err()		- tunnel64_err()
>    - ipip_err()			- ipip6_err()
>    - ip4ip6_err()		- ip6ip6_err() 
>    - ipcomp4_err()		- ipcomp6_err()
> 
> ---------------------------------------------------------------------------
>  The following two are the exceptions in question.
>  The question is whether there is a reason for the code marked below.
> ---------------------------------------------------------------------------
> 
> --- a/net/ipv4/tcp_ipv4.c
> +++ b/net/ipv4/tcp_ipv4.c
> @@ -344,11 +344,6 @@ void tcp_v4_err(struct sk_buff *skb, u32 info)
>  	int err;
>  	struct net *net = dev_net(skb->dev);
>  
> -	if (skb->len < (iph->ihl << 2) + 8) {
> -		ICMP_INC_STATS_BH(net, ICMP_MIB_INERRORS);
> -		return;
> -	}
> -
>  	sk = inet_lookup(net, &tcp_hashinfo, iph->daddr, th->dest,
>  			iph->saddr, th->source, inet_iif(skb));
>  	if (!sk) {
> --- a/net/sctp/input.c
> +++ b/net/sctp/input.c
> @@ -534,11 +534,6 @@ void sctp_v4_err(struct sk_buff *skb, __u32 info)
>  	sk_buff_data_t saveip, savesctp;
>  	int err;
>  
> -	if (skb->len < ihlen + 8) {
> -		ICMP_INC_STATS_BH(&init_net, ICMP_MIB_INERRORS);
> -		return;
> -	}
> -
>  	/* Fix up skb to look at the embedded net header. */
>  	saveip = skb->network_header;
>  	savesctp = skb->transport_header;

This one looks useless.  Thanks for noticing.  I'll remove it.

-vlad

> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [RFC] sctp/tcp: Question -- ICMPv4 length check (not) redundant?

[David Miller]

From: Vlad Yasevich <vladislav.yasevich@hp.com>
Date: Fri, 25 Jul 2008 22:15:33 -0400

> Gerrit Renker wrote:
> >  * icmp_unreach() in net/ipv4/icmp.c already has this test:
> > 
> > 	/* Checkin full IP header plus 8 bytes of protocol to
> > 	 * avoid additional coding at protocol handlers.
> > 	 */
> >         if (!pskb_may_pull(skb, iph->ihl * 4 + 8))
> >                 goto out;

I just noticed that this won't bump the ICMP MIB counter.

The ICMP code will only bump the counter if the IP header isn't there,
or has a bogus header length.

But it is clear in the callers that the intention is (or was) to bump
the counter if the 8 bytes of post IP header bits are not there.

Re: [RFC] sctp/tcp: Question -- ICMPv4 length check (not) redundant?

[Gerrit Renker]

| >  * icmp_unreach() in net/ipv4/icmp.c already has this test:
| > 
| > 	/* Checkin full IP header plus 8 bytes of protocol to
| > 	 * avoid additional coding at protocol handlers.
| > 	 */
| >         if (!pskb_may_pull(skb, iph->ihl * 4 + 8))
| >                 goto out;
| 
| I just noticed that this won't bump the ICMP MIB counter.
| 
| The ICMP code will only bump the counter if the IP header isn't there,
| or has a bogus header length.
| 
| But it is clear in the callers that the intention is (or was) to bump
| the counter if the 8 bytes of post IP header bits are not there.
| 
If `goto out' is replaced with `goto out_err' in icmp_unreach(),
it may be worth considering the analogous case in icmpv6_notify():

	/* Checkin header including 8 bytes of inner protocol header. */
        if (!pskb_may_pull(skb, inner_offset+8))
                return;

and the `discard_it' jump label in icmpv6_rcv().

Re: [RFC] sctp/tcp: Question -- ICMPv4 length check (not) redundant?

[David Miller]

From: Gerrit Renker <gerrit@erg.abdn.ac.uk>
Date: Sat, 26 Jul 2008 08:03:08 +0100

> If `goto out' is replaced with `goto out_err' in icmp_unreach(),
> it may be worth considering the analogous case in icmpv6_notify():
> 
> 	/* Checkin header including 8 bytes of inner protocol header. */
>         if (!pskb_may_pull(skb, inner_offset+8))
>                 return;
> 
> and the `discard_it' jump label in icmpv6_rcv().

What if the lower protocol doesn't have a header that it
at least 8 bytes? :-)

I think that's yet another reason why the check and counter bump were
originally down in the per-protocol hander.

Re: [RFC] sctp/tcp: Question -- ICMPv4 length check (not) redundant?

[Gerrit Renker]

| What if the lower protocol doesn't have a header that it
| at least 8 bytes? :-)
| 
| I think that's yet another reason why the check and counter bump were
| originally down in the per-protocol hander.
| 
The only reason I can think of is RFC 1122, 3.2.2:
 "Every ICMP error message includes the Internet header and at
  least the first 8 data octets of the datagram that triggered
  the error; more than 8 octets MAY be sent [...]"

But for ICMPv6 the requirement is less strict (RFC 4443, 2.4):
 "Every ICMPv6 error message (type < 128) MUST include as much of
  the IPv6 offending (invoking) packet (the packet that caused the
  error) as possible without making the error message packet exceed
  the minimum IPv6 MTU [IPv6]."

Re: [RFC] sctp/tcp: Question -- ICMPv4 length check (not) redundant?

[Herbert Xu]

David Miller <davem@davemloft.net> wrote:
>
> What if the lower protocol doesn't have a header that it
> at least 8 bytes? :-)

When such a protocol enters our tree then we can undo this patch :)

Seriously apart from raw sockets it isn't possible to generate
anything with less than 8 bytes in the IP payload.  As our raw
error handler is separate anyway all we have to do is move the
common check below the raw handler and then we can proceed to
remove the protocol-specific checks as Gerrit proposed.

The stastic is just an oversight AFAICS from looking at git history.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [RFC] sctp/tcp: Question -- ICMPv4 length check (not) redundant?

[Herbert Xu]

On Sun, Jul 27, 2008 at 12:48:11PM +0800, Herbert Xu wrote:
> David Miller <davem@davemloft.net> wrote:
> >
> > What if the lower protocol doesn't have a header that it
> > at least 8 bytes? :-)
> 
> When such a protocol enters our tree then we can undo this patch :)

OK I take that back as IPComp is only 4 bytes long :)
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [RFC] sctp/tcp: Question -- ICMPv4 length check (not) redundant?

[Gerrit Renker]

Thank you for the input. To sum up,
 * removing the per-protocol "ICMP payload too short" test and error counter
   increment as initially suggested does not seem right;
 * there are protocols such as IPComp which have a header length less than
   8 bytes(and for these the test in the ICMP handler may be too much);
 * there are protocols such as DCCP which need more than 8 bytes (at least 12)
   to interpret the ICMP message in a meaningful way;
 * the requirement of having at least 8 bytes of transport-layer data available
   is stringent (afaik) only for ICMPv4, but not ICMPv6;
 * only TCP/SCTP seem to have a proper per-protocol "payload too short" test;
 * for DCCP, the work is actually doubled since 
   - first the ICMP handler tests for minimally 8 bytes, 
   - then the DCCP error handler tests for required minimum of 12 bytes.

Thus the patch at the begginning of this thread should be disregarded.
It might be worth to consider per-protocol handlers.

Gerrit

Re: [RFC] sctp/tcp: Question -- ICMPv4 length check (not) redundant?

[Herbert Xu]

On Mon, Jul 28, 2008 at 12:25:27PM +0100, Gerrit Renker wrote:
> Thank you for the input. To sum up,
>  * removing the per-protocol "ICMP payload too short" test and error counter
>    increment as initially suggested does not seem right;
>  * there are protocols such as IPComp which have a header length less than
>    8 bytes(and for these the test in the ICMP handler may be too much);
>  * there are protocols such as DCCP which need more than 8 bytes (at least 12)
>    to interpret the ICMP message in a meaningful way;
>  * the requirement of having at least 8 bytes of transport-layer data available
>    is stringent (afaik) only for ICMPv4, but not ICMPv6;
>  * only TCP/SCTP seem to have a proper per-protocol "payload too short" test;
>  * for DCCP, the work is actually doubled since 
>    - first the ICMP handler tests for minimally 8 bytes, 
>    - then the DCCP error handler tests for required minimum of 12 bytes.
> 
> Thus the patch at the begginning of this thread should be disregarded.
> It might be worth to consider per-protocol handlers.

Yes the centralised test predates protocols such as IPComp and
DCCP.  I think it's best to just kill it and add checks in the
protocol handlers where needed.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [RFC] sctp/tcp: Question -- ICMPv4 length check (not) redundant?

[Vlad Yasevich]

Ok, I seem to be confused here.


Gerrit Renker wrote:
> Thank you for the input. To sum up,
>  * removing the per-protocol "ICMP payload too short" test and error counter
>    increment as initially suggested does not seem right;
>  * there are protocols such as IPComp which have a header length less than
>    8 bytes(and for these the test in the ICMP handler may be too much);

How so.  Either 8 bytes will be there or not.  If the 8 bytes aren't there,
we won't event make to the error handlers as it stands right now.  As is, if
the ICMP packet doesn't contain the 8 bytes, it's too short according to ICMP spec.

>  * there are protocols such as DCCP which need more than 8 bytes (at least 12)
>    to interpret the ICMP message in a meaningful way;

If you need more then 8 bytes, that protocol needs to have a check for the extra
space.  8 byte are mandatory.

>  * the requirement of having at least 8 bytes of transport-layer data available
>    is stringent (afaik) only for ICMPv4, but not ICMPv6;

Splitting hairs here, but ICMPv6 is much more stringent.  It forces you send as
close to 1280 byte error messages as possible.

>  * only TCP/SCTP seem to have a proper per-protocol "payload too short" test;

Hm..  In the standard case, these do seem to be redundant since 8 bytes are required
by ICMP spec.

>  * for DCCP, the work is actually doubled since 
>    - first the ICMP handler tests for minimally 8 bytes, 
>    - then the DCCP error handler tests for required minimum of 12 bytes.

DCCP and any other protocol that requires more error data should check for it in
its own handler.  8 bytes should be guaranteed to such handler.

What am I missing?

Thanks
-vlad

> 
> Thus the patch at the begginning of this thread should be disregarded.
> It might be worth to consider per-protocol handlers.
> 
> Gerrit
> 

Re: [RFC] sctp/tcp: Question -- ICMPv4 length check (not) redundant?

[Gerrit Renker]

| >  * only TCP/SCTP seem to have a proper per-protocol "payload too short" test;
| 
| Hm..  In the standard case, these do seem to be redundant since 8 bytes are required
| by ICMP spec.
| 
| >  * for DCCP, the work is actually doubled since 
| >    - first the ICMP handler tests for minimally 8 bytes, 
| >    - then the DCCP error handler tests for required minimum of 12 bytes.
| 
| DCCP and any other protocol that requires more error data should check for it in
| its own handler.  8 bytes should be guaranteed to such handler.
| 
| What am I missing?
| 
As per last message, please disregard the patch suggestion made at the
beginning of the thread.

In TCP, the 8 bytes happen to be enough for doing sequence number checks. Other
protocols have different header lengths and semantics. Thus doing the checks
at the transport layer makes more sense than in the ICMP handler.

RFC 1122 is almost 20 years old, from a time before IPcomp, SCTP, or DCCP.

Re: [RFC] sctp/tcp: Question -- ICMPv4 length check (not) redundant?

[Vlad Yasevich]

Gerrit Renker wrote:
> | >  * only TCP/SCTP seem to have a proper per-protocol "payload too short" test;
> | 
> | Hm..  In the standard case, these do seem to be redundant since 8 bytes are required
> | by ICMP spec.
> | 
> | >  * for DCCP, the work is actually doubled since 
> | >    - first the ICMP handler tests for minimally 8 bytes, 
> | >    - then the DCCP error handler tests for required minimum of 12 bytes.
> | 
> | DCCP and any other protocol that requires more error data should check for it in
> | its own handler.  8 bytes should be guaranteed to such handler.
> | 
> | What am I missing?
> | 
> As per last message, please disregard the patch suggestion made at the
> beginning of the thread.
> 
> In TCP, the 8 bytes happen to be enough for doing sequence number checks. Other
> protocols have different header lengths and semantics. Thus doing the checks
> at the transport layer makes more sense than in the ICMP handler.
> 
> RFC 1122 is almost 20 years old, from a time before IPcomp, SCTP, or DCCP.

So the suggestion really is then to remove the length check icmp_unreach()?

Because as it stands right now, the protocol error handler will not be invoked
if we don't have the iphdr + 8 bytes worth of data.  That's is actually a requirement
from the ICMP rfc 792.

Seems that not including those 8 bytes is a violation of that spec.  So, icmp_unreach()
should keep current code and jump to out_err upon failure.

Each upper layer protocol that requires additional space, should validate the existence
of that additional space and handle it appropriately.

It doesn't matter that IPcomp header is only 4 bytes.  ICMP error requires ihlen + 8 bytes and 
at least that much should be provided  Otherwise, it a malformed ICMP packet.

-vlad

Re: [RFC] sctp/tcp: Question -- ICMPv4 length check (not) redundant?

[Gerrit Renker]

| > In TCP, the 8 bytes happen to be enough for doing sequence number checks. Other
| > protocols have different header lengths and semantics. Thus doing the checks
| > at the transport layer makes more sense than in the ICMP handler.
| > 
| > RFC 1122 is almost 20 years old, from a time before IPcomp, SCTP, or DCCP.
| 
| So the suggestion really is then to remove the length check icmp_unreach()?
| 
Yes, but there are a large number of handlers in which the check is absent
(TCPv4, SCTPv4 and DCCP are exceptions). This would need to be added.

The ipv6/icmp.c code agrees with your suggestion of using 8 bytes as
lower bound.

I did not want to jump to the conclusion of writing a patch, since there are
more complex uses of ICMP (such as in a nested tunnel, perhaps with IPsec).
This needs to be understood.

Gerrit

Re: [RFC] sctp/tcp: Question -- ICMPv4 length check (not) redundant?

[Vlad Yasevich]

Gerrit Renker wrote:
> | > In TCP, the 8 bytes happen to be enough for doing sequence number checks. Other
> | > protocols have different header lengths and semantics. Thus doing the checks
> | > at the transport layer makes more sense than in the ICMP handler.
> | > 
> | > RFC 1122 is almost 20 years old, from a time before IPcomp, SCTP, or DCCP.
> | 
> | So the suggestion really is then to remove the length check icmp_unreach()?
> | 
> Yes, but there are a large number of handlers in which the check is absent
> (TCPv4, SCTPv4 and DCCP are exceptions). This would need to be added.
> 
> The ipv6/icmp.c code agrees with your suggestion of using 8 bytes as
> lower bound.
> 
> I did not want to jump to the conclusion of writing a patch, since there are
> more complex uses of ICMP (such as in a nested tunnel, perhaps with IPsec).
> This needs to be understood.
> 

Well, simply from the ICMP protocol perspective the 8 byte lower bound is all
that's required.  Each tunnel decapsulation point would have to provide it's own
additional validation on top of the 8 byte, but everyone should be guaranteed at
least 8 bytes for IPv4 ICMP errors.

The IPv6 checks are much different.  The MUST requirement is to provide as much
data as possible upto IPv6 min mtu.  So, the IPv6 icmp code should probably look
to see if min(payload_len, min_mtu) is provided.

-vlad

Re: [RFC] sctp/tcp: Question -- ICMPv4 length check (not) redundant?

[David Miller]

From: Vlad Yasevich <vladislav.yasevich@hp.com>
Date: Mon, 28 Jul 2008 14:09:55 -0400

> The IPv6 checks are much different.  The MUST requirement is to provide as much
> data as possible upto IPv6 min mtu.  So, the IPv6 icmp code should probably look
> to see if min(payload_len, min_mtu) is provided.

"Be liberal in what you accept..." -Jon Postel

Also, do you know any case where 576 bytes won't provide enough
quoted packet bytes? :-)

Re: [RFC] sctp/tcp: Question -- ICMPv4 length check (not) redundant?

[Vlad Yasevich]

David Miller wrote:
> From: Vlad Yasevich <vladislav.yasevich@hp.com>
> Date: Mon, 28 Jul 2008 14:09:55 -0400
> 
>> The IPv6 checks are much different.  The MUST requirement is to provide as much
>> data as possible upto IPv6 min mtu.  So, the IPv6 icmp code should probably look
>> to see if min(payload_len, min_mtu) is provided.
> 
> "Be liberal in what you accept..." -Jon Postel

Yeah, that check is really too strict.

> 
> Also, do you know any case where 576 bytes won't provide enough
> quoted packet bytes? :-)
> 

Yes, I've seen a case where a full 1280 was needed.  It involved a
very convoluted packet that had extension headers and then went
through 2 encapsulations.  This was an extreme test scenario and
not a real world example.

-vlad

Re: [RFC] sctp/tcp: Question -- ICMPv4 length check (not) redundant?

[Herbert Xu]

On Mon, Jul 28, 2008 at 01:27:46PM -0400, Vlad Yasevich wrote:
> 
> So the suggestion really is then to remove the length check icmp_unreach()?

Yes.

> Because as it stands right now, the protocol error handler will not be invoked
> if we don't have the iphdr + 8 bytes worth of data.  That's is actually a requirement
> from the ICMP rfc 792.

That requirement only makes sense if the original packet has at
least 8 bytes of payload.  Since the RFC doesn't talk about
padding in case it doesn't have 8 byte, the behaviour in that
case is clearly undefined.

As far as Linux is concerned, we've never done padding if there
is less than 8 bytes of payload.  So as such we must be prepared
to deal with that on the input side as well.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt