From 73acff277a1da6a5a51d36f8447b7494c6147f0f Mon Sep 17 00:00:00 2001 From: Brian Carr Date: Tue, 2 Jun 2026 18:19:47 +0300 Subject: [PATCH] pldm-fw: ua: send actual component count in RequestUpdate request_update() hardcoded the RequestUpdate NumberOfComponents field to 1. DSP0267 Table 27 defines this field as "the number of components that will be passed to the FD during the update", and the FD may use it to compare against the number of PassComponentTable/UpdateComponent commands it receives. When a package applies more than one component, the UA passes all of them (pass_component_table and update_components_progress iterate over update.components) while still announcing only 1, which a conformant FD can reject as a mismatch. Derive the count from update.components, the same list driven through the rest of the update flow, so the announced value matches what is actually sent. The value is fallibly converted to u16 to guard the (practically impossible) >65535 component case rather than truncating. Add a unit test (request_update_reports_actual_component_count) that drives request_update over a mock MCTP ReqChannel and asserts the RequestUpdate NumberOfComponents field reflects the actual component count, along with the cfg(test) package-builder and temp-file helpers it depends on. Signed-off-by: Brian Carr --- Cargo.lock | 1 + pldm-fw/Cargo.toml | 3 + pldm-fw/src/pkg.rs | 129 +++++++++++++++++++++++++++++++++++++++++ pldm-fw/src/ua.rs | 140 ++++++++++++++++++++++++++++++++++++++++++++- 4 files changed, 272 insertions(+), 1 deletion(-) diff --git a/Cargo.lock b/Cargo.lock index b1c4ecf9..6aac75f4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1174,6 +1174,7 @@ dependencies = [ "num-derive", "num-traits", "pldm", + "tempfile", "thiserror 1.0.69", "uuid", ] diff --git a/pldm-fw/Cargo.toml b/pldm-fw/Cargo.toml index 40bed560..1a4f86a9 100644 --- a/pldm-fw/Cargo.toml +++ b/pldm-fw/Cargo.toml @@ -25,3 +25,6 @@ uuid = { workspace = true, features = ["v1"] } default = ["std"] alloc = ["pldm/alloc", "nom/alloc"] std = ["alloc", "pldm/std", "mctp/std", "nom/std", "chrono/clock", "uuid/std", "dep:thiserror"] + +[dev-dependencies] +tempfile = "3.27.0" diff --git a/pldm-fw/src/pkg.rs b/pldm-fw/src/pkg.rs index b97e6b65..236de534 100644 --- a/pldm-fw/src/pkg.rs +++ b/pldm-fw/src/pkg.rs @@ -313,3 +313,132 @@ impl Package { Ok(self.file.read_at(buf, file_offset)?) } } + +/// Write `bytes` to a fresh anonymous temporary file and return a handle to +/// it, seeked back to the start ready for reading. +#[cfg(test)] +pub(crate) fn temp_file_with(bytes: &[u8]) -> std::fs::File { + use std::io::{Seek, SeekFrom, Write}; + + let mut f = tempfile::tempfile().unwrap(); + f.write_all(bytes).unwrap(); + f.seek(SeekFrom::Start(0)).unwrap(); + f +} + +#[cfg(test)] +pub(crate) mod tests { + use super::*; + + /// Build the bytes of a minimal, well-formed v1.1.x firmware update + /// package. + /// + /// The package describes a single device identified by one PCI Vendor + /// ID descriptor (`vid`), whose component bitmap selects every supplied + /// component. Each entry of `components` becomes a component image + /// appended after the (CRC-protected) package header, with matching + /// file offset/size fields. + pub(crate) fn build_v11_package(vid: u16, components: &[&[u8]]) -> Vec { + let ncomp = components.len(); + assert!(ncomp >= 1); + + // --- single device record --- + let bitmap_bytes = ncomp.div_ceil(8); + let mut bitmap = vec![0u8; bitmap_bytes]; + for i in 0..ncomp { + bitmap[i / 8] |= 1u8 << (i % 8); + } + let set_ver = b"0000"; + + let mut descs = Vec::new(); + descs.extend_from_slice(&0x0000u16.to_le_bytes()); // type: PCI Vendor ID + descs.extend_from_slice(&2u16.to_le_bytes()); // length + descs.extend_from_slice(&vid.to_le_bytes()); // data + let desc_count = 1u8; + let pkg_data_len = 0u16; + + let mut rec_body = Vec::new(); + rec_body.extend_from_slice(&bitmap); + rec_body.extend_from_slice(set_ver); + rec_body.extend_from_slice(&descs); + let rec_len = (11 + rec_body.len()) as u16; + + let mut device = Vec::new(); + device.extend_from_slice(&rec_len.to_le_bytes()); + device.push(desc_count); + device.extend_from_slice(&0u32.to_le_bytes()); // option flags + device.push(1u8); // set version string type (utf-8) + device.push(set_ver.len() as u8); + device.extend_from_slice(&pkg_data_len.to_le_bytes()); + device.extend_from_slice(&rec_body); + + // Header bytes following the 19-byte init region, excluding the + // trailing 4-byte checksum. `offsets` carries each component's + // absolute file offset. + let build_pre = |offsets: &[usize]| -> Vec { + let mut pre = Vec::new(); + pre.extend_from_slice(&[0u8; 13]); // release date/time + pre.extend_from_slice(&(ncomp as u16).to_le_bytes()); // bitmap length (bits) + + // package version string (type, length, data) + pre.push(1u8); + pre.push(4u8); + pre.extend_from_slice(b"0000"); + // device id record area + pre.push(1u8); // device count + pre.extend_from_slice(&device); + // downstream device id record area (1.1.x): none + pre.push(0u8); + // component image information area + pre.extend_from_slice(&(ncomp as u16).to_le_bytes()); + for (i, c) in components.iter().enumerate() { + pre.extend_from_slice(&0x000au16.to_le_bytes()); // classification: firmware + pre.extend_from_slice(&(i as u16).to_le_bytes()); // identifier + pre.extend_from_slice(&0u32.to_le_bytes()); // comparison stamp + pre.extend_from_slice(&0u16.to_le_bytes()); // options + pre.extend_from_slice(&0u16.to_le_bytes()); // activation method + pre.extend_from_slice(&(offsets[i] as u32).to_le_bytes()); + pre.extend_from_slice(&(c.len() as u32).to_le_bytes()); + // component version string (type, length, data) + pre.push(1u8); + pre.push(4u8); + pre.extend_from_slice(b"0000"); + } + pre + }; + + const HDR_INIT_SIZE: usize = 16 + 1 + 2; + + // First pass with placeholder offsets to learn the header size, + // which is independent of the (fixed-size) offset field values. + let pre_len = build_pre(&vec![0usize; ncomp]).len(); + let hdr_size = HDR_INIT_SIZE + pre_len + 4; + + // Second pass: real offsets point past the header into the payload + // area. + let mut offsets = vec![0usize; ncomp]; + let mut cum = hdr_size; + for (i, c) in components.iter().enumerate() { + offsets[i] = cum; + cum += c.len(); + } + let pre = build_pre(&offsets); + + let mut header = Vec::new(); + header.extend_from_slice(PKG_UUID_1_1_X.as_bytes()); + header.push(1u8); // header format revision + header.extend_from_slice(&(hdr_size as u16).to_le_bytes()); + header.extend_from_slice(&pre); + + let crc32 = crc::Crc::::new(&crc::CRC_32_ISO_HDLC); + let checksum = crc32.checksum(&header); + header.extend_from_slice(&checksum.to_le_bytes()); + assert_eq!(header.len(), hdr_size); + + let mut file = header; + for c in components { + file.extend_from_slice(c); + } + file + } +} diff --git a/pldm-fw/src/ua.rs b/pldm-fw/src/ua.rs index 9e4c2fcf..554cf14d 100644 --- a/pldm-fw/src/ua.rs +++ b/pldm-fw/src/ua.rs @@ -180,9 +180,13 @@ pub fn request_update( check_fd_state(comm, PldmFDState::Idle)?; let sz = XFER_SIZE as u32; + let num_components: u16 = + update.components.len().try_into().map_err(|_| { + PldmUpdateError::new_update("too many components".into()) + })?; let mut data = vec![]; data.extend_from_slice(&sz.to_le_bytes()); - data.extend_from_slice(&1u16.to_le_bytes()); // NumberOfComponents + data.extend_from_slice(&num_components.to_le_bytes()); // NumberOfComponents data.extend_from_slice(&1u8.to_le_bytes()); // MaximumOutstandingTransferRequests data.extend_from_slice(&0u16.to_le_bytes()); // PackageDataLength update.package.version.write_utf8_bytes(&mut data); @@ -582,3 +586,137 @@ fn check_fd_state( Ok(()) } + +#[cfg(test)] +mod tests { + use super::{request_update, Update}; + use crate::{ + Descriptor, DescriptorString, DeviceCapabilities, DeviceIdentifiers, + FirmwareParameters, GetStatusResponse, PldmFDState, PLDM_TYPE_FW, + }; + use mctp::{Eid, MsgIC, MsgType, ReqChannel, MCTP_TYPE_PLDM}; + use std::collections::VecDeque; + + // --- Mock MCTP transport --------------------------------------------- + // + // request_update only drives the UA-initiated `ReqChannel` (`comm`): + // a GetStatus precondition followed by RequestUpdate. The mock replays + // scripted FD responses and records everything the UA sends so the test + // can assert on the wire format. + + #[derive(Default)] + struct MockComm { + responses: VecDeque>, + sent: Vec>, + } + + impl MockComm { + fn new(responses: Vec>) -> Self { + Self { + responses: responses.into(), + sent: Vec::new(), + } + } + + fn sent(&self) -> Vec> { + self.sent.clone() + } + } + + impl ReqChannel for MockComm { + fn send_vectored( + &mut self, + _typ: MsgType, + _ic: MsgIC, + bufs: &[&[u8]], + ) -> mctp::Result<()> { + let mut msg = Vec::new(); + for b in bufs { + msg.extend_from_slice(b); + } + self.sent.push(msg); + Ok(()) + } + + fn recv<'f>( + &mut self, + buf: &'f mut [u8], + ) -> mctp::Result<(MsgType, MsgIC, &'f mut [u8])> { + let resp = + self.responses.pop_front().ok_or(mctp::Error::RxFailure)?; + buf[..resp.len()].copy_from_slice(&resp); + Ok((MCTP_TYPE_PLDM, MsgIC(false), &mut buf[..resp.len()])) + } + + fn remote_eid(&self) -> Eid { + Eid(8) + } + } + + // --- frame helpers --------------------------------------------------- + + /// A PLDM response frame as returned by the FD over `comm`. + fn resp_frame(cmd: u8, cc: u8, data: &[u8]) -> Vec { + let mut v = vec![0u8, PLDM_TYPE_FW, cmd, cc]; + v.extend_from_slice(data); + v + } + + fn status_data(state: PldmFDState) -> Vec { + let s = GetStatusResponse { + current_state: state, + previous_state: PldmFDState::Idle, + aux_state: 0, + aux_state_status: 0, + progress_percent: 0, + reason_code: 0, + update_option_flags_enabled: 0, + }; + let mut b = [0u8; 16]; + let l = s.write_buf(&mut b).unwrap(); + b[..l].to_vec() + } + + fn make_update(vid: u16, components: &[&[u8]]) -> Update { + let bytes = crate::pkg::tests::build_v11_package(vid, components); + let pkg = + crate::pkg::Package::parse(crate::pkg::temp_file_with(&bytes)) + .unwrap(); + let dev = DeviceIdentifiers { + ids: vec![Descriptor::PciVid(vid)], + }; + let fwp = FirmwareParameters { + caps: DeviceCapabilities::from_u32(0), + components: Vec::new().into(), + active: DescriptorString::empty(), + pending: DescriptorString::empty(), + }; + Update::new(&dev, &fwp, pkg, None, None, vec![]).unwrap() + } + + // --- RequestUpdate --------------------------------------------------- + + #[test] + fn request_update_reports_actual_component_count() { + let img: &[u8] = &[0u8; 64]; + let update = make_update(0xabcd, &[img, img]); + + let mut comm = MockComm::new(vec![ + resp_frame(0x1b, 0, &status_data(PldmFDState::Idle)), + // RequestUpdateResponse: FirmwareDeviceMetaDataLength + flag + resp_frame(0x10, 0, &[0, 0, 0]), + ]); + + request_update(&mut comm, &update).unwrap(); + + let sent = comm.sent(); + assert_eq!(sent.len(), 2); + // sent[1] is RequestUpdate: [0x80, type, 0x10, ] + let ru = &sent[1]; + assert_eq!(ru[2], 0x10); + let data = &ru[3..]; + // payload: MaxTransferSize(4), NumberOfComponents(2), ... + let num_components = u16::from_le_bytes([data[4], data[5]]); + assert_eq!(num_components, 2); + } +}