Release the DRBG mutex when generation fails

[EdouardMALOT]

Summary

_nx_crypto_drbg() takes NX_CRYPTO_DRBG_MUTEX_GET at the top, but returns early when _nx_crypto_drbg_generate() fails, so the mutex is never released.

Both macros are empty by default and nothing in the repository defines them, so a default build is unaffected. Ports that do define them get a permanent deadlock instead of a returned error.

The failure path is not obscure. _nx_crypto_drbg_generate() has three failure returns, but two are NX_CRYPTO_SIZE_ERROR on additional_input_len, which _nx_crypto_drbg() always passes as 0 — unreachable from this caller. The third, NX_CRYPTO_NO_INSTANCE (nx_crypto_drbg.c:509-512), fires exactly when _nx_crypto_drbg_initialize() on the line above failed. So the leak is on the one error this caller can actually produce, driven by the ignored return value right above it.

It also matters where this sits: in NX_CRYPTO_SELF_TEST builds _nx_crypto_drbg() is NX_CRYPTO_RBG (nx_crypto.h:92), so on a port that defines the mutex macros, this deadlocks the generator behind EC key generation and ECDSA signing (nx_crypto_ec.c:2945 and :4671) — the RNG for the whole FIPS configuration, not a peripheral path.

Found while working on #399.

Changes

• crypto_libraries/src/nx_crypto_drbg.c — move NX_CRYPTO_DRBG_MUTEX_PUT to just after _nx_crypto_drbg_generate(), before the status check. This also leaves the masking of the extra bits outside the critical section, which is fine since it only touches the caller's buffer.

The ignored _nx_crypto_drbg_initialize() return value on the line above is a separate question: after this fix, an initialisation failure already propagates correctly (NX_CRYPTO_NO_INSTANCE, mutex released) via _nx_crypto_drbg_generate()'s check, so there's no defect left there — only a readability improvement to surface the real error code sooner. Leaving it for a follow-up.

Test plan

• No behaviour change in a default build, where both macros expand to nothing
• Compiles clean under -Wall -Wextra -Wconversion
• Expanded both macros to function calls and checked there is exactly one get and one put, with the put now unconditional
• Not exercised at runtime: I have no port here that defines the mutex macros, so the deadlock itself is reasoned from the code rather than reproduced

[fdesbiens]

Thank you — this is a clean find and a clean fix, and the test plan is honest in a way I wish more were: leaving the runtime box unchecked with the reason stated is exactly right, and it told me precisely which claim to check myself.

Everything verifies:

The leak is real and there is only one of it. NX_CRYPTO_DRBG_MUTEX_GET at :1169, _nx_crypto_drbg_generate() at :1176, and on dev the if (status) return(status); sits above the release, so that return abandons the mutex. I swept the whole file: :1169 is the only acquire and :1182 the only release, both inside _nx_crypto_drbg(), so there is no second imbalance hiding elsewhere and your one-line move closes it completely.

The macros are as you describe. crypto_libraries/inc/nx_crypto_drbg.h:79-84 defines both as empty under #ifndef, so a port can supply real ones, and nothing in the repository does. A default build is unaffected, and your #ifndef reasoning about ports is right.

The placement is the better of the two options. Moving the single release above the status check is preferable to adding a second one on the error path, which would have left two call sites to keep in step. And leaving the bit masking outside the critical section is safe: it reads bits, computes mask and writes result[0], all local or caller-owned, with no DRBG state involved.

Worth knowing how reachable this is, since your description says only that ports which define the macros "get a permanent deadlock instead of a returned error" — which undersells it in one respect and is worth stating in the other. In NX_CRYPTO_SELF_TEST builds _nx_crypto_drbg() is NX_CRYPTO_RBG (nx_crypto.h:92), so on a port that defines the mutex macros this deadlocks the generator behind EC key generation and ECDSA signing (nx_crypto_ec.c:2945 and :4671). That is the RNG for the whole FIPS configuration, not a peripheral path.

And on the trigger — see my comment on line 1178.

[fdesbiens]

I went looking for how often _nx_crypto_drbg_generate() actually fails, on the assumption that a rarely-taken error path would make this mostly theoretical. It is the opposite, and the two things you mention in the description turn out to be the same story.

_nx_crypto_drbg_generate() has three failure returns. Two are NX_CRYPTO_SIZE_ERROR on additional_input_len, and _nx_crypto_drbg() passes 0 for that, so neither is reachable from this caller. The third is at :509-512:

    if (!drbg_ptr -> nx_crypto_drbg_instantiated)
    {
        return(NX_CRYPTO_NO_INSTANCE);
    }

And nx_crypto_drbg_instantiated is false at that point in exactly one circumstance: _nx_crypto_drbg_initialize() on the line above failed, which is the ignored return value you noticed.

So the leak is not on an obscure error path. It is on the one failure this caller can actually produce, and the thing that produces it is the unchecked call immediately above. Saying that in the commit message would make the change read as the targeted fix it is rather than as defensive tidying.

It also means your fix already handles the consequence of the ignored return: with the release moved, an initialisation failure now propagates NX_CRYPTO_NO_INSTANCE to the caller with the mutex released, which is correct behaviour.

[fdesbiens]

You asked whether to fold the ignored _nx_crypto_drbg_initialize() return into this PR. My answer is no, and the reason is that after your change the behaviour is already correct.

If initialisation fails, nx_crypto_drbg_instantiated stays false, _nx_crypto_drbg_generate() returns NX_CRYPTO_NO_INSTANCE, and with the release relocated the caller gets that error with the mutex free. There is no defect left — only indirection, in that the failure is detected one call later than it occurs and reported as a different code than _nx_crypto_drbg_initialize() would have returned.

Checking it explicitly would be a readability improvement rather than a bug fix:

    if (!_nx_crypto_drbg_ctx.nx_crypto_drbg_instantiated)
    {
        status = _nx_crypto_drbg_initialize();
        if (status)
        {
            NX_CRYPTO_DRBG_MUTEX_PUT;
            return(status);
        }
    }

That surfaces the real error code and makes the dependency local instead of relying on generate() to catch it. Worth doing, but as its own change — this PR is a memory-safety fix with a one-line diff and a clear story, and I would rather not widen it. Open a follow-up and I will review it.

[fdesbiens]

I have asked for regression tests on most PRs in this batch, so it is worth explaining the exception rather than leaving it as an inconsistency.

There is no configuration the project builds in which this change is observable. Both macros expand to nothing everywhere in-tree, so a test compiled against any existing build would pass identically before and after — it would be coverage in name only. Reproducing the deadlock needs both a port that defines the macros and an induced initialisation failure, neither of which exists here.

The one thing that would have caught this, and would catch the next one, is a translation unit that defines the two macros as increment and decrement of a counter and asserts the count returns to zero. That is a real option if you think the macro pair is likely to grow more call sites, but with exactly one acquire and one release in the file today I do not think it earns its keep, and your test-plan item confirming one get and one put by expanding the macros achieves the same thing by inspection.

Your fourth checkbox is the right way to have handled the runtime gap, and I would rather have that than a test that proves nothing.

[EdouardMALOT]

@fdesbiens description updated