Skip to content

Commit

Permalink
Fix TSAN s2n_shutdown failures
Browse files Browse the repository at this point in the history
  • Loading branch information
lrstewart committed Jun 14, 2023
1 parent b4aee0c commit d0322c0
Show file tree
Hide file tree
Showing 13 changed files with 164 additions and 49 deletions.
18 changes: 12 additions & 6 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -221,11 +221,6 @@ if(S2N_UNSAFE_FUZZING_MODE)
target_compile_options(${PROJECT_NAME} PRIVATE -fsanitize-coverage=trace-pc-guard -fsanitize=address,undefined,leak -fuse-ld=gold -DS2N_ADDRESS_SANITIZER=1)
endif()

if(TSAN)
target_compile_options(${PROJECT_NAME} PUBLIC -fsanitize=thread)
target_link_options(${PROJECT_NAME} PUBLIC -fsanitize=thread)
endif()

list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake/modules")

if (NOT $ENV{S2N_LIBCRYPTO} MATCHES "awslc")
Expand Down Expand Up @@ -357,6 +352,13 @@ if (NOT S2N_EXECINFO_AVAILABLE)
endif()
feature_probe_result(S2N_STACKTRACE ${S2N_STACKTRACE})

if(TSAN)
target_compile_options(${PROJECT_NAME} PUBLIC -fsanitize=thread)
target_link_options(${PROJECT_NAME} PUBLIC -fsanitize=thread)
# For now, only enable real atomics for TSAN
feature_probe_result(S2N_ATOMIC_ENABLED ${S2N_ATOMIC_SUPPORTED})
endif()

set(S2N_KYBER512R3_AVX2_BMI2 FALSE)
if(NOT S2N_NO_PQ_ASM)
# Kyber Round-3 code has several different optimizations which require
Expand Down Expand Up @@ -510,7 +512,11 @@ if (BUILD_TESTING)
message(FATAL_ERROR "TSAN suppression file ${TSAN_SUPPRESSIONS_FILE} missing")
endif()
set(UNIT_TEST_ENVS ${UNIT_TEST_ENVS} S2N_ADDRESS_SANITIZER=1)
set(UNIT_TEST_ENVS ${UNIT_TEST_ENVS} TSAN_OPTIONS=suppressions=${TSAN_SUPPRESSIONS_FILE})
set(TSAN_OPTIONS suppressions=${TSAN_SUPPRESSIONS_FILE})
if(DEFINED ENV{TSAN_OPTIONS})
set(TSAN_OPTIONS "${TSAN_OPTIONS} $ENV{TSAN_OPTIONS}")
endif()
set(UNIT_TEST_ENVS ${UNIT_TEST_ENVS} TSAN_OPTIONS=${TSAN_OPTIONS})
endif()
message(STATUS "Running tests with environment: ${UNIT_TEST_ENVS}")

Expand Down
1 change: 1 addition & 0 deletions error/s2n_errno.c
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,7 @@ static const char *no_such_error = "Internal s2n error";
ERR_ENTRY(S2N_ERR_INTERNAL_LIBCRYPTO_ERROR, "An internal error has occurred in the libcrypto API") \
ERR_ENTRY(S2N_ERR_NO_RENEGOTIATION, "Only secure, server-initiated renegotiation is supported") \
ERR_ENTRY(S2N_ERR_APP_DATA_BLOCKED, "Blocked on application data during handshake") \
ERR_ENTRY(S2N_ERR_ATOMIC, "Atomic operations in this environment would require locking") \
/* clang-format on */

#define ERR_STR_CASE(ERR, str) \
Expand Down
1 change: 1 addition & 0 deletions error/s2n_errno.h
Original file line number Diff line number Diff line change
Expand Up @@ -306,6 +306,7 @@ typedef enum {
S2N_ERR_SECRET_SCHEDULE_STATE,
S2N_ERR_CERT_OWNERSHIP,
S2N_ERR_INTERNAL_LIBCRYPTO_ERROR,
S2N_ERR_ATOMIC,
S2N_ERR_T_USAGE_END,
} s2n_error;

Expand Down
23 changes: 23 additions & 0 deletions tests/features/S2N_ATOMIC_SUPPORTED.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
/*
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License").
* You may not use this file except in compliance with the License.
* A copy of the License is located at
*
* http://aws.amazon.com/apache2.0
*
* or in the "license" file accompanying this file. This file is distributed
* on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
* express or implied. See the License for the specific language governing
* permissions and limitations under the License.
*/

#include <signal.h>

int main() {
sig_atomic_t atomic = 0;
__atomic_test_and_set(&atomic, __ATOMIC_RELAXED);
__atomic_clear(&atomic, __ATOMIC_RELAXED);
return 0;
}
1 change: 1 addition & 0 deletions tests/features/S2N_ATOMIC_SUPPORTED.flags
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
-Werror
6 changes: 4 additions & 2 deletions tls/s2n_alerts.c
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
#include "tls/s2n_record.h"
#include "tls/s2n_resume.h"
#include "tls/s2n_tls_parameters.h"
#include "utils/s2n_atomic.h"
#include "utils/s2n_blob.h"
#include "utils/s2n_safety.h"

Expand Down Expand Up @@ -203,8 +204,8 @@ int s2n_process_alert_fragment(struct s2n_connection *conn)
if (s2n_stuffer_data_available(&conn->alert_in) == 2) {
/* Close notifications are handled as shutdowns */
if (conn->alert_in_data[1] == S2N_TLS_ALERT_CLOSE_NOTIFY) {
conn->read_closed = 1;
conn->close_notify_received = true;
s2n_atomic_set(&conn->read_closed);
s2n_atomic_set(&conn->close_notify_received);
return 0;
}

Expand All @@ -221,6 +222,7 @@ int s2n_process_alert_fragment(struct s2n_connection *conn)

/* All other alerts are treated as fatal errors */
POSIX_GUARD_RESULT(s2n_connection_set_closed(conn));
s2n_atomic_set(&conn->error_alert_received);
POSIX_BAIL(S2N_ERR_ALERT);
}
}
Expand Down
21 changes: 12 additions & 9 deletions tls/s2n_connection.c
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
#include "tls/s2n_security_policies.h"
#include "tls/s2n_tls.h"
#include "tls/s2n_tls_parameters.h"
#include "utils/s2n_atomic.h"
#include "utils/s2n_blob.h"
#include "utils/s2n_compiler.h"
#include "utils/s2n_mem.h"
Expand Down Expand Up @@ -1168,8 +1169,8 @@ S2N_CLEANUP_RESULT s2n_connection_apply_error_blinding(struct s2n_connection **c
S2N_RESULT s2n_connection_set_closed(struct s2n_connection *conn)
{
RESULT_ENSURE_REF(conn);
conn->read_closed = 1;
conn->write_closed = 1;
s2n_atomic_set(&conn->read_closed);
s2n_atomic_set(&conn->write_closed);
return S2N_RESULT_OK;
}

Expand Down Expand Up @@ -1547,7 +1548,9 @@ bool s2n_connection_check_io_status(struct s2n_connection *conn, s2n_io_status s
return false;
}

const bool is_full_duplex = !conn->read_closed && !conn->write_closed;
bool read_closed = s2n_atomic_check(&conn->read_closed);
bool write_closed = s2n_atomic_check(&conn->write_closed);
bool full_duplex = !read_closed && !write_closed;

/*
*= https://tools.ietf.org/rfc/rfc8446#section-6.1
Expand All @@ -1561,21 +1564,21 @@ bool s2n_connection_check_io_status(struct s2n_connection *conn, s2n_io_status s
case S2N_IO_WRITABLE:
case S2N_IO_READABLE:
case S2N_IO_FULL_DUPLEX:
return is_full_duplex;
return full_duplex;
case S2N_IO_CLOSED:
return !is_full_duplex;
return !full_duplex;
}
}

switch (status) {
case S2N_IO_WRITABLE:
return !conn->write_closed;
return !write_closed;
case S2N_IO_READABLE:
return !conn->read_closed;
return !read_closed;
case S2N_IO_FULL_DUPLEX:
return is_full_duplex;
return full_duplex;
case S2N_IO_CLOSED:
return conn->read_closed && conn->write_closed;
return read_closed && write_closed;
}

return false;
Expand Down
18 changes: 9 additions & 9 deletions tls/s2n_connection.h
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
#include "tls/s2n_security_policies.h"
#include "tls/s2n_tls_parameters.h"
#include "tls/s2n_x509_validator.h"
#include "utils/s2n_atomic.h"
#include "utils/s2n_mem.h"
#include "utils/s2n_timer.h"

Expand Down Expand Up @@ -110,9 +111,6 @@ struct s2n_connection {
* even when setting a new config. */
unsigned psk_mode_overridden : 1;

/* Have we received a close notify alert from the peer. */
unsigned close_notify_received : 1;

/* Connection negotiated an EMS */
unsigned ems_negotiated : 1;

Expand Down Expand Up @@ -272,6 +270,10 @@ struct s2n_connection {
uint8_t reader_warning_out;
bool alert_sent;

/* Receiving error or close_notify alerts changes the behavior of s2n_shutdown_send */
s2n_atomic error_alert_received;
s2n_atomic close_notify_received;

/* Our handshake state machine */
struct s2n_handshake handshake;

Expand Down Expand Up @@ -308,13 +310,11 @@ struct s2n_connection {
uint64_t wire_bytes_out;
uint64_t early_data_bytes;

/* Is the connection open or closed?
*
* We use C's only atomic type as an error requires shutting down both read
* and write, so both the reader and writer threads may access both fields.
/* Either the reader or the writer can trigger both sides of the connection
* to close in response to a fatal error.
*/
sig_atomic_t read_closed;
sig_atomic_t write_closed;
s2n_atomic read_closed;
s2n_atomic write_closed;

/* TLS extension data */
char server_name[S2N_MAX_SERVER_NAME + 1];
Expand Down
4 changes: 2 additions & 2 deletions tls/s2n_recv.c
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ S2N_RESULT s2n_read_in_bytes(struct s2n_connection *conn, struct s2n_stuffer *ou
errno = 0;
int r = s2n_connection_recv_stuffer(output, conn, remaining);
if (r == 0) {
conn->read_closed = 1;
s2n_atomic_set(&conn->read_closed);
RESULT_BAIL(S2N_ERR_CLOSED);
} else if (r < 0) {
if (errno == EWOULDBLOCK || errno == EAGAIN) {
Expand Down Expand Up @@ -127,7 +127,7 @@ ssize_t s2n_recv_impl(struct s2n_connection *conn, void *buf, ssize_t size, s2n_
*# closed, the TLS implementation MUST receive a "close_notify" alert
*# before indicating end-of-data to the application layer.
*/
POSIX_ENSURE(conn->close_notify_received, S2N_ERR_CLOSED);
POSIX_ENSURE(s2n_atomic_check(&conn->close_notify_received), S2N_ERR_CLOSED);
*blocked = S2N_NOT_BLOCKED;
return 0;
}
Expand Down
26 changes: 5 additions & 21 deletions tls/s2n_shutdown.c
Original file line number Diff line number Diff line change
Expand Up @@ -17,29 +17,13 @@
#include "tls/s2n_alerts.h"
#include "tls/s2n_connection.h"
#include "tls/s2n_tls.h"
#include "utils/s2n_atomic.h"
#include "utils/s2n_safety.h"

static bool s2n_error_alert_received(struct s2n_connection *conn)
{
/* We don't check s2n_connection_get_alert() or s2n_stuffer_data_available()
* because of https://github.com/aws/s2n-tls/issues/3933.
* We need to check if the stuffer contains an alert, regardless of its
* read state.
*/
if (conn->alert_in.write_cursor == 0) {
return false;
}
/* Verify that the stuffer doesn't just contain a close_notify alert */
if (conn->close_notify_received) {
return false;
}
return true;
}

static bool s2n_shutdown_expect_close_notify(struct s2n_connection *conn)
{
/* No close_notify expected if we already received an error instead */
if (s2n_error_alert_received(conn)) {
if (s2n_atomic_check(&conn->error_alert_received)) {
return false;
}

Expand Down Expand Up @@ -83,11 +67,11 @@ int s2n_shutdown_send(struct s2n_connection *conn, s2n_blocked_status *blocked)
}

/* Flush any outstanding data */
conn->write_closed = 1;
s2n_atomic_set(&conn->write_closed);
POSIX_GUARD(s2n_flush(conn, blocked));

/* For a connection closed due to receiving an alert, we don't send anything. */
if (s2n_error_alert_received(conn)) {
if (s2n_atomic_check(&conn->error_alert_received)) {
return S2N_SUCCESS;
}

Expand Down Expand Up @@ -136,7 +120,7 @@ int s2n_shutdown(struct s2n_connection *conn, s2n_blocked_status *blocked)
uint8_t record_type = 0;
int isSSLv2 = false;
*blocked = S2N_BLOCKED_ON_READ;
while (!conn->close_notify_received) {
while (!s2n_atomic_check(&conn->close_notify_received)) {
POSIX_GUARD(s2n_read_full_record(conn, &record_type, &isSSLv2));
POSIX_ENSURE(!isSSLv2, S2N_ERR_BAD_MESSAGE);
if (record_type == TLS_ALERT) {
Expand Down
58 changes: 58 additions & 0 deletions utils/s2n_atomic.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
/*
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License").
* You may not use this file except in compliance with the License.
* A copy of the License is located at
*
* http://aws.amazon.com/apache2.0
*
* or in the "license" file accompanying this file. This file is distributed
* on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
* express or implied. See the License for the specific language governing
* permissions and limitations under the License.
*/

#include "utils/s2n_atomic.h"

#include "utils/s2n_safety.h"

const s2n_atomic set_val = 1;
const s2n_atomic clear_val = 0;

S2N_RESULT s2n_atomic_init()
{
#if S2N_ATOMIC_ENABLED
RESULT_ENSURE(__atomic_always_lock_free(sizeof(s2n_atomic), NULL), S2N_ERR_ATOMIC);
#endif
return S2N_RESULT_OK;
}

void s2n_atomic_set(s2n_atomic *var)
{
#if S2N_ATOMIC_ENABLED
__atomic_store(var, &set_val, __ATOMIC_RELAXED);
#else
*var = 1;
#endif
}

void s2n_atomic_clear(s2n_atomic *var)
{
#if S2N_ATOMIC_ENABLED
__atomic_store(var, &clear_val, __ATOMIC_RELAXED);
#else
*var = 0;
#endif
}

bool s2n_atomic_check(s2n_atomic *var)
{
#if S2N_ATOMIC_ENABLED
s2n_atomic result = 0;
__atomic_load(var, &result, __ATOMIC_RELAXED);
return result;
#else
return *var;
#endif
}
35 changes: 35 additions & 0 deletions utils/s2n_atomic.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
/*
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License").
* You may not use this file except in compliance with the License.
* A copy of the License is located at
*
* http://aws.amazon.com/apache2.0
*
* or in the "license" file accompanying this file. This file is distributed
* on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
* express or implied. See the License for the specific language governing
* permissions and limitations under the License.
*/
#pragma once

#include <signal.h>

#include "utils/s2n_result.h"

/* s2n-tls allows s2n_send and s2n_recv to be called concurrently.
* There are a handful of values that both methods need to access.
* However, C99 has no concept of concurrency, so provides no atomic data types.
*
* Traditionally, s2n-tls uses sig_atomic_t for its weak guarantee of atomicity for interrupts.
*/
typedef sig_atomic_t s2n_atomic;

/* These methods use compiler atomic built-ins if available and lock-free, but otherwise
* rely on setting / clearing a small value generally being atomic in practice.
*/
S2N_RESULT s2n_atomic_init();
void s2n_atomic_set(s2n_atomic *var);
void s2n_atomic_clear(s2n_atomic *var);
bool s2n_atomic_check(s2n_atomic *var);
1 change: 1 addition & 0 deletions utils/s2n_init.c
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ int s2n_init(void)
POSIX_GUARD(s2n_extension_type_init());
POSIX_GUARD_RESULT(s2n_pq_init());
POSIX_GUARD_RESULT(s2n_tls13_empty_transcripts_init());
POSIX_GUARD_RESULT(s2n_atomic_init());

if (atexit_cleanup) {
POSIX_ENSURE_OK(atexit(s2n_cleanup_atexit), S2N_ERR_ATEXIT);
Expand Down

0 comments on commit d0322c0

Please sign in to comment.