runtime: fix UAF from spad lifetime mismatch in prepare_and_execute_txn

src/discof/bank/fd_bank_tile.c

@@ -161,88 +161,92 @@ handle_microblock( fd_bank_ctx_t *     ctx,
 
     txn->flags &= ~FD_TXN_P_FLAGS_SANITIZE_SUCCESS;
 
-    int err = fd_runtime_prepare_and_execute_txn( ctx->banks, ctx->_bank_idx, txn_ctx, txn, ctx->exec_spad, NULL, 0 );
-    if( FD_UNLIKELY( !(txn_ctx->flags & FD_TXN_P_FLAGS_SANITIZE_SUCCESS ) ) ) {
-      ctx->metrics.txn_result[ fd_bank_err_from_runtime_err( err ) ]++;
-      continue;
-    }
+    int err = FD_RUNTIME_EXECUTE_SUCCESS;
+    FD_SPAD_FRAME_BEGIN( ctx->exec_spad ) {
+        err = fd_runtime_prepare_and_execute_txn( ctx->banks, ctx->_bank_idx, txn_ctx, txn, NULL, 0 );
 
-    /* The account keys in the transaction context are laid out such
-       that first the non-alt accounts are laid out, then the writable
-       alt accounts, and finally the read-only alt accounts. */
-    fd_txn_t * txn_descriptor = TXN( &txn_ctx->txn );
-    for( ushort i=txn_descriptor->acct_addr_cnt; i<txn_descriptor->acct_addr_cnt+txn_descriptor->addr_table_adtl_writable_cnt; i++ ) {
-      writable_alt[ i-txn_descriptor->acct_addr_cnt ] = fd_type_pun_const( &txn_ctx->account_keys[ i ] );
-    }
+      if( FD_UNLIKELY( !(txn_ctx->flags & FD_TXN_P_FLAGS_SANITIZE_SUCCESS ) ) ) {
+        ctx->metrics.txn_result[ fd_bank_err_from_runtime_err( err ) ]++;
+        continue;
+      }
 
-    txn->flags |= FD_TXN_P_FLAGS_SANITIZE_SUCCESS;
+      /* The account keys in the transaction context are laid out such
+        that first the non-alt accounts are laid out, then the writable
+        alt accounts, and finally the read-only alt accounts. */
+      fd_txn_t * txn_descriptor = TXN( &txn_ctx->txn );
+      for( ushort i=txn_descriptor->acct_addr_cnt; i<txn_descriptor->acct_addr_cnt+txn_descriptor->addr_table_adtl_writable_cnt; i++ ) {
+        writable_alt[ i-txn_descriptor->acct_addr_cnt ] = fd_type_pun_const( &txn_ctx->account_keys[ i ] );
+      }
 
-    uint requested_exec_plus_acct_data_cus = txn->pack_cu.requested_exec_plus_acct_data_cus;
-    uint non_execution_cus                 = txn->pack_cu.non_execution_cus;
+      txn->flags |= FD_TXN_P_FLAGS_SANITIZE_SUCCESS;
 
-    /* Assume failure, set below if success.  If it doesn't land in the
-       block, rebate the non-execution CUs too. */
-    txn->bank_cu.actual_consumed_cus = 0U;
-    txn->bank_cu.rebated_cus = requested_exec_plus_acct_data_cus + non_execution_cus;
-    txn->flags               &= ~FD_TXN_P_FLAGS_EXECUTE_SUCCESS;
+      uint requested_exec_plus_acct_data_cus = txn->pack_cu.requested_exec_plus_acct_data_cus;
+      uint non_execution_cus                 = txn->pack_cu.non_execution_cus;
 
-    /* Stash the result in the flags value so that pack can inspect it. */
-    /* TODO: Need to translate the err to a hacky Frankendancer style err
-             that pack and GUI expect ... */
-    txn->flags = (txn->flags & 0x00FFFFFFU) | ((uint)(-err)<<24);
+      /* Assume failure, set below if success.  If it doesn't land in the
+        block, rebate the non-execution CUs too. */
+      txn->bank_cu.actual_consumed_cus = 0U;
+      txn->bank_cu.rebated_cus = requested_exec_plus_acct_data_cus + non_execution_cus;
+      txn->flags               &= ~FD_TXN_P_FLAGS_EXECUTE_SUCCESS;
 
-    ctx->metrics.txn_result[ fd_bank_err_from_runtime_err( err ) ]++;
+      /* Stash the result in the flags value so that pack can inspect it. */
+      /* TODO: Need to translate the err to a hacky Frankendancer style err
+              that pack and GUI expect ... */
+      txn->flags = (txn->flags & 0x00FFFFFFU) | ((uint)(-err)<<24);
 
-    uint actual_execution_cus = (uint)(txn_ctx->compute_budget_details.compute_unit_limit - txn_ctx->compute_budget_details.compute_meter);
-    uint actual_acct_data_cus = (uint)(txn_ctx->loaded_accounts_data_size_cost);
+      ctx->metrics.txn_result[ fd_bank_err_from_runtime_err( err ) ]++;
 
-    int is_simple_vote = 0;
-    if( FD_UNLIKELY( is_simple_vote = fd_txn_is_simple_vote_transaction( TXN(txn), txn->payload ) ) ) {
-      /* Simple votes are charged fixed amounts of compute regardless of
-         the real cost they incur.  Unclear what cost is returned by
-         fd_execute txn, however, so we override it here. */
-      actual_execution_cus = FD_PACK_VOTE_DEFAULT_COMPUTE_UNITS;
-      actual_acct_data_cus = 0U;
-    }
+      uint actual_execution_cus = (uint)(txn_ctx->compute_budget_details.compute_unit_limit - txn_ctx->compute_budget_details.compute_meter);
+      uint actual_acct_data_cus = (uint)(txn_ctx->loaded_accounts_data_size_cost);
 
-    /* FeesOnly transactions are transactions that failed to load
-       before they even reach the VM stage. They have zero execution
-       cost but do charge for the account data they are able to load.
-       FeesOnly votes are charged the fixed voe cost. */
-    txn->bank_cu.rebated_cus = requested_exec_plus_acct_data_cus - ( actual_execution_cus + actual_acct_data_cus );
-    txn->bank_cu.actual_consumed_cus = non_execution_cus + actual_execution_cus + actual_acct_data_cus;
-
-    /* TXN_P_FLAGS_EXECUTE_SUCCESS means that it should be included in
-       the block.  It's a bit of a misnomer now that there are fee-only
-       transactions. */
-    FD_TEST( txn_ctx->flags & FD_TXN_P_FLAGS_EXECUTE_SUCCESS );
-    txn->flags |= FD_TXN_P_FLAGS_EXECUTE_SUCCESS;
-
-    /* The VM will stop executing and fail an instruction immediately if
-       it exceeds its requested CUs.  A transaction which requests less
-       account data than it actually consumes will fail in the account
-       loading stage. */
-    if( FD_UNLIKELY( actual_execution_cus+actual_acct_data_cus>requested_exec_plus_acct_data_cus ) ) {
-      FD_LOG_HEXDUMP_WARNING(( "txn", txn->payload, txn->payload_sz ));
-      FD_LOG_ERR(( "Actual CUs unexpectedly exceeded requested amount. actual_execution_cus (%u) actual_acct_data_cus "
-                   "(%u) requested_exec_plus_acct_data_cus (%u) is_simple_vote (%i) exec_failed (%i)",
-                   actual_execution_cus, actual_acct_data_cus, requested_exec_plus_acct_data_cus, is_simple_vote,
-                   err ));
-    }
+      int is_simple_vote = 0;
+      if( FD_UNLIKELY( is_simple_vote = fd_txn_is_simple_vote_transaction( TXN(txn), txn->payload ) ) ) {
+        /* Simple votes are charged fixed amounts of compute regardless of
+          the real cost they incur.  Unclear what cost is returned by
+          fd_execute txn, however, so we override it here. */
+        actual_execution_cus = FD_PACK_VOTE_DEFAULT_COMPUTE_UNITS;
+        actual_acct_data_cus = 0U;
+      }
+
+      /* FeesOnly transactions are transactions that failed to load
+        before they even reach the VM stage. They have zero execution
+        cost but do charge for the account data they are able to load.
+        FeesOnly votes are charged the fixed voe cost. */
+      txn->bank_cu.rebated_cus = requested_exec_plus_acct_data_cus - ( actual_execution_cus + actual_acct_data_cus );
+      txn->bank_cu.actual_consumed_cus = non_execution_cus + actual_execution_cus + actual_acct_data_cus;
+
+      /* TXN_P_FLAGS_EXECUTE_SUCCESS means that it should be included in
+        the block.  It's a bit of a misnomer now that there are fee-only
+        transactions. */
+      FD_TEST( txn_ctx->flags & FD_TXN_P_FLAGS_EXECUTE_SUCCESS );
+      txn->flags |= FD_TXN_P_FLAGS_EXECUTE_SUCCESS;
+
+      /* The VM will stop executing and fail an instruction immediately if
+        it exceeds its requested CUs.  A transaction which requests less
+        account data than it actually consumes will fail in the account
+        loading stage. */
+      if( FD_UNLIKELY( actual_execution_cus+actual_acct_data_cus>requested_exec_plus_acct_data_cus ) ) {
+        FD_LOG_HEXDUMP_WARNING(( "txn", txn->payload, txn->payload_sz ));
+        FD_LOG_ERR(( "Actual CUs unexpectedly exceeded requested amount. actual_execution_cus (%u) actual_acct_data_cus "
+                    "(%u) requested_exec_plus_acct_data_cus (%u) is_simple_vote (%i) exec_failed (%i)",
+                    actual_execution_cus, actual_acct_data_cus, requested_exec_plus_acct_data_cus, is_simple_vote,
+                    err ));
+      }
 
-    /* Commit must succeed so no failure path.  Once commit is called,
-       the transactions MUST be mixed into the PoH otherwise we will
-       fork and diverge, so the link from here til PoH mixin must be
-       completely reliable with nothing dropped.
-
-       fd_runtime_finalize_txn checks if the transaction fits into the
-       block with the cost tracker.  If it doesn't fit, flags is set to
-       zero.  A key invariant of the leader pipeline is that pack
-       ensures all transactions must fit already, so it is a fatal error
-       if that happens.  We cannot reject the transaction here as there
-       would be no way to undo the partially applied changes to the bank
-       in finalize anyway. */
-    fd_runtime_finalize_txn( ctx->txn_ctx->funk, txn_ctx->status_cache, txn_ctx->xid, txn_ctx, bank, NULL );
+      /* Commit must succeed so no failure path.  Once commit is called,
+        the transactions MUST be mixed into the PoH otherwise we will
+        fork and diverge, so the link from here til PoH mixin must be
+        completely reliable with nothing dropped.
+
+        fd_runtime_finalize_txn checks if the transaction fits into the
+        block with the cost tracker.  If it doesn't fit, flags is set to
+        zero.  A key invariant of the leader pipeline is that pack
+        ensures all transactions must fit already, so it is a fatal error
+        if that happens.  We cannot reject the transaction here as there
+        would be no way to undo the partially applied changes to the bank
+        in finalize anyway. */
+      fd_runtime_finalize_txn( ctx->txn_ctx->funk, txn_ctx->status_cache, txn_ctx->xid, txn_ctx, bank, NULL );
+    } FD_SPAD_FRAME_END;
     FD_TEST( txn->flags );
   }
 
@@ -326,7 +330,7 @@ handle_bundle( fd_bank_ctx_t *     ctx,
 
     fd_exec_txn_ctx_t txn_ctx[ 1 ]; // TODO ... bank manager ?
     txn->flags &= ~(FD_TXN_P_FLAGS_SANITIZE_SUCCESS | FD_TXN_P_FLAGS_EXECUTE_SUCCESS);
-    int err = fd_runtime_prepare_and_execute_txn( NULL, ULONG_MAX, txn_ctx, txn, NULL, NULL, 0 ); /* TODO ... */
+    int err = fd_runtime_prepare_and_execute_txn( NULL, ULONG_MAX, txn_ctx, txn, NULL, 0 ); /* TODO ... */
 
     transaction_err[ i ] = err;
     if( FD_UNLIKELY( err ) ) {

src/discof/exec/fd_exec_tile.c

@@ -115,12 +115,15 @@ during_frag( fd_exec_tile_ctx_t * ctx,
     if( FD_LIKELY( (sig>>32)==EXEC_NEW_TXN_SIG ) ) {
       fd_exec_txn_msg_t * txn = (fd_exec_txn_msg_t *)fd_chunk_to_laddr( ctx->replay_in->mem, chunk );
 
+
+
+      /* Keep spad frame until txn is finalized (per txn spad frame) */
+      fd_spad_push( ctx->exec_spad );
       ctx->txn_ctx->exec_err = fd_runtime_prepare_and_execute_txn(
           ctx->banks,
           txn->bank_idx,
           ctx->txn_ctx,
           &txn->txn,
-          ctx->exec_spad,
           ctx->capture_ctx,
           1 );
     } else {
@@ -184,6 +187,9 @@ after_frag( fd_exec_tile_ctx_t * ctx,
         fd_banks_mark_bank_dead( ctx->banks, bank );
       }
 
+      /* Pop the per txn spad frame */
+      fd_spad_pop( ctx->exec_spad );
+
       /* Notify the replay tile that we are done with this txn. */
       ctx->pending_txn_finalized_msg = 1;
     } else {

src/flamenco/runtime/fd_runtime.c

@@ -1213,10 +1213,8 @@ fd_runtime_prepare_and_execute_txn( fd_banks_t *        banks,
                                     ulong               bank_idx,
                                     fd_exec_txn_ctx_t * txn_ctx,
                                     fd_txn_p_t *        txn,
-                                    fd_spad_t *         exec_spad,
                                     fd_capture_ctx_t *  capture_ctx,
                                     uchar               do_sigverify ) {
-  FD_SPAD_FRAME_BEGIN( exec_spad ) {
   int exec_res = 0;
 
   fd_bank_t * bank = fd_banks_bank_query( banks, bank_idx );
@@ -1268,7 +1266,6 @@ fd_runtime_prepare_and_execute_txn( fd_banks_t *        banks,
 
   return exec_res;
 
-  } FD_SPAD_FRAME_END;
 }
 
 /* fd_executor_txn_verify and fd_runtime_pre_execute_check are responisble

src/flamenco/runtime/fd_runtime.h

@@ -461,7 +461,6 @@ fd_runtime_prepare_and_execute_txn( fd_banks_t *        banks,
                                     ulong               bank_idx,
                                     fd_exec_txn_ctx_t * txn_ctx,
                                     fd_txn_p_t *        txn,
-                                    fd_spad_t *         exec_spad,
                                     fd_capture_ctx_t *  capture_ctx,
                                     uchar               do_sigverify );
 

src/flamenco/runtime/tests/fd_txn_harness.c

@@ -353,7 +353,6 @@ fd_runtime_fuzz_txn_ctx_exec( fd_solfuzz_runner_t *     runner,
       0UL,
       txn_ctx,
       txn,
-      runner->spad,
       NULL,
       0 );