CHERIoT-Platform / CHERIoT-Platform/network-stack
Race condition in network_socket_close in reset scenario
- Dominant language
- C++
- Stars
- 9
- Forks
- 14
- Avg merge
- 6d 17h
- Merged PRs (30d)
- 1
Description
### Summary
If two threads call `network_socket_close()` concurrently on the same socket after a network stack restart, both may enter the cleanup path at the same time. One thread can destroy the socket wrapper while the other tries to dereference it, that will cause Use-After-Free, trigger hardware exception, and lead to another network stack restart.
### Root cause
For a socket from last epoch, both threads fail to acquire `socketLock`, but the the `network_socket_close` allows both threads to continue when it finds this is a restart case. The race can occur near [heap_free()](https://github.com/CHERIoT-Platform/network-stack/blob/075337c1197aec5abf9078684b3f901382f198ce/lib/tcpip/network_wrapper.cc#L992), and here is the problematic order of interleaving.
```text
Thread 1 Thread 2
some prior checks
some prior checks
heap_free(..., socket->socket)
token_obj_destroy(..., sealedSocket)
*heap_free(..., socket->socket) // crash
```
After Thread 1 destroys the socket wrapper, Thread 2 dereferences the freed wrapper to read `socket->socket`. This triggers a harware fault and thus lead to another network stack restart.
### Affected component
`lib/tcpip/network_wrapper.cc`, old epoch cleanup path in `network_socket_close()`.
### Impact
A caller that closes the same old socket concurrently from two threads stand a chance to crash and restart the network stack again during reset, which is not expected. But this is not a fatal error, the bug won't be triggered after the second crash, since the socket wrapper has already been freed after the first crash, `with_sealed_socket` will detect that and return the error code instead of triggering another hardware exception.
### PoC
Here is the test code. The test adds a guard immediately before [`heap_free()`](https://github.com/CHERIoT-Platform/network-stack/blob/075337c1197aec5abf9078684b3f901382f198ce/lib/tcpip/network_wrapper.cc#L992) to force the race. Both threads stop at the guard. The fast thread will continue first and executes `heap_free()` and `token_obj_destroy()` together. The delayed thread is then released and dereferences the freed socket wrapper, triggering the hardware exception.
```c
poc_test.cc
void __cheri_compartment("http_server_example") example()
{
// ... some initializations
Debug::log("Injecting network-stack fault.");
network_inject_fault();
Timeout sleep{1000};
/**
* During that time gap, send a packet to
* the network stack to trigger the reset.
*/
thread_sleep(&sleep);
thread_pool::async([socket]() {
int ret = close_after_restart(socket, true); // wait = true, which represent the delayed thread.
Debug::log("Delayed close returned {}.", ret);
WorkersFinished.fetch_add(1);
});
thread_pool::async([socket]() {
int ret = close_after_restart(socket, false); // wait = false, which represent the fast thread.
Debug::log("Fast close returned {}.", ret);
WorkersFinished.fetch_add(1);
});
while (WorkersFinished.load() < 2)
{
Timeout wait{1};
thread_sleep(&wait);
}
Debug::log("Both close workers finished.");
auto heapAtEnd = heap_quota_remaining(TEST_MALLOC);
if (heapAtEnd < heapAtStart)
{
Debug::log(
"Warning: The implementation leaked {} bytes "
"(start: {} vs. end: {}).",
heapAtStart - heapAtEnd,
heapAtStart,
heapAtEnd);
}
else
{
Debug::log(
"No leaks detected (start: {} vs. end: {}).",
heapAtStart,
heapAtEnd);
}
Debug::log("Terminating the test.");
}
```
```c
network_wrapper.cc
network_socket_close(...) {
// ... everything before heap_free
// temporary code block to force the buggy execution order.
if (wait)
{
while (Freed == false) {
Timeout waitingTime{100};
thread_sleep(&waitingTime);
}
}
// Drop the caller's claim on the socket.
if (heap_free(mallocCapability, socket->socket) != 0)
{
// This is not supposed to happen, since we did a
// `heap_can_free` earlier (unless we did not have
// enough stack, or a concurrent free happened). If
// it does, we may be leaking the socket.
Debug::log("Failed to free socket.");
// Don't return yet, try to at least free the token.
ret = -ENOTRECOVERABLE;
}
if (token_obj_destroy(
mallocCapability, socket_key(), sealedSocket) != 0)
{
// This is not supposed to happen, since we did a
// `token_obj_can_destroy` earlier (see comment
// above). If it does, we're leaking the token.
Debug::log("Failed to free token.");
ret = -ENOTRECOVERABLE;
}
if (!Freed)
{
Freed = true;
}
// everything else ...
```
Ran the test on the board, and here is the result:
### Suggested fix
1. putting a `heap_claim_ephemeral` right before the `heap_free` as the guard. If the socket has been freed already, it will return an error code. If it is still here, ephemeral will project the object until `heap_free` is called.
2. putting a heap_claim_ephemeral` in `with_sealed_socket`, and right before the first cross-compartment call: `heap_can_free` in `network_socket_close`.
Based on the test results for these approaches, both options can fix the bug.
Contributor guide
No contributing guide indexed for this repository
Research direction
Start with lib/tcpip/network_wrapper.cc at network_socket_close(), especially the old-epoch cleanup near heap_free(). Read and run the concurrent reset PoC in poc_test.cc on the board, then verify that both close workers finish without a hardware fault, repeated network-stack restart, or leaked heap space.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- networking, security
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 58/100