bugfix: Mk3 Detect RNG_SR_SEIS and RNG_SR_SECS, retry safely, and fail closed on persistent faults.

[scgbckbone]

• added forgotten changelog

[scgbckbone]

same as https://github.com/Coldcard/firmware/pull/693/changes but for Mk3

[scgbckbone]

#695

[scgbckbone]

accidentally closed

[veerapatyok]

hi,
in my software engineer experience and i do not that much in embedded world.
I have some question why you use while (1) in line 65 file rng.c in the future it should accidentally to infinite loop some reason i think it should be

// in retry for loop
uint32_t sr;
while(!((sr = RNG->SR) & RNG_SEED_ERROR_MASK)) {
    if(!(sr & RNG_FLAG_DRDY)) {
        // Missing clocks are a hard failure. Preserve the existing
        // fail-closed behaviour and wait rather than use bad data.
        continue;
    }

    uint32_t rv = RNG->DR;

    // Recheck after reading DR to close the documented polling race.
    if(RNG->SR & RNG_SEED_ERROR_MASK) {
        break;
    }

    if(rv != last_rng_result && rv) {
        last_rng_result = rv;
        return rv;
    }

    // Zero or repeat: poll for another word without consuming an attempt.
}

if(attempt + 1 < RNG_MAX_ATTEMPTS) {
    rng_recover();
}

sorry for waste your time

[veerapatyok]

it should be

// Make one bounded attempt to obtain a trustworthy, non-zero word.
static bool rng_try_once(uint32_t *value)
{
    uint32_t start = HAL_GetTick();
    uint32_t sr;

    // Seed errors can suppress DRDY, so check for them while polling.
    while (!((sr = RNG->SR) & RNG_SR_DRDY)) {
        if (sr & RNG_SEED_ERROR_MASK) {
            return false;
        }

        if (HAL_GetTick() - start >= RNG_TIMEOUT_MS) {
            return false;
        }
    }

    uint32_t sample = RNG->DR;

    // Recheck after reading DR to close the polling race; zero is also suspect.
    if (!sample || (RNG->SR & RNG_SEED_ERROR_MASK)) {
        return false;
    }

    *value = sample;
    return true;
}

[scgbckbone]

hi, in my software engineer experience and i do not that much in embedded world. I have some question why you use while (1) in line 65 file rng.c in the future it should accidentally to infinite loop some reason i think it should be

// in retry for loop
uint32_t sr;
while(!((sr = RNG->SR) & RNG_SEED_ERROR_MASK)) {
    if(!(sr & RNG_FLAG_DRDY)) {
        // Missing clocks are a hard failure. Preserve the existing
        // fail-closed behaviour and wait rather than use bad data.
        continue;
    }

    uint32_t rv = RNG->DR;

    // Recheck after reading DR to close the documented polling race.
    if(RNG->SR & RNG_SEED_ERROR_MASK) {
        break;
    }

    if(rv != last_rng_result && rv) {
        last_rng_result = rv;
        return rv;
    }

    // Zero or repeat: poll for another word without consuming an attempt.
}

if(attempt + 1 < RNG_MAX_ATTEMPTS) {
    rng_recover();
}

sorry for waste your time

thanks for review @veerapatyok!

Current loop is already bounded by timeout. your version checks data-ready first, so if data-ready and a seed error are both set, it reads the random register before noticing the error. current ordering avoids that. the bootloader loop is intentionally unbounded to fail closed, so i’d keep it as-is.