Re: Should the return result of CRYPTO_UP_REF() / CRYPTO_DOWN_REF() be checked?

2020-02-10 Thread Bernd Edlinger
On 2/10/20 6:29 PM, Kurt Roeckx wrote:
> On Mon, Feb 10, 2020 at 04:19:20PM +, Matt Caswell wrote:
>>
>>
>> On 10/02/2020 00:15, SHANE LONTIS wrote:
>>> With the new architecture changes there are quite a few new calls to
>>>
>>> CRYPTO_UP_REF()
>>> CRYPTO_DOWN_REF()
>>>
>>> These methods return an int that is not being checked in lots of places.
>>>
>>> This return value only seems to affect fallback code that calls 
>>> CRYPTO_atomic_add (which can return 0 on lock or unlock failure)
>>>
>>> SO the question is should we be checking this return value?
>>
>> Yes, I think we should be.
> 
> I think that as long as we have that fallback code, that it should
> be checked.
> 
> 

Yes, although I wonder why a function that checks
the return value of CRYPTO_THREAD_write_lock and
CRYPTO_THREAD_unlock does not check for
possible overflow of the addition, which is
far more likely to happen.


Bernd.


Re: Should the return result of CRYPTO_UP_REF() / CRYPTO_DOWN_REF() be checked?

2020-02-10 Thread Kurt Roeckx
On Mon, Feb 10, 2020 at 04:19:20PM +, Matt Caswell wrote:
> 
> 
> On 10/02/2020 00:15, SHANE LONTIS wrote:
> > With the new architecture changes there are quite a few new calls to
> > 
> > CRYPTO_UP_REF()
> > CRYPTO_DOWN_REF()
> > 
> > These methods return an int that is not being checked in lots of places.
> > 
> > This return value only seems to affect fallback code that calls 
> > CRYPTO_atomic_add (which can return 0 on lock or unlock failure)
> > 
> > SO the question is should we be checking this return value?
> 
> Yes, I think we should be.

I think that as long as we have that fallback code, that it should
be checked.


Kurt



Re: Should the return result of CRYPTO_UP_REF() / CRYPTO_DOWN_REF() be checked?

2020-02-10 Thread Matt Caswell



On 10/02/2020 00:15, SHANE LONTIS wrote:
> With the new architecture changes there are quite a few new calls to
> 
> CRYPTO_UP_REF()
> CRYPTO_DOWN_REF()
> 
> These methods return an int that is not being checked in lots of places.
> 
> This return value only seems to affect fallback code that calls 
> CRYPTO_atomic_add (which can return 0 on lock or unlock failure)
> 
> SO the question is should we be checking this return value?

Yes, I think we should be.

Matt

> 
> 
> Note that not checking has resulted in a few assumptions in other code…
> e.g the following function returns void.
>  
> /crypto/evp/keymgmt_lib.c: 165 in evp_keymgmt_util_cache_pkey()
> 159 }
> 160 
> 161 void evp_keymgmt_util_cache_pkey(EVP_PKEY *pk, size_t index,
> 162  EVP_KEYMGMT *keymgmt, void *keydata)
> 163 {
> 164 if (keydata != NULL) {
CID 1458170:  Error handling issues  (CHECKED_RETURN)
Calling "EVP_KEYMGMT_up_ref" without checking return value (as is done 
 elsewhere 4 out of 5 times).
> 165 EVP_KEYMGMT_up_ref(keymgmt);
> 
> NOTE: EVP_KEYMGMT_up_ref() just does an CRYPTO_UP_REF() call and always 
> returns 1.
> 
> 


Should the return result of CRYPTO_UP_REF() / CRYPTO_DOWN_REF() be checked?

2020-02-09 Thread SHANE LONTIS
With the new architecture changes there are quite a few new calls to

CRYPTO_UP_REF()
CRYPTO_DOWN_REF()

These methods return an int that is not being checked in lots of places.

This return value only seems to affect fallback code that calls 
CRYPTO_atomic_add (which can return 0 on lock or unlock failure)

SO the question is should we be checking this return value?


Note that not checking has resulted in a few assumptions in other code…
e.g the following function returns void.
 
/crypto/evp/keymgmt_lib.c: 165 in evp_keymgmt_util_cache_pkey()
159 }
160 
161 void evp_keymgmt_util_cache_pkey(EVP_PKEY *pk, size_t index,
162  EVP_KEYMGMT *keymgmt, void *keydata)
163 {
164 if (keydata != NULL) {
>>>CID 1458170:  Error handling issues  (CHECKED_RETURN)
>>>Calling "EVP_KEYMGMT_up_ref" without checking return value (as is done 
>>> elsewhere 4 out of 5 times).
165 EVP_KEYMGMT_up_ref(keymgmt);

NOTE: EVP_KEYMGMT_up_ref() just does an CRYPTO_UP_REF() call and always returns 
1.