From 544074e26e2c660bb49e32a6d5e84a6943eae50a Mon Sep 17 00:00:00 2001 From: Georgia Kouveli Date: Wed, 18 Jun 2025 16:24:32 +0100 Subject: [PATCH] [gcs] Fix concurrency issue with Simulator::stacks_ We were accessing the Simulator's `stacks_` member without locking, which can lead to issues if allocating a new stack requires reallocating the underlying memory. Avoid this by caching the pointer to the active GuardedControlStack* and lock for converting an index to a GuardedControlStack pointer. --- src/aarch64/simulator-aarch64.cc | 20 ++-- src/aarch64/simulator-aarch64.h | 127 +++++++++++++++++-------- test/aarch64/test-api-aarch64.cc | 4 +- test/aarch64/test-assembler-aarch64.cc | 8 +- 4 files changed, 102 insertions(+), 57 deletions(-) diff --git a/src/aarch64/simulator-aarch64.cc b/src/aarch64/simulator-aarch64.cc index c7a29fbf..3fad1faf 100644 --- a/src/aarch64/simulator-aarch64.cc +++ b/src/aarch64/simulator-aarch64.cc @@ -566,7 +566,7 @@ Simulator::Simulator(Decoder* decoder, FILE* stream, SimStack::Allocated stack) : memory_(std::move(stack)), last_instr_(NULL), cpu_features_auditor_(decoder, CPUFeatures::All()), - gcs_(kGCSNoStack), + gcs_({nullptr, kGCSNoStack}), gcs_enabled_(false) { // Ensure that shift operations act as the simulator expects. VIXL_ASSERT((static_cast(-1) >> 1) == -1); @@ -729,9 +729,7 @@ Simulator::~Simulator() { close(placeholder_pipe_fd_[0]); close(placeholder_pipe_fd_[1]); #endif - if (IsAllocatedGCS(gcs_)) { - GetGCSManager().FreeStack(gcs_); - } + GetGCSManager().FreeStack(GetGCSToken()); } @@ -1836,9 +1834,9 @@ void Simulator::PrintSystemRegister(SystemRegister id) { void Simulator::PrintGCS(bool is_push, uint64_t addr, size_t entry) { const char* arrow = is_push ? "<-" : "->"; fprintf(stream_, - "# %sgcs0x%04" PRIx64 "[%zx]: %s %s 0x%016" PRIx64 "\n", + "# %sgcs0x%04" PRIu64 "[%zx]: %s %s 0x%016" PRIx64 "\n", clr_flag_name, - gcs_, + GCSManager::GetGCSIndexFromToken(GetGCSToken()), entry, clr_normal, arrow, @@ -7157,10 +7155,10 @@ void Simulator::VisitSystem(const Instruction* instr) { uint64_t incoming_size = rt >> 32; // Drop upper 32 bits to get GCS index. uint64_t incoming_gcs = rt & 0xffffffff; - uint64_t outgoing_gcs = ActivateGCS(incoming_gcs); + GuardedControlStack outgoing_gcs = ActivateGCS(incoming_gcs); uint64_t incoming_seal = GCSPop(); if (((incoming_seal ^ rt) != 1) || - (GetActiveGCSPtr()->size() != incoming_size)) { + (GetGCSStorage()->size() != incoming_size)) { char msg[128]; snprintf(msg, sizeof(msg), @@ -7168,7 +7166,7 @@ void Simulator::VisitSystem(const Instruction* instr) { incoming_seal); ReportGCSFailure(msg); } - GCSPush(outgoing_gcs + 5); + GCSPush(outgoing_gcs.token + 5); } else if (sysop == GCSPUSHM) { GCSPush(ReadXRegister(instr->GetRt())); } else { @@ -7194,11 +7192,11 @@ void Simulator::VisitSystem(const Instruction* instr) { outgoing_gcs); ReportGCSFailure(msg); } - uint64_t incoming_gcs = ActivateGCS(outgoing_gcs); outgoing_gcs &= ~UINT64_C(0x3ff); + GuardedControlStack incoming_gcs = ActivateGCS(outgoing_gcs); // Encode the size into the outgoing stack seal, to check later. - uint64_t size = GetActiveGCSPtr()->size(); + uint64_t size = GetGCSStorage()->size(); VIXL_ASSERT(IsUint32(size)); VIXL_ASSERT(IsUint32(outgoing_gcs + 1)); uint64_t outgoing_seal = (size << 32) | (outgoing_gcs + 1); diff --git a/src/aarch64/simulator-aarch64.h b/src/aarch64/simulator-aarch64.h index b72326ce..e04296a3 100644 --- a/src/aarch64/simulator-aarch64.h +++ b/src/aarch64/simulator-aarch64.h @@ -5482,19 +5482,54 @@ class Simulator : public DecoderVisitor { // The Guarded Control Stack is represented using a vector, where the more // recently stored addresses are at higher-numbered indices. - using GuardedControlStack = std::vector; + using GuardedControlStackStorage = std::vector; + public: + struct GuardedControlStack { + GuardedControlStackStorage* ptr; + uint64_t token; + }; + + private: // The GCSManager handles the synchronisation of GCS across multiple // Simulator instances. Each Simulator has its own stack, but all share // a GCSManager instance. This allows exchanging stacks between Simulators // in a threaded application. class GCSManager { public: - // Allocate a new Guarded Control Stack and add it to the vector of stacks. + // Interface for users outside the Simulator. + + // Allocate a new Guarded Control Stack. This method returns a token, which + // uniquely identifies the GCS, and can be passed *once* to the stack + // switching instructions (GSSS*), when the stack has not been used yet. + // Later arguments to the stack switching instructions must either be + // fresh tokens, or return values from the stack switching instructions. uint64_t AllocateStack() { + GuardedControlStack gcs = AllocateStackInternal(); + return gcs.token; + } + + // Free a Guarded Control Stack based on its token. + void FreeStack(uint64_t token) { + const std::lock_guard lock(stacks_mtx_); + uint64_t gcs_index = GetGCSIndexFromToken(token); + GuardedControlStackStorage* gcsptr = stacks_[gcs_index]; + if (gcsptr == nullptr) { + VIXL_ABORT_WITH_MSG("Tried to double free GCS "); + } else { + delete gcsptr; + // To ensure other tokens remain valid, we do not remove this element + // but set it to nullptr instead. + stacks_[gcs_index] = nullptr; + } + } + + private: + // Allocate a new Guarded Control Stack and add it to the vector of stacks. + GuardedControlStack AllocateStackInternal() { const std::lock_guard lock(stacks_mtx_); - GuardedControlStack* new_stack = new GuardedControlStack; + GuardedControlStackStorage* new_stack = new GuardedControlStackStorage; uint64_t result; // Put the new stack into the first available slot. @@ -5510,42 +5545,46 @@ class Simulator : public DecoderVisitor { stacks_.push_back(new_stack); } - // Shift the index to look like a stack pointer aligned to a page. - result <<= kPageSizeLog2; + result = GetGCSTokenFromIndex(result); // Push the tagged index onto the new stack as a seal. new_stack->push_back(result + 1); - return result; + + return {new_stack, result}; } - // Free a Guarded Control Stack and set the stacks_ slot to null. - void FreeStack(uint64_t gcs) { + // Get a pointer to the GCS storage using a GCS index. + GuardedControlStackStorage* GetGCSPtr(uint64_t gcs_index) { const std::lock_guard lock(stacks_mtx_); - uint64_t gcs_index = GetGCSIndex(gcs); - GuardedControlStack* gcsptr = stacks_[gcs_index]; - if (gcsptr == nullptr) { - VIXL_ABORT_WITH_MSG("Tried to free unallocated GCS "); - } else { - delete gcsptr; - stacks_[gcs_index] = nullptr; - } + return stacks_.at(GetGCSIndexFromToken(gcs_index)); } - // Get a pointer to the GCS vector using a GCS id. - GuardedControlStack* GetGCSPtr(uint64_t gcs) const { - return stacks_[GetGCSIndex(gcs)]; + // Get an index into stacks_ given a GCS token. + static uint64_t GetGCSIndexFromToken(uint64_t token) { + return token >> kPageSizeLog2; } - private: - uint64_t GetGCSIndex(uint64_t gcs) const { return gcs >> 12; } + // Get a GCS token from an index into stacks_. + static uint64_t GetGCSTokenFromIndex(uint64_t index) { + // Shift the index to look like a stack pointer aligned to a page. + return index << kPageSizeLog2; + } - std::vector stacks_; + std::vector stacks_; std::mutex stacks_mtx_; + + friend class Simulator; }; + GuardedControlStackStorage* GetGCSStorage() { return gcs_.ptr; } + uint64_t GetGCSToken() { return gcs_.token; } + // A GCS id indicating no GCS has been allocated. static const uint64_t kGCSNoStack = kPageSize - 1; - uint64_t gcs_; + // We cache both the GCS token, and the pointer to the GCS underlying + // storage, which allows us to avoid calls into GCSManager that + // would require synchronisation. + GuardedControlStack gcs_; bool gcs_enabled_; public: @@ -5559,37 +5598,45 @@ class Simulator : public DecoderVisitor { bool IsGCSCheckEnabled() const { return gcs_enabled_; } private: - bool IsAllocatedGCS(uint64_t gcs) const { return gcs != kGCSNoStack; } void ResetGCSState() { GCSManager& m = GetGCSManager(); - if (IsAllocatedGCS(gcs_)) { - m.FreeStack(gcs_); + // This method is also called in the constructor, before we have set up the + // GCS, so the call to FreeStack must be conditional. + if (GetGCSStorage() != nullptr) { + m.FreeStack(GetGCSToken()); } - ActivateGCS(m.AllocateStack()); + ActivateGCS(m.AllocateStackInternal()); GCSPop(); // Remove seal. } - GuardedControlStack* GetGCSPtr(uint64_t gcs) { + GuardedControlStackStorage* GetGCSPtr(uint64_t gcs) { GCSManager& m = GetGCSManager(); - GuardedControlStack* result = m.GetGCSPtr(gcs); + GuardedControlStackStorage* result = m.GetGCSPtr(gcs); return result; } - GuardedControlStack* GetActiveGCSPtr() { return GetGCSPtr(gcs_); } - uint64_t ActivateGCS(uint64_t gcs) { - uint64_t outgoing_gcs = gcs_; - gcs_ = gcs; - return outgoing_gcs; + GuardedControlStack ActivateGCS(GuardedControlStack incoming) { + GuardedControlStack outgoing = gcs_; + gcs_ = incoming; + return outgoing; + } + + GuardedControlStack ActivateGCS(uint64_t token) { + GuardedControlStack incoming = {GetGCSPtr(token), token}; + GuardedControlStack outgoing = gcs_; + gcs_ = incoming; + return outgoing; } void GCSPush(uint64_t addr) { - GetActiveGCSPtr()->push_back(addr); - size_t entry = GetActiveGCSPtr()->size() - 1; + GuardedControlStackStorage* gcs = GetGCSStorage(); + gcs->push_back(addr); + size_t entry = gcs->size() - 1; LogGCS(/* is_push = */ true, addr, entry); } uint64_t GCSPop() { - GuardedControlStack* gcs = GetActiveGCSPtr(); + GuardedControlStackStorage* gcs = GetGCSStorage(); if (gcs->empty()) { return 0; } @@ -5601,7 +5648,7 @@ class Simulator : public DecoderVisitor { } uint64_t GCSPeek() { - GuardedControlStack* gcs = GetActiveGCSPtr(); + GuardedControlStackStorage* gcs = GetGCSStorage(); if (gcs->empty()) { return 0; } @@ -5610,8 +5657,8 @@ class Simulator : public DecoderVisitor { } void ReportGCSFailure(const char* msg) { + GuardedControlStackStorage* gcs = GetGCSStorage(); if (IsGCSCheckEnabled()) { - GuardedControlStack* gcs = GetActiveGCSPtr(); printf("%s", msg); if (gcs == nullptr) { printf("GCS pointer is null\n"); @@ -5624,7 +5671,7 @@ class Simulator : public DecoderVisitor { gcs->pop_back(); int index = most_recent_index - i; printf(" gcs%" PRIu64 "[%d]: 0x%016" PRIx64 "\n", - gcs_, + GCSManager::GetGCSIndexFromToken(GetGCSToken()), index, entry); } diff --git a/test/aarch64/test-api-aarch64.cc b/test/aarch64/test-api-aarch64.cc index 3ac9efb7..eb893eee 100644 --- a/test/aarch64/test-api-aarch64.cc +++ b/test/aarch64/test-api-aarch64.cc @@ -1770,8 +1770,8 @@ void AllocateAndFreeGCS() { Simulator s(&d); for (int i = 0; i < 100000; i++) { - uint64_t gcs = s.GetGCSManager().AllocateStack(); - s.GetGCSManager().FreeStack(gcs); + uint64_t gcs_token = s.GetGCSManager().AllocateStack(); + s.GetGCSManager().FreeStack(gcs_token); } } diff --git a/test/aarch64/test-assembler-aarch64.cc b/test/aarch64/test-assembler-aarch64.cc index fb01644e..17fcf7e8 100644 --- a/test/aarch64/test-assembler-aarch64.cc +++ b/test/aarch64/test-assembler-aarch64.cc @@ -15192,8 +15192,8 @@ TEST(gcs_gcsss1) { START(); #ifdef VIXL_INCLUDE_SIMULATOR_AARCH64 - uint64_t new_gcs = simulator.GetGCSManager().AllocateStack(); - __ Mov(x0, new_gcs); + uint64_t new_gcs_token = simulator.GetGCSManager().AllocateStack(); + __ Mov(x0, new_gcs_token); #else // TODO: Request new GCS from the operating system. #endif @@ -15224,8 +15224,8 @@ TEST(gcs_stack_swap) { START(); Label stack_swap, sub_fn, end; #ifdef VIXL_INCLUDE_SIMULATOR_AARCH64 - uint64_t new_gcs = simulator.GetGCSManager().AllocateStack(); - __ Mov(x0, new_gcs); + uint64_t new_gcs_token = simulator.GetGCSManager().AllocateStack(); + __ Mov(x0, new_gcs_token); #else // TODO: Request new GCS from the operating system. #endif -- GitLab