* CRYPTO_MAX_ALG_NAME is too low @ 2017-03-10 11:55 Alexander Sverdlin 2017-03-16 14:16 ` Alexander Sverdlin 0 siblings, 1 reply; 18+ messages in thread From: Alexander Sverdlin @ 2017-03-10 11:55 UTC (permalink / raw) To: Herbert Xu, David S. Miller; +Cc: linux-crypto 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: CRYPTO_MAX_ALG_NAME is too low 2017-03-10 11:55 CRYPTO_MAX_ALG_NAME is too low Alexander Sverdlin @ 2017-03-16 14:16 ` Alexander Sverdlin 2017-04-06 8:15 ` [PATCH 0/4] crypto: " Herbert Xu 0 siblings, 1 reply; 18+ messages in thread From: Alexander Sverdlin @ 2017-03-16 14:16 UTC (permalink / raw) To: Herbert Xu, David S. Miller; +Cc: linux-crypto 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 0/4] crypto: CRYPTO_MAX_ALG_NAME is too low 2017-03-16 14:16 ` Alexander Sverdlin @ 2017-04-06 8:15 ` Herbert Xu 2017-04-06 8:16 ` [PATCH 1/4] crypto: user - Prepare for CRYPTO_MAX_ALG_NAME expansion Herbert Xu ` (5 more replies) 0 siblings, 6 replies; 18+ messages in thread From: Herbert Xu @ 2017-04-06 8:15 UTC (permalink / raw) To: Alexander Sverdlin; +Cc: David S. Miller, linux-crypto, netdev 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/4] crypto: user - Prepare for CRYPTO_MAX_ALG_NAME expansion 2017-04-06 8:15 ` [PATCH 0/4] crypto: " Herbert Xu @ 2017-04-06 8:16 ` Herbert Xu 2017-04-06 15:10 ` Alexander Sverdlin 2017-04-06 8:16 ` [PATCH 2/4] crypto: af_alg - Allow arbitrarily long algorithm names Herbert Xu ` (4 subsequent siblings) 5 siblings, 1 reply; 18+ messages in thread From: Herbert Xu @ 2017-04-06 8:16 UTC (permalink / raw) To: Alexander Sverdlin, David S. Miller, linux-crypto, netdev 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; ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 1/4] crypto: user - Prepare for CRYPTO_MAX_ALG_NAME expansion 2017-04-06 8:16 ` [PATCH 1/4] crypto: user - Prepare for CRYPTO_MAX_ALG_NAME expansion Herbert Xu @ 2017-04-06 15:10 ` Alexander Sverdlin 0 siblings, 0 replies; 18+ messages in thread From: Alexander Sverdlin @ 2017-04-06 15:10 UTC (permalink / raw) To: Herbert Xu, David S. Miller, linux-crypto, netdev 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/4] crypto: af_alg - Allow arbitrarily long algorithm names 2017-04-06 8:15 ` [PATCH 0/4] crypto: " Herbert Xu 2017-04-06 8:16 ` [PATCH 1/4] crypto: user - Prepare for CRYPTO_MAX_ALG_NAME expansion Herbert Xu @ 2017-04-06 8:16 ` Herbert Xu 2017-04-06 12:32 ` Alexander Sverdlin 2017-11-08 16:51 ` [2/4] crypto: af_alg - Allow arbitrarily long algorithm names" email-alg_bind.txt Lukasz Odzioba 2017-04-06 8:16 ` [PATCH 3/4] xfrm: Prepare for CRYPTO_MAX_ALG_NAME expansion Herbert Xu ` (3 subsequent siblings) 5 siblings, 2 replies; 18+ messages in thread From: Herbert Xu @ 2017-04-06 8:16 UTC (permalink / raw) To: Alexander Sverdlin, David S. Miller, linux-crypto, netdev 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) { ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 2/4] crypto: af_alg - Allow arbitrarily long algorithm names 2017-04-06 8:16 ` [PATCH 2/4] crypto: af_alg - Allow arbitrarily long algorithm names Herbert Xu @ 2017-04-06 12:32 ` Alexander Sverdlin 2017-04-07 5:25 ` Herbert Xu 2017-11-08 16:51 ` [2/4] crypto: af_alg - Allow arbitrarily long algorithm names" email-alg_bind.txt Lukasz Odzioba 1 sibling, 1 reply; 18+ messages in thread From: Alexander Sverdlin @ 2017-04-06 12:32 UTC (permalink / raw) To: Herbert Xu, David S. Miller, linux-crypto, netdev 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/4] crypto: af_alg - Allow arbitrarily long algorithm names 2017-04-06 12:32 ` Alexander Sverdlin @ 2017-04-07 5:25 ` Herbert Xu 0 siblings, 0 replies; 18+ messages in thread From: Herbert Xu @ 2017-04-07 5:25 UTC (permalink / raw) To: Alexander Sverdlin; +Cc: David S. Miller, linux-crypto, netdev 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [2/4] crypto: af_alg - Allow arbitrarily long algorithm names" email-alg_bind.txt 2017-04-06 8:16 ` [PATCH 2/4] crypto: af_alg - Allow arbitrarily long algorithm names Herbert Xu 2017-04-06 12:32 ` Alexander Sverdlin @ 2017-11-08 16:51 ` Lukasz Odzioba 2017-11-27 5:54 ` Herbert Xu 1 sibling, 1 reply; 18+ messages in thread From: Lukasz Odzioba @ 2017-11-08 16:51 UTC (permalink / raw) To: herbert, linux-kernel; +Cc: 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [2/4] crypto: af_alg - Allow arbitrarily long algorithm names" email-alg_bind.txt 2017-11-08 16:51 ` [2/4] crypto: af_alg - Allow arbitrarily long algorithm names" email-alg_bind.txt Lukasz Odzioba @ 2017-11-27 5:54 ` Herbert Xu 0 siblings, 0 replies; 18+ messages in thread From: Herbert Xu @ 2017-11-27 5:54 UTC (permalink / raw) To: Lukasz Odzioba; +Cc: linux-kernel 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 3/4] xfrm: Prepare for CRYPTO_MAX_ALG_NAME expansion 2017-04-06 8:15 ` [PATCH 0/4] crypto: " Herbert Xu 2017-04-06 8:16 ` [PATCH 1/4] crypto: user - Prepare for CRYPTO_MAX_ALG_NAME expansion Herbert Xu 2017-04-06 8:16 ` [PATCH 2/4] crypto: af_alg - Allow arbitrarily long algorithm names Herbert Xu @ 2017-04-06 8:16 ` Herbert Xu 2017-04-06 15:10 ` Alexander Sverdlin 2017-04-06 21:15 ` Steffen Klassert 2017-04-06 8:16 ` [PATCH 4/4] crypto: api - Extend algorithm name limit to 128 bytes Herbert Xu ` (2 subsequent siblings) 5 siblings, 2 replies; 18+ messages in thread From: Herbert Xu @ 2017-04-06 8:16 UTC (permalink / raw) To: Alexander Sverdlin, David S. Miller, linux-crypto, netdev 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; } ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 3/4] xfrm: Prepare for CRYPTO_MAX_ALG_NAME expansion 2017-04-06 8:16 ` [PATCH 3/4] xfrm: Prepare for CRYPTO_MAX_ALG_NAME expansion Herbert Xu @ 2017-04-06 15:10 ` Alexander Sverdlin 2017-04-06 21:15 ` Steffen Klassert 1 sibling, 0 replies; 18+ messages in thread From: Alexander Sverdlin @ 2017-04-06 15:10 UTC (permalink / raw) To: Herbert Xu, David S. Miller, linux-crypto, netdev 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/4] xfrm: Prepare for CRYPTO_MAX_ALG_NAME expansion 2017-04-06 8:16 ` [PATCH 3/4] xfrm: Prepare for CRYPTO_MAX_ALG_NAME expansion Herbert Xu 2017-04-06 15:10 ` Alexander Sverdlin @ 2017-04-06 21:15 ` Steffen Klassert 1 sibling, 0 replies; 18+ messages in thread From: Steffen Klassert @ 2017-04-06 21:15 UTC (permalink / raw) To: Herbert Xu; +Cc: Alexander Sverdlin, David S. Miller, linux-crypto, netdev 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> ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 4/4] crypto: api - Extend algorithm name limit to 128 bytes 2017-04-06 8:15 ` [PATCH 0/4] crypto: " Herbert Xu ` (2 preceding siblings ...) 2017-04-06 8:16 ` [PATCH 3/4] xfrm: Prepare for CRYPTO_MAX_ALG_NAME expansion Herbert Xu @ 2017-04-06 8:16 ` Herbert Xu 2017-04-06 15:11 ` Alexander Sverdlin 2017-04-06 15:10 ` [PATCH 0/4] crypto: CRYPTO_MAX_ALG_NAME is too low Alexander Sverdlin 2017-04-06 20:58 ` David Miller 5 siblings, 1 reply; 18+ messages in thread From: Herbert Xu @ 2017-04-06 8:16 UTC (permalink / raw) To: Alexander Sverdlin, David S. Miller, linux-crypto, netdev 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 4/4] crypto: api - Extend algorithm name limit to 128 bytes 2017-04-06 8:16 ` [PATCH 4/4] crypto: api - Extend algorithm name limit to 128 bytes Herbert Xu @ 2017-04-06 15:11 ` Alexander Sverdlin 0 siblings, 0 replies; 18+ messages in thread From: Alexander Sverdlin @ 2017-04-06 15:11 UTC (permalink / raw) To: Herbert Xu, David S. Miller, linux-crypto, netdev 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/4] crypto: CRYPTO_MAX_ALG_NAME is too low 2017-04-06 8:15 ` [PATCH 0/4] crypto: " Herbert Xu ` (3 preceding siblings ...) 2017-04-06 8:16 ` [PATCH 4/4] crypto: api - Extend algorithm name limit to 128 bytes Herbert Xu @ 2017-04-06 15:10 ` Alexander Sverdlin 2017-04-06 20:58 ` David Miller 5 siblings, 0 replies; 18+ messages in thread From: Alexander Sverdlin @ 2017-04-06 15:10 UTC (permalink / raw) To: Herbert Xu; +Cc: David S. Miller, linux-crypto, netdev 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/4] crypto: CRYPTO_MAX_ALG_NAME is too low 2017-04-06 8:15 ` [PATCH 0/4] crypto: " Herbert Xu ` (4 preceding siblings ...) 2017-04-06 15:10 ` [PATCH 0/4] crypto: CRYPTO_MAX_ALG_NAME is too low Alexander Sverdlin @ 2017-04-06 20:58 ` David Miller 2017-04-06 21:14 ` Steffen Klassert 5 siblings, 1 reply; 18+ messages in thread From: David Miller @ 2017-04-06 20:58 UTC (permalink / raw) To: herbert; +Cc: alexander.sverdlin, linux-crypto, netdev 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/4] crypto: CRYPTO_MAX_ALG_NAME is too low 2017-04-06 20:58 ` David Miller @ 2017-04-06 21:14 ` Steffen Klassert 0 siblings, 0 replies; 18+ messages in thread From: Steffen Klassert @ 2017-04-06 21:14 UTC (permalink / raw) To: David Miller; +Cc: herbert, alexander.sverdlin, linux-crypto, netdev 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2017-11-27 5:54 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-03-10 11:55 CRYPTO_MAX_ALG_NAME is too low Alexander Sverdlin 2017-03-16 14:16 ` Alexander Sverdlin 2017-04-06 8:15 ` [PATCH 0/4] crypto: " Herbert Xu 2017-04-06 8:16 ` [PATCH 1/4] crypto: user - Prepare for CRYPTO_MAX_ALG_NAME expansion Herbert Xu 2017-04-06 15:10 ` Alexander Sverdlin 2017-04-06 8:16 ` [PATCH 2/4] crypto: af_alg - Allow arbitrarily long algorithm names Herbert Xu 2017-04-06 12:32 ` Alexander Sverdlin 2017-04-07 5:25 ` Herbert Xu 2017-11-08 16:51 ` [2/4] crypto: af_alg - Allow arbitrarily long algorithm names" email-alg_bind.txt Lukasz Odzioba 2017-11-27 5:54 ` Herbert Xu 2017-04-06 8:16 ` [PATCH 3/4] xfrm: Prepare for CRYPTO_MAX_ALG_NAME expansion Herbert Xu 2017-04-06 15:10 ` Alexander Sverdlin 2017-04-06 21:15 ` Steffen Klassert 2017-04-06 8:16 ` [PATCH 4/4] crypto: api - Extend algorithm name limit to 128 bytes Herbert Xu 2017-04-06 15:11 ` Alexander Sverdlin 2017-04-06 15:10 ` [PATCH 0/4] crypto: CRYPTO_MAX_ALG_NAME is too low Alexander Sverdlin 2017-04-06 20:58 ` David Miller 2017-04-06 21:14 ` Steffen Klassert
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.