Re: [PATCH 3/4] xfrm: Prepare for CRYPTO_MAX_ALG_NAME expansion
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 <herb...@gondor.apana.org.au> Acked-by: Alexander Sverdlin <alexander.sverd...@nokia.com> Tested-by: Alexander Sverdlin <alexander.sverd...@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
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.sverd...@nokia.com> > Signed-off-by: Herbert Xu <herb...@gondor.apana.org.au> Acked-by: Alexander Sverdlin <alexander.sverd...@nokia.com> Tested-by: Alexander Sverdlin <alexander.sverd...@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 1/4] crypto: user - Prepare for CRYPTO_MAX_ALG_NAME expansion
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 <herb...@gondor.apana.org.au>
Acked-by: Alexander Sverdlin <alexander.sverd...@nokia.com>
Tested-by: Alexander Sverdlin <alexander.sverd...@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), ))
> 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), ))
> @@ -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), ))
> @@ -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), ))
> @@ -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), ))
> 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 CR
Re: [PATCH 0/4] crypto: CRYPTO_MAX_ALG_NAME is too low
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 2/4] crypto: af_alg - Allow arbitrarily long algorithm names
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 <herb...@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: CRYPTO_MAX_ALG_NAME is too low
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.
CRYPTO_MAX_ALG_NAME is too low
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.
