Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

esp_ds: ignore releasing mutex if not called from same task (IDFGH-10131) #11402

Merged

Conversation

cbaechler
Copy link
Contributor

The mutex s_ds_lock in esp_rsa_sign_alt.c is always unlocked unconditionally whether or not it was locked before. In our case, working with multiple connections, this resulted in triggering the assertion in

#if ( configUSE_MUTEXES == 1 && configCHECK_MUTEX_GIVEN_BY_OWNER == 1 )
configASSERT( pxQueue->uxQueueType != queueQUEUE_IS_MUTEX ||
pxQueue->u.xSemaphore.xMutexHolder == NULL ||
pxQueue->u.xSemaphore.xMutexHolder == xTaskGetCurrentTaskHandle() );
#endif

When connecting, the mutex is locked in esp_create_mbedtls_handle -> set_client_config -> set_pki_context -> esp_mbedtls_init_pk_ctx_for_ds -> esp_ds_init_data_ctx but only if there is a client certificate or ds_data set.

Unlocking the mutex is done unconditionally from multiple places, e.g. after handshake (esp_mbedtls_handshake -> esp_ds_release_ds_lock) or on cleanup (esp_mbedtls_cleanup -> esp_ds_release_ds_lock).

Consider following scenario:

  • Task 1 connects to MQTT broker, with client and server certificate
  • Task 2 opens another connection, without client certificate (this task has higher priority than task 1)

This might trigger the following:

  • Task 1: Calls esp_create_mbedtls_handle with client certificate or ds_data fields filled -> locks mutex
  • Task 2: Calls esp_create_mbedtls_handle without client certificate, thus not locking the mutex
  • Task 2: Calls esp_ds_release_ds_lock, and the assertion will trigger because the mutex was locked by task 1, but the releasing task is different

FYI @KonssnoK

@KonssnoK
Copy link
Contributor

thanks Christoph.

@espressif-bot espressif-bot added the Status: Opened Issue is new label May 15, 2023
@github-actions github-actions bot changed the title esp_ds: ignore releasing mutex if not called from same task esp_ds: ignore releasing mutex if not called from same task (IDFGH-10131) May 15, 2023
@mahavirj
Copy link
Member

sha=704dfc91857250bab7e92e2d357d326cd72e07f8

@mahavirj mahavirj added the PR-Sync-Merge Pull request sync as merge commit label May 15, 2023
@espressif-bot espressif-bot added Status: Done Issue is done internally Resolution: NA Issue resolution is unavailable and removed Status: Opened Issue is new labels May 19, 2023
@espressif-bot espressif-bot merged commit 1747f2e into espressif:master May 21, 2023
@KonssnoK
Copy link
Contributor

@mahavirj please let us know when it's backported to v4.4. thanks

@AxelLin
Copy link
Contributor

AxelLin commented May 21, 2023

And v4.3 please, thanks.

@AxelLin
Copy link
Contributor

AxelLin commented Jun 19, 2023

@mahavirj please let us know when it's backported to v4.4. thanks

v4.4 fix: d007b0e

@AxelLin
Copy link
Contributor

AxelLin commented Jun 20, 2023

And v4.3 please, thanks.

@mahavirj v4.3 still needs fix, the patch can be applied to v4.3 without conflict.

@mahavirj
Copy link
Member

Yes, 4.3 backport is in progress, should be available on GitHub with next sync.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR-Sync-Merge Pull request sync as merge commit Resolution: NA Issue resolution is unavailable Status: Done Issue is done internally
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants