[RFC PATCH] IP: do not modify ingress packet IP option in ip_options_echo()

[RFC PATCH] IP: do not modify ingress packet IP option in ip_options_echo()

[Paolo Abeni]

While computing the response option set for LRSS, ip_options_echo()
also changes the ingress packet LRSS addresses list, setting
the last one to the dst specific address for the ingress packet
- via memset(start[ ...
The only visible effect of such change - beyond possibly damaging
shared/cloned skbs - is modifying the data carried by ICMP replies
changing the header information for reported the ingress packet,
whichi violates RFC1122 3.2.2.6.
All the others call sites just ignore the ingress packet IP options
after calling ip_options_echo().
This behavior predates git history and apparently was present into
the initial ip_options_echo() implementation in linux 1.3.30 but
still looks wrong.

Removing the fib_compute_spec_dst() call will allow completely
removing, with a later patch, skb->dst usage in ip_options_echo(),
so that we can release the dst entry early in the rx path and
drop a couple of related hacks.

This will evenutally target net-next.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/ipv4/ip_options.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/net/ipv4/ip_options.c b/net/ipv4/ip_options.c
index 93157f2..fdda973 100644
--- a/net/ipv4/ip_options.c
+++ b/net/ipv4/ip_options.c
@@ -174,9 +174,6 @@ int __ip_options_echo(struct ip_options *dopt, struct sk_buff *skb,
 				doffset -= 4;
 		}
 		if (doffset > 3) {
-			__be32 daddr = fib_compute_spec_dst(skb);
-
-			memcpy(&start[doffset-1], &daddr, 4);
 			dopt->faddr = faddr;
 			dptr[0] = start[0];
 			dptr[1] = doffset+3;
-- 
2.9.4

Re: [RFC PATCH] IP: do not modify ingress packet IP option in ip_options_echo()

[David Miller]

From: Paolo Abeni <pabeni@redhat.com>
Date: Fri, 21 Jul 2017 15:55:18 +0200

> diff --git a/net/ipv4/ip_options.c b/net/ipv4/ip_options.c
> index 93157f2..fdda973 100644
> --- a/net/ipv4/ip_options.c
> +++ b/net/ipv4/ip_options.c
> @@ -174,9 +174,6 @@ int __ip_options_echo(struct ip_options *dopt, struct sk_buff *skb,
>  				doffset -= 4;
>  		}
>  		if (doffset > 3) {
> -			__be32 daddr = fib_compute_spec_dst(skb);
> -
> -			memcpy(&start[doffset-1], &daddr, 4);
>  			dopt->faddr = faddr;

This transformation is required, but in the destination not the source.

The red flag is that we are indexing 'start' with 'doffset' instead of
'soffset'.

Re: [RFC PATCH] IP: do not modify ingress packet IP option in ip_options_echo()

[Paolo Abeni]

On Mon, 2017-07-24 at 14:12 -0700, David Miller wrote:
> From: Paolo Abeni <pabeni@redhat.com>
> Date: Fri, 21 Jul 2017 15:55:18 +0200
> 
> > diff --git a/net/ipv4/ip_options.c b/net/ipv4/ip_options.c
> > index 93157f2..fdda973 100644
> > --- a/net/ipv4/ip_options.c
> > +++ b/net/ipv4/ip_options.c
> > @@ -174,9 +174,6 @@ int __ip_options_echo(struct ip_options *dopt, struct sk_buff *skb,
> >                               doffset -= 4;
> >               }
> >               if (doffset > 3) {
> > -                     __be32 daddr = fib_compute_spec_dst(skb);
> > -
> > -                     memcpy(&start[doffset-1], &daddr, 4);
> >                       dopt->faddr = faddr;
> 
> This transformation is required, but in the destination not the source.
> 
> The red flag is that we are indexing 'start' with 'doffset' instead of
> 'soffset'.

Thank you for your reply. I spent some time trying to understand the
reasoning behind the code above, but still it baffles me. 

I have a reproducer sending TCP syns to a closed port using LSRR
routing towards different namespaces without any explicit route for the
server address nor for the way back.
With the vanilla kernel, the ICMP port unreachable replyes contain a
corrupted version of the ingress packet, with the last item of LSRR IP
option addresses list changed to the TCP server endpoint's IP.
With the patched kernel, ICMP port unreachable packets look correct.
The LSRR addresses list used for the backward path is unchanged and
looks correct in both cases (replies are correctly received from the
sender).

Please allow me to articulate a bit more longer.

In the above context 'start' is:

     unsigned char *start = sptr+sopt->srr; // ip_options.c line 156

and sptr is:

     sptr = skb_network_header(skb); // ip_options.c line 94

The patche code is effectively updating the ingress packet source
routing option list. Specifically it's overwriting the last item
('doffset') with the preferred local address used to reach ip_hdr-
>saddr - that is, with the source host address for the backward path.

Updating the last item of the return/echoed source route option list
looks doubtful: at this stage such item should contain the last hop to
be used to reach ip_hdr->saddr on the backward path. The destination
address will be added to LSRR list later by ip_options_build().

According to RFC 1812, section 4.2.2.1:

"The option as received (the recorded route) MUST be passed up to the
transport layer (or to ICMP message processing)."

The ingress packet source route list must not be modified.

"A router MUST provide a means whereby transport protocols and
applications can reverse the source route in a received datagram.  This
reversed source route MUST be inserted into datagrams they originate
(see [INTRO:2] for details) when the router is unaware of policy
constraints"

The return/echoed source route list must be the reverse of the ingress
one, plus:

"When a source route option is created (which would happen when
the router is originating a source routed datagram or is
inserting a source route option as a result of a special
filter), it MUST be correctly formed even if it is being
created by reversing a recorded route that erroneously includes
the source host"

AFACS this is exactly what the code is currently doing in lines 166-
174 of ip_options.c. We must not include the source host address in the
source route address list, as replacing 'start' with 'dptr' would do.

According to all the above I think we should just drop the lines
reported in the patch, am I missing something?

Thanks!

Paolo

Re: [RFC PATCH] IP: do not modify ingress packet IP option in ip_options_echo()

[Hannes Frederic Sowa]

Hello,

David Miller <davem@davemloft.net> writes:

> From: Paolo Abeni <pabeni@redhat.com>
> Date: Fri, 21 Jul 2017 15:55:18 +0200
>
>> diff --git a/net/ipv4/ip_options.c b/net/ipv4/ip_options.c
>> index 93157f2..fdda973 100644
>> --- a/net/ipv4/ip_options.c
>> +++ b/net/ipv4/ip_options.c
>> @@ -174,9 +174,6 @@ int __ip_options_echo(struct ip_options *dopt, struct sk_buff *skb,
>>  				doffset -= 4;
>>  		}
>>  		if (doffset > 3) {
>> -			__be32 daddr = fib_compute_spec_dst(skb);
>> -
>> -			memcpy(&start[doffset-1], &daddr, 4);
>>  			dopt->faddr = faddr;
>
> This transformation is required, but in the destination not the source.

We put a lot of effort trying to figure out why this code is needed,
what intend it has and couldn't still figure out why things are how they
are.

Above, I think you are referring to the update in the options header
that directs the packet to the final hosts, such closing the loop and
allowing the packet to reach its final destination.

This update to the header happens in another function, later in the
stack in ip_options_build:

|	if (opt->srr)
|		memcpy(iph+opt->srr+iph[opt->srr+1]-4, &daddr, 4);

Here we correctly copy the destination address into the last option
field.

> The red flag is that we are indexing 'start' with 'doffset' instead of
> 'soffset'.

As Paolo pointed out, we should not put our own source address into the
options array.

Do you have any idea why this code was added in the first place? I read
through 2.0.33 code and even in context of this version, I couldn't
figure that out.

Because it seems the code and update is dead anyways (besides
incorrectly updating the ICMP reflected payload) I don't see any problem
with removing this code. Do you?

Thanks,
Hannes

Re: [RFC PATCH] IP: do not modify ingress packet IP option in ip_options_echo()

[David Miller]

From: Hannes Frederic Sowa <hannes@stressinduktion.org>
Date: Fri, 28 Jul 2017 16:47:42 -0400

> David Miller <davem@davemloft.net> writes:
> 
>> The red flag is that we are indexing 'start' with 'doffset' instead of
>> 'soffset'.
> 
> As Paolo pointed out, we should not put our own source address into the
> options array.
> 
> Do you have any idea why this code was added in the first place? I read
> through 2.0.33 code and even in context of this version, I couldn't
> figure that out.
> 
> Because it seems the code and update is dead anyways (besides
> incorrectly updating the ICMP reflected payload) I don't see any problem
> with removing this code. Do you?

Ok, now I'm convinced it's dead code.

Thanks for explaining.