From 1df5490719012b5c282164a876f28fb0fe05c673 Mon Sep 17 00:00:00 2001 From: Matt Johnston Date: Fri, 15 Aug 2025 12:03:16 +0800 Subject: [PATCH 1/4] pldm-platform: No CRC in GetPDRResp::new_single StartAndEnd packets shouldn't include the CRC. Signed-off-by: Matt Johnston --- pldm-platform/src/proto.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/pldm-platform/src/proto.rs b/pldm-platform/src/proto.rs index e93c71d..b12677b 100644 --- a/pldm-platform/src/proto.rs +++ b/pldm-platform/src/proto.rs @@ -675,8 +675,7 @@ impl GetPDRResp { next_data_transfer_handle: 0, transfer_flag: xfer_flag::START_AND_END, record_data: Default::default(), - // TODO crc - crc: Some(0), + crc: None, }; let cap = s.record_data.capacity(); s.record_data.resize_default(cap).unwrap(); From 11227c1eadcec2cf38807105c92b87c7f2d1e5cf Mon Sep 17 00:00:00 2001 From: Matt Johnston Date: Mon, 18 Aug 2025 11:51:08 +0800 Subject: [PATCH 2/4] pldm-file: Use static CRC32. Update crc crate Signed-off-by: Matt Johnston --- Cargo.lock | 4 ++-- Cargo.toml | 2 +- pldm-file/src/host.rs | 7 +++++-- 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 66e2477..8d4946a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -371,9 +371,9 @@ checksum = "06ea2b9bc92be3c2baa9334a323ebca2d6f074ff852cd1d7b11064035cd3868f" [[package]] name = "crc" -version = "3.0.1" +version = "3.3.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "86ec7a15cbe22e59248fc7eadb1907dab5ba09372595da4d73dd805ed4417dfe" +checksum = "9710d3b3739c2e349eb44fe848ad0b7c8cb1e42bd87ee49371df2f7acaf3e675" dependencies = [ "crc-catalog", ] diff --git a/Cargo.toml b/Cargo.toml index 3981216..9e9826f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -11,7 +11,7 @@ repository = "https://github.com/CodeConstruct/mctp-rs" anyhow = "1.0.80" argh = "0.1.12" chrono = { version = "0.4", default-features = false } -crc = "3.0" +crc = "3.3" defmt = "0.3" deku = { git = "https://github.com/CodeConstruct/deku.git", tag = "cc/deku-v0.19.1/no-alloc-3", default-features = false } embedded-io-adapters = { version = "0.6", features = ["std", "futures-03"] } diff --git a/pldm-file/src/host.rs b/pldm-file/src/host.rs index 830de17..696ccb0 100644 --- a/pldm-file/src/host.rs +++ b/pldm-file/src/host.rs @@ -80,6 +80,9 @@ impl std::fmt::Display for PldmFileError { } } +const CRC32: crc::Crc> = + crc::Crc::>::new(&crc::CRC_32_ISO_HDLC); + type Result = std::result::Result; impl Responder { @@ -402,8 +405,8 @@ impl Responder { let data = &mut resp_data[l..]; host.read(data, xfer_ctx.start + offset) .map_err(|_| CCode::ERROR)?; - let crc32 = crc::Crc::::new(&crc::CRC_32_ISO_HDLC); - let cs = crc32.checksum(data); + + let cs = CRC32.checksum(data); resp_data.extend_from_slice(&cs.to_le_bytes()); xfer_ctx.offset = offset; From 7bb3e3d6c1ac07b137ca50341fbae93087932c6a Mon Sep 17 00:00:00 2001 From: Jeremy Kerr Date: Mon, 18 Aug 2025 12:06:34 +0800 Subject: [PATCH 3/4] pldm-file: use cumulative crc for multipart transfers Regarding the part checksum field, DSP0240 says: 32-bit cumulative CRC for the entirety of data received so far for this section (all parts concatenated together, excluding checksums). Shall be included with all part transfers. So, we need to keep the checksum Digest around, and use the cumulative value in responses & response checks. We also shift the comment on restarting from a FIRST_PART, as it was incorrectly placed in the non-restart case. Signed-off-by: Jeremy Kerr --- pldm-file/src/client.rs | 7 ++++--- pldm-file/src/host.rs | 36 +++++++++++++++++++++++++----------- 2 files changed, 29 insertions(+), 14 deletions(-) diff --git a/pldm-file/src/client.rs b/pldm-file/src/client.rs index 1f6bb88..2975440 100644 --- a/pldm-file/src/client.rs +++ b/pldm-file/src/client.rs @@ -167,6 +167,8 @@ where req_offset, req_length, }; + let crc32 = crc::Crc::::new(&crc::CRC_32_ISO_HDLC); + let mut digest = crc32.digest(); loop { let mut tx_buf = [0; 18]; let l = req.to_slice(&mut tx_buf).map_err(|_| PldmError::NoSpace)?; @@ -196,12 +198,11 @@ where let (resp_data, resp_cs) = rest.split_at(resp_data_len); - let crc32 = crc::Crc::::new(&crc::CRC_32_ISO_HDLC); - let calc_cs = crc32.checksum(resp_data); + digest.update(resp_data); // unwrap: we have asserted the lengths above let cs = u32::from_le_bytes(resp_cs.try_into().unwrap()); - if calc_cs != cs { + if digest.clone().finalize() != cs { return Err(proto_error!("data checksum mismatch")); } diff --git a/pldm-file/src/host.rs b/pldm-file/src/host.rs index 696ccb0..bbd67f9 100644 --- a/pldm-file/src/host.rs +++ b/pldm-file/src/host.rs @@ -23,6 +23,9 @@ pub trait Host { fn read(&self, buf: &mut [u8], offset: usize) -> std::io::Result; } +const CRC32: crc::Crc> = + crc::Crc::>::new(&crc::CRC_32_ISO_HDLC); + // Created at the first stage (XFER_FIRST_PART) of a MultpartReceive, // where we have the offset and size. struct FileTransferContext { @@ -31,6 +34,23 @@ struct FileTransferContext { len: usize, // Current transfer 0..len offset: usize, + digest: crc::Digest<'static, u32, crc::Table<16>>, +} + +impl FileTransferContext { + fn new(start: usize, len: usize) -> Self { + Self { + start, + len, + offset: 0, + digest: CRC32.digest(), + } + } + + fn reset(&mut self) { + self.offset = 0; + self.digest = CRC32.digest(); + } } // Created on DfOpen @@ -80,9 +100,6 @@ impl std::fmt::Display for PldmFileError { } } -const CRC32: crc::Crc> = - crc::Crc::>::new(&crc::CRC_32_ISO_HDLC); - type Result = std::result::Result; impl Responder { @@ -349,10 +366,10 @@ impl Responder { // Set new transfer context if cmd.xfer_op == pldm::control::xfer_op::FIRST_PART { if let Some(ctx) = file_ctx.xfer_ctx.as_mut() { - ctx.offset = 0; + // a repeated FIRST_PART is valid, and restarts the transfer + ctx.reset(); } else { let new_ctx = Self::init_read(&cmd)?; - // a repeated FIRST_PART is valid, and restarts the transfer file_ctx.xfer_ctx.replace(new_ctx); }; } @@ -406,7 +423,8 @@ impl Responder { host.read(data, xfer_ctx.start + offset) .map_err(|_| CCode::ERROR)?; - let cs = CRC32.checksum(data); + xfer_ctx.digest.update(data); + let cs = xfer_ctx.digest.clone().finalize(); resp_data.extend_from_slice(&cs.to_le_bytes()); xfer_ctx.offset = offset; @@ -421,11 +439,7 @@ impl Responder { trace!("init_read {req:?}"); let start = req.req_offset as usize; let len = req.req_length as usize; - Ok(FileTransferContext { - start, - len, - offset: 0, - }) + Ok(FileTransferContext::new(start, len)) } } From 77da3b1c4d35b7c9ba37747da968d29fc4cde696 Mon Sep 17 00:00:00 2001 From: Jeremy Kerr Date: Mon, 18 Aug 2025 12:17:28 +0800 Subject: [PATCH 4/4] pldm-file: remove Responder::init_read Since we have FileTransferContext::new(), this doesn't do much, and is infallible. So, just call FileTransferContext::new() directly. Signed-off-by: Jeremy Kerr --- pldm-file/src/host.rs | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/pldm-file/src/host.rs b/pldm-file/src/host.rs index bbd67f9..9888716 100644 --- a/pldm-file/src/host.rs +++ b/pldm-file/src/host.rs @@ -369,8 +369,11 @@ impl Responder { // a repeated FIRST_PART is valid, and restarts the transfer ctx.reset(); } else { - let new_ctx = Self::init_read(&cmd)?; - file_ctx.xfer_ctx.replace(new_ctx); + let ctx = FileTransferContext::new( + cmd.req_offset as usize, + cmd.req_length as usize, + ); + file_ctx.xfer_ctx.replace(ctx); }; } @@ -432,15 +435,6 @@ impl Responder { Ok(resp) } - - fn init_read( - req: &pldm::control::MultipartReceiveReq, - ) -> Result { - trace!("init_read {req:?}"); - let start = req.req_offset as usize; - let len = req.req_length as usize; - Ok(FileTransferContext::new(start, len)) - } } impl Default for Responder {