Skip to content

Add support for running multiple TA instances with page table isolation#632

Merged
sangho2 merged 14 commits intomainfrom
sanghle/lvbs/multi_ta
Feb 8, 2026
Merged

Add support for running multiple TA instances with page table isolation#632
sangho2 merged 14 commits intomainfrom
sanghle/lvbs/multi_ta

Conversation

@sangho2
Copy link
Contributor

@sangho2 sangho2 commented Feb 2, 2026

This PR adds support for running multiple TAs concurrently with per-instance page table isolation on LVBS.

Page Table Management

  • PageTableManager manages base page table and task-specific page tables
  • Each TA instance gets its own task page table
  • Base page table contains only VTL1 kernel mappings

Session/Instance Management

  • New session.rs module manages TA session/instance
  • Each instance tracks its page table ID for memory isolation
  • Sessions can share the same instance with a try-lock
  • Proper cleanup of page tables when sessions/instances are closed or TA panics

OP-TEE Shim Updates

  • Update message handler and syscalls to work with per-instance page tables
  • Refactor pointer validation (ptr.rs) for the new isolation model
  • Enhance ELF loader integration with session-aware memory management

Runner Updates

  • Significant refactoring to support multi-TA dispatch

@sangho2 sangho2 force-pushed the sanghle/lvbs/multi_ta branch from 2da1c61 to c434698 Compare February 2, 2026 16:53
@sangho2 sangho2 marked this pull request as ready for review February 2, 2026 16:56
@sangho2 sangho2 requested a review from wdcui February 2, 2026 16:56
@wdcui
Copy link
Member

wdcui commented Feb 2, 2026

🔍 PR #632 Review Summary: Multi-TA Page Table Isolation

Critical Findings Consensus

Category Severity Issue Location
Correctness/Security 🔴 Critical Arc reference counting bugs in cleanup logic cause resource leaks lib.rs:442, 795, 915
Correctness 🔴 Critical instance_count() double-counts single-instance TAs lib.rs:198
Compatibility 🔴 Critical TeeOrigin enum values changed (0→1 based) - ABI break litebox_common_optee/lib.rs:818-821
Compatibility 🔴 Critical Platform thread function signatures changed (return type removed) litebox_platform_lvbs/lib.rs:1251
Security 🟠 High Race condition between Arc::strong_count check and cleanup lib.rs:915
Security 🟠 High Page table deletion while references may exist (use-after-free risk) litebox_platform_lvbs/lib.rs:230-240
Compatibility 🟠 High TeeResult::Unknown variant removed lib.rs:931-1022
Compatibility 🟠 High OpteeMsgParamFmem.offs_high changed u32→u16 lib.rs:1310
Performance 🟠 High O(N) page table lookup on every syscall litebox_platform_lvbs/lib.rs:124-131
Performance 🟡 Medium Two lock acquisitions per OpenSession for counting lib.rs:197-199

📋 Consolidated Recommendations

Must Fix (Blockers):

  1. Fix Arc reference counting logic - The strong_count checks are fundamentally broken. Remove items from cache before checking count, or use dedicated reference tracking
  2. Fix instance_count() calculation - Currently double-counts single-instance TAs
  3. Revert TeeOrigin enum values to 0-based to maintain OP-TEE ABI compatibility
  4. Restore TeeResult::Unknown or make enum #[non_exhaustive]
  5. Restore original thread function signatures or provide deprecation path

Should Fix:

  • Add reverse lookup HashMap for CR3→page table ID (O(1) vs O(N))
  • Cache physical frame in PageTable struct
  • Add safety comments to all unsafe blocks in paging.rs
  • Document TOCTOU assumptions in pointer validation

Consider:

  • Atomic counter for instance count instead of dual-lock approach
  • RwLock for session map to allow concurrent reads

✅ Positive Aspects Noted

  • Proper memory isolation via separate page tables
  • Clean session/instance lifecycle management design
  • Proper cleanup of page tables on session close
  • Try-lock pattern for preventing concurrent TA access

Overall Verdict: The PR implements valuable multi-TA isolation features, but has critical bugs in cleanup logic and breaking ABI/API changes that must be addressed before merge.


Review conducted by 5 specialized agents analyzing: correctness, security, performance, compatibility, and code quality.

@wdcui
Copy link
Member

wdcui commented Feb 3, 2026

@CvvT it would be very helpful to have you take a look at the low-level code changes in litebox_platform_lvbs. Thanks!

Copy link
Member

@wdcui wdcui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this big code change. I left some comments below.

Copy link
Contributor

@CvvT CvvT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The page table manager seems to be useful for SNP as well. Current implementation of SNP runner can only run one program because it can only set one page table once. I'm considering having a common crate to share it.

@sangho2 sangho2 force-pushed the sanghle/lvbs/multi_ta branch 2 times, most recently from 0df05d1 to 8ea2327 Compare February 6, 2026 04:45
Copy link
Member

@wdcui wdcui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this substantial PR.

@sangho2 sangho2 force-pushed the sanghle/lvbs/multi_ta branch from 8ea2327 to 8b34080 Compare February 8, 2026 03:24
@github-actions
Copy link

github-actions bot commented Feb 8, 2026

🤖 SemverChecks 🤖 ⚠️ Potential breaking API changes detected ⚠️

Click for details
--- failure enum_struct_variant_field_added: pub enum struct variant field added ---

Description:
An enum's exhaustive struct variant has a new field, which has to be included when constructing or matching on this variant.
        ref: https://doc.rust-lang.org/reference/attributes/type_system.html#the-non_exhaustive-attribute
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.46.0/src/lints/enum_struct_variant_field_added.ron

Failed in:
  field msg_args of variant OpteeSmcResult::CallWithArg in /home/runner/work/litebox/litebox/litebox_common_optee/src/lib.rs:1661

--- failure enum_struct_variant_field_missing: pub enum struct variant's field removed or renamed ---

Description:
A publicly-visible enum has a struct variant whose field is no longer available under its prior name. It may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.46.0/src/lints/enum_struct_variant_field_missing.ron

Failed in:
  field msg_arg of variant OpteeSmcResult::CallWithArg, previously in file /home/runner/work/litebox/litebox/target/semver-checks/git-main/24e54ec61cf39f72d29e245b0550ac25ed090e0e/litebox_common_optee/src/lib.rs:1571

--- failure enum_variant_missing: pub enum variant removed or renamed ---

Description:
A publicly-visible enum has at least one variant that is no longer available under its prior name. It may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.46.0/src/lints/enum_variant_missing.ron

Failed in:
  variant TeeResult::Unknown, previously in file /home/runner/work/litebox/litebox/target/semver-checks/git-main/24e54ec61cf39f72d29e245b0550ac25ed090e0e/litebox_common_optee/src/lib.rs:934

--- failure inherent_method_missing: pub method removed or renamed ---

Description:
A publicly-visible method or associated fn is no longer available under its prior name. It may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.46.0/src/lints/inherent_method_missing.ron

Failed in:
  OpteeSmcArgs::optee_msg_arg_phys_addr, previously in file /home/runner/work/litebox/litebox/target/semver-checks/git-main/24e54ec61cf39f72d29e245b0550ac25ed090e0e/litebox_common_optee/src/lib.rs:1496
  TeeUuid::from_u32_array, previously in file /home/runner/work/litebox/litebox/target/semver-checks/git-main/24e54ec61cf39f72d29e245b0550ac25ed090e0e/litebox_common_optee/src/lib.rs:583

--- failure function_now_returns_unit: pub fn now returns unit ---

Description:
A public function that used to return a value now returns `()`.
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.46.0/src/lints/function_now_returns_unit.ron
Failed in:
  litebox_platform_linux_userland::reenter_thread in /home/runner/work/litebox/litebox/litebox_platform_linux_userland/src/lib.rs:345
  litebox_platform_linux_userland::run_thread in /home/runner/work/litebox/litebox/litebox_platform_linux_userland/src/lib.rs:317

--- failure function_missing: pub fn removed or renamed ---

Description:
A publicly-visible function cannot be imported by its prior path. A `pub use` may have been removed, or the function itself may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.46.0/src/lints/function_missing.ron

Failed in:
  function litebox_platform_lvbs::reenter_thread, previously in file /home/runner/work/litebox/litebox/target/semver-checks/git-main/24e54ec61cf39f72d29e245b0550ac25ed090e0e/litebox_platform_lvbs/src/lib.rs:943

--- failure function_now_returns_unit: pub fn now returns unit ---

Description:
A public function that used to return a value now returns `()`.
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.46.0/src/lints/function_now_returns_unit.ron
Failed in:
  litebox_platform_lvbs::run_thread in /home/runner/work/litebox/litebox/litebox_platform_lvbs/src/lib.rs:1306

--- failure constructible_struct_adds_field: externally-constructible struct adds field ---

Description:
A pub struct constructible with a struct literal has a new pub field. Existing struct literals must be updated to include the new field.
        ref: https://doc.rust-lang.org/reference/expressions/struct-expr.html
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.46.0/src/lints/constructible_struct_adds_field.ron

Failed in:
  field TaRequestInfo.client_identity in /home/runner/work/litebox/litebox/litebox_shim_optee/src/msg_handler.rs:194
  field LoadedProgram.ta_flags in /home/runner/work/litebox/litebox/litebox_shim_optee/src/lib.rs:307

--- failure function_missing: pub fn removed or renamed ---

Description:
A publicly-visible function cannot be imported by its prior path. A `pub use` may have been removed, or the function itself may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.46.0/src/lints/function_missing.ron

Failed in:
  function litebox_shim_optee::msg_handler::handle_optee_msg_arg, previously in file /home/runner/work/litebox/litebox/target/semver-checks/git-main/24e54ec61cf39f72d29e245b0550ac25ed090e0e/litebox_shim_optee/src/msg_handler.rs:147
  function litebox_shim_optee::msg_handler::prepare_for_return_to_normal_world, previously in file /home/runner/work/litebox/litebox/target/semver-checks/git-main/24e54ec61cf39f72d29e245b0550ac25ed090e0e/litebox_shim_optee/src/msg_handler.rs:351

--- failure method_parameter_count_changed: pub method parameter count changed ---

Description:
A publicly-visible method now takes a different number of parameters, not counting the receiver (self) parameter.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#fn-change-arity
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.46.0/src/lints/method_parameter_count_changed.ron

Failed in:
  litebox_shim_optee::OpteeShim::load_ldelf now takes 5 parameters instead of 4, in /home/runner/work/litebox/litebox/litebox_shim_optee/src/lib.rs:207

--- failure struct_missing: pub struct removed or renamed ---

Description:
A publicly-visible struct cannot be imported by its prior path. A `pub use` may have been removed, or the struct itself may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.46.0/src/lints/struct_missing.ron

Failed in:
  struct litebox_shim_optee::SessionIdPool, previously in file /home/runner/work/litebox/litebox/target/semver-checks/git-main/24e54ec61cf39f72d29e245b0550ac25ed090e0e/litebox_shim_optee/src/lib.rs:1202

@sangho2 sangho2 added this pull request to the merge queue Feb 8, 2026
Merged via the queue into main with commit 86e26df Feb 8, 2026
14 checks passed
@sangho2 sangho2 deleted the sanghle/lvbs/multi_ta branch February 8, 2026 04:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants