Re: [PATCH][IPSEC][6/7] inter address family ipsec tunnel

[David Miller <davem@davemloft.net>]

From: Kazunori MIYAZAWA <kazunori@miyazawa.org>
Date: Mon, 04 Dec 2006 11:50:09 +0900

> Thank you for your tracing the bug.
> 
> I understood the issue.
> Mmm, if we can not use ut->family, can we use
> ut->id.family instead?
> 
> Or is it also uninitialized?

struct xfrm_id does not have a family field

struct xfrm_id
{
	xfrm_address_t	daddr;
	__be32		spi;
	__u8		proto;
};

That's what ut->id is.

You're thinking of xfrm_usersa_id, which is used by
another structure, xfrm_aevent_id.

For the time being I'm thinking of using the following
patch.  I removed the xp->family clobbering, the AF_KEY
changes don't do this, so there is no reason for the
xfrm_user side to do it either.

Every path that leads to copy_templates() will perform
a validate_tmpl() which will change ut->family == 0
to the policy's family value.  Any other non-supported
value will trigger an error.

Let's hope this fix is sufficient.  I'm about to test
this now.

diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 6f97665..311205f 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -858,7 +858,6 @@ static void copy_templates(struct xfrm_p
 	int i;
 
 	xp->xfrm_nr = nr;
-	xp->family = ut->family;
 	for (i = 0; i < nr; i++, ut++) {
 		struct xfrm_tmpl *t = &xp->xfrm_vec[i];
 
@@ -876,19 +875,53 @@ static void copy_templates(struct xfrm_p
 	}
 }
 
+static int validate_tmpl(int nr, struct xfrm_user_tmpl *ut, u16 family)
+{
+	int i;
+
+	if (nr > XFRM_MAX_DEPTH)
+		return -EINVAL;
+
+	for (i = 0; i < nr; i++) {
+		/* We never validated the ut->family value, so many
+		 * applications simply leave it at zero.  The check was
+		 * never made and ut->family was ignored because all
+		 * templates could be assumed to have the same family as
+		 * the policy itself.  Now that we will have ipv4-in-ipv6
+		 * and ipv6-in-ipv4 tunnels, this is no longer true.
+		 */
+		if (!ut[i].family)
+			ut[i].family = family;
+
+		switch (ut[i].family) {
+		case AF_INET:
+			break;
+#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
+		case AF_INET6:
+			break;
+#endif
+		default:
+			return -EINVAL;
+		};
+	}
+
+	return 0;
+}
+
 static int copy_from_user_tmpl(struct xfrm_policy *pol, struct rtattr **xfrma)
 {
 	struct rtattr *rt = xfrma[XFRMA_TMPL-1];
-	struct xfrm_user_tmpl *utmpl;
-	int nr;
 
 	if (!rt) {
 		pol->xfrm_nr = 0;
 	} else {
-		nr = (rt->rta_len - sizeof(*rt)) / sizeof(*utmpl);
+		struct xfrm_user_tmpl *utmpl = RTA_DATA(rt);
+		int nr = (rt->rta_len - sizeof(*rt)) / sizeof(*utmpl);
+		int err;
 
-		if (nr > XFRM_MAX_DEPTH)
-			return -EINVAL;
+		err = validate_tmpl(nr, utmpl, pol->family);
+		if (err)
+			return err;
 
 		copy_templates(pol, RTA_DATA(rt), nr);
 	}
@@ -1530,7 +1563,8 @@ static int xfrm_add_acquire(struct sk_bu
 	}
 
 	/*   build an XP */
-	xp = xfrm_policy_construct(&ua->policy, (struct rtattr **) xfrma, &err);        if (!xp) {
+	xp = xfrm_policy_construct(&ua->policy, (struct rtattr **) xfrma, &err);
+	if (!xp) {
 		kfree(x);
 		return err;
 	}
@@ -1979,7 +2013,7 @@ #endif
 		return NULL;
 
 	nr = ((len - sizeof(*p)) / sizeof(*ut));
-	if (nr > XFRM_MAX_DEPTH)
+	if (validate_tmpl(nr, ut, p->sel.family))
 		return NULL;
 
 	if (p->dir > XFRM_POLICY_OUT)