Protect Crypspr initialisation with a mutex

[oviano]

Multiple threads can be initialising the SPR at one time. This is a particular problem with the MbedTLS implementation as there is some additional static initialisation performed for this SPR which can lead to a crash.

[jeandube]

Serious? turning CRYSPR to C++ to use sync. Sorry I cannot review this. I am biased, being 100% with Linus Torvalds about C++, and then incompetent to approve or reject.

[oviano]

You'd better get rid of all the other C++ files in your library then... :o)

Seriously, if you have an alternative suggestion to a mutex in that piece of code, then I'm all ears.

[ethouris]

@jeandube : there wouldn't be a problem with turning scoped locking into explicit lock/unlock calls, but the problem is with a lazy initialization of the global-static mutex object; nice if you can find some solution for it. Hmm... pthread_setspecific, or whatever it was...?

@oviano : what you wrote (with the static mutex) might be likely not a problem in C++11, but SRT is allowed also in C++03, which knows nothing about threads, and therefore there are concerns as to whether a lazy initialization of a local static variable in a multithreaded environment can be safely done in this standard. Likely not.

[oviano]

Ah ok, I did search for another precedent in the code actually and found a similar example of a static srt::sync::mutex in the function srt::sync::genRandomInt.

So I figured it was ok, but I understand.

[ethouris]

Yes, but I don't think you read the comment in this function ;D

[oviano]

No, I didn't read it properly, you're right.

However, I still don't fully understand - shouldn't that mutex in genRandomInt be inside the HAVE_CX11 then if it's useless in earlier versions?

Anyway, I don't really know the best solution, just that you need some sort of sync to prevent the issues I was getting before I added the mutex.

[ethouris]

The current situation is that in C++11 you can use the static variables with no problems. In C++03, and in C, there's no good method for this; the global variables get then initialization through the srt_startup() call.

The problem is, even in cryspr this must be done portable way and include the C++11 version of sync. So, there could be simply a global variable for a mutex like this, done appropriate way for particular standard (check the sync version flag, not just C++11), and expose only functions for locking and unlocking it, as extern "C" so that they can be used in cryspr, plus a startup/cleanup functions that will be called from srt_startup() and srt_cleanup() calls. This way only this global init mutex wrapper will have to be C++ and the existing file may remain C.

[oviano]

I am thinking of a slight variation to the above:

• revert cryspr-xxx files to .c with no changes from the originals
• add static void globalInit(void) and static void globalCleanup(void) functions to CCryptoControl class
• add static srt::sync::Mutex* s_HaiCryptCrysprInstLock to CCryptoControl class, created and deleted by above functions
• call these functions from srt::CUDTUnited::startup() and srt::CUDTUnited::cleanup() (analogous to how PacketFilter::globalInit is called from there)
• protect call to HaiCryptCryspr_Get_Instance as below

    {
        srt::sync::ScopedLock lck(*s_HaiCryptCrysprInstLock);
        crypto_cfg.cryspr = HaiCryptCryspr_Get_Instance();
    }

The advantage is it encapsulates the change into just the CCryptoControl class, and we don't need any extern global variables etc. Just the two function calls to init and cleanup.

Does that tick all the boxes, or have I missed another flaw...?

[ethouris]

I don't exactly like it by one reason - you are locking way more execution steps than needed. But on the other hand, the frequency of the call to createCryptoCtx function is tied to creating connections, so it shouldn't be much of an overall performance burden.

You don't have to create the whole big facility for a global variable. The CUDTUnited class is intended to keep all global variables of SRT, so you can place the mutex there, and appropriate initializations can be added to the existing ones. You can access this class through the CUDT::uglobal() method, and the CUDT object is written to m_parent field in the constructor.

[oviano]

Ok, makes sense.

In terms of execution steps, do you mean the extra steps due to moving it up a level into CCyptoControl where it calls the function, or do think even as it was before with the first proposal it was locking too much?

Two threads calling this function at the same time is a problem, they'd both be accessing the same static data.

CRYSPR_methods *crysprMbedtls(void)
{
    if (crysprMbedtls_methods.open)
        return(&crysprMbedtls_methods);

    crysprInit(&crysprMbedtls_methods); /* Set default methods */

    /* CryptoLib Primitive API */
    crysprMbedtls_methods.prng           = crysprMbedtls_Prng;
    crysprMbedtls_methods.aes_set_key    = crysprMbedtls_AES_SetKey;
#if CRYSPR_HAS_AESCTR
    crysprMbedtls_methods.aes_ctr_cipher = crysprMbedtls_AES_CtrCipher;
#endif
#if !(CRYSPR_HAS_AESCTR && CRYSPR_HAS_AESKWRAP)
    /* AES-ECB only required if cryspr has no AES-CTR or no AES KeyWrap */
    crysprMbedtls_methods.aes_ecb_cipher = crysprMbedtls_AES_EcbCipher;
#endif
#if !CRYSPR_HAS_PBKDF2
    crysprMbedtls_methods.sha1_msg_digest= crysprMbedtls_SHA1_MsgDigest; //Onl required if using generic KmPbkdf2
#endif

    //--Crypto Session (Top API)
#ifdef CRYSPR2
        crysprMbedtls_methods.open     = crysprMbedtls_Open;
        crysprMbedtls_methods.close    = crysprMbedtls_Close;
#else
    //  crysprMbedtls_methods.open     =
    //  crysprMbedtls_methods.close    =
#endif
    //--Keying material (km) encryption
    crysprMbedtls_methods.km_pbkdf2  = crysprMbedtls_KmPbkdf2;
    //	crysprMbedtls_methods.km_setkey  =
    //  crysprMbedtls_methods.km_wrap    =
    //  crysprMbedtls_methods.km_unwrap  =
    //--Media stream (ms) encryption
    //  crysprMbedtls_methods.ms_setkey  =
    //	crysprMbedtls_methods.ms_encrypt =
    //	crysprMbedtls_methods.ms_decrypt =

    // Initialize extra static data
    mbedtls_entropy_init( &crysprMbedtls_entropy );
    mbedtls_ctr_drbg_init( &crysprMbedtls_ctr_drbg );

    int ret;
    if ( (ret = mbedtls_ctr_drbg_seed( &crysprMbedtls_ctr_drbg, mbedtls_entropy_func,
                    &crysprMbedtls_entropy, NULL, 0)) != 0 )
    {
        HCRYPT_LOG(LOG_CRIT, "crysprMbedtls: STATIC INIT FAILED on mbedtls_ctr_drbg_init: -0x%04x", -ret);
        return NULL;
    }

    return(&crysprMbedtls_methods);
}

[ethouris]

Right, but the lock at createCryptoCtx locks much more than this. But I think it doesn't matter that much and should be acceptable. @maxsharabayko what do you think?

[oviano]

It’s just locking the call to receive the instance, which either just returns it or initialises it?

Anyway I’ll update the PR, as maybe it’s not clear from my description.

[maxsharabayko]

I am thinking of a slight variation to the above:

• revert cryspr-xxx files to .c with no changes from the originals
  ...
• call these functions from srt::CUDTUnited::startup() and srt::CUDTUnited::cleanup() (analogous to how PacketFilter::globalInit is called from there)
  ...

I like this direction most. I guess HaiCryptCryspr_Get_Instance() can just be placed in the srt::CUDTUnited::startup() without any mutex. HaiCryptCryspr_Get_Instance() initializes the crypto library, and thus it can be used later on.

[oviano]

Even simpler - but are you happy to initialise it even though it might not be used?

[ethouris]

Wait - so the problem wasn't about simultaneous access to the statically shared data when creating new crypter context, but with the lazy global initialization of the internal data of mbedtls? If so, then of course, it should be done in srt_startup.

[oviano]

So basically you guys need to decide if it’s:

A. Global mutex, accessed by crypto to lock getting instance. Crypto only initialised when first used. Small mutex overhead.

B. Startup code calls crypto initialisation (calls get instance). No mutex required but initialisation code always runs regardless of whether crypto ever required.

Happy to change it to B, let me know.

[oviano]

Wait - so the problem wasn't about simultaneous access to the statically shared data when creating new crypter context, but with the lazy global initialization of the internal data of mbedtls? If so, then of course, it should be done in srt_startup.

My last comment here #2412 explained the issue as best as I could.

[ethouris]

If B really solves the problem, this is the best solution. I don't think it's costly to initialize the encryption library, even if the application doesn't use encryption (unless SRT is compiled with disabled encryption, see conditional macros).

Of course, @maxsharabayko please confirm if so.

[maxsharabayko]

I vote for "B. Startup code calls crypto initialisation (calls get instance). No mutex required but initialisation code always runs regardless of whether crypto ever required."

I even think it makes much more sense to initialize the crypto library at the startup, to not put a performance burden on the connection establishment.

@jeandube should also be pleased with this solution 😄

[oviano]

Ok, I've changed it to B.

I have placed the call to HaiCryptCryspr_Get_Instance inside a CCryptoControl::globalInit() function as this seemed slightly cleaner as 1) it is the class that normally calls the HaiCryptCryspr_Get_Instance 2) it leaves a logical place for any future further initializations required.

But if you prefer to call HaiCryptCryspr_Get_Instance directly from startup then I can change it.

Main thing is no mutex, and any Linus fans aren't offended...

[ethouris]

Unsure as to whether any further global init would be required in the future, but still, it's cleaner to have a CCryptoControl::globalInit anyway.

This still won't stop the Linux kernel from being written in C++ some day. ;)