From ec1c09d8f809e3be3943b2621ef0f339b7f2d2f6 Mon Sep 17 00:00:00 2001 From: Pedro Henrique Penna Date: Mon, 19 Oct 2026 23:54:31 -0300 Subject: [PATCH] nvx-rs: decode the MAPS entries once for both payload decoders decode_maps_payload, the text decoder that the fake OpenVMM uses, and decode_guest_maps_payload, the byte decoder that the guest agent uses, each carried a copy of the loop that reads and checks the entries of a MAPS payload: their flags and lengths, their sources and targets, the path rules, and the trailing bytes. The copies differed only in how they read a path, as UTF-8 text or as bytes, and in the entry that they build. The path rules were written twice as well, as acceptable for text and acceptable_bytes for bytes, whose checks that a path is not empty and that a source is not `.` its other checks already made. - decode_maps_entries reads and checks the entries once. Each decoder passes it a function that converts a path as soon as it is read and one that builds its entry, and keeps what is its own: the guest decoder's bound on the number of entries, checked before any entry is read, and the allocation that it sizes from that bound. - acceptable checks the bytes of a path, so the encoder and both decoders share it. `/` and NUL occur in UTF-8 only as themselves, so text passes the byte rules exactly when it passed the text rules. What the encoder and each decoder accept, their error messages, and the order of their checks are unchanged: the host decoder still requires a source to be UTF-8 before it reads the target. The host decoder now advances its offset with the guest decoder's checked additions, which follow successful bounds checks and so never fail. A new protocol test pins the messages that both decoders share and the two that differ, and passes before and after the change. A throwaway differential test compared the old and new code on 14.7 million payloads and 332,000 mapping tables: the results of both decoders and of the encoder, messages included, were identical. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- nvx-rs/src/protocol/maps.rs | 116 ++++++++++++++++------------------- nvx-rs/src/protocol/tests.rs | 53 ++++++++++++++++ 2 files changed, 106 insertions(+), 63 deletions(-) diff --git a/nvx-rs/src/protocol/maps.rs b/nvx-rs/src/protocol/maps.rs index 78de1379a..a660e0e7d 100644 --- a/nvx-rs/src/protocol/maps.rs +++ b/nvx-rs/src/protocol/maps.rs @@ -48,7 +48,7 @@ pub fn encode_maps_payloads(maps: &[HostMap], limit: usize) -> Result Result { - let truncated = || violation("the host mapping request is truncated"); let first = u32_at(payload, 0).ok_or_else(truncated)?; let count = u32_at(payload, 4).ok_or_else(truncated)?; let mut maps = Vec::new(); - let mut offset = MAPS_HEADER_LEN; - for _ in 0..count { - let (Some(flags), Some(source_len), Some(target_len)) = ( - u16_at(payload, offset), - u16_at(payload, offset + 2), - u16_at(payload, offset + 4), - ) else { - return Err(truncated()); - }; - offset += MAP_ENTRY_HEADER_LEN; - let mut text = |length: u16| -> Result { - let end = offset + usize::from(length); - let bytes = payload.get(offset..end).ok_or_else(truncated)?; - offset = end; + decode_maps_entries( + payload, + count, + &mut maps, + |bytes| { String::from_utf8(bytes.to_vec()) .map_err(|_| violation("a host mapping is not valid UTF-8")) - }; - let source = text(source_len)?; - let target = text(target_len)?; - if flags & !MAP_READ_ONLY != 0 || !acceptable(&source, &target) { - return Err(violation( - "a host mapping has unknown flags or a source or target that the guest agent \ - refuses", - )); - } - - maps.push(HostMap { + }, + |source, target, read_only| HostMap { source, target, - read_only: flags & MAP_READ_ONLY != 0, - }); - } - if offset != payload.len() { - return Err(violation("the host mapping request has trailing bytes")); - } + read_only, + }, + )?; Ok(MapsPayload { first, maps }) } @@ -170,7 +148,6 @@ pub fn decode_maps_payload(payload: &[u8]) -> Result /// [`decode_maps_payload`] remains UTF-8 for host and fake-OpenVMM callers. This decoder accepts /// every non-NUL Linux path byte that the guest can mount. pub fn decode_guest_maps_payload(payload: &[u8]) -> Result { - let truncated = || violation("the host mapping request is truncated"); let first = u32_at(payload, 0).ok_or_else(truncated)?; let count = u32_at(payload, 4).ok_or_else(truncated)?; let capacity = usize::try_from(count) @@ -178,6 +155,33 @@ pub fn decode_guest_maps_payload(payload: &[u8]) -> Result, M>( + payload: &[u8], + count: u32, + maps: &mut Vec, + path: impl Fn(&[u8]) -> Result, + entry: impl Fn(P, P, bool) -> M, +) -> Result<(), ProtocolError> { let mut offset = MAPS_HEADER_LEN; for _ in 0..count { let (Some(flags), Some(source_len), Some(target_len)) = ( @@ -190,53 +194,40 @@ pub fn decode_guest_maps_payload(payload: &[u8]) -> Result Result, ProtocolError> { + let mut take = |length: u16| -> Result { let end = offset .checked_add(usize::from(length)) .ok_or_else(truncated)?; - let bytes = payload.get(offset..end).ok_or_else(truncated)?.to_vec(); + let bytes = payload.get(offset..end).ok_or_else(truncated)?; offset = end; - Ok(bytes) + path(bytes) }; let source = take(source_len)?; let target = take(target_len)?; - if flags & !MAP_READ_ONLY != 0 || !acceptable_bytes(&source, &target) { + if flags & !MAP_READ_ONLY != 0 || !acceptable(source.as_ref(), target.as_ref()) { return Err(violation( "a host mapping has unknown flags or a source or target that the guest agent \ refuses", )); } - maps.push(GuestHostMap { - source, - target, - read_only: flags & MAP_READ_ONLY != 0, - }); + maps.push(entry(source, target, flags & MAP_READ_ONLY != 0)); } if offset != payload.len() { return Err(violation("the host mapping request has trailing bytes")); } - Ok(GuestMapsPayload { first, maps }) + Ok(()) } -/// Whether the guest agent accepts the paths of a mapping: a relative `source` and an absolute -/// `target` of at most [`MAX_MAP_PATH_BYTES`] without NUL characters, whose `/`-separated -/// components are neither empty, `.`, nor `..`. -fn acceptable(source: &str, target: &str) -> bool { - let normal = |components: &str| { - !components.is_empty() - && components - .split('/') - .all(|component| !matches!(component, "" | "." | "..")) - }; - [source, target] - .iter() - .all(|path| path.len() <= MAX_MAP_PATH_BYTES && !path.contains('\0')) - && normal(source) - && target.strip_prefix('/').is_some_and(normal) +/// Returns the error of a `MAPS` payload that ends before its header or an entry does. +fn truncated() -> ProtocolError { + violation("the host mapping request is truncated") } -/// Applies the guest agent's byte-oriented source and target path rules. -fn acceptable_bytes(source: &[u8], target: &[u8]) -> bool { +/// Whether the guest agent accepts the paths of a mapping: a relative `source` and an absolute +/// `target` of at most [`MAX_MAP_PATH_BYTES`] without NUL characters, whose `/`-separated +/// components are neither empty, `.`, nor `..`. UTF-8 text follows these rules as its bytes, in +/// which `/` and NUL occur only as themselves. +fn acceptable(source: &[u8], target: &[u8]) -> bool { let normal = |components: &[u8]| { !components.is_empty() && components @@ -245,8 +236,7 @@ fn acceptable_bytes(source: &[u8], target: &[u8]) -> bool { }; [source, target] .iter() - .all(|path| !path.is_empty() && path.len() <= MAX_MAP_PATH_BYTES && !path.contains(&0)) - && source != b"." + .all(|path| path.len() <= MAX_MAP_PATH_BYTES && !path.contains(&0)) && normal(source) && target.strip_prefix(b"/").is_some_and(normal) } diff --git a/nvx-rs/src/protocol/tests.rs b/nvx-rs/src/protocol/tests.rs index b96263b89..312071b12 100644 --- a/nvx-rs/src/protocol/tests.rs +++ b/nvx-rs/src/protocol/tests.rs @@ -1317,3 +1317,56 @@ fn guest_maps_decoder_accepts_non_utf8_linux_paths() { let decoded = decode_guest_maps_payload(&payload).unwrap(); assert_eq!(decoded.maps[0].source, b"0/\xff"); } + +/// Both `MAPS` decoders refuse an entry with the same message, except that the host's requires +/// each path to be UTF-8 as soon as it reads it, and the guest's bounds the number of entries +/// before it reads any. +#[test] +fn maps_decoders_refuse_entries_with_the_same_messages() { + let valid = bytes(concat!("0000000001000000", "000001000200", "30", "2f77")); + let mut trailing = valid.clone(); + trailing.push(0); + let mut unknown_flag = valid.clone(); + unknown_flag[8] = 2; + let mut relative_target = valid.clone(); + relative_target[15] = b'w'; + let refused = "a host mapping has unknown flags or a source or target that the guest agent \ + refuses"; + for (payload, expected) in [ + (&valid[..7], "the host mapping request is truncated"), + ( + &valid[..valid.len() - 1], + "the host mapping request is truncated", + ), + (&trailing[..], "the host mapping request has trailing bytes"), + (&unknown_flag[..], refused), + (&relative_target[..], refused), + ] { + assert_eq!( + decode_maps_payload(payload).unwrap_err().message(), + expected + ); + assert_eq!( + decode_guest_maps_payload(payload).unwrap_err().message(), + expected + ); + } + let not_utf8_then_truncated = bytes(concat!("0000000001000000", "000001000200", "ff")); + assert_eq!( + decode_maps_payload(¬_utf8_then_truncated) + .unwrap_err() + .message(), + "a host mapping is not valid UTF-8" + ); + assert_eq!( + decode_guest_maps_payload(¬_utf8_then_truncated) + .unwrap_err() + .message(), + "the host mapping request is truncated" + ); + let too_many = bytes("0000000001100000"); + assert_eq!( + decode_guest_maps_payload(&too_many).unwrap_err().message(), + "the host mapping request has too many entries" + ); +}