Skip to content

DRAFT: migtd: harden EnableLogArea handling (reserved validation + gated set_max_level)#886

Draft
sgrams wants to merge 1 commit into
intel:mainfrom
sgrams:t9-enable-logarea-hardening
Draft

DRAFT: migtd: harden EnableLogArea handling (reserved validation + gated set_max_level)#886
sgrams wants to merge 1 commit into
intel:mainfrom
sgrams:t9-enable-logarea-hardening

Conversation

@sgrams

@sgrams sgrams commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Summary

Hardens the GHCI 1.5 v6 EnableLogArea (operation 4) request path. Three correctness issues are addressed in a single PR.
Closes #664

Spec reference

GHCI 1.5 v6 Table 3-50 defines the EnableLogArea request payload as a 16-byte struct:

Offset Length Field
0 8 mig_request_id
8 1 log_max_level
9 7 reserved (must be 0)

log_max_level must be one of 1=Error, 2=Warn, 3=Info, 4=Debug, 5=Trace. The VMM is the producer; MigTD must validate and propagate.

Issues fixed

  1. log::set_max_level was called unconditionally in both transport handlers (src/migtd/src/bin/migtd/main.rs and src/migtd/src/bin/migtd/cvmemu.rs), even when enable_logarea() had rejected the request with InvalidParameter. A malformed VMM request (e.g. log_max_level = 0xFF) would leave LOGGING_INFORMATION.maxloglevel at 0 (Off) — the structured-log gate — but still flip the crate-global log::max_level() to Trace via the 5 | _ arm of u8_to_levelfilter. The two filters disagreed.

  2. EnableLogAreaInfo.reserved[7] was never validated. PR fix(rebinding): validate all reserved bytes 10..15 #882 added "reserved must be zero" checks for the Prepare Migration / Prepare Rebinding payloads; the same hardening was missing for op 4 (EnableLogArea).

  3. The error-path log message in enable_logarea() incorrectly reused the success message text ("Logging has been enabled with MaxLevel: …") even when rejecting an invalid level.

Fixes

  • src/migtd/src/migration/mod.rs — Replace impl_read_from_bytes!(EnableLogAreaInfo) with a custom read_from_bytes that rejects non-zero reserved bytes with MigrationResult::InvalidParameter (matching the PR fix(rebinding): validate all reserved bytes 10..15 #882 pattern). The now-unused impl_read_from_bytes! macro is removed.
  • src/migtd/src/migration/logging.rs — Move the log::set_max_level(u8_to_levelfilter(log_max_level)) call inside enable_logarea() so it only runs on the validated success path, after the u8_to_loglevel check. The crate-global gate now always matches the per-MigTD LOGGING_INFORMATION.maxloglevel. Correct the error-path log message to say "rejected request with invalid MaxLogLevel".
  • src/migtd/src/bin/migtd/main.rs and src/migtd/src/bin/migtd/cvmemu.rs — Drop the duplicated log::set_max_level and the preceding "Setting log level to …" info! from both handlers (now handled inside enable_logarea). Drop the now-unused u8_to_levelfilter import from cvmemu.rs.

Tests

cargo test -p migtd --lib --features vmcall-raw
…
test result: ok. 15 passed; 0 failed; 0 ignored

All 11 pre-existing migration::logging::test::* cases pass, including test_enable_logarea which exercises both the failure-before-create_logarea path and the success path.

…_max_level)

GHCI 1.5 v6 Table 3-50 specifies the EnableLogArea request as a
16-byte payload: mig_request_id (u64) | log_max_level (u8) |
reserved[7] (MUST be 0). Several issues existed in the handler:

1. log::set_max_level was called unconditionally in both transport
   handlers (main.rs and cvmemu.rs), even when enable_logarea() had
   rejected the request with InvalidParameter. A malformed VMM
   request (e.g. log_max_level = 0xFF) would leave LOGGING_INFORMATION
   .maxloglevel at 0 (Off) but still flip the crate-global log gate
   to Trace via the 5 | _ arm of u8_to_levelfilter, so the two
   filters disagreed.

2. EnableLogAreaInfo.reserved[7] was never validated. PR intel#882 added
   reserved-must-be-zero checks for Prepare Migration / Prepare
   Rebinding payloads; the same hardening was missing for op 4.

3. The error-path log message in enable_logarea() incorrectly
   reused the success message text ("Logging has been enabled
   with MaxLevel") even when rejecting an invalid level.

Fixes:

- Replace impl_read_from_bytes!(EnableLogAreaInfo) with a custom
  read_from_bytes that rejects non-zero reserved bytes with
  InvalidParameter (matching the PR intel#882 pattern). The now-unused
  impl_read_from_bytes! macro is removed.
- Move the log::set_max_level call inside enable_logarea() so it
  only runs on the validated success path, after the u8_to_loglevel
  check. The crate-global gate now always matches the per-MigTD
  gate.
- Drop the duplicated log::set_max_level + 'Setting log level to'
  info! from main.rs and cvmemu.rs handlers, and drop the now-unused
  u8_to_levelfilter import from cvmemu.rs.
- Correct the error-path log message in enable_logarea() to say
  'rejected request with invalid MaxLogLevel'.

Signed-off-by: Stanislaw Grams <stanislaw.grams@intel.com>
@sgrams sgrams self-assigned this Jun 8, 2026
@sgrams sgrams requested a review from MichalTarnacki June 9, 2026 06:49
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.

maxloglevel might not be interpreted correctly

1 participant