[PATCH 0/2] crypto: lrw - Fixes for the 'create()' function

[PATCH 0/2] crypto: lrw - Fixes for the 'create()' function

[Christophe JAILLET]

The first patch is the same as the one committed for crypto/xts.c applied a
few days ago.
(commit 5125e4e867ab ("crypto: xts - Fix an error handling path in 'create()'")
in /git/herbert/crypto-2.6.git)

The 2nd one is a pure speculation from me.
The create function in 'crypto/xts.c' and 'crypto/lrw.c' look very
similar.
However, there is one more sanity check in xts. This patch proposes to
add the same test in lrw. Not sure at all if correct!
I just send it for consistency.

Christophe JAILLET (2):
  crypto: lrw - Fix an error handling path in 'create()'
  crypto: lrw - Check for incorrect cipher name

 crypto/lrw.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

-- 
2.11.0

[PATCH 1/2] crypto: lrw - Fix an error handling path in 'create()'

[Christophe JAILLET]

All error handling paths 'goto err_drop_spawn' except this one.
In order to avoid some resources leak, we should do it as well here.

Fixes: 700cb3f5fe75 ("crypto: lrw - Convert to skcipher")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 crypto/lrw.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/crypto/lrw.c b/crypto/lrw.c
index a8bfae4451bf..eb681e9fe574 100644
--- a/crypto/lrw.c
+++ b/crypto/lrw.c
@@ -610,8 +610,10 @@ static int create(struct crypto_template *tmpl, struct rtattr **tb)
 		ecb_name[len - 1] = 0;
 
 		if (snprintf(inst->alg.base.cra_name, CRYPTO_MAX_ALG_NAME,
-			     "lrw(%s)", ecb_name) >= CRYPTO_MAX_ALG_NAME)
-			return -ENAMETOOLONG;
+			     "lrw(%s)", ecb_name) >= CRYPTO_MAX_ALG_NAME) {
+			err = -ENAMETOOLONG;
+			goto err_drop_spawn;
+		}
 	}
 
 	inst->alg.base.cra_flags = alg->base.cra_flags & CRYPTO_ALG_ASYNC;
-- 
2.11.0

Re: [PATCH 1/2] crypto: lrw - Fix an error handling path in 'create()'

[walter harms]

Am 08.10.2017 11:39, schrieb Christophe JAILLET:
> All error handling paths 'goto err_drop_spawn' except this one.
> In order to avoid some resources leak, we should do it as well here.
> 
> Fixes: 700cb3f5fe75 ("crypto: lrw - Convert to skcipher")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
>  crypto/lrw.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/crypto/lrw.c b/crypto/lrw.c
> index a8bfae4451bf..eb681e9fe574 100644
> --- a/crypto/lrw.c
> +++ b/crypto/lrw.c
> @@ -610,8 +610,10 @@ static int create(struct crypto_template *tmpl, struct rtattr **tb)
>  		ecb_name[len - 1] = 0;
>  
>  		if (snprintf(inst->alg.base.cra_name, CRYPTO_MAX_ALG_NAME,
> -			     "lrw(%s)", ecb_name) >= CRYPTO_MAX_ALG_NAME)

	this check can be done more easy,
	the length of ecb_name is len
	the length of inst->alg.base.cra_name is CRYPTO_MAX_ALG_NAME
	if CRYPTO_MAX_ALG_NAME-len < "lrw()" < 5
		no need to involve snprintf()

	just my 2 cents
	re,
 		wh

> -			return -ENAMETOOLONG;
> +			     "lrw(%s)", ecb_name) >= CRYPTO_MAX_ALG_NAME) {
> +			err = -ENAMETOOLONG;
> +			goto err_drop_spawn;
> +		}
>  	}
>  
>  	inst->alg.base.cra_flags = alg->base.cra_flags & CRYPTO_ALG_ASYNC;

Re: [PATCH 1/2] crypto: lrw - Fix an error handling path in 'create()'

[Christophe JAILLET]

Le 09/10/2017 à 23:22, walter harms a écrit :
> Am 08.10.2017 11:39, schrieb Christophe JAILLET:
>> All error handling paths 'goto err_drop_spawn' except this one.
>> In order to avoid some resources leak, we should do it as well here.
>>
>> Fixes: 700cb3f5fe75 ("crypto: lrw - Convert to skcipher")
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>> ---
>>   crypto/lrw.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/crypto/lrw.c b/crypto/lrw.c
>> index a8bfae4451bf..eb681e9fe574 100644
>> --- a/crypto/lrw.c
>> +++ b/crypto/lrw.c
>> @@ -610,8 +610,10 @@ static int create(struct crypto_template *tmpl, struct rtattr **tb)
>>   		ecb_name[len - 1] = 0;
>>   
>>   		if (snprintf(inst->alg.base.cra_name, CRYPTO_MAX_ALG_NAME,
>> -			     "lrw(%s)", ecb_name) >= CRYPTO_MAX_ALG_NAME)
> 	this check can be done more easy,
> 	the length of ecb_name is len
> 	the length of inst->alg.base.cra_name is CRYPTO_MAX_ALG_NAME
> 	if CRYPTO_MAX_ALG_NAME-len < "lrw()" < 5
> 		no need to involve snprintf()
>
> 	just my 2 cents
> 	re,
>   		wh
It does not only check for the length, it also copies some data.
The test should be read: "If the copy succeeds (i.e if there is enough 
space for the copy to succeed)", and not "if the string is too long".
IMHO, the snprintf is just fine here.

CJ
>> -			return -ENAMETOOLONG;
>> +			     "lrw(%s)", ecb_name) >= CRYPTO_MAX_ALG_NAME) {
>> +			err = -ENAMETOOLONG;
>> +			goto err_drop_spawn;
>> +		}
>>   	}
>>   
>>   	inst->alg.base.cra_flags = alg->base.cra_flags & CRYPTO_ALG_ASYNC;

Re: [PATCH 1/2] crypto: lrw - Fix an error handling path in 'create()'

[walter harms]

Am 10.10.2017 08:05, schrieb Christophe JAILLET:
> Le 09/10/2017 à 23:22, walter harms a écrit :
>> Am 08.10.2017 11:39, schrieb Christophe JAILLET:
>>> All error handling paths 'goto err_drop_spawn' except this one.
>>> In order to avoid some resources leak, we should do it as well here.
>>>
>>> Fixes: 700cb3f5fe75 ("crypto: lrw - Convert to skcipher")
>>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>>> ---
>>>   crypto/lrw.c | 6 ++++--
>>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/crypto/lrw.c b/crypto/lrw.c
>>> index a8bfae4451bf..eb681e9fe574 100644
>>> --- a/crypto/lrw.c
>>> +++ b/crypto/lrw.c
>>> @@ -610,8 +610,10 @@ static int create(struct crypto_template *tmpl,
>>> struct rtattr **tb)
>>>           ecb_name[len - 1] = 0;
>>>             if (snprintf(inst->alg.base.cra_name, CRYPTO_MAX_ALG_NAME,
>>> -                 "lrw(%s)", ecb_name) >= CRYPTO_MAX_ALG_NAME)
>>     this check can be done more easy,
>>     the length of ecb_name is len
>>     the length of inst->alg.base.cra_name is CRYPTO_MAX_ALG_NAME
>>     if CRYPTO_MAX_ALG_NAME-len < "lrw()" < 5
>>         no need to involve snprintf()
>>
>>     just my 2 cents
>>     re,
>>           wh
> It does not only check for the length, it also copies some data.
> The test should be read: "If the copy succeeds (i.e if there is enough
> space for the copy to succeed)", and not "if the string is too long".
> IMHO, the snprintf is just fine here.


under "normal" circumstance i would say "does not matter" when
something ends up as crippled string but in case of crypto sameone
(the maintainer) needs to be careful. I have no idea about the
consequences, i can only point to strange looking things and
say "be careful".

re,
 wh

> 
> CJ
>>> -            return -ENAMETOOLONG;
>>> +                 "lrw(%s)", ecb_name) >= CRYPTO_MAX_ALG_NAME) {
>>> +            err = -ENAMETOOLONG;
>>> +            goto err_drop_spawn;
>>> +        }
>>>       }
>>>         inst->alg.base.cra_flags = alg->base.cra_flags &
>>> CRYPTO_ALG_ASYNC;
> 
> 
> -- 
> To unsubscribe from this list: send the line "unsubscribe
> kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[PATCH 2/2] crypto: lrw - Check for incorrect cipher name

[Christophe JAILLET]

If the cipher name does not start with 'ecb(' we should bail out, as done
in the 'create()' function in 'crypto/xts.c'.

Fixes: 700cb3f5fe75 ("crypto: lrw - Convert to skcipher")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
This patch is 100% speculative.
It is based on comparison with the 'create()' function from 'crypto/xts.c'
Code looks the same, but this aditionnal test is in xts.c and not in
lrw.c.
The 2 should maybe be consistent?
---
 crypto/lrw.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/crypto/lrw.c b/crypto/lrw.c
index eb681e9fe574..92df312b8c6e 100644
--- a/crypto/lrw.c
+++ b/crypto/lrw.c
@@ -614,7 +614,8 @@ static int create(struct crypto_template *tmpl, struct rtattr **tb)
 			err = -ENAMETOOLONG;
 			goto err_drop_spawn;
 		}
-	}
+	} else
+		goto err_drop_spawn;
 
 	inst->alg.base.cra_flags = alg->base.cra_flags & CRYPTO_ALG_ASYNC;
 	inst->alg.base.cra_priority = alg->base.cra_priority;
-- 
2.11.0

Re: [PATCH 0/2] crypto: lrw - Fixes for the 'create()' function

[Herbert Xu]

On Sun, Oct 08, 2017 at 11:39:48AM +0200, Christophe JAILLET wrote:
> The first patch is the same as the one committed for crypto/xts.c applied a
> few days ago.
> (commit 5125e4e867ab ("crypto: xts - Fix an error handling path in 'create()'")
> in /git/herbert/crypto-2.6.git)
> 
> The 2nd one is a pure speculation from me.
> The create function in 'crypto/xts.c' and 'crypto/lrw.c' look very
> similar.
> However, there is one more sanity check in xts. This patch proposes to
> add the same test in lrw. Not sure at all if correct!
> I just send it for consistency.
> 
> Christophe JAILLET (2):
>   crypto: lrw - Fix an error handling path in 'create()'
>   crypto: lrw - Check for incorrect cipher name

All applied.  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