[PATCH] ipv4: fix a bug in SRR option matching.

[PATCH] ipv4: fix a bug in SRR option matching.

[Li Wei]

Since commit 7be799a7 (ipv4: Remove rt->rt_dst reference from
ip_forward_options()) and commit 0374d9ce (ipv4: Kill spurious
write to iph->daddr in ip_forward_options()) we use iph->daddr
for SRR option matching and assume iph->daddr equals to rt->rt_dst,
Unfortunately skb_rtable(skb) has been updated in ip_options_rcv_srr()
for the nexthop in SRR option but iph->daddr *not* updated,
We should use the updated rt->rt_dst for SRR option matching
and update iph->daddr here.

Signed-off-by: Li Wei <lw@cn.fujitsu.com>
---
 net/ipv4/ip_options.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/net/ipv4/ip_options.c b/net/ipv4/ip_options.c
index ec93335..8dca67c 100644
--- a/net/ipv4/ip_options.c
+++ b/net/ipv4/ip_options.c
@@ -568,12 +568,13 @@ void ip_forward_options(struct sk_buff *skb)
 		     ) {
 			if (srrptr + 3 > srrspace)
 				break;
-			if (memcmp(&ip_hdr(skb)->daddr, &optptr[srrptr-1], 4) == 0)
+			if (memcmp(&rt->rt_dst, &optptr[srrptr-1], 4) == 0)
 				break;
 		}
 		if (srrptr + 3 <= srrspace) {
 			opt->is_changed = 1;
 			ip_rt_get_source(&optptr[srrptr-1], skb, rt);
+			ip_hdr(skb)->daddr = rt->rt_dst;
 			optptr[2] = srrptr+4;
 		} else if (net_ratelimit())
 			printk(KERN_CRIT "ip_forward(): Argh! Destination lost!\n");
-- 
1.7.3.2

Re: [PATCH] ipv4: fix a bug in SRR option matching.

[David Miller]

From: Li Wei <lw@cn.fujitsu.com>
Date: Tue, 08 Nov 2011 15:56:40 +0800

> Since commit 7be799a7 (ipv4: Remove rt->rt_dst reference from
> ip_forward_options()) and commit 0374d9ce (ipv4: Kill spurious
> write to iph->daddr in ip_forward_options()) we use iph->daddr
> for SRR option matching and assume iph->daddr equals to rt->rt_dst,
> Unfortunately skb_rtable(skb) has been updated in ip_options_rcv_srr()
> for the nexthop in SRR option but iph->daddr *not* updated,
> We should use the updated rt->rt_dst for SRR option matching
> and update iph->daddr here.
> 
> Signed-off-by: Li Wei <lw@cn.fujitsu.com>

Please replace this by whatever logic ip_options_rcv_srr() uses to
determine the destination address.

I would strongly encourage you, when fixing bugs like this, to use
as a hint the intentions of the commit which introduced the bug.  And
try as hard as possible to retain the goals of the guilty commit.

In this case, that means not introducing references to rt->rt_dst
back into the code.

Thank you.

Re: [PATCH] ipv4: fix a bug in SRR option matching.

[Li Wei]

> From: Li Wei <lw@cn.fujitsu.com>
> Date: Tue, 08 Nov 2011 15:56:40 +0800
> 
>> Since commit 7be799a7 (ipv4: Remove rt->rt_dst reference from
>> ip_forward_options()) and commit 0374d9ce (ipv4: Kill spurious
>> write to iph->daddr in ip_forward_options()) we use iph->daddr
>> for SRR option matching and assume iph->daddr equals to rt->rt_dst,
>> Unfortunately skb_rtable(skb) has been updated in ip_options_rcv_srr()
>> for the nexthop in SRR option but iph->daddr *not* updated,
>> We should use the updated rt->rt_dst for SRR option matching
>> and update iph->daddr here.
>>
>> Signed-off-by: Li Wei <lw@cn.fujitsu.com>
> 
> Please replace this by whatever logic ip_options_rcv_srr() uses to
> determine the destination address.
> 
> I would strongly encourage you, when fixing bugs like this, to use
> as a hint the intentions of the commit which introduced the bug.  And
> try as hard as possible to retain the goals of the guilty commit.
> 
> In this case, that means not introducing references to rt->rt_dst
> back into the code.
> 
> Thank you.
> 
> 

Thank you for your advice, I reviewed the code again think that as you said
in commit def57687, "No matter what kind of header mangling occurs due to IP
options processing, rt->rt_dst will always equal iph->daddr in the packet",
iph->daddr in ip_options_rcv_srr() should be updated either as skb_rtable(skb)
has been updated for 'nexthop'. So we can elide all rt->rt_dst reference
in ip_forward() and ip_forward_options().

I will submit another patch to fix this bug.