Repository navigation
Commit f2ec3a5
committed
autosar: thread safety, MAC services, device offload, key handles, DET
Fixed in the Csm/CryIf/Crypto port:
- Two data races: the MAX_JOBS job table had no lock, so two SW-Cs calling
Csm_Encrypt() could claim one slot and initialize one Aes twice, and GetKey()
returned a keystore pointer after dropping the mutex, so a concurrent
Csm_KeyElementSet() could overwrite the key in use. The table has its own
mutex and the key is copied out under the lock. ThreadSanitizer: 125 races
before, none after.
- Key input redirection did not compile at all -- GetKey() used runtime struct
members as switch case labels -- and redirect_test is gated behind the same
macro, so nothing built or ran it.
- Cipher jobs take the key length from the keystore, so 128, 192 and 256 bit
keys all work; csm.c asked every job for 16 bytes and the lookup needed an
exact match.
- Development errors used a local 0-based enum shadowing Csm.h's values, so
CSM_E_PARAM_POINTER reached a DET as 0, which the specification assigns to no
error; it also misspelled CSM_E_UNINIT.
- WOLFSSL_AUTOSAR_DET did not link without the MAC services, the test program
defining Det_ReportError() only alongside CMAC.
- Crypto_Init() was not safe to re-run although a repeated Csm_Init() is
supported: re-initialized live mutexes, freed the RNG unlocked, and cleared
the job table without freeing the contexts in it. Its mutexes are created all
or nothing now, and the new configuration is published while holding every
lock its readers use -- jobs_mutex, rngMutex and crypto_mutex, in that order
-- so a concurrent job cannot recreate the DRBG from the old devId just after
it was freed, or take one field from each configuration.
- A re-init is refused outright while any job owns a slot or is starting one.
A job runs its Aes or Cmac context outside the locks, so freeing one under a
live job trades a leak for a use-after-free; the previous configuration stays
in force instead, nothing half-applied. Because the service returns void, the
refusal goes to the DET as CSM_E_INIT_FAILED rather than nowhere. The caller
FINISHes its jobs, or cancels them -- see Csm_CancelJob() below.
- A START is a sequence, not a step: it copies the key out of the keystore,
checks it, reads the configured devId and only then claims a slot. Slot
ownership is what the refusal above looks at, which says nothing about a
START that has not reached its slot yet, so the whole of Crypto_Init() could
run inside that window -- wipe the keystore, publish a new heap and devId --
and leave the job building a context for the newly configured device out of
the previous configuration's key. A counter makes the window itself visible
to the refusal; the START paths are factored out so it covers every exit.
- A key label of exactly MAX_KEY_LABEL_LEN characters was rejected, the buffer
having no room for the terminator; both label buffers are one byte longer so
the documented maximum fits. That maximum is AES_MAX_LABEL_LEN - 1, 31:
wolfCrypt's CMAC keeps a label only when it is strictly shorter than
AES_MAX_LABEL_LEN and silently keeps none at 32, where AES accepts it, so the
limit the port documents has to be one both back ends carry. A static
assertion holds the two together.
- A cipher job failing after START kept its slot, and ClaimJobSlot() did not
check for an existing jobId, so the next START claimed a second slot while
UPDATE and FINISH ran on the stale first. START restarts the job now.
- CRYPTO_OPERATIONMODE_FINISH on a job never started reported success; the DRBG
used device ID 0 rather than INVALID_DEVID.
- WOLFSSL_AUTOSAR=yes under CMake defined the macro without compiling the port
sources, so Csm_* calls compiled and then failed to link. That option also
rejects a build without AES, or with AES but without AES-CBC, now -- the
Crypto driver is built around Aes in CBC mode, so the prerequisite belongs to
the port and not only to its MAC services.
- Key material is wiped with ForceZero(), a failed CMAC init is freed, and the
port test includes options.h before settings.h.
Added:
- --enable-autosar-cmac: Csm_MacGenerate()/Csm_MacVerify() over AES-CMAC. It
enables wolfCrypt CMAC itself, WOLFSSL_AES_DIRECT included, and refuses at
compile time a user_settings.h build defining WOLFSSL_AUTOSAR_CMAC without
them. Verify enforces WOLFSSL_AUTOSAR_MAC_MIN_SZ, default 3 bytes for SecOC
profile 1 -- below wolfCrypt's WC_CMAC_TAG_MIN_SZ deliberately, raisable.
- devId beside heap in Csm_ConfigType, passed down through CryIf_Init() to
Crypto_Init() instead of being discarded, so the driver can front an HSM.
WOLFSSL_AUTOSAR_DEVID is the default for a build with no config to plumb and
WOLFSSL_CSM_CONFIG_DEFAULT initializes one, so leaving devId alone does not
select device 0. CryIf_ConfigType and Crypto_ConfigType gained the field too:
an application built against the older headers must be rebuilt.
- Key handles: a slot can name a key held in a device, so the key never enters
RAM. wolfSSL_Csm_KeyElementSetId()/...SetLabel() point a slot at it and the
driver uses wc_AesInit_Id()/wc_AesInit_Label() or the CMAC equivalents. Needs
WOLF_PRIVATE_KEY_ID and a devId. Cipher jobs additionally need
WOLFSSL_AES_REQUIRE_KEY_SET, absent on the ports under
WC_AES_KEY_SET_CHECK_UNSUPPORTED in aes.h, since the empty key schedule is
only an error while something checks for it; MAC jobs need no equivalent,
CMAC init rejecting a NULL key once the callback has declined.
- Naming a key slot, because element IDs cannot: the specification scopes them
per key, so CRYPTO_KE_CIPHER_KEY and CRYPTO_KE_MAC_KEY are both 0x01 and this
keystore is flat. Resolving by element took the first slot holding 0x01, so a
MAC job could run under the AES-CBC key and the verifying side would accept
it. wolfSSL_Csm_{Encrypt,Decrypt,MacGenerate,MacVerify}WithKey() put a slot
in the job's cryIfKeyId, which is the field AUTOSAR has for this.
- DET reporting shaped after Det_ReportError(), forwarded to it under
WOLFSSL_AUTOSAR_DET without the port including Det.h -- declared as the
specification has it, returning Std_ReturnType, with the result discarded
explicitly, a failed report changing nothing the caller can act on -- plus
the module
identity a dependent module checks (WOLFSSL_CSM_MODULE_ID 110, a vendor ID,
the AR_RELEASE macros), which Csm_GetVersionInfo() reports. Service IDs are
marked in the header as unconfirmed against the specification's table.
- Csm_CancelJob(), CryIf_CancelJob() having been a stub returning E_NOT_OK. It
releases the slot of a job that will never send its FINISH -- an abandoned
stream, or one left by an SW-C that has been restarted. Without it such a
slot is held until the ECU resets and, now that a re-init is refused while
one is, reconfiguration could never happen again. Cancel a job from its own
owner: the driver is not safe against a cancel racing an UPDATE of the same
job, which is the same reason Csm_Init() refuses rather than tearing down.
Two things that can stop a build or a job that used to work:
- An unnamed job is refused when more than one slot holds element 0x01.
WOLFSSL_AUTOSAR_ALLOW_AMBIGUOUS_KEY_ELEMENT restores first-match for an ECU
keeping several keys for one service.
- Redirection with --enable-autosar-cmac is refused at build time: it maps 0x01
to one slot for every service, leaving nothing to disambiguate. Refusing per
job instead would leave no unnamed job able to resolve 0x01, so the standard
entry points would stop working and only the wolfSSL extensions would be
usable. Key input redirection never separated the two, whatever an earlier
draft of this port documented.
Mind the MAC length units, which differ between the two services because the
specification has them that way. Csm_MacVerify's macLength is in BITS: the
driver converts to ceil(macLength / 8) bytes, compares the whole bytes and
then the trailing partial byte under a mask, and rejects anything longer than
the 128 bit tag -- which is where a byte count passed by mistake lands. The
floor is applied to the bit count before that rounding, 17 bits being short of
a three byte minimum however they round up.
Csm_MacGenerate's macLengthPtr is in BYTES, and its specified unit is
unverified here. So feeding the length Csm_MacGenerate returned straight into
Csm_MacVerify asks for eight times less than intended: 16 becomes 16 bits, two
bytes, below the floor and refused. wolfSSL_Csm_MacVerifyWithKey() is
byte-based throughout, being a wolfSSL extension, and takes the buffer size
beside the length.
Tests and CI: the port test provisions its key and IV into the slots
redirection points at, so it passes either way, and covers a CMAC known answer,
a devId reaching a crypto callback, a named key producing the same ciphertext
as the key itself, a MAC job whose key never enters RAM, a DET report's IDs, a
cipher and a MAC key provisioned at once, the job slot lifecycle, every AES key
length, the MAC floor and buffer-size refusals, a repeat Csm_Init() refused
while a job is open and accepted once it has been finished, and cancelling an
abandoned one -- the context free observed through the device's free
notifications, a cleared table alone being indistinguishable from the API. One
case is threaded: cipher jobs against a concurrent Csm_Init(), which holds the
invariant that a job that ran to completion ran under the key provisioned, and
is clean under ThreadSanitizer. It does not fail with the START gate removed,
checked by removing it -- that window is a time-of-check race whose accesses
are all still under a lock -- so it is traffic over the window and not a proof
of it.
Each was checked against the unfixed driver to confirm it fails. The new
autosar.yml runs nine configurations, each asserting its feature-gated test
actually ran, since a compiled-out test's silence looks like success; its
negative configure probe runs in tree before the real configure, automake's
VPATH guard otherwise rejecting an already-configured source directory and
logging that instead. --enable-autosar had no CI coverage at all before.1 parent 799bd17 commit f2ec3a5
14 files changed
Lines changed: 4823 additions & 241 deletions
File tree
- .github/workflows
- cmake
- wolfcrypt/src/port/autosar
- wolfssl/wolfcrypt/port/autosar
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
3817 | 3817 | | |
3818 | 3818 | | |
3819 | 3819 | | |
| 3820 | + | |
| 3821 | + | |
| 3822 | + | |
| 3823 | + | |
| 3824 | + | |
| 3825 | + | |
| 3826 | + | |
| 3827 | + | |
| 3828 | + | |
| 3829 | + | |
| 3830 | + | |
| 3831 | + | |
| 3832 | + | |
| 3833 | + | |
3820 | 3834 | | |
3821 | 3835 | | |
3822 | 3836 | | |
3823 | 3837 | | |
3824 | 3838 | | |
| 3839 | + | |
| 3840 | + | |
| 3841 | + | |
| 3842 | + | |
| 3843 | + | |
| 3844 | + | |
| 3845 | + | |
| 3846 | + | |
| 3847 | + | |
| 3848 | + | |
| 3849 | + | |
| 3850 | + | |
| 3851 | + | |
| 3852 | + | |
| 3853 | + | |
| 3854 | + | |
| 3855 | + | |
| 3856 | + | |
| 3857 | + | |
| 3858 | + | |
| 3859 | + | |
| 3860 | + | |
| 3861 | + | |
| 3862 | + | |
| 3863 | + | |
| 3864 | + | |
3825 | 3865 | | |
3826 | 3866 | | |
3827 | 3867 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
48 | 48 | | |
49 | 49 | | |
50 | 50 | | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
51 | 55 | | |
52 | 56 | | |
53 | 57 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
127 | 127 | | |
128 | 128 | | |
129 | 129 | | |
| 130 | + | |
130 | 131 | | |
131 | 132 | | |
132 | 133 | | |
| |||
1376 | 1377 | | |
1377 | 1378 | | |
1378 | 1379 | | |
| 1380 | + | |
| 1381 | + | |
| 1382 | + | |
| 1383 | + | |
| 1384 | + | |
| 1385 | + | |
| 1386 | + | |
| 1387 | + | |
| 1388 | + | |
| 1389 | + | |
1379 | 1390 | | |
1380 | 1391 | | |
1381 | 1392 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
577 | 577 | | |
578 | 578 | | |
579 | 579 | | |
| 580 | + | |
| 581 | + | |
| 582 | + | |
580 | 583 | | |
581 | 584 | | |
582 | 585 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
12336 | 12336 | | |
12337 | 12337 | | |
12338 | 12338 | | |
| 12339 | + | |
| 12340 | + | |
| 12341 | + | |
| 12342 | + | |
| 12343 | + | |
| 12344 | + | |
| 12345 | + | |
| 12346 | + | |
| 12347 | + | |
| 12348 | + | |
| 12349 | + | |
| 12350 | + | |
| 12351 | + | |
| 12352 | + | |
| 12353 | + | |
| 12354 | + | |
| 12355 | + | |
| 12356 | + | |
| 12357 | + | |
| 12358 | + | |
| 12359 | + | |
| 12360 | + | |
| 12361 | + | |
| 12362 | + | |
| 12363 | + | |
12339 | 12364 | | |
12340 | 12365 | | |
12341 | 12366 | | |
| |||
14910 | 14935 | | |
14911 | 14936 | | |
14912 | 14937 | | |
| 14938 | + | |
14913 | 14939 | | |
14914 | 14940 | | |
14915 | 14941 | | |
| |||
0 commit comments