[XFRM]: Fix ICMP tempsel

[XFRM]: Fix ICMP tempsel

[Patrick McHardy]

[-- Attachment #1: Type: text/plain, Size: 244 bytes --]

The selector ports are initialized to fl_ip_sport/fl_ip_dport instead
of xfrm_flowi_sport(fl)/xfrm_flowi_dport(fl). This is wrong for ICMP,
type and code should be stored in sport and dport, in struct flowi both
are contained in fl_ip_sport.



[-- Attachment #2: x --]
[-- Type: text/plain, Size: 1664 bytes --]

# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
#   2005/02/19 12:35:18+01:00 kaber@coreworks.de 
#   [XFRM]: Fix ICMP tempsel
#   
#   Signed-off-by: Patrick McHardy <kaber@trash.net>
# 
# net/ipv6/xfrm6_state.c
#   2005/02/19 12:35:06+01:00 kaber@coreworks.de +2 -2
#   [XFRM]: Fix ICMP tempsel
#   
#   Signed-off-by: Patrick McHardy <kaber@trash.net>
# 
# net/ipv4/xfrm4_state.c
#   2005/02/19 12:35:06+01:00 kaber@coreworks.de +2 -2
#   [XFRM]: Fix ICMP tempsel
#   
#   Signed-off-by: Patrick McHardy <kaber@trash.net>
# 
diff -Nru a/net/ipv4/xfrm4_state.c b/net/ipv4/xfrm4_state.c
--- a/net/ipv4/xfrm4_state.c	2005-02-19 12:36:31 +01:00
+++ b/net/ipv4/xfrm4_state.c	2005-02-19 12:36:31 +01:00
@@ -20,9 +20,9 @@
 {
 	x->sel.daddr.a4 = fl->fl4_dst;
 	x->sel.saddr.a4 = fl->fl4_src;
-	x->sel.dport = fl->fl_ip_dport;
+	x->sel.dport = xfrm_flowi_dport(fl);
 	x->sel.dport_mask = ~0;
-	x->sel.sport = fl->fl_ip_sport;
+	x->sel.sport = xfrm_flowi_sport(fl);
 	x->sel.sport_mask = ~0;
 	x->sel.prefixlen_d = 32;
 	x->sel.prefixlen_s = 32;
diff -Nru a/net/ipv6/xfrm6_state.c b/net/ipv6/xfrm6_state.c
--- a/net/ipv6/xfrm6_state.c	2005-02-19 12:36:31 +01:00
+++ b/net/ipv6/xfrm6_state.c	2005-02-19 12:36:31 +01:00
@@ -27,9 +27,9 @@
 	 * to current session. */
 	ipv6_addr_copy((struct in6_addr *)&x->sel.daddr, &fl->fl6_dst);
 	ipv6_addr_copy((struct in6_addr *)&x->sel.saddr, &fl->fl6_src);
-	x->sel.dport = fl->fl_ip_dport;
+	x->sel.dport = xfrm_flowi_dport(fl);
 	x->sel.dport_mask = ~0;
-	x->sel.sport = fl->fl_ip_sport;
+	x->sel.sport = xfrm_flowi_sport(fl);
 	x->sel.sport_mask = ~0;
 	x->sel.prefixlen_d = 128;
 	x->sel.prefixlen_s = 128;

Re: [XFRM]: Fix ICMP tempsel

[YOSHIFUJI Hideaki / 吉藤英明]

In article <4217266F.6090700@trash.net> (at Sat, 19 Feb 2005 12:43:43 +0100), Patrick McHardy <kaber@trash.net> says:

> The selector ports are initialized to fl_ip_sport/fl_ip_dport instead
> of xfrm_flowi_sport(fl)/xfrm_flowi_dport(fl). This is wrong for ICMP,
> type and code should be stored in sport and dport, in struct flowi both
> are contained in fl_ip_sport.

I agree.

--yoshfuji

Re: [XFRM]: Fix ICMP tempsel

[Herbert Xu]

On Sat, Feb 19, 2005 at 12:43:43PM +0100, Patrick McHardy wrote:
> The selector ports are initialized to fl_ip_sport/fl_ip_dport instead
> of xfrm_flowi_sport(fl)/xfrm_flowi_dport(fl). This is wrong for ICMP,
> type and code should be stored in sport and dport, in struct flowi both
> are contained in fl_ip_sport.

I know this comment is probably a bit late but why didn't we simply put
type/code into sport/dport in struct flowi instead of introducing the
monstrosities of xfrm_flowi_sport/xfrm_flowi_dport?

Something like

struct {
	__u16	type;
	__u16	code;
} icmpt;

would've done (and still would do) the trick, no?

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: [XFRM]: Fix ICMP tempsel

[Patrick McHardy]

Herbert Xu wrote:

>I know this comment is probably a bit late but why didn't we simply put
>type/code into sport/dport in struct flowi instead of introducing the
>monstrosities of xfrm_flowi_sport/xfrm_flowi_dport?
>
>Something like
>
>struct {
>	__u16	type;
>	__u16	code;
>} icmpt;
>
>would've done (and still would do) the trick, no?
>
I agree, that is better.

Regards
Patrick

Re: [XFRM]: Fix ICMP tempsel

[Patrick McHardy]

[-- Attachment #1: Type: text/plain, Size: 500 bytes --]

Herbert Xu wrote:

> I know this comment is probably a bit late but why didn't we simply put
> type/code into sport/dport in struct flowi instead of introducing the
> monstrosities of xfrm_flowi_sport/xfrm_flowi_dport?
> 
> Something like
> 
> struct {
> 	__u16	type;
> 	__u16	code;
> } icmpt;
> 
> would've done (and still would do) the trick, no?

Here is an updated patch that kills xfrm_flowi_{sport,dport}.
I've checked around, there seems to be nothing that relies
on type and code beeing u8.


[-- Attachment #2: x --]
[-- Type: text/plain, Size: 4116 bytes --]

# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
#   2005/02/20 06:19:59+01:00 kaber@coreworks.de 
#   [XFRM]: Fix ICMP tempsel
#   
#   The selector ports are initialized to fl_ip_sport/fl_ip_dport instead
#   of xfrm_flowi_sport(fl)/xfrm_flowi_dport(fl). This is wrong for ICMP,
#   type and code should be stored in sport and dport, in struct flowi both
#   are contained in fl_ip_sport.
#   
#   This patch adjusts struct flowi to store ICMP type/code in sport/dport
#   and kills xfrm_flowi_sport/dport instead of changing xfrm_init_tempsel(),
#   as suggested by Herbert Xu.
#   
#   Signed-off-by: Patrick McHardy <kaber@trash.net>
# 
# include/net/xfrm.h
#   2005/02/20 06:19:50+01:00 kaber@coreworks.de +4 -44
#   [XFRM]: Fix ICMP tempsel
#   
#   The selector ports are initialized to fl_ip_sport/fl_ip_dport instead
#   of xfrm_flowi_sport(fl)/xfrm_flowi_dport(fl). This is wrong for ICMP,
#   type and code should be stored in sport and dport, in struct flowi both
#   are contained in fl_ip_sport.
#   
#   This patch adjusts struct flowi to store ICMP type/code in sport/dport
#   and kills xfrm_flowi_sport/dport instead of changing xfrm_init_tempsel(),
#   as suggested by Herbert Xu.
#   
#   Signed-off-by: Patrick McHardy <kaber@trash.net>
# 
# include/net/flow.h
#   2005/02/20 06:19:50+01:00 kaber@coreworks.de +2 -2
#   [XFRM]: Fix ICMP tempsel
#   
#   The selector ports are initialized to fl_ip_sport/fl_ip_dport instead
#   of xfrm_flowi_sport(fl)/xfrm_flowi_dport(fl). This is wrong for ICMP,
#   type and code should be stored in sport and dport, in struct flowi both
#   are contained in fl_ip_sport.
#   
#   This patch adjusts struct flowi to store ICMP type/code in sport/dport
#   and kills xfrm_flowi_sport/dport instead of changing xfrm_init_tempsel(),
#   as suggested by Herbert Xu.
#   
#   Signed-off-by: Patrick McHardy <kaber@trash.net>
# 
diff -Nru a/include/net/flow.h b/include/net/flow.h
--- a/include/net/flow.h	2005-02-20 06:20:34 +01:00
+++ b/include/net/flow.h	2005-02-20 06:20:34 +01:00
@@ -58,8 +58,8 @@
 		} ports;
 
 		struct {
-			__u8	type;
-			__u8	code;
+			__u16	type;
+			__u16	code;
 		} icmpt;
 
 		struct {
diff -Nru a/include/net/xfrm.h b/include/net/xfrm.h
--- a/include/net/xfrm.h	2005-02-20 06:20:34 +01:00
+++ b/include/net/xfrm.h	2005-02-20 06:20:34 +01:00
@@ -417,53 +417,13 @@
 	return 1;
 }
 
-static __inline__
-u16 xfrm_flowi_sport(struct flowi *fl)
-{
-	u16 port;
-	switch(fl->proto) {
-	case IPPROTO_TCP:
-	case IPPROTO_UDP:
-	case IPPROTO_SCTP:
-		port = fl->fl_ip_sport;
-		break;
-	case IPPROTO_ICMP:
-	case IPPROTO_ICMPV6:
-		port = htons(fl->fl_icmp_type);
-		break;
-	default:
-		port = 0;	/*XXX*/
-	}
-	return port;
-}
-
-static __inline__
-u16 xfrm_flowi_dport(struct flowi *fl)
-{
-	u16 port;
-	switch(fl->proto) {
-	case IPPROTO_TCP:
-	case IPPROTO_UDP:
-	case IPPROTO_SCTP:
-		port = fl->fl_ip_dport;
-		break;
-	case IPPROTO_ICMP:
-	case IPPROTO_ICMPV6:
-		port = htons(fl->fl_icmp_code);
-		break;
-	default:
-		port = 0;	/*XXX*/
-	}
-	return port;
-}
-
 static inline int
 __xfrm4_selector_match(struct xfrm_selector *sel, struct flowi *fl)
 {
 	return  addr_match(&fl->fl4_dst, &sel->daddr, sel->prefixlen_d) &&
 		addr_match(&fl->fl4_src, &sel->saddr, sel->prefixlen_s) &&
-		!((xfrm_flowi_dport(fl) ^ sel->dport) & sel->dport_mask) &&
-		!((xfrm_flowi_sport(fl) ^ sel->sport) & sel->sport_mask) &&
+		!((fl->fl_ip_dport ^ sel->dport) & sel->dport_mask) &&
+		!((fl->fl_ip_sport ^ sel->sport) & sel->sport_mask) &&
 		(fl->proto == sel->proto || !sel->proto) &&
 		(fl->oif == sel->ifindex || !sel->ifindex);
 }
@@ -473,8 +433,8 @@
 {
 	return  addr_match(&fl->fl6_dst, &sel->daddr, sel->prefixlen_d) &&
 		addr_match(&fl->fl6_src, &sel->saddr, sel->prefixlen_s) &&
-		!((xfrm_flowi_dport(fl) ^ sel->dport) & sel->dport_mask) &&
-		!((xfrm_flowi_sport(fl) ^ sel->sport) & sel->sport_mask) &&
+		!((fl->fl_ip_dport ^ sel->dport) & sel->dport_mask) &&
+		!((fl->fl_ip_sport ^ sel->sport) & sel->sport_mask) &&
 		(fl->proto == sel->proto || !sel->proto) &&
 		(fl->oif == sel->ifindex || !sel->ifindex);
 }

Re: [XFRM]: Fix ICMP tempsel

[YOSHIFUJI Hideaki / 吉藤英明]

In article <42182082.9060301@trash.net> (at Sun, 20 Feb 2005 06:30:42 +0100), Patrick McHardy <kaber@trash.net> says:

> Herbert Xu wrote:
> 
> > I know this comment is probably a bit late but why didn't we simply put
> > type/code into sport/dport in struct flowi instead of introducing the
> > monstrosities of xfrm_flowi_sport/xfrm_flowi_dport?
> > 
> > Something like
> > 
> > struct {
> > 	__u16	type;
> > 	__u16	code;
> > } icmpt;
> > 
> > would've done (and still would do) the trick, no?
> 
> Here is an updated patch that kills xfrm_flowi_{sport,dport}.
> I've checked around, there seems to be nothing that relies
> on type and code beeing u8.

I didn't this because there are several places which depend on u8.
If we go this way, we need to fix other places as well. e.g.

  net/ipv4/raw.c:raw_probe_proto_opt()
  net/ipv4/xfrm4_policy.c:_decode_session4()
  net/ipv6/raw.c:rawv6_probe_proto_opt()
  net/ipv6/netfilter/ip6t_REJECT.c:send_unreach()
  net/ipv6/xfrm6_policy.c:_decode_session6()
  net/ipv6/ndisc.c:ndisc_flow_init()
  net/ipv6/icmp.c:icmpv6_send()
  net/ipv6/icmp.c:icmpv6_echo_reply()

(Note that type and code are stored in network-byte order.)

--yoshfuji

Re: [XFRM]: Fix ICMP tempsel

[Patrick McHardy]

YOSHIFUJI Hideaki / ^[$B5HF#1QL@^[ wrote:

> I didn't this because there are several places which depend on u8.
> If we go this way, we need to fix other places as well. e.g.
> 
>   net/ipv4/raw.c:raw_probe_proto_opt()

                                 get_user(fl->fl_icmp_type, type);
                                 __get_user(fl->fl_icmp_code, code);

On x86_64 both care only about the pointer type, not the target type.

>   net/ipv4/xfrm4_policy.c:_decode_session4()

                                 u8 *icmp = xprth;

                                 fl->fl_icmp_type = icmp[0];
                                 fl->fl_icmp_code = icmp[1];

No problem here.

>   net/ipv6/raw.c:rawv6_probe_proto_opt()

Same as IPv4.

>   net/ipv6/netfilter/ip6t_REJECT.c:send_unreach()

Not in mainline :)

>   net/ipv6/xfrm6_policy.c:_decode_session6()

Same as IPv4.

>   net/ipv6/ndisc.c:ndisc_flow_init()

         fl->fl_icmp_type        = type;
         fl->fl_icmp_code        = 0;

No problem.

>   net/ipv6/icmp.c:icmpv6_send()

         fl.fl_icmp_type = type;
         fl.fl_icmp_code = code;

Also ok.

>   net/ipv6/icmp.c:icmpv6_echo_reply()

         fl.fl_icmp_type = ICMPV6_ECHO_REPLY;

Same here.

> 
> (Note that type and code are stored in network-byte order.)

Both are u8 so there is no byte order.

Regards
Patrick

Re: [XFRM]: Fix ICMP tempsel

[Herbert Xu]

[-- Attachment #1: Type: text/plain, Size: 833 bytes --]

On Sun, Feb 20, 2005 at 03:55:27PM +0900, YOSHIFUJI Hideaki / ?$B5HF#1QL@ wrote:
> 
> I didn't this because there are several places which depend on u8.
> If we go this way, we need to fix other places as well. e.g.

Good point.  However, I think it's better to fix things up once when
we cross from user-space to the kernel rather than converting it for
every single packet.

So here we can do something like this.  Please note that this patch
needs to be used with Patrick's earlier work.  Patrick, if you're
OK with it please merge it in with your patch.

By all means give xfrm_selector_fixup a better name :)

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

[-- Attachment #2: p --]
[-- Type: text/plain, Size: 2552 bytes --]

===== net/key/af_key.c 1.69 vs edited =====
--- 1.69/net/key/af_key.c	2005-01-26 16:53:19 +11:00
+++ edited/net/key/af_key.c	2005-02-20 18:26:57 +11:00
@@ -1914,6 +1914,8 @@
 	if (xp->selector.dport)
 		xp->selector.dport_mask = ~0;
 
+	xfrm_selector_fixup(&xp->selector);
+
 	xp->lft.soft_byte_limit = XFRM_INF;
 	xp->lft.hard_byte_limit = XFRM_INF;
 	xp->lft.soft_packet_limit = XFRM_INF;
@@ -2004,6 +2006,7 @@
 	if (sel.dport)
 		sel.dport_mask = ~0;
 
+	xfrm_selector_fixup(&sel);
 	xp = xfrm_policy_bysel(pol->sadb_x_policy_dir-1, &sel, 1);
 	if (xp == NULL)
 		return -ENOENT;
===== net/xfrm/xfrm_user.c 1.52 vs edited =====
--- 1.52/net/xfrm/xfrm_user.c	2005-01-26 16:53:19 +11:00
+++ edited/net/xfrm/xfrm_user.c	2005-02-20 18:23:41 +11:00
@@ -204,6 +204,7 @@
 {
 	memcpy(&x->id, &p->id, sizeof(x->id));
 	memcpy(&x->sel, &p->sel, sizeof(x->sel));
+	xfrm_selector_fixup(&x->sel);
 	memcpy(&x->lft, &p->lft, sizeof(x->lft));
 	x->props.mode = p->mode;
 	x->props.replay_window = p->replay_window;
@@ -626,6 +627,7 @@
 	xp->priority = p->priority;
 	xp->index = p->index;
 	memcpy(&xp->selector, &p->sel, sizeof(xp->selector));
+	xfrm_selector_fixup(&xp->selector);
 	memcpy(&xp->lft, &p->lft, sizeof(xp->lft));
 	xp->action = p->action;
 	xp->flags = p->flags;
@@ -808,6 +810,7 @@
 	struct xfrm_userpolicy_id *p;
 	int err;
 	int delete;
+	struct xfrm_selector sel;
 
 	p = NLMSG_DATA(nlh);
 	delete = nlh->nlmsg_type == XFRM_MSG_DELPOLICY;
@@ -818,8 +821,11 @@
 
 	if (p->index)
 		xp = xfrm_policy_byid(p->dir, p->index, delete);
-	else
+	else {
+		memcpy(&sel, &p->sel, sizeof(sel));
+		xfrm_selector_fixup(&sel);
 		xp = xfrm_policy_bysel(p->dir, &p->sel, delete);
+	}
 	if (xp == NULL)
 		return -ENOENT;
 
===== include/net/flow.h 1.11 vs edited =====
--- 1.11/include/net/flow.h	2004-03-19 15:20:28 +11:00
+++ edited/include/net/flow.h	2005-02-20 18:07:39 +11:00
@@ -58,7 +58,9 @@
 		} ports;
 
 		struct {
+			__u8	pad1;
 			__u8	type;
+			__u8	pad2;
 			__u8	code;
 		} icmpt;
 
===== include/net/xfrm.h 1.74 vs edited =====
--- 1.74/include/net/xfrm.h	2005-01-26 16:53:19 +11:00
+++ edited/include/net/xfrm.h	2005-02-20 18:29:01 +11:00
@@ -492,6 +492,17 @@
 	return 0;
 }
 
+static inline void xfrm_selector_fixup(struct xfrm_selector *sel)
+{
+	switch (sel->proto) {
+	case IPPROTO_ICMP:
+	case IPPROTO_ICMPV6:
+		sel->sport_mask &= htons(0xff);
+		sel->dport_mask &= htons(0xff);
+		break;
+	}
+}
+
 /* A struct encoding bundle of transformations to apply to some set of flow.
  *
  * dst->child points to the next element of bundle.

Re: [XFRM]: Fix ICMP tempsel

[Patrick McHardy]

[-- Attachment #1: Type: text/plain, Size: 508 bytes --]

Herbert Xu wrote:
> So here we can do something like this.  Please note that this patch
> needs to be used with Patrick's earlier work.  Patrick, if you're
> OK with it please merge it in with your patch.

Attached. Now I understand what Yoshifuji meant with byteorder :)

> 
> By all means give xfrm_selector_fixup a better name :)

I'm not very talented with choosing good names myself, so I kept it.

On second thought .. isn't there a risk of confusing userspace by
changing the masks ?

Regards
Patrick

[-- Attachment #2: x --]
[-- Type: text/plain, Size: 7790 bytes --]

# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
#   2005/02/20 09:29:12+01:00 kaber@coreworks.de 
#   [XFRM]: Fix ICMP tempsel
#   
#   The selector ports are initialized to fl_ip_sport/fl_ip_dport instead
#   of xfrm_flowi_sport(fl)/xfrm_flowi_dport(fl). This is wrong for ICMP,
#   type and code should be stored in sport and dport, in struct flowi both
#   are contained in fl_ip_sport.
#      
#   This patch adjusts struct flowi to store ICMP type/code in sport/dport,
#   kills xfrm_flowi_{sport,dport} and converts the selector values only once
#   when they enter the kernel.
#   
#   Mostly done by Herbert Xu <herbert@gondor.apana.org.au>
#   
#   Signed-off-by: Patrick McHardy <kaber@trash.net>
# 
# net/xfrm/xfrm_user.c
#   2005/02/20 09:29:04+01:00 kaber@coreworks.de +7 -1
#   [XFRM]: Fix ICMP tempsel
#   
#   The selector ports are initialized to fl_ip_sport/fl_ip_dport instead
#   of xfrm_flowi_sport(fl)/xfrm_flowi_dport(fl). This is wrong for ICMP,
#   type and code should be stored in sport and dport, in struct flowi both
#   are contained in fl_ip_sport.
#      
#   This patch adjusts struct flowi to store ICMP type/code in sport/dport,
#   kills xfrm_flowi_{sport,dport} and converts the selector values only once
#   when they enter the kernel.
#   
#   Mostly done by Herbert Xu <herbert@gondor.apana.org.au>
#   
#   Signed-off-by: Patrick McHardy <kaber@trash.net>
# 
# net/key/af_key.c
#   2005/02/20 09:29:04+01:00 kaber@coreworks.de +3 -0
#   [XFRM]: Fix ICMP tempsel
#   
#   The selector ports are initialized to fl_ip_sport/fl_ip_dport instead
#   of xfrm_flowi_sport(fl)/xfrm_flowi_dport(fl). This is wrong for ICMP,
#   type and code should be stored in sport and dport, in struct flowi both
#   are contained in fl_ip_sport.
#      
#   This patch adjusts struct flowi to store ICMP type/code in sport/dport,
#   kills xfrm_flowi_{sport,dport} and converts the selector values only once
#   when they enter the kernel.
#   
#   Mostly done by Herbert Xu <herbert@gondor.apana.org.au>
#   
#   Signed-off-by: Patrick McHardy <kaber@trash.net>
# 
# include/net/xfrm.h
#   2005/02/20 09:29:04+01:00 kaber@coreworks.de +15 -44
#   [XFRM]: Fix ICMP tempsel
#   
#   The selector ports are initialized to fl_ip_sport/fl_ip_dport instead
#   of xfrm_flowi_sport(fl)/xfrm_flowi_dport(fl). This is wrong for ICMP,
#   type and code should be stored in sport and dport, in struct flowi both
#   are contained in fl_ip_sport.
#      
#   This patch adjusts struct flowi to store ICMP type/code in sport/dport,
#   kills xfrm_flowi_{sport,dport} and converts the selector values only once
#   when they enter the kernel.
#   
#   Mostly done by Herbert Xu <herbert@gondor.apana.org.au>
#   
#   Signed-off-by: Patrick McHardy <kaber@trash.net>
# 
# include/net/flow.h
#   2005/02/20 09:29:04+01:00 kaber@coreworks.de +2 -0
#   [XFRM]: Fix ICMP tempsel
#   
#   The selector ports are initialized to fl_ip_sport/fl_ip_dport instead
#   of xfrm_flowi_sport(fl)/xfrm_flowi_dport(fl). This is wrong for ICMP,
#   type and code should be stored in sport and dport, in struct flowi both
#   are contained in fl_ip_sport.
#      
#   This patch adjusts struct flowi to store ICMP type/code in sport/dport,
#   kills xfrm_flowi_{sport,dport} and converts the selector values only once
#   when they enter the kernel.
#   
#   Mostly done by Herbert Xu <herbert@gondor.apana.org.au>
#   
#   Signed-off-by: Patrick McHardy <kaber@trash.net>
# 
diff -Nru a/include/net/flow.h b/include/net/flow.h
--- a/include/net/flow.h	2005-02-20 09:30:04 +01:00
+++ b/include/net/flow.h	2005-02-20 09:30:04 +01:00
@@ -58,7 +58,9 @@
 		} ports;
 
 		struct {
+			__u8	pad1;
 			__u8	type;
+			__u8	pad2;
 			__u8	code;
 		} icmpt;
 
diff -Nru a/include/net/xfrm.h b/include/net/xfrm.h
--- a/include/net/xfrm.h	2005-02-20 09:30:04 +01:00
+++ b/include/net/xfrm.h	2005-02-20 09:30:04 +01:00
@@ -417,53 +417,13 @@
 	return 1;
 }
 
-static __inline__
-u16 xfrm_flowi_sport(struct flowi *fl)
-{
-	u16 port;
-	switch(fl->proto) {
-	case IPPROTO_TCP:
-	case IPPROTO_UDP:
-	case IPPROTO_SCTP:
-		port = fl->fl_ip_sport;
-		break;
-	case IPPROTO_ICMP:
-	case IPPROTO_ICMPV6:
-		port = htons(fl->fl_icmp_type);
-		break;
-	default:
-		port = 0;	/*XXX*/
-	}
-	return port;
-}
-
-static __inline__
-u16 xfrm_flowi_dport(struct flowi *fl)
-{
-	u16 port;
-	switch(fl->proto) {
-	case IPPROTO_TCP:
-	case IPPROTO_UDP:
-	case IPPROTO_SCTP:
-		port = fl->fl_ip_dport;
-		break;
-	case IPPROTO_ICMP:
-	case IPPROTO_ICMPV6:
-		port = htons(fl->fl_icmp_code);
-		break;
-	default:
-		port = 0;	/*XXX*/
-	}
-	return port;
-}
-
 static inline int
 __xfrm4_selector_match(struct xfrm_selector *sel, struct flowi *fl)
 {
 	return  addr_match(&fl->fl4_dst, &sel->daddr, sel->prefixlen_d) &&
 		addr_match(&fl->fl4_src, &sel->saddr, sel->prefixlen_s) &&
-		!((xfrm_flowi_dport(fl) ^ sel->dport) & sel->dport_mask) &&
-		!((xfrm_flowi_sport(fl) ^ sel->sport) & sel->sport_mask) &&
+		!((fl->fl_ip_dport ^ sel->dport) & sel->dport_mask) &&
+		!((fl->fl_ip_sport ^ sel->sport) & sel->sport_mask) &&
 		(fl->proto == sel->proto || !sel->proto) &&
 		(fl->oif == sel->ifindex || !sel->ifindex);
 }
@@ -473,8 +433,8 @@
 {
 	return  addr_match(&fl->fl6_dst, &sel->daddr, sel->prefixlen_d) &&
 		addr_match(&fl->fl6_src, &sel->saddr, sel->prefixlen_s) &&
-		!((xfrm_flowi_dport(fl) ^ sel->dport) & sel->dport_mask) &&
-		!((xfrm_flowi_sport(fl) ^ sel->sport) & sel->sport_mask) &&
+		!((fl->fl_ip_dport ^ sel->dport) & sel->dport_mask) &&
+		!((fl->fl_ip_sport ^ sel->sport) & sel->sport_mask) &&
 		(fl->proto == sel->proto || !sel->proto) &&
 		(fl->oif == sel->ifindex || !sel->ifindex);
 }
@@ -490,6 +450,17 @@
 		return __xfrm6_selector_match(sel, fl);
 	}
 	return 0;
+}
+
+static inline void xfrm_selector_fixup(struct xfrm_selector *sel)
+{
+	switch (sel->proto) {
+	case IPPROTO_ICMP:
+	case IPPROTO_ICMPV6:
+		sel->sport_mask &= htons(0xff);
+		sel->dport_mask &= htons(0xff);
+		break;
+	}
 }
 
 /* A struct encoding bundle of transformations to apply to some set of flow.
diff -Nru a/net/key/af_key.c b/net/key/af_key.c
--- a/net/key/af_key.c	2005-02-20 09:30:04 +01:00
+++ b/net/key/af_key.c	2005-02-20 09:30:04 +01:00
@@ -1909,6 +1909,8 @@
 	if (xp->selector.dport)
 		xp->selector.dport_mask = ~0;
 
+	xfrm_selector_fixup(&xp->selector);
+
 	xp->lft.soft_byte_limit = XFRM_INF;
 	xp->lft.hard_byte_limit = XFRM_INF;
 	xp->lft.soft_packet_limit = XFRM_INF;
@@ -1999,6 +2001,7 @@
 	if (sel.dport)
 		sel.dport_mask = ~0;
 
+	xfrm_selector_fixup(&sel);
 	xp = xfrm_policy_bysel(pol->sadb_x_policy_dir-1, &sel, 1);
 	if (xp == NULL)
 		return -ENOENT;
diff -Nru a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
--- a/net/xfrm/xfrm_user.c	2005-02-20 09:30:04 +01:00
+++ b/net/xfrm/xfrm_user.c	2005-02-20 09:30:04 +01:00
@@ -204,6 +204,7 @@
 {
 	memcpy(&x->id, &p->id, sizeof(x->id));
 	memcpy(&x->sel, &p->sel, sizeof(x->sel));
+	xfrm_selector_fixup(&x->sel);
 	memcpy(&x->lft, &p->lft, sizeof(x->lft));
 	x->props.mode = p->mode;
 	x->props.replay_window = p->replay_window;
@@ -626,6 +627,7 @@
 	xp->priority = p->priority;
 	xp->index = p->index;
 	memcpy(&xp->selector, &p->sel, sizeof(xp->selector));
+	xfrm_selector_fixup(&xp->selector);
 	memcpy(&xp->lft, &p->lft, sizeof(xp->lft));
 	xp->action = p->action;
 	xp->flags = p->flags;
@@ -808,6 +810,7 @@
 	struct xfrm_userpolicy_id *p;
 	int err;
 	int delete;
+	struct xfrm_selector sel;
 
 	p = NLMSG_DATA(nlh);
 	delete = nlh->nlmsg_type == XFRM_MSG_DELPOLICY;
@@ -818,8 +821,11 @@
 
 	if (p->index)
 		xp = xfrm_policy_byid(p->dir, p->index, delete);
-	else
+	else {
+		memcpy(&sel, &p->sel, sizeof(sel));
+		xfrm_selector_fixup(&sel);
 		xp = xfrm_policy_bysel(p->dir, &p->sel, delete);
+	}
 	if (xp == NULL)
 		return -ENOENT;
 

Re: [XFRM]: Fix ICMP tempsel

[YOSHIFUJI Hideaki]

In article <42184BC2.1030907@trash.net> (at Sun, 20 Feb 2005 09:35:14 +0100), Patrick McHardy <kaber@trash.net> says:

> Herbert Xu wrote:
> > So here we can do something like this.  Please note that this patch
> > needs to be used with Patrick's earlier work.  Patrick, if you're
> > OK with it please merge it in with your patch.
> 
> Attached. Now I understand what Yoshifuji meant with byteorder :)

Sorry, still it seems wrong.
Please allow me several hours.

--yoshfuji @ on the way home...

Re: [XFRM]: Fix ICMP tempsel

[Herbert Xu]

On Sun, Feb 20, 2005 at 09:35:14AM +0100, Patrick McHardy wrote:
> 
> On second thought .. isn't there a risk of confusing userspace by
> changing the masks ?

It only affects xfrm_user users, of which I only know two -- Openswan
and ip(8).  Neither of which should care about it.

However, I forgot to do the fixup in __xfrm[46]_init_tempsel and
that is going to confuse the kernel itself :)

It is looking more and more like a nasty hack though so maybe
it's not worth it since the policy checks are still bloated enough
even after we do this.

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: [XFRM]: Fix ICMP tempsel

[Patrick McHardy]

Herbert Xu wrote:
> It is looking more and more like a nasty hack though so maybe
> it's not worth it since the policy checks are still bloated enough
> even after we do this.

Agreed. Let's just take the first patch.

Regards
Patrick

Re: [XFRM]: Fix ICMP tempsel

[YOSHIFUJI Hideaki / 吉藤英明]

In article <4218729A.4080201@trash.net> (at Sun, 20 Feb 2005 12:20:58 +0100), Patrick McHardy <kaber@trash.net> says:

> Herbert Xu wrote:
> > It is looking more and more like a nasty hack though so maybe
> > it's not worth it since the policy checks are still bloated enough
> > even after we do this.
> 
> Agreed. Let's just take the first patch.

I totally agree. I belive it is the safest fix.

--yoshfuji

Re: [XFRM]: Fix ICMP tempsel

[David S. Miller]

On Sat, 19 Feb 2005 12:43:43 +0100
Patrick McHardy <kaber@trash.net> wrote:

> The selector ports are initialized to fl_ip_sport/fl_ip_dport instead
> of xfrm_flowi_sport(fl)/xfrm_flowi_dport(fl). This is wrong for ICMP,
> type and code should be stored in sport and dport, in struct flowi both
> are contained in fl_ip_sport.

This resulted in a long thread, many newer versions of the patch
trying to clean this up in other ways, but then we determined that
this original patch was the best and safest fix for now.

So I applied this first patch.

Thanks everyone.