STM32L4: fix byte-swapped IV writeback in mbedTLS AES-CBC decrypt

[61ca52]

Summary of changes

mbedtls_aes_crypt_cbc() in connectivity/drivers/mbedtls/TARGET_STM/aes_alt_stm32l4.c saves the chained IV for the next call by reading the AES peripherals IVR3 through IVR0 registers directly. The STM HAL writes those registers byte-reversed relative to the callers IV buffer, but the read back does not reverse them again. The saved IV is then byte-swapped within each 32-bit word.

Therefore, any CBC decryption split across more than one call to the mbedtls_aes_crypt_cbc() function returns wrong plaintext. This issue can be resolved by applying __REV() to the readback, ensuring that the write and read paths are consistent.

For further clarification, here is how the L4 HAL writes to and reads from the IVR3 through IVR0 registers:

mbed-os/targets/TARGET_STM/TARGET_STM32L4/STM32Cube_FW/STM32L4xx_HAL_Driver/stm32l4xx_hal_cryp.c:1586-1592

    hcryp->Instance->IVR3 = __REV(*(uint32_t*)(ivaddr));
    ivaddr+=4U;
    hcryp->Instance->IVR2 = __REV(*(uint32_t*)(ivaddr));
    ivaddr+=4U;
    hcryp->Instance->IVR1 = __REV(*(uint32_t*)(ivaddr));
    ivaddr+=4U;
    hcryp->Instance->IVR0 = __REV(*(uint32_t*)(ivaddr));

mbed-os/targets/TARGET_STM/TARGET_STM32L4/STM32Cube_FW/STM32L4xx_HAL_Driver/stm32l4xx_hal_cryp_ex.c:1865-1875

void HAL_CRYPEx_Read_IVRegisters(CRYP_HandleTypeDef *hcryp, uint8_t* Output)
{
  uint32_t outputaddr = (uint32_t)Output;

  *(uint32_t*)(outputaddr) = __REV(hcryp->Instance->IVR3);
  outputaddr+=4U;
  *(uint32_t*)(outputaddr) = __REV(hcryp->Instance->IVR2);
  outputaddr+=4U;
  *(uint32_t*)(outputaddr) = __REV(hcryp->Instance->IVR1);
  outputaddr+=4U;
  *(uint32_t*)(outputaddr) = __REV(hcryp->Instance->IVR0);
}

Impact of changes

This will only affect decryption on STM32L4 devices. The encrypt branch already saves the IV correctly, by copying the last ciphertext block out of the output buffer memcpy(iv, output, 16), so it never touches the IVR registers and is unaffected. And what makes this so difficult to catch it only affects chained or streaming use. A single self-contained call never consumes the IV it wrote back. So the corruption is partial, only the first 16-byte block of each follow-up call is wrong. Later blocks in that call chain correctly off the input ciphertext, so the failure looks like intermittent or partial corruption and not like a clean break.

All other STM32 devices are out of scope and rely on aes_alt.c.

Migration actions required

Documentation

---

Pull request type

[X] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

---

Test results

[] No Tests required for this change (E.g docs only update)
[] Covered by existing mbed-os tests (Greentea or Unittest)
[X] Tests / results supplied as part of this PR

As there is no greentea test for this, it was verified using the mbed TLS AES self-test function (mbedtls_aes_self_test(1);) and a NUCLEO-L4A6ZG.

Output before and after:

 AES-ECB-128 (dec): passed
  AES-ECB-128 (enc): passed
  AES-ECB-192 (dec): skipped
  AES-ECB-192 (enc): skipped
  AES-ECB-256 (dec): passed
  AES-ECB-256 (enc): passed

  AES-CBC-128 (dec): failed

  AES-ECB-128 (dec): passed
  AES-ECB-128 (enc): passed
  AES-ECB-192 (dec): skipped
  AES-ECB-192 (enc): skipped
  AES-ECB-256 (dec): passed
  AES-ECB-256 (enc): passed

  AES-CBC-128 (dec): passed
  AES-CBC-128 (enc): passed
  AES-CBC-192 (dec): skipped
  AES-CBC-192 (enc): skipped
  AES-CBC-256 (dec): passed
  AES-CBC-256 (enc): passed

  AES-CTR-128 (dec): passed
  AES-CTR-128 (enc): passed
  AES-CTR-128 (dec): passed
  AES-CTR-128 (enc): passed
  AES-CTR-128 (dec): passed
  AES-CTR-128 (enc): passed

---

[multiplemonomials]

Nice find on this!

Do you think it would be possible to add a greentea test that reproduces the issue, either in this PR or in a subsequent one? I'd rather have a regression test for this type of issue on other targets if possible.

[multiplemonomials]

Also, by the way, we really appreciate all the fixes you've contributed over the last few weeks! If you are interested in contributing to Mbed CE long-term and in having a say in future development, we'd love to invite you to our Discord server and add you to the GitHub organization. Sound good?

[61ca52]

Thanks, and that request turned out to be a good one, because writing the test immediately found a second, independent defect in the same function.

The now to this PR added regression test (connectivity/mbedtls/tests/TESTS/mbedtls/aes_cbc/), is plain mbedTLS API level, so it is equally valid on software-only targets and should be useful as general coverage rather than be STM32L4 specific.

With it I found that the __REV fix in this PR was correct but incomplete. The encrypt path of the same function saved output[0..15], the first ciphertext block of the call, where CBC requires the last:

/* current output is the IV vector for the next encrypt */
memcpy(iv, output, 16);

Reproduced to the byte against the values of the appendix under F.2.1 NIST SP 800-38A (https://nvlpubs.nist.gov/nistpubs/Legacy/SP/nistspecialpublication800-38a.pdf) with a 48+16 split, byte 48 of the ciphertext comes out 0x29 instead of the expected 0xb2, and everything from there on is wrong. Blocks 1 to 3 are perfect, which is why nothing downstream ever pointed at the encrypt path.

I assume that both defects survived this long since mbedtls_aes_self_test() calls mbedtls_aes_crypt_cbc() with length hardcoded to 16, 10000 times, but always a single block. At one block the first and last ciphertext block are the same address, so the only CBC coverage in the tree is structurally blind to this entire class of bug. The self-test passing was never evidence for or against either fix.

The split shape decides detection, and the two defects have different profiles. The __REV decrypt bug corrupts the saved IV unconditionally and so is caught by any split with two or more calls, but the encrypt first/last mix-up is only observable when a multi-block call is followed by another call, which means 48+16, 32+32 and 16+32+16 catch it while 64, 16+48 and 16+16+16+16 all pass with the bug present. A sweep that only uses single-block chunks, or that never puts a multi-block call in front of another call, is exactly the shape that lets it through. The first draft of this test caught it only by luck of which split had been assigned to which case, and that hole is closed by sweeping all six splits against both directions and both key sizes.

Beyond the output comparison, the suite also asserts the documented contract directly, that on return iv[] holds the last ciphertext block the call touched, so a failure is reported on the offending call rather than one call later as corrupted output. There is also a zero-length no-op case and an interleaved-contexts case (two live contexts, different key sizes, alternating block by block), which is the scenario the saved IV exists for in the first place.

Run on three boards:

• NUCLEO_L4A6ZG, STM32L4 hardware AES, with both fixes applied
• NUCLEO_L552ZE_Q, falls back to software mbedTLS
• EFM32GG_STK3700, Silicon Labs hardware AES

greentea-log-l4a6-aes-cbc.txt
greentea-log-l552-aes-cbc.txt
greentea-log-stk3700-aes-cbc.txt

And other targets seems to be affected by the encrypt defect as well!

connectivity/drivers/mbedtls/TARGET_STM/aes_alt.cpp, the other ST driver, used by F4/F7/L562/WB, has the same mix-up:

mbed-os/connectivity/drivers/mbedtls/TARGET_STM/aes_alt.cpp:457-469

    } else {
        if (HAL_CRYP_Encrypt(&ctx->hcryp_aes,
                             (uint32_t *)input,
                             length,
                             (uint32_t *)output,
                             ST_CRYP_TIMEOUT) != HAL_OK) {
            ret = MBEDTLS_ERR_PLATFORM_HW_ACCEL_FAILED;
            goto exit;
        }

        /* current output is the IV vector for the next encrypt */
        memcpy(iv, output, 16);
    }

Note this is a different decrypt bug from the L4 one. aes_alt.cpp never touches the IV registers, so it is not an endianness problem, it is the same first/last mix-up, and it is masked by the same splits as the encrypt case rather than failing everywhere.

The same pattern is also present in STs current Cube examples, for instance STM32CubeH7 Crypto_Selftest/Src/aes_alt.c, so it is not something that was introduced downstream here.

This is code reading only, as I do not own any of the affected ST boards. The only other ST part I have is a U585I, and the crypto support for it is currently missing in mbed-ce, so it does not reach this driver. If anyone with a DISCO_L562QE, NUCLEO_WB55RG or any F4/F7 board can flash this test suite against current master, that would confirm it in a few seconds. I would then send the aes_alt.cpp fix as its own PR rather than folding a second file and a third defect into this one.

To be sure, since I am a little bit surprised that this has survived this long and is still present in the STM ecosystem, I looked at implementations of other vendors. The Silicon Labs driver implements exactly the corrected pattern, written independently:

mbed-os/connectivity/drivers/mbedtls/TARGET_Silicon_Labs/aes_aes.c:247-253

    if ( length >= 16 )
    {
        if ( mode == MBEDTLS_AES_ENCRYPT )
            memcpy( iv, &output[length-16], 16 );
        else
            memcpy( iv, tmpIv, 16 );
    }

Two things are worth noting there. The decrypt IV is saved before the operation, which is necessary since input may alias output, and both branches are guarded by length >= 16. That guard is why the zero-length no-op case is part of this PRs fix as well: the existing if (length % 16) check accepts a length of zero, and with the writeback in place that is a 16-byte read past the end of the buffer.

For the EFM32GG_STK3700 target i needed to add "device_has_remove": [ "TRNG" ] to the target definition otherwise the test would fail to build.

---

Yes, I would like to contribute to mbed-ce in the long term. However, I cannot guarantee fix regularity as I will only be able to contribute as much time as my free time allows. However, I would be happy to do so and would like to be invited.

[multiplemonomials]

Sounds good, what's your Discord username?

[multiplemonomials]

Hmm, fwiw, there is an easier way to do this with the Greentea test framework. You could instead define run_all_splits like:

template<int mode, unsigned char *key, unsigned int keybits, const unsigned char *input, const unsigned char *expected>
static void run_all_splits()
{
    ...

and then in the test cases block have:

Case("AES-128-CBC (enc) call all splits", run_all_splits<MBEDTLS_AES_ENCRYPT, AES128_KEY, 128, PLAINTEXT, AES128_CIPHERTEXT>),

I'd say this is the more idiomatic way to do multiple different test cases that use the same test code with different parameters. But it also works fine the way it is now so up to you.

[multiplemonomials]

Also wait you have a EFM32GG_STK3700 to test with?? That's been on my list to try but it's a bit pricey. I am glad to hear that Mbed CE still actually works on that target, and if you have time, I'd appreciate if you could contribute an upload method file for it!

[multiplemonomials]

I do have a NUCLEO_WB55RG to test with but I am in the middle of moving and it's packed away. I trust you to make the fix for now, and I will run the tests on it when I get a chance in a month or so.

[multiplemonomials]

Can you add a copyright (you can copyright it to yourself) and an SPDX-License-Identifier here? That's why CI is failing.