CRYPTO_MAX_ALG_NAME is too low

CRYPTO_MAX_ALG_NAME is too low

[Alexander Sverdlin]

Hello crypto maintainers!

We've found and example of the ipsec algorithm combination, which doesn't fit
into CRYPTO_MAX_ALG_NAME long buffers:

ip x s add src 1.1.1.1 dst 1.1.1.2 proto esp spi 0 mode tunnel enc des3_ede 0x0 auth sha256 0x0 flag esn replay-window 256

produces "echainiv(authencesn(hmac(sha256-generic),cbc(des3_ede-generic)))"
on the machines without optimized crypto drivers, which doesn't fit into current
64-bytes buffers.

I see two possible options:

a) split CRYPTO_MAX_ALG_NAME into CRYPTO_MAX_ALG_NAME + CRYPTO_MAX_DRV_NAME pair
and make later, say, 96, because the former probably cannot be changed because of
numerous user-space exports. And change half of the code to use new define.

b) rename *-generic algorithms to *-gen, so that cra_driver_name will be shortened,
while MODULE_ALIAS_CRYPTO() could still be maintained in old and new form.

What are your thoughts?

-- 
Best regards,
Alexander Sverdlin.

Re: CRYPTO_MAX_ALG_NAME is too low

[Alexander Sverdlin]

Hello!

On 10/03/17 12:55, Alexander Sverdlin wrote:
> Hello crypto maintainers!
> 
> We've found and example of the ipsec algorithm combination, which doesn't fit
> into CRYPTO_MAX_ALG_NAME long buffers:
> 
> ip x s add src 1.1.1.1 dst 1.1.1.2 proto esp spi 0 mode tunnel enc des3_ede 0x0 auth sha256 0x0 flag esn replay-window 256
> 
> produces "echainiv(authencesn(hmac(sha256-generic),cbc(des3_ede-generic)))"
> on the machines without optimized crypto drivers, which doesn't fit into current
> 64-bytes buffers.
> 
> I see two possible options:
> 
> a) split CRYPTO_MAX_ALG_NAME into CRYPTO_MAX_ALG_NAME + CRYPTO_MAX_DRV_NAME pair
> and make later, say, 96, because the former probably cannot be changed because of
> numerous user-space exports. And change half of the code to use new define.
> 
> b) rename *-generic algorithms to *-gen, so that cra_driver_name will be shortened,
> while MODULE_ALIAS_CRYPTO() could still be maintained in old and new form.
> 
> What are your thoughts?

Any?

This is a regression caused by 856e3f4092
("crypto: seqiv - Add support for new AEAD interface")

As I've said above, I can offer one of the two solutions, which patch should I send?
Or do you see any better alternatives?

-- 
Best regards,
Alexander Sverdlin.

[PATCH 0/4] crypto: CRYPTO_MAX_ALG_NAME is too low

[Herbert Xu]

On Thu, Mar 16, 2017 at 03:16:29PM +0100, Alexander Sverdlin wrote:
>
> This is a regression caused by 856e3f4092
> ("crypto: seqiv - Add support for new AEAD interface")
> 
> As I've said above, I can offer one of the two solutions, which patch should I send?
> Or do you see any better alternatives?

Here is a series of patches which should fix the problem.

The first three patches prepare the user-space interfaces to deal
with longer names.  The final patch extends it.

Note that with crypto_user I haven't actually extended it to
configure longer names.  It'll only be able to configure names
less than 64 bytes.  However, it should be able to dump/read
algorithms with longer names, albeit the name will be truncated
to 64 bytes length.

Steffen, when convenient could you look into extending the crypto
user interface to handle longer names (preferably arbitraty length
since netlink should be able to deal with that)?

Likewise xfrm is still fixed to 64 bytes long.  But this should
be OK as the problematic case only arises with IV generators for
now and we do not allow IV generators to be specified through xfrm.

af_alg on the other hand now allows arbitrarily long names.

As the final patch depends on all three it would be easiest if
we pushed the xfrm patch through the crypto tree.  Steffen/David?

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[PATCH 1/4] crypto: user - Prepare for CRYPTO_MAX_ALG_NAME expansion

[Herbert Xu]

This patch hard-codes CRYPTO_MAX_NAME in the user-space API to
64, which is the current value of CRYPTO_MAX_ALG_NAME.  This patch
also replaces all remaining occurences of CRYPTO_MAX_ALG_NAME
in the user-space API with CRYPTO_MAX_NAME.

This way the user-space API will not be modified when we raise
the value of CRYPTO_MAX_ALG_NAME.

Furthermore, the code has been updated to handle names longer than
the user-space API.  They will be truncated.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 crypto/crypto_user.c            |   18 +++++++++---------
 include/uapi/linux/cryptouser.h |   10 +++++-----
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/crypto/crypto_user.c b/crypto/crypto_user.c
index a90404a..89acaab 100644
--- a/crypto/crypto_user.c
+++ b/crypto/crypto_user.c
@@ -83,7 +83,7 @@ static int crypto_report_cipher(struct sk_buff *skb, struct crypto_alg *alg)
 {
 	struct crypto_report_cipher rcipher;
 
-	strncpy(rcipher.type, "cipher", sizeof(rcipher.type));
+	strlcpy(rcipher.type, "cipher", sizeof(rcipher.type));
 
 	rcipher.blocksize = alg->cra_blocksize;
 	rcipher.min_keysize = alg->cra_cipher.cia_min_keysize;
@@ -102,7 +102,7 @@ static int crypto_report_comp(struct sk_buff *skb, struct crypto_alg *alg)
 {
 	struct crypto_report_comp rcomp;
 
-	strncpy(rcomp.type, "compression", sizeof(rcomp.type));
+	strlcpy(rcomp.type, "compression", sizeof(rcomp.type));
 	if (nla_put(skb, CRYPTOCFGA_REPORT_COMPRESS,
 		    sizeof(struct crypto_report_comp), &rcomp))
 		goto nla_put_failure;
@@ -116,7 +116,7 @@ static int crypto_report_acomp(struct sk_buff *skb, struct crypto_alg *alg)
 {
 	struct crypto_report_acomp racomp;
 
-	strncpy(racomp.type, "acomp", sizeof(racomp.type));
+	strlcpy(racomp.type, "acomp", sizeof(racomp.type));
 
 	if (nla_put(skb, CRYPTOCFGA_REPORT_ACOMP,
 		    sizeof(struct crypto_report_acomp), &racomp))
@@ -131,7 +131,7 @@ static int crypto_report_akcipher(struct sk_buff *skb, struct crypto_alg *alg)
 {
 	struct crypto_report_akcipher rakcipher;
 
-	strncpy(rakcipher.type, "akcipher", sizeof(rakcipher.type));
+	strlcpy(rakcipher.type, "akcipher", sizeof(rakcipher.type));
 
 	if (nla_put(skb, CRYPTOCFGA_REPORT_AKCIPHER,
 		    sizeof(struct crypto_report_akcipher), &rakcipher))
@@ -146,7 +146,7 @@ static int crypto_report_kpp(struct sk_buff *skb, struct crypto_alg *alg)
 {
 	struct crypto_report_kpp rkpp;
 
-	strncpy(rkpp.type, "kpp", sizeof(rkpp.type));
+	strlcpy(rkpp.type, "kpp", sizeof(rkpp.type));
 
 	if (nla_put(skb, CRYPTOCFGA_REPORT_KPP,
 		    sizeof(struct crypto_report_kpp), &rkpp))
@@ -160,10 +160,10 @@ static int crypto_report_kpp(struct sk_buff *skb, struct crypto_alg *alg)
 static int crypto_report_one(struct crypto_alg *alg,
 			     struct crypto_user_alg *ualg, struct sk_buff *skb)
 {
-	strncpy(ualg->cru_name, alg->cra_name, sizeof(ualg->cru_name));
-	strncpy(ualg->cru_driver_name, alg->cra_driver_name,
+	strlcpy(ualg->cru_name, alg->cra_name, sizeof(ualg->cru_name));
+	strlcpy(ualg->cru_driver_name, alg->cra_driver_name,
 		sizeof(ualg->cru_driver_name));
-	strncpy(ualg->cru_module_name, module_name(alg->cra_module),
+	strlcpy(ualg->cru_module_name, module_name(alg->cra_module),
 		sizeof(ualg->cru_module_name));
 
 	ualg->cru_type = 0;
@@ -176,7 +176,7 @@ static int crypto_report_one(struct crypto_alg *alg,
 	if (alg->cra_flags & CRYPTO_ALG_LARVAL) {
 		struct crypto_report_larval rl;
 
-		strncpy(rl.type, "larval", sizeof(rl.type));
+		strlcpy(rl.type, "larval", sizeof(rl.type));
 		if (nla_put(skb, CRYPTOCFGA_REPORT_LARVAL,
 			    sizeof(struct crypto_report_larval), &rl))
 			goto nla_put_failure;
diff --git a/include/uapi/linux/cryptouser.h b/include/uapi/linux/cryptouser.h
index 11d21fc..b4def5c 100644
--- a/include/uapi/linux/cryptouser.h
+++ b/include/uapi/linux/cryptouser.h
@@ -31,7 +31,7 @@ enum {
 #define CRYPTO_MSG_MAX (__CRYPTO_MSG_MAX - 1)
 #define CRYPTO_NR_MSGTYPES (CRYPTO_MSG_MAX + 1 - CRYPTO_MSG_BASE)
 
-#define CRYPTO_MAX_NAME CRYPTO_MAX_ALG_NAME
+#define CRYPTO_MAX_NAME 64
 
 /* Netlink message attributes.  */
 enum crypto_attr_type_t {
@@ -53,9 +53,9 @@ enum crypto_attr_type_t {
 };
 
 struct crypto_user_alg {
-	char cru_name[CRYPTO_MAX_ALG_NAME];
-	char cru_driver_name[CRYPTO_MAX_ALG_NAME];
-	char cru_module_name[CRYPTO_MAX_ALG_NAME];
+	char cru_name[CRYPTO_MAX_NAME];
+	char cru_driver_name[CRYPTO_MAX_NAME];
+	char cru_module_name[CRYPTO_MAX_NAME];
 	__u32 cru_type;
 	__u32 cru_mask;
 	__u32 cru_refcnt;
@@ -73,7 +73,7 @@ struct crypto_report_hash {
 };
 
 struct crypto_report_cipher {
-	char type[CRYPTO_MAX_ALG_NAME];
+	char type[CRYPTO_MAX_NAME];
 	unsigned int blocksize;
 	unsigned int min_keysize;
 	unsigned int max_keysize;

[PATCH 2/4] crypto: af_alg - Allow arbitrarily long algorithm names

[Herbert Xu]

This patch removes the hard-coded 64-byte limit on the length
of the algorithm name through bind(2).  The address length can
now exceed that.  The user-space structure remains unchanged.
In order to use a longer name simply extend the salg_name array
beyond its defined 64 bytes length.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 crypto/af_alg.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index 690deca..3556d8e 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -160,11 +160,11 @@ static int alg_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
 	if (sock->state == SS_CONNECTED)
 		return -EINVAL;
 
-	if (addr_len != sizeof(*sa))
+	if (addr_len < sizeof(*sa))
 		return -EINVAL;
 
 	sa->salg_type[sizeof(sa->salg_type) - 1] = 0;
-	sa->salg_name[sizeof(sa->salg_name) - 1] = 0;
+	sa->salg_name[sizeof(sa->salg_name) + addr_len - sizeof(*sa) - 1] = 0;
 
 	type = alg_get_type(sa->salg_type);
 	if (IS_ERR(type) && PTR_ERR(type) == -ENOENT) {

[PATCH 3/4] xfrm: Prepare for CRYPTO_MAX_ALG_NAME expansion

[Herbert Xu]

This patch fixes the xfrm_user code to use the actual array size
rather than the hard-coded CRYPTO_MAX_ALG_NAME length.  This is
because the array size is fixed at 64 bytes while we want to increase
the in-kernel CRYPTO_MAX_ALG_NAME value.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 net/xfrm/xfrm_user.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 9705c27..96557cf 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -55,7 +55,7 @@ static int verify_one_alg(struct nlattr **attrs, enum xfrm_attr_type_t type)
 		return -EINVAL;
 	}
 
-	algp->alg_name[CRYPTO_MAX_ALG_NAME - 1] = '\0';
+	algp->alg_name[sizeof(algp->alg_name) - 1] = '\0';
 	return 0;
 }
 
@@ -71,7 +71,7 @@ static int verify_auth_trunc(struct nlattr **attrs)
 	if (nla_len(rt) < xfrm_alg_auth_len(algp))
 		return -EINVAL;
 
-	algp->alg_name[CRYPTO_MAX_ALG_NAME - 1] = '\0';
+	algp->alg_name[sizeof(algp->alg_name) - 1] = '\0';
 	return 0;
 }
 
@@ -87,7 +87,7 @@ static int verify_aead(struct nlattr **attrs)
 	if (nla_len(rt) < aead_len(algp))
 		return -EINVAL;
 
-	algp->alg_name[CRYPTO_MAX_ALG_NAME - 1] = '\0';
+	algp->alg_name[sizeof(algp->alg_name) - 1] = '\0';
 	return 0;
 }
 

[PATCH 4/4] crypto: api - Extend algorithm name limit to 128 bytes

[Herbert Xu]

With the new explicit IV generators, we may now exceed the 64-byte
length limit on the algorithm name, e.g., with

	echainiv(authencesn(hmac(sha256-generic),cbc(des3_ede-generic)))

This patch extends the length limit to 128 bytes.

Reported-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 include/linux/crypto.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/crypto.h b/include/linux/crypto.h
index c0b0cf3..84da997 100644
--- a/include/linux/crypto.h
+++ b/include/linux/crypto.h
@@ -123,7 +123,7 @@
 /*
  * Miscellaneous stuff.
  */
-#define CRYPTO_MAX_ALG_NAME		64
+#define CRYPTO_MAX_ALG_NAME		128
 
 /*
  * The macro CRYPTO_MINALIGN_ATTR (along with the void * type in the actual

Re: [PATCH 2/4] crypto: af_alg - Allow arbitrarily long algorithm names

[Alexander Sverdlin]

Hi!

On 06/04/17 10:16, Herbert Xu wrote:
> This patch removes the hard-coded 64-byte limit on the length
> of the algorithm name through bind(2).  The address length can
> now exceed that.  The user-space structure remains unchanged.
> In order to use a longer name simply extend the salg_name array
> beyond its defined 64 bytes length.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> ---
> 
>  crypto/af_alg.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/crypto/af_alg.c b/crypto/af_alg.c
> index 690deca..3556d8e 100644
> --- a/crypto/af_alg.c
> +++ b/crypto/af_alg.c
> @@ -160,11 +160,11 @@ static int alg_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
>  	if (sock->state == SS_CONNECTED)
>  		return -EINVAL;
>  
> -	if (addr_len != sizeof(*sa))
> +	if (addr_len < sizeof(*sa))
>  		return -EINVAL;
>  
>  	sa->salg_type[sizeof(sa->salg_type) - 1] = 0;
> -	sa->salg_name[sizeof(sa->salg_name) - 1] = 0;
> +	sa->salg_name[sizeof(sa->salg_name) + addr_len - sizeof(*sa) - 1] = 0;
>  
>  	type = alg_get_type(sa->salg_type);
>  	if (IS_ERR(type) && PTR_ERR(type) == -ENOENT) {

Why should userspace ever extend the structure if salg_name is hardcoded to 64 in if_alg.h?
This patch doesn't change the behavior at all, or am I missing something?

-- 
Best regards,
Alexander Sverdlin.

Re: [PATCH 0/4] crypto: CRYPTO_MAX_ALG_NAME is too low

[Alexander Sverdlin]

Hi!

On 06/04/17 10:15, Herbert Xu wrote:
> On Thu, Mar 16, 2017 at 03:16:29PM +0100, Alexander Sverdlin wrote:
>> This is a regression caused by 856e3f4092
>> ("crypto: seqiv - Add support for new AEAD interface")
>>
>> As I've said above, I can offer one of the two solutions, which patch should I send?
>> Or do you see any better alternatives?
> Here is a series of patches which should fix the problem.
> 
> The first three patches prepare the user-space interfaces to deal
> with longer names.  The final patch extends it.
> 
> Note that with crypto_user I haven't actually extended it to
> configure longer names.  It'll only be able to configure names
> less than 64 bytes.  However, it should be able to dump/read
> algorithms with longer names, albeit the name will be truncated
> to 64 bytes length.
> 
> Steffen, when convenient could you look into extending the crypto
> user interface to handle longer names (preferably arbitraty length
> since netlink should be able to deal with that)?
> 
> Likewise xfrm is still fixed to 64 bytes long.  But this should
> be OK as the problematic case only arises with IV generators for
> now and we do not allow IV generators to be specified through xfrm.
> 
> af_alg on the other hand now allows arbitrarily long names.

I'm not sure about patch 2 (as I've replied separately), but I've applied
and tested the whole series and it at least solves the original problem
with long algorithm name.

> As the final patch depends on all three it would be easiest if
> we pushed the xfrm patch through the crypto tree.  Steffen/David?

-- 
Best regards,
Alexander Sverdlin.

Re: [PATCH 1/4] crypto: user - Prepare for CRYPTO_MAX_ALG_NAME expansion

[Alexander Sverdlin]

On 06/04/17 10:16, Herbert Xu wrote:
> This patch hard-codes CRYPTO_MAX_NAME in the user-space API to
> 64, which is the current value of CRYPTO_MAX_ALG_NAME.  This patch
> also replaces all remaining occurences of CRYPTO_MAX_ALG_NAME
> in the user-space API with CRYPTO_MAX_NAME.
> 
> This way the user-space API will not be modified when we raise
> the value of CRYPTO_MAX_ALG_NAME.
> 
> Furthermore, the code has been updated to handle names longer than
> the user-space API.  They will be truncated.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Acked-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
Tested-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>

> ---
> 
>  crypto/crypto_user.c            |   18 +++++++++---------
>  include/uapi/linux/cryptouser.h |   10 +++++-----
>  2 files changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/crypto/crypto_user.c b/crypto/crypto_user.c
> index a90404a..89acaab 100644
> --- a/crypto/crypto_user.c
> +++ b/crypto/crypto_user.c
> @@ -83,7 +83,7 @@ static int crypto_report_cipher(struct sk_buff *skb, struct crypto_alg *alg)
>  {
>  	struct crypto_report_cipher rcipher;
>  
> -	strncpy(rcipher.type, "cipher", sizeof(rcipher.type));
> +	strlcpy(rcipher.type, "cipher", sizeof(rcipher.type));
>  
>  	rcipher.blocksize = alg->cra_blocksize;
>  	rcipher.min_keysize = alg->cra_cipher.cia_min_keysize;
> @@ -102,7 +102,7 @@ static int crypto_report_comp(struct sk_buff *skb, struct crypto_alg *alg)
>  {
>  	struct crypto_report_comp rcomp;
>  
> -	strncpy(rcomp.type, "compression", sizeof(rcomp.type));
> +	strlcpy(rcomp.type, "compression", sizeof(rcomp.type));
>  	if (nla_put(skb, CRYPTOCFGA_REPORT_COMPRESS,
>  		    sizeof(struct crypto_report_comp), &rcomp))
>  		goto nla_put_failure;
> @@ -116,7 +116,7 @@ static int crypto_report_acomp(struct sk_buff *skb, struct crypto_alg *alg)
>  {
>  	struct crypto_report_acomp racomp;
>  
> -	strncpy(racomp.type, "acomp", sizeof(racomp.type));
> +	strlcpy(racomp.type, "acomp", sizeof(racomp.type));
>  
>  	if (nla_put(skb, CRYPTOCFGA_REPORT_ACOMP,
>  		    sizeof(struct crypto_report_acomp), &racomp))
> @@ -131,7 +131,7 @@ static int crypto_report_akcipher(struct sk_buff *skb, struct crypto_alg *alg)
>  {
>  	struct crypto_report_akcipher rakcipher;
>  
> -	strncpy(rakcipher.type, "akcipher", sizeof(rakcipher.type));
> +	strlcpy(rakcipher.type, "akcipher", sizeof(rakcipher.type));
>  
>  	if (nla_put(skb, CRYPTOCFGA_REPORT_AKCIPHER,
>  		    sizeof(struct crypto_report_akcipher), &rakcipher))
> @@ -146,7 +146,7 @@ static int crypto_report_kpp(struct sk_buff *skb, struct crypto_alg *alg)
>  {
>  	struct crypto_report_kpp rkpp;
>  
> -	strncpy(rkpp.type, "kpp", sizeof(rkpp.type));
> +	strlcpy(rkpp.type, "kpp", sizeof(rkpp.type));
>  
>  	if (nla_put(skb, CRYPTOCFGA_REPORT_KPP,
>  		    sizeof(struct crypto_report_kpp), &rkpp))
> @@ -160,10 +160,10 @@ static int crypto_report_kpp(struct sk_buff *skb, struct crypto_alg *alg)
>  static int crypto_report_one(struct crypto_alg *alg,
>  			     struct crypto_user_alg *ualg, struct sk_buff *skb)
>  {
> -	strncpy(ualg->cru_name, alg->cra_name, sizeof(ualg->cru_name));
> -	strncpy(ualg->cru_driver_name, alg->cra_driver_name,
> +	strlcpy(ualg->cru_name, alg->cra_name, sizeof(ualg->cru_name));
> +	strlcpy(ualg->cru_driver_name, alg->cra_driver_name,
>  		sizeof(ualg->cru_driver_name));
> -	strncpy(ualg->cru_module_name, module_name(alg->cra_module),
> +	strlcpy(ualg->cru_module_name, module_name(alg->cra_module),
>  		sizeof(ualg->cru_module_name));
>  
>  	ualg->cru_type = 0;
> @@ -176,7 +176,7 @@ static int crypto_report_one(struct crypto_alg *alg,
>  	if (alg->cra_flags & CRYPTO_ALG_LARVAL) {
>  		struct crypto_report_larval rl;
>  
> -		strncpy(rl.type, "larval", sizeof(rl.type));
> +		strlcpy(rl.type, "larval", sizeof(rl.type));
>  		if (nla_put(skb, CRYPTOCFGA_REPORT_LARVAL,
>  			    sizeof(struct crypto_report_larval), &rl))
>  			goto nla_put_failure;
> diff --git a/include/uapi/linux/cryptouser.h b/include/uapi/linux/cryptouser.h
> index 11d21fc..b4def5c 100644
> --- a/include/uapi/linux/cryptouser.h
> +++ b/include/uapi/linux/cryptouser.h
> @@ -31,7 +31,7 @@ enum {
>  #define CRYPTO_MSG_MAX (__CRYPTO_MSG_MAX - 1)
>  #define CRYPTO_NR_MSGTYPES (CRYPTO_MSG_MAX + 1 - CRYPTO_MSG_BASE)
>  
> -#define CRYPTO_MAX_NAME CRYPTO_MAX_ALG_NAME
> +#define CRYPTO_MAX_NAME 64
>  
>  /* Netlink message attributes.  */
>  enum crypto_attr_type_t {
> @@ -53,9 +53,9 @@ enum crypto_attr_type_t {
>  };
>  
>  struct crypto_user_alg {
> -	char cru_name[CRYPTO_MAX_ALG_NAME];
> -	char cru_driver_name[CRYPTO_MAX_ALG_NAME];
> -	char cru_module_name[CRYPTO_MAX_ALG_NAME];
> +	char cru_name[CRYPTO_MAX_NAME];
> +	char cru_driver_name[CRYPTO_MAX_NAME];
> +	char cru_module_name[CRYPTO_MAX_NAME];
>  	__u32 cru_type;
>  	__u32 cru_mask;
>  	__u32 cru_refcnt;
> @@ -73,7 +73,7 @@ struct crypto_report_hash {
>  };
>  
>  struct crypto_report_cipher {
> -	char type[CRYPTO_MAX_ALG_NAME];
> +	char type[CRYPTO_MAX_NAME];
>  	unsigned int blocksize;
>  	unsigned int min_keysize;
>  	unsigned int max_keysize;
> 

-- 
Best regards,
Alexander Sverdlin.

Re: [PATCH 3/4] xfrm: Prepare for CRYPTO_MAX_ALG_NAME expansion

[Alexander Sverdlin]

On 06/04/17 10:16, Herbert Xu wrote:
> This patch fixes the xfrm_user code to use the actual array size
> rather than the hard-coded CRYPTO_MAX_ALG_NAME length.  This is
> because the array size is fixed at 64 bytes while we want to increase
> the in-kernel CRYPTO_MAX_ALG_NAME value.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Acked-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
Tested-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>

> ---
> 
>  net/xfrm/xfrm_user.c |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
> index 9705c27..96557cf 100644
> --- a/net/xfrm/xfrm_user.c
> +++ b/net/xfrm/xfrm_user.c
> @@ -55,7 +55,7 @@ static int verify_one_alg(struct nlattr **attrs, enum xfrm_attr_type_t type)
>  		return -EINVAL;
>  	}
>  
> -	algp->alg_name[CRYPTO_MAX_ALG_NAME - 1] = '\0';
> +	algp->alg_name[sizeof(algp->alg_name) - 1] = '\0';
>  	return 0;
>  }
>  
> @@ -71,7 +71,7 @@ static int verify_auth_trunc(struct nlattr **attrs)
>  	if (nla_len(rt) < xfrm_alg_auth_len(algp))
>  		return -EINVAL;
>  
> -	algp->alg_name[CRYPTO_MAX_ALG_NAME - 1] = '\0';
> +	algp->alg_name[sizeof(algp->alg_name) - 1] = '\0';
>  	return 0;
>  }
>  
> @@ -87,7 +87,7 @@ static int verify_aead(struct nlattr **attrs)
>  	if (nla_len(rt) < aead_len(algp))
>  		return -EINVAL;
>  
> -	algp->alg_name[CRYPTO_MAX_ALG_NAME - 1] = '\0';
> +	algp->alg_name[sizeof(algp->alg_name) - 1] = '\0';
>  	return 0;
>  }
>  
> 

-- 
Best regards,
Alexander Sverdlin.

Re: [PATCH 4/4] crypto: api - Extend algorithm name limit to 128 bytes

[Alexander Sverdlin]

On 06/04/17 10:16, Herbert Xu wrote:
> With the new explicit IV generators, we may now exceed the 64-byte
> length limit on the algorithm name, e.g., with
> 
> 	echainiv(authencesn(hmac(sha256-generic),cbc(des3_ede-generic)))
> 
> This patch extends the length limit to 128 bytes.
> 
> Reported-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Acked-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
Tested-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>

> ---
> 
>  include/linux/crypto.h |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/crypto.h b/include/linux/crypto.h
> index c0b0cf3..84da997 100644
> --- a/include/linux/crypto.h
> +++ b/include/linux/crypto.h
> @@ -123,7 +123,7 @@
>  /*
>   * Miscellaneous stuff.
>   */
> -#define CRYPTO_MAX_ALG_NAME		64
> +#define CRYPTO_MAX_ALG_NAME		128
>  
>  /*
>   * The macro CRYPTO_MINALIGN_ATTR (along with the void * type in the actual
> 

-- 
Best regards,
Alexander Sverdlin.

Re: [PATCH 0/4] crypto: CRYPTO_MAX_ALG_NAME is too low

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Thu, 6 Apr 2017 16:15:09 +0800

> As the final patch depends on all three it would be easiest if
> we pushed the xfrm patch through the crypto tree.  Steffen/David?

No objections from me for this going through the crypto tree.

Re: [PATCH 0/4] crypto: CRYPTO_MAX_ALG_NAME is too low

[Steffen Klassert]

On Thu, Apr 06, 2017 at 01:58:32PM -0700, David Miller wrote:
> From: Herbert Xu <herbert@gondor.apana.org.au>
> Date: Thu, 6 Apr 2017 16:15:09 +0800
> 
> > As the final patch depends on all three it would be easiest if
> > we pushed the xfrm patch through the crypto tree.  Steffen/David?
> 
> No objections from me for this going through the crypto tree.

I have no objections too.

Re: [PATCH 3/4] xfrm: Prepare for CRYPTO_MAX_ALG_NAME expansion

[Steffen Klassert]

On Thu, Apr 06, 2017 at 04:16:10PM +0800, Herbert Xu wrote:
> This patch fixes the xfrm_user code to use the actual array size
> rather than the hard-coded CRYPTO_MAX_ALG_NAME length.  This is
> because the array size is fixed at 64 bytes while we want to increase
> the in-kernel CRYPTO_MAX_ALG_NAME value.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Acked-by: Steffen Klassert <steffen.klassert@secunet.com>

Re: [PATCH 2/4] crypto: af_alg - Allow arbitrarily long algorithm names

[Herbert Xu]

On Thu, Apr 06, 2017 at 02:32:27PM +0200, Alexander Sverdlin wrote:
>
> > diff --git a/crypto/af_alg.c b/crypto/af_alg.c
> > index 690deca..3556d8e 100644
> > --- a/crypto/af_alg.c
> > +++ b/crypto/af_alg.c
> > @@ -160,11 +160,11 @@ static int alg_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
> >  	if (sock->state == SS_CONNECTED)
> >  		return -EINVAL;
> >  
> > -	if (addr_len != sizeof(*sa))
> > +	if (addr_len < sizeof(*sa))
> >  		return -EINVAL;
> >  
> >  	sa->salg_type[sizeof(sa->salg_type) - 1] = 0;
> > -	sa->salg_name[sizeof(sa->salg_name) - 1] = 0;
> > +	sa->salg_name[sizeof(sa->salg_name) + addr_len - sizeof(*sa) - 1] = 0;
> >  
> >  	type = alg_get_type(sa->salg_type);
> >  	if (IS_ERR(type) && PTR_ERR(type) == -ENOENT) {
> 
> Why should userspace ever extend the structure if salg_name is hardcoded to 64 in if_alg.h?
> This patch doesn't change the behavior at all, or am I missing something?

We cannot change the structure in the user-space API by increasing
its length because that would break backwards compatibility.

What this patch does is relax the size check so that user-space can
pass in a name that is longer than 64 bytes.  This would fail on
older kernels with EINVAL.  Of course, user-space must be modified
to allow for such longer names.  That is beyond the scope of this
patch.

For names shorter than 64 bytes the existing API needs be respected
and user-space must pad it to exactly 64 bytes.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

RE: [2/4] crypto: af_alg - Allow arbitrarily long algorithm names" email-alg_bind.txt

[Lukasz Odzioba]

Hi,
I found this patch by accident and it got my attention.
I think we can't make this name arbitrarily long because bind syscall checks addrlen before feeding protocol with it.
Current limit on my machine is 128 bytes and I can't even reach alg_bind() function if I specify more than that.
We may want to revert that.

net/socket.c:
SYSCALL_DEFINE3(bind, int, fd, struct sockaddr __user *, umyaddr, int, addrlen)
{
	struct socket *sock;
	struct sockaddr_storage address;
	int err, fput_needed;

	sock = sockfd_lookup_light(fd, &err, &fput_needed);
	if (sock) {
		err = move_addr_to_kernel(umyaddr, addrlen, (struct sockaddr *)&address);
(...snip...)

int move_addr_to_kernel(void __user *uaddr, int ulen, struct sockaddr_storage *kaddr)
{
	if (ulen < 0 || ulen > sizeof(struct sockaddr_storage))
		return -EINVAL;
		
Thanks,
Lukas

Re: [2/4] crypto: af_alg - Allow arbitrarily long algorithm names" email-alg_bind.txt

[Herbert Xu]

On Wed, Nov 08, 2017 at 05:51:36PM +0100, Lukasz Odzioba wrote:
> Hi,
> I found this patch by accident and it got my attention.
> I think we can't make this name arbitrarily long because bind syscall checks addrlen before feeding protocol with it.
> Current limit on my machine is 128 bytes and I can't even reach alg_bind() function if I specify more than that.
> We may want to revert that.

Thanks for noticing this.  It's obviously still limited by the
underlying system call interface.

However, this isn't actually a problem because the crypto API
is currently limited to 128 bytes.  If and ever we need to inrease
that again then it would become a problem.  The solution would
probably be to switch to a different interface for specifying
such long names.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt