diff mbox

CMAC support for CryptoAPI, fixed patch issues, indent, and testmgr build issues

Message ID 20130124112410.8535.75598.stgit@localhost6.localdomain6
State RFC, archived
Delegated to: David Miller
Headers show

Commit Message

Jussi Kivilinna Jan. 24, 2013, 11:25 a.m. UTC 
Quoting Steffen Klassert <steffen.klassert@secunet.com>:

> On Wed, Jan 23, 2013 at 05:35:10PM +0200, Jussi Kivilinna wrote:
>>
>> Problem seems to be that PFKEYv2 does not quite work with IKEv2, and
>> XFRM API should be used instead. There is new numbers assigned for
>> IKEv2: 
>> https://www.iana.org/assignments/ikev2-parameters/ikev2-parameters.xml#ikev2-parameters-7
>>
>> For new SADB_X_AALG_*, I'd think you should use value from "Reserved
>> for private use" range. Maybe 250?
>
> This would be an option, but we have just a few slots for private
> algorithms.
>
>>
>> But maybe better solution might be to not make AES-CMAC (or other
>> new algorithms) available throught PFKEY API at all, just XFRM?
>>
>
> It is probably the best to make new algorithms unavailable for pfkey
> as long as they have no official ikev1 iana transform identifier.
>
> But how to do that? Perhaps we can assign SADB_X_AALG_NOPFKEY to
> the private value 255 and return -EINVAL if pfkey tries to register
> such an algorithm. The netlink interface does not use these
> identifiers, everything should work as expected. So it should be
> possible to use these algoritms with iproute2 and the most modern
> ike deamons.

Maybe it would be cleaner to not mess with pfkeyv2.h at all, but instead mark algorithms that do not support pfkey with flag. See patch below.

Then I started looking up if sadb_alg_id is being used somewhere outside pfkey. Seems that its value is just being copied around.. but at "http://lxr.linux.no/linux+v3.7/net/xfrm/xfrm_policy.c#L1991" it's used as bit-index. So do larger values than 31 break some stuff? Can multiple algorithms have same sadb_alg_id value? Also in af_key.c, sadb_alg_id being used as bit-index.

-Jussi

---
ONLY COMPILE TESTED!
---
 include/net/xfrm.h   |    5 +++--
 net/key/af_key.c     |   39 +++++++++++++++++++++++++++++++--------
 net/xfrm/xfrm_algo.c |   12 ++++++------
 3 files changed, 40 insertions(+), 16 deletions(-)


--
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

Comments

Steffen Klassert Jan. 24, 2013, 12:32 p.m. UTC | #1 
On Thu, Jan 24, 2013 at 01:25:46PM +0200, Jussi Kivilinna wrote:
> 
> Maybe it would be cleaner to not mess with pfkeyv2.h at all, but instead mark algorithms that do not support pfkey with flag. See patch below.
> 

Yes, would be an option too. I would be fine with that,
but let's here if someone else has an opinion on this.
Anyway, we need a solution to integrate Tom's patch soon.

> Then I started looking up if sadb_alg_id is being used somewhere outside pfkey. Seems that its value is just being copied around.. but at "http://lxr.linux.no/linux+v3.7/net/xfrm/xfrm_policy.c#L1991" it's used as bit-index. So do larger values than 31 break some stuff? Can multiple algorithms have same sadb_alg_id value? Also in af_key.c, sadb_alg_id being used as bit-index.
> 

Herbert tried to address this already in git commit c5d18e984
([IPSEC]: Fix catch-22 with algorithm IDs above 31) some years
ago.

But this looks still messy. If the aalgos, ealgos and calgos mask is ~0, 
we allow all algorithms. If this is not the case, xfrm and pfkey check
the aalgos mask against the algorithm ID, only pfkey checks the ealgo
mask and noone checks the calgos mask.

--
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
Tom St Denis Jan. 24, 2013, 12:37 p.m. UTC | #2 
----- Original Message -----
> From: "Steffen Klassert" <steffen.klassert@secunet.com>
> To: "Jussi Kivilinna" <jussi.kivilinna@mbnet.fi>
> Cc: "Herbert Xu" <herbert@gondor.apana.org.au>, netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
> linux-crypto@vger.kernel.org, "Tom St Denis" <tstdenis@elliptictech.com>, "David Miller" <davem@davemloft.net>
> Sent: Thursday, 24 January, 2013 7:32:10 AM
> Subject: Re: [PATCH] CMAC support for CryptoAPI, fixed patch issues, indent, and testmgr build issues
> 
> On Thu, Jan 24, 2013 at 01:25:46PM +0200, Jussi Kivilinna wrote:
> > 
> > Maybe it would be cleaner to not mess with pfkeyv2.h at all, but
> > instead mark algorithms that do not support pfkey with flag. See
> > patch below.
> > 
> 
> Yes, would be an option too. I would be fine with that,
> but let's here if someone else has an opinion on this.
> Anyway, we need a solution to integrate Tom's patch soon.

Has anyone addressed the testmgr issues too?  I wasn't able to run the self-tests so I don't even know if the vectors were copied correctly.

Also a question for the netdev folk, in order to be timely would it be acceptable to patch ah4 and then ah6 with the AEAD changes?  Or would the team require both to be patched simultaneously?
 
Tom
--
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
Steffen Klassert Jan. 24, 2013, 12:52 p.m. UTC | #3 
On Thu, Jan 24, 2013 at 07:37:50AM -0500, Tom St Denis wrote:
> 
> Also a question for the netdev folk, in order to be timely would it be acceptable to patch ah4 and then ah6 with the AEAD changes?  Or would the team require both to be patched simultaneously?
>  

We would need patches for both, but the changes to ah4/ah6 should
e very similar.
--
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
Tom St Denis Jan. 24, 2013, 1 p.m. UTC | #4 
----- Original Message -----
> From: "Steffen Klassert" <steffen.klassert@secunet.com>
> To: "Tom St Denis" <tstdenis@elliptictech.com>
> Cc: "Herbert Xu" <herbert@gondor.apana.org.au>, netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
> linux-crypto@vger.kernel.org, "David Miller" <davem@davemloft.net>, "Jussi Kivilinna" <jussi.kivilinna@mbnet.fi>
> Sent: Thursday, 24 January, 2013 7:52:34 AM
> Subject: Re: [PATCH] CMAC support for CryptoAPI, fixed patch issues, indent, and testmgr build issues
> 
> On Thu, Jan 24, 2013 at 07:37:50AM -0500, Tom St Denis wrote:
> > 
> > Also a question for the netdev folk, in order to be timely would it
> > be acceptable to patch ah4 and then ah6 with the AEAD changes?  Or
> > would the team require both to be patched simultaneously?
> >  
> 
> We would need patches for both, but the changes to ah4/ah6 should
> e very similar.

I suspect, I just don't know how my time table will look like so I was trying to not block the submission of one by the other.  Fortunately, I don't need hardware to test these changes so I can probably skunkwork them from home.

Tom
--
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
Steffen Klassert Jan. 29, 2013, 9:33 a.m. UTC | #5 
On Thu, Jan 24, 2013 at 01:25:46PM +0200, Jussi Kivilinna wrote:
> 
> Maybe it would be cleaner to not mess with pfkeyv2.h at all, but instead mark algorithms that do not support pfkey with flag. See patch below.
> 

As nobody seems to have another opinion, we could go either with your
approach, or we can invert the logic and mark all existing algorithms
as pfkey supported. Then we would not need to bother about pfkey again.

I'd be fine with both. Do you want to submit a patch?

--
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
Jussi Kivilinna Jan. 31, 2013, 9:35 a.m. UTC | #6 
Quoting Steffen Klassert <steffen.klassert@secunet.com>:

> On Thu, Jan 24, 2013 at 01:25:46PM +0200, Jussi Kivilinna wrote:
>>
>> Maybe it would be cleaner to not mess with pfkeyv2.h at all, but  
>> instead mark algorithms that do not support pfkey with flag. See  
>> patch below.
>>
>
> As nobody seems to have another opinion, we could go either with your
> approach, or we can invert the logic and mark all existing algorithms
> as pfkey supported. Then we would not need to bother about pfkey again.
>
> I'd be fine with both. Do you want to submit a patch?
>

Ok, I'll invert the logic and send new patch.

-Jussi



--
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
diff mbox

Patch

diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index 421f764..5d5eec2 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -1320,6 +1320,7 @@  struct xfrm_algo_desc {
 	char *name;
 	char *compat;
 	u8 available:1;
+	u8 sadb_disabled:1;
 	union {
 		struct xfrm_algo_aead_info aead;
 		struct xfrm_algo_auth_info auth;
@@ -1561,8 +1562,8 @@  extern void xfrm_input_init(void);
 extern int xfrm_parse_spi(struct sk_buff *skb, u8 nexthdr, __be32 *spi, __be32 *seq);
 
 extern void xfrm_probe_algs(void);
-extern int xfrm_count_auth_supported(void);
-extern int xfrm_count_enc_supported(void);
+extern int xfrm_count_sadb_auth_supported(void);
+extern int xfrm_count_sadb_enc_supported(void);
 extern struct xfrm_algo_desc *xfrm_aalg_get_byidx(unsigned int idx);
 extern struct xfrm_algo_desc *xfrm_ealg_get_byidx(unsigned int idx);
 extern struct xfrm_algo_desc *xfrm_aalg_get_byid(int alg_id);
diff --git a/net/key/af_key.c b/net/key/af_key.c
index 5b426a6..307cf1d 100644
--- a/net/key/af_key.c
+++ b/net/key/af_key.c
@@ -816,18 +816,21 @@  static struct sk_buff *__pfkey_xfrm_state2msg(const struct xfrm_state *x,
 	sa->sadb_sa_auth = 0;
 	if (x->aalg) {
 		struct xfrm_algo_desc *a = xfrm_aalg_get_byname(x->aalg->alg_name, 0);
-		sa->sadb_sa_auth = a ? a->desc.sadb_alg_id : 0;
+		sa->sadb_sa_auth = (a && !a->sadb_disabled) ?
+					a->desc.sadb_alg_id : 0;
 	}
 	sa->sadb_sa_encrypt = 0;
 	BUG_ON(x->ealg && x->calg);
 	if (x->ealg) {
 		struct xfrm_algo_desc *a = xfrm_ealg_get_byname(x->ealg->alg_name, 0);
-		sa->sadb_sa_encrypt = a ? a->desc.sadb_alg_id : 0;
+		sa->sadb_sa_encrypt = (a && !a->sadb_disabled) ?
+					a->desc.sadb_alg_id : 0;
 	}
 	/* KAME compatible: sadb_sa_encrypt is overloaded with calg id */
 	if (x->calg) {
 		struct xfrm_algo_desc *a = xfrm_calg_get_byname(x->calg->alg_name, 0);
-		sa->sadb_sa_encrypt = a ? a->desc.sadb_alg_id : 0;
+		sa->sadb_sa_encrypt = (a && !a->sadb_disabled) ?
+					a->desc.sadb_alg_id : 0;
 	}
 
 	sa->sadb_sa_flags = 0;
@@ -1138,7 +1141,7 @@  static struct xfrm_state * pfkey_msg2xfrm_state(struct net *net,
 	if (sa->sadb_sa_auth) {
 		int keysize = 0;
 		struct xfrm_algo_desc *a = xfrm_aalg_get_byid(sa->sadb_sa_auth);
-		if (!a) {
+		if (!a || a->sadb_disabled) {
 			err = -ENOSYS;
 			goto out;
 		}
@@ -1160,7 +1163,7 @@  static struct xfrm_state * pfkey_msg2xfrm_state(struct net *net,
 	if (sa->sadb_sa_encrypt) {
 		if (hdr->sadb_msg_satype == SADB_X_SATYPE_IPCOMP) {
 			struct xfrm_algo_desc *a = xfrm_calg_get_byid(sa->sadb_sa_encrypt);
-			if (!a) {
+			if (!a || a->sadb_disabled) {
 				err = -ENOSYS;
 				goto out;
 			}
@@ -1172,7 +1175,7 @@  static struct xfrm_state * pfkey_msg2xfrm_state(struct net *net,
 		} else {
 			int keysize = 0;
 			struct xfrm_algo_desc *a = xfrm_ealg_get_byid(sa->sadb_sa_encrypt);
-			if (!a) {
+			if (!a || a->sadb_disabled) {
 				err = -ENOSYS;
 				goto out;
 			}
@@ -1578,13 +1581,13 @@  static struct sk_buff *compose_sadb_supported(const struct sadb_msg *orig,
 	struct sadb_msg *hdr;
 	int len, auth_len, enc_len, i;
 
-	auth_len = xfrm_count_auth_supported();
+	auth_len = xfrm_count_sadb_auth_supported();
 	if (auth_len) {
 		auth_len *= sizeof(struct sadb_alg);
 		auth_len += sizeof(struct sadb_supported);
 	}
 
-	enc_len = xfrm_count_enc_supported();
+	enc_len = xfrm_count_sadb_enc_supported();
 	if (enc_len) {
 		enc_len *= sizeof(struct sadb_alg);
 		enc_len += sizeof(struct sadb_supported);
@@ -1615,6 +1618,8 @@  static struct sk_buff *compose_sadb_supported(const struct sadb_msg *orig,
 			struct xfrm_algo_desc *aalg = xfrm_aalg_get_byidx(i);
 			if (!aalg)
 				break;
+			if (aalg->sadb_disabled)
+				continue;
 			if (aalg->available)
 				*ap++ = aalg->desc;
 		}
@@ -1634,6 +1639,8 @@  static struct sk_buff *compose_sadb_supported(const struct sadb_msg *orig,
 			struct xfrm_algo_desc *ealg = xfrm_ealg_get_byidx(i);
 			if (!ealg)
 				break;
+			if (ealg->sadb_disabled)
+				continue;
 			if (ealg->available)
 				*ap++ = ealg->desc;
 		}
@@ -2825,6 +2832,8 @@  static int count_ah_combs(const struct xfrm_tmpl *t)
 		const struct xfrm_algo_desc *aalg = xfrm_aalg_get_byidx(i);
 		if (!aalg)
 			break;
+		if (aalg->sadb_disabled)
+			continue;
 		if (aalg_tmpl_set(t, aalg) && aalg->available)
 			sz += sizeof(struct sadb_comb);
 	}
@@ -2840,6 +2849,9 @@  static int count_esp_combs(const struct xfrm_tmpl *t)
 		if (!ealg)
 			break;
 
+		if (ealg->sadb_disabled)
+			continue;
+
 		if (!(ealg_tmpl_set(t, ealg) && ealg->available))
 			continue;
 
@@ -2848,6 +2860,9 @@  static int count_esp_combs(const struct xfrm_tmpl *t)
 			if (!aalg)
 				break;
 
+			if (aalg->sadb_disabled)
+				continue;
+
 			if (aalg_tmpl_set(t, aalg) && aalg->available)
 				sz += sizeof(struct sadb_comb);
 		}
@@ -2871,6 +2886,9 @@  static void dump_ah_combs(struct sk_buff *skb, const struct xfrm_tmpl *t)
 		if (!aalg)
 			break;
 
+		if (aalg->sadb_disabled)
+			continue;
+
 		if (aalg_tmpl_set(t, aalg) && aalg->available) {
 			struct sadb_comb *c;
 			c = (struct sadb_comb*)skb_put(skb, sizeof(struct sadb_comb));
@@ -2903,6 +2921,9 @@  static void dump_esp_combs(struct sk_buff *skb, const struct xfrm_tmpl *t)
 		if (!ealg)
 			break;
 
+		if (ealg->sadb_disabled)
+			continue;
+
 		if (!(ealg_tmpl_set(t, ealg) && ealg->available))
 			continue;
 
@@ -2911,6 +2932,8 @@  static void dump_esp_combs(struct sk_buff *skb, const struct xfrm_tmpl *t)
 			const struct xfrm_algo_desc *aalg = xfrm_aalg_get_byidx(k);
 			if (!aalg)
 				break;
+			if (aalg->sadb_disabled)
+				continue;
 			if (!(aalg_tmpl_set(t, aalg) && aalg->available))
 				continue;
 			c = (struct sadb_comb*)skb_put(skb, sizeof(struct sadb_comb));
diff --git a/net/xfrm/xfrm_algo.c b/net/xfrm/xfrm_algo.c
index 4694cca..dab881c 100644
--- a/net/xfrm/xfrm_algo.c
+++ b/net/xfrm/xfrm_algo.c
@@ -713,27 +713,27 @@  void xfrm_probe_algs(void)
 }
 EXPORT_SYMBOL_GPL(xfrm_probe_algs);
 
-int xfrm_count_auth_supported(void)
+int xfrm_count_sadb_auth_supported(void)
 {
 	int i, n;
 
 	for (i = 0, n = 0; i < aalg_entries(); i++)
-		if (aalg_list[i].available)
+		if (aalg_list[i].available && !aalg_list[i].sadb_disabled)
 			n++;
 	return n;
 }
-EXPORT_SYMBOL_GPL(xfrm_count_auth_supported);
+EXPORT_SYMBOL_GPL(xfrm_count_sadb_auth_supported);
 
-int xfrm_count_enc_supported(void)
+int xfrm_count_sadb_enc_supported(void)
 {
 	int i, n;
 
 	for (i = 0, n = 0; i < ealg_entries(); i++)
-		if (ealg_list[i].available)
+		if (ealg_list[i].available && !ealg_list[i].sadb_disabled)
 			n++;
 	return n;
 }
-EXPORT_SYMBOL_GPL(xfrm_count_enc_supported);
+EXPORT_SYMBOL_GPL(xfrm_count_sadb_enc_supported);
 
 #if defined(CONFIG_INET_ESP) || defined(CONFIG_INET_ESP_MODULE) || defined(CONFIG_INET6_ESP) || defined(CONFIG_INET6_ESP_MODULE)