This serie is divided into 2 patches. They are more or less related to the
same issue, but the first patch is not a bug in itself, just a clean-up
(IMHO).
If I'm correct, the 2nd one, is a real (unlikely) issue.
Christophe JAILLET (2):
crypto: chcr - Improve error checking
crypto: chcr - Fix error checking
drivers/crypto/chelsio/chcr_algo.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
--
2.11.0
'chcr_alloc_shash()' can return NULL. Here it is not possible because this
code is reached only if 'get_alg_config()' a few lines above has succeeded.
So we are garanteed that the value of 'max_authsize' is a correct
parameter.
Anyway, this is harmless to add a check for NULL.
Signed-off-by: Christophe JAILLET <[email protected]>
---
drivers/crypto/chelsio/chcr_algo.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/crypto/chelsio/chcr_algo.c b/drivers/crypto/chelsio/chcr_algo.c
index 41bc7f4f58cd..f19590ac8775 100644
--- a/drivers/crypto/chelsio/chcr_algo.c
+++ b/drivers/crypto/chelsio/chcr_algo.c
@@ -2294,7 +2294,7 @@ static int chcr_authenc_setkey(struct crypto_aead *authenc, const u8 *key,
aeadctx->enckey_len << 3);
base_hash = chcr_alloc_shash(max_authsize);
- if (IS_ERR(base_hash)) {
+ if (IS_ERR_OR_NULL(base_hash)) {
pr_err("chcr : Base driver cannot be loaded\n");
goto out;
}
--
2.11.0
If 'chcr_alloc_shash()' a few lines above fails, 'base_hash' can be an
error pointer when we 'goto out'.
So checking for NULL here is not enough because it is likely that
'chcr_free_shash' will crash if we pass an error pointer.
Signed-off-by: Christophe JAILLET <[email protected]>
---
Another solution, amybe safer, would be to instrument 'chcr_free_shash' or
'crypto_free_shash' to accept an error pointer and return immediatelly in
such a case.
---
drivers/crypto/chelsio/chcr_algo.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/crypto/chelsio/chcr_algo.c b/drivers/crypto/chelsio/chcr_algo.c
index f19590ac8775..41750b97f43c 100644
--- a/drivers/crypto/chelsio/chcr_algo.c
+++ b/drivers/crypto/chelsio/chcr_algo.c
@@ -2351,7 +2351,7 @@ static int chcr_authenc_setkey(struct crypto_aead *authenc, const u8 *key,
}
out:
aeadctx->enckey_len = 0;
- if (base_hash)
+ if (!IS_ERR_OR_NULL(base_hash))
chcr_free_shash(base_hash);
return -EINVAL;
}
--
2.11.0
On Thu, Apr 13, 2017 at 02:14:19PM +0200, Christophe JAILLET wrote:
> 'chcr_alloc_shash()' can return NULL. Here it is not possible because this
> code is reached only if 'get_alg_config()' a few lines above has succeeded.
> So we are garanteed that the value of 'max_authsize' is a correct
> parameter.
> Anyway, this is harmless to add a check for NULL.
>
> Signed-off-by: Christophe JAILLET <[email protected]>
> ---
> drivers/crypto/chelsio/chcr_algo.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/crypto/chelsio/chcr_algo.c b/drivers/crypto/chelsio/chcr_algo.c
> index 41bc7f4f58cd..f19590ac8775 100644
> --- a/drivers/crypto/chelsio/chcr_algo.c
> +++ b/drivers/crypto/chelsio/chcr_algo.c
> @@ -2294,7 +2294,7 @@ static int chcr_authenc_setkey(struct crypto_aead *authenc, const u8 *key,
> aeadctx->enckey_len << 3);
>
> base_hash = chcr_alloc_shash(max_authsize);
> - if (IS_ERR(base_hash)) {
> + if (IS_ERR_OR_NULL(base_hash)) {
> pr_err("chcr : Base driver cannot be loaded\n");
> goto out;
Ugh... When you mix NULL and error pointers, it should be because NULL
is a valid return. We should change chcr_alloc_shash() to return
ERR_PTR(-EINVAL) instead of NULL.
Also the "goto out;" is buggy, of course. The problem with magical free
everything style error handling is that "base_hash" wasn't allocated so
this will oops for both NULL and error pointers.
regards,
dan carpenter
On Thu, Apr 13, 2017 at 02:14:30PM +0200, Christophe JAILLET wrote:
> If 'chcr_alloc_shash()' a few lines above fails, 'base_hash' can be an
> error pointer when we 'goto out'.
> So checking for NULL here is not enough because it is likely that
> 'chcr_free_shash' will crash if we pass an error pointer.
>
> Signed-off-by: Christophe JAILLET <[email protected]>
> ---
> Another solution, amybe safer, would be to instrument 'chcr_free_shash' or
> 'crypto_free_shash' to accept an error pointer and return immediatelly in
> such a case.
> ---
> drivers/crypto/chelsio/chcr_algo.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/crypto/chelsio/chcr_algo.c b/drivers/crypto/chelsio/chcr_algo.c
> index f19590ac8775..41750b97f43c 100644
> --- a/drivers/crypto/chelsio/chcr_algo.c
> +++ b/drivers/crypto/chelsio/chcr_algo.c
> @@ -2351,7 +2351,7 @@ static int chcr_authenc_setkey(struct crypto_aead *authenc, const u8 *key,
> }
> out:
> aeadctx->enckey_len = 0;
> - if (base_hash)
> + if (!IS_ERR_OR_NULL(base_hash))
> chcr_free_shash(base_hash);
Ah... Ok. Fine, but redo the first patch anyway because it shouldn't
ever be NULL.
regards,
dan carpenter
Le 13/04/2017 ? 16:04, Dan Carpenter a ?crit :
> On Thu, Apr 13, 2017 at 02:14:30PM +0200, Christophe JAILLET wrote:
>> If 'chcr_alloc_shash()' a few lines above fails, 'base_hash' can be an
>> error pointer when we 'goto out'.
>> So checking for NULL here is not enough because it is likely that
>> 'chcr_free_shash' will crash if we pass an error pointer.
>>
>> Signed-off-by: Christophe JAILLET <[email protected]>
>> ---
>> Another solution, amybe safer, would be to instrument 'chcr_free_shash' or
>> 'crypto_free_shash' to accept an error pointer and return immediatelly in
>> such a case.
>> ---
>> drivers/crypto/chelsio/chcr_algo.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/crypto/chelsio/chcr_algo.c b/drivers/crypto/chelsio/chcr_algo.c
>> index f19590ac8775..41750b97f43c 100644
>> --- a/drivers/crypto/chelsio/chcr_algo.c
>> +++ b/drivers/crypto/chelsio/chcr_algo.c
>> @@ -2351,7 +2351,7 @@ static int chcr_authenc_setkey(struct crypto_aead *authenc, const u8 *key,
>> }
>> out:
>> aeadctx->enckey_len = 0;
>> - if (base_hash)
>> + if (!IS_ERR_OR_NULL(base_hash))
>> chcr_free_shash(base_hash);
> Ah... Ok. Fine, but redo the first patch anyway because it shouldn't
> ever be NULL.
>
> regards,
> dan carpenter
Hi Dan,
I will update the first patch as you proposed in order to:
- teach 'chcr_alloc_shash' not to return NULL
- initialize 'base_hash' with ERR_PTR(-EINVAL)
- update the above test to !IS_ERR.
The 2 patches will be merged in only 1.
Thanks for your suggestions.
Best regards,
CJ
On Thu, Apr 13, 2017 at 8:20 PM, Christophe JAILLET
<[email protected]> wrote:
> Le 13/04/2017 à 16:04, Dan Carpenter a écrit :
>>
>> On Thu, Apr 13, 2017 at 02:14:30PM +0200, Christophe JAILLET wrote:
>>>
>>> If 'chcr_alloc_shash()' a few lines above fails, 'base_hash' can be an
>>> error pointer when we 'goto out'.
>>> So checking for NULL here is not enough because it is likely that
>>> 'chcr_free_shash' will crash if we pass an error pointer.
>>>
>>> Signed-off-by: Christophe JAILLET <[email protected]>
>>> ---
>>> Another solution, amybe safer, would be to instrument 'chcr_free_shash'
>>> or
>>> 'crypto_free_shash' to accept an error pointer and return immediatelly in
>>> such a case.
>>> ---
>>> drivers/crypto/chelsio/chcr_algo.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/crypto/chelsio/chcr_algo.c
>>> b/drivers/crypto/chelsio/chcr_algo.c
>>> index f19590ac8775..41750b97f43c 100644
>>> --- a/drivers/crypto/chelsio/chcr_algo.c
>>> +++ b/drivers/crypto/chelsio/chcr_algo.c
>>> @@ -2351,7 +2351,7 @@ static int chcr_authenc_setkey(struct crypto_aead
>>> *authenc, const u8 *key,
>>> }
>>> out:
>>> aeadctx->enckey_len = 0;
>>> - if (base_hash)
>>> + if (!IS_ERR_OR_NULL(base_hash))
>>> chcr_free_shash(base_hash);
>>
>> Ah... Ok. Fine, but redo the first patch anyway because it shouldn't
>> ever be NULL.
>>
>> regards,
>> dan carpenter
>
> Hi Dan,
>
> I will update the first patch as you proposed in order to:
> - teach 'chcr_alloc_shash' not to return NULL
> - initialize 'base_hash' with ERR_PTR(-EINVAL)
> - update the above test to !IS_ERR.
> The 2 patches will be merged in only 1.
>
> Thanks for your suggestions.
Thanks for pointing the error. or You can simply return instead of
goto. Just like that.
1.3 @@ -2455,7 +2455,8 @@ static int chcr_authenc_setkey(struct cr
1.4 base_hash = chcr_alloc_shash(max_authsize);
1.5 if (IS_ERR(base_hash)) {
1.6 pr_err("chcr : Base driver cannot be loaded\n");
1.7 - goto out;
1.8 + aeadctx->enckey_len = 0;
1.9 + return -EINVAL;
1.10 }
1.11 {
1.12 SHASH_DESC_ON_STACK(shash, base_hash);
>
> Best regards,
> CJ
>
On Thu, Apr 13, 2017 at 08:37:50PM +0530, Harsh Jain wrote:
> On Thu, Apr 13, 2017 at 8:20 PM, Christophe JAILLET
> <[email protected]> wrote:
> > Le 13/04/2017 ? 16:04, Dan Carpenter a ?crit :
> >>
> >> On Thu, Apr 13, 2017 at 02:14:30PM +0200, Christophe JAILLET wrote:
> >>>
> >>> If 'chcr_alloc_shash()' a few lines above fails, 'base_hash' can be an
> >>> error pointer when we 'goto out'.
> >>> So checking for NULL here is not enough because it is likely that
> >>> 'chcr_free_shash' will crash if we pass an error pointer.
> >>>
> >>> Signed-off-by: Christophe JAILLET <[email protected]>
> >>> ---
> >>> Another solution, amybe safer, would be to instrument 'chcr_free_shash'
> >>> or
> >>> 'crypto_free_shash' to accept an error pointer and return immediatelly in
> >>> such a case.
> >>> ---
> >>> drivers/crypto/chelsio/chcr_algo.c | 2 +-
> >>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/crypto/chelsio/chcr_algo.c
> >>> b/drivers/crypto/chelsio/chcr_algo.c
> >>> index f19590ac8775..41750b97f43c 100644
> >>> --- a/drivers/crypto/chelsio/chcr_algo.c
> >>> +++ b/drivers/crypto/chelsio/chcr_algo.c
> >>> @@ -2351,7 +2351,7 @@ static int chcr_authenc_setkey(struct crypto_aead
> >>> *authenc, const u8 *key,
> >>> }
> >>> out:
> >>> aeadctx->enckey_len = 0;
> >>> - if (base_hash)
> >>> + if (!IS_ERR_OR_NULL(base_hash))
> >>> chcr_free_shash(base_hash);
> >>
> >> Ah... Ok. Fine, but redo the first patch anyway because it shouldn't
> >> ever be NULL.
> >>
> >> regards,
> >> dan carpenter
> >
> > Hi Dan,
> >
> > I will update the first patch as you proposed in order to:
> > - teach 'chcr_alloc_shash' not to return NULL
> > - initialize 'base_hash' with ERR_PTR(-EINVAL)
> > - update the above test to !IS_ERR.
> > The 2 patches will be merged in only 1.
> >
> > Thanks for your suggestions.
>
> Thanks for pointing the error. or You can simply return instead of
> goto. Just like that.
>
> 1.3 @@ -2455,7 +2455,8 @@ static int chcr_authenc_setkey(struct cr
> 1.4 base_hash = chcr_alloc_shash(max_authsize);
> 1.5 if (IS_ERR(base_hash)) {
> 1.6 pr_err("chcr : Base driver cannot be loaded\n");
> 1.7 - goto out;
> 1.8 + aeadctx->enckey_len = 0;
> 1.9 + return -EINVAL;
Don't do that. There should be a goto.
regards,
dan carpenter
Le 13/04/2017 ? 18:13, Dan Carpenter a ?crit :
> On Thu, Apr 13, 2017 at 08:37:50PM +0530, Harsh Jain wrote:
>> On Thu, Apr 13, 2017 at 8:20 PM, Christophe JAILLET
>> <[email protected]> wrote:
>>> Le 13/04/2017 ? 16:04, Dan Carpenter a ?crit :
>>>> On Thu, Apr 13, 2017 at 02:14:30PM +0200, Christophe JAILLET wrote:
>>>>> If 'chcr_alloc_shash()' a few lines above fails, 'base_hash' can be an
>>>>> error pointer when we 'goto out'.
>>>>> So checking for NULL here is not enough because it is likely that
>>>>> 'chcr_free_shash' will crash if we pass an error pointer.
>>>>>
>>>>> Signed-off-by: Christophe JAILLET <[email protected]>
>>>>> ---
>>>>> Another solution, amybe safer, would be to instrument 'chcr_free_shash'
>>>>> or
>>>>> 'crypto_free_shash' to accept an error pointer and return immediatelly in
>>>>> such a case.
>>>>> ---
>>>>> drivers/crypto/chelsio/chcr_algo.c | 2 +-
>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/crypto/chelsio/chcr_algo.c
>>>>> b/drivers/crypto/chelsio/chcr_algo.c
>>>>> index f19590ac8775..41750b97f43c 100644
>>>>> --- a/drivers/crypto/chelsio/chcr_algo.c
>>>>> +++ b/drivers/crypto/chelsio/chcr_algo.c
>>>>> @@ -2351,7 +2351,7 @@ static int chcr_authenc_setkey(struct crypto_aead
>>>>> *authenc, const u8 *key,
>>>>> }
>>>>> out:
>>>>> aeadctx->enckey_len = 0;
>>>>> - if (base_hash)
>>>>> + if (!IS_ERR_OR_NULL(base_hash))
>>>>> chcr_free_shash(base_hash);
>>>> Ah... Ok. Fine, but redo the first patch anyway because it shouldn't
>>>> ever be NULL.
>>>>
>>>> regards,
>>>> dan carpenter
>>> Hi Dan,
>>>
>>> I will update the first patch as you proposed in order to:
>>> - teach 'chcr_alloc_shash' not to return NULL
>>> - initialize 'base_hash' with ERR_PTR(-EINVAL)
>>> - update the above test to !IS_ERR.
>>> The 2 patches will be merged in only 1.
>>>
>>> Thanks for your suggestions.
>> Thanks for pointing the error. or You can simply return instead of
>> goto. Just like that.
>>
>> 1.3 @@ -2455,7 +2455,8 @@ static int chcr_authenc_setkey(struct cr
>> 1.4 base_hash = chcr_alloc_shash(max_authsize);
>> 1.5 if (IS_ERR(base_hash)) {
>> 1.6 pr_err("chcr : Base driver cannot be loaded\n");
>> 1.7 - goto out;
>> 1.8 + aeadctx->enckey_len = 0;
>> 1.9 + return -EINVAL;
> Don't do that. There should be a goto.
>
> regards,
> dan carpenter
>
>
Agreed.
Having direct return after some other gotos statement puzzles my
coccinelle scripts and are spurious (at least IMHO).
best regards,
CJ