Refactor AES context to be shallow-copyable

[wernerlewis]

Description

Replaces use of a round key pointer in mbedtls_aes_context with a buffer offset, so that the structure is shallow-copyable. This retains support for alignment requirements with PadLock, removal of rk results in an ABI change. A test is added to confirm a shallow-copy of the structure is valid. Fixes #2147.

Status

READY

Requires Backporting

NO

Todos

• Tests
• Changelog updated

Steps to test or reproduce

Initialise an mbedtls_aes_context structure, set encryption keys and then encrypt plaintext. Shallow-copy and then zero the original context, and then repeat encryption of the same plaintext using the copied context. The two ciphertexts should match, but will not prior to this change.
A similar test has been added in this PR.

[gilles-peskine-arm]

Looks mostly good, just a couple of minor points.

[gilles-peskine-arm]

This line is not indented correctly. The indentation should reflect the fact that ctx->rk_offset = 0; is not executed if aes_padlock_ace is nonzero (whereas the assignment to RK is unconditional).

When we have an interaction between preprocessor nesting and instruction nesting, I prefer to arrange for the indentation to be correct if you hide the disabled preprocessor directives. For if/else, with our brace style, this can be done by putting the “either else or sole code path” line in braces:

#if defined(MBEDTLS_PADLOCK_C) && defined(MBEDTLS_PADLOCK_ALIGN16)
    if( aes_padlock_ace == -1 )
        aes_padlock_ace = mbedtls_padlock_has_support( MBEDTLS_PADLOCK_ACE );

    if( aes_padlock_ace )
        ctx->rk_offset = MBEDTLS_PADLOCK_ALIGN16( ctx->buf ) - ctx->buf;
    else
#endif
    {
        ctx->rk_offset = 0;
    }
    RK = ctx->buf + ctx->rk_offset;

But in fact I would find this code more readable as

    ctx->rk_offset = 0;
#if defined(MBEDTLS_PADLOCK_C) && defined(MBEDTLS_PADLOCK_ALIGN16)
    if( aes_padlock_ace == -1 )
        aes_padlock_ace = mbedtls_padlock_has_support( MBEDTLS_PADLOCK_ACE );

    if( aes_padlock_ace )
        ctx->rk_offset = MBEDTLS_PADLOCK_ALIGN16( ctx->buf ) - ctx->buf;
#endif
    RK = ctx->buf + ctx->rk_offset;

Same thing in setkey_dec.

[wernerlewis]

Thanks for the information on indentation in this case, I've updated to use the latter formatting.

[gilles-peskine-arm]

Please document the unit. Both bytes and array elements (which is uint32_t) are plausible. Turns out the offset is a number of array elements.

[AndrzejKurek]

I'm not sure if I follow the logic here - isn't this just a 16-byte alignment offset kept along an unaligned buffer, so bytes?

[tom-cosgrove-arm]

No, rk_offset is the difference between two uint32_t *s. I agree the name MBEDTLS_PADLOCK_ALIGN16 doesn't make it clear, but it includes a cast to uint32_t *.

See

library/padlock.h:#define MBEDTLS_PADLOCK_ALIGN16(x) (uint32_t *) (16 + ((int32_t) (x) & ~15))

library/aes.c:ctx->rk_offset = MBEDTLS_PADLOCK_ALIGN16( ctx->buf ) - ctx->buf;

and

library/aes.c:RK = ctx->buf + ctx->rk_offset;

[AndrzejKurek]

OK, but even though this is a difference between two uint32_t's, this contains an address offset aligned to 16 bytes, and the result of MBEDTLS_PADLOCK_ALIGN16( buf ) - buf can be at most 16.
For uint32_t elements, this means that it's at most an offset of half of an element, not the number of elements. ctx->rk_offset is not used when accesing a specific index of RK, where I would interpret it as an offset of given array elements.
Again - what am I missing here?
Edit: Sorry, mixed up the sizes here. It can be up to 16 bytes, while the elements have 4 bytes, so if it was to be used as a number of array elements, it should rather be divided by 4.

[tom-cosgrove-arm]

Err, yes, (char *)MBEDTLS_PADLOCK_ALIGN16( buf ) - (char *)buf will be 0, 4, 8 or 12 bytes, but MBEDTLS_PADLOCK_ALIGN16( buf ) - buf will be 0, 1, 2 or 3.

this is a difference between two uint32_t's

It's actually the difference between two uint32_t *s.

If buf (i.e. &buf[0]) is at (vaddr_t)1000 (decimal), &buf[1] is at 1004, &buf[2] is at 1008 and &buf[3] is at 1012, MBEDTLS_PADLOCK_ALIGN16(buf) will give us (uint32_t *)1008, which will give us 2 - the third one in the list) when we subtract buf from it

[AndrzejKurek]

All clear, I failed at pointer-land arithmetics :) Sorry for taking so long to realize!

[gilles-peskine-arm]

We're testing the sequence {setkey_enc; move; crypt}, and the sequence {init; move; setkey_enc}, but not the sequence {setkey_dec; move; crypt}. Since the problematic code is present in both setkey_enc and setkey_dec, we should test both, i.e. move the context after setkey_dec.

The sequence {init; move; setkey} is not particularly risky since setkey is unlikely to have a flaw where it would depend on previous content: that bug would be likely to hit on a freshly initialized context too. We can test it just in case, but I don't think it's required.

[gilles-peskine-arm]

On code inspection, I believe that VIA padlock support (which was the reason for the offset due to its alignment constraint) is preserved. However we do not have the hardware to test it. I sent a message to the mailing list to let people know and ask for someone to test it if they can.

[gilles-peskine-arm]

Please address review comments with subsequent commits (one commit for each unrelated change), not by rewriting old commits.

[gilles-peskine-arm]

The latest commits are missing a signoff line.

git rebase --signoff -i HEAD~3

[paul-elliott-arm]

LGTM.

Re-iterating previous comment about adding new commits to address review comments, rather than re-writing and force pushing.

[AndrzejKurek]

Looks good apart from the one minor comment that I'm not sure about.

[tom-cosgrove-arm]

Since we normally backport bug fixes, should we backport this one?

[tom-cosgrove-arm]

Answer: no backport, as it would break the ABI on LTS, which we try hard not to break (except for security fixes)

[tom-cosgrove-arm]

Hmm, a thought about this PR: the whole point is to make the structure shallow-copyable, so instead of having an embedded pointer into buf, it becomes rk_offset. Of course, if the buffer is copied, the offset needed for alignment might need to be changed, so: is what we have done right?

[AndrzejKurek]

Hmm, a thought about this PR: the whole point is to make the structure shallow-copyable, so instead of having an embedded pointer into buf, it becomes rk_offset. Of course, if the buffer is copied, the offset needed for alignment might need to be changed, so: is what we have done right?

Looking at the use case and steps to reproduce - mbedtls_aes_setkey_enc/mbedtls_aes_setkey_dec is not called after the shallow copy is done, so the alignment might as well be wrong, if I understand correctly. Good catch!

[wernerlewis]

Yes, this could lead to a change in alignment, which would cause failures with MBEDTLS_PADLOCK_C, good spot. If we want to support shallow copying in this case, then it would require a different implementation, or a way to realign RK after copying.

[tom-cosgrove-arm]

Actually, all you need to do is check that ctx->buf + ctx->rk_offset is 16-byte aligned in mbedtls_padlock_xcryptecb() and mbedtls_padlock_xcryptcbc() (which already do this for input and output) returning MBEDTLS_ERR_PADLOCK_DATA_MISALIGNED if not aligned, at which point the calling code will fall back to the slow (but working) path.

This means that a non-shallow-copied (OG) ctx will still use Padlock acceleration, and a (lucky) aligned shallow-copied ctx will use Padlock, but an unlucky not-aligned shallow-copied ctx will at least work.

That seems reasonable - you can copy the context, but it might be slow on the (rare, these days) VIA Padlock system.

[tom-cosgrove-arm]

LGTM - thanks for fixing so quickly

[daverodgman]

CI failures are all ABI, as expected