diff --git a/.gitignore b/.gitignore index 2ea0c23..f69dcae 100644 --- a/.gitignore +++ b/.gitignore @@ -326,3 +326,7 @@ Testing/ # Fuzz corpus fuzz/corpus/ fuzz/artifacts/ + +.omniscient/ + +.DS_Store diff --git a/src/lib.rs b/src/lib.rs index edc2d04..bfb11ab 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -47,8 +47,8 @@ pub use services::{ RequestDownloadRequest, RequestDownloadResponse, RequestFileTransferRequest, RequestFileTransferResponse, RequestTransferExitRequest, RequestTransferExitResponse, ResetType, RoutineControlRequest, RoutineControlResponse, RoutineControlSubFunction, - SecurityAccessRequest, SecurityAccessResponse, SecurityAccessType, SentDataPayload, - SizePayload, TesterPresentRequest, TesterPresentResponse, TransferDataRequest, + SecurityAccessLevel, SecurityAccessRequest, SecurityAccessResponse, SecurityAccessType, + SentDataPayload, SizePayload, TesterPresentRequest, TesterPresentResponse, TransferDataRequest, TransferDataResponse, WriteDataByIdentifierRequest, WriteDataByIdentifierResponse, }; @@ -206,7 +206,13 @@ mod no_std_api_tests { fn const_construction() { // Verify const construction works at compile time const _REQ: TransferDataRequest<'static> = TransferDataRequest::new(1, &[0x01, 0x02, 0x03]); - const _SEC: SecurityAccessRequest<'static> = - SecurityAccessRequest::new(false, SecurityAccessType::RequestSeed(0x01), &[0xAA, 0xBB]); + const _SEC: SecurityAccessRequest<'static> = SecurityAccessRequest::new( + false, + SecurityAccessType::RequestSeed(match SecurityAccessLevel::new(0x01) { + Ok(level) => level, + Err(_) => panic!("0x01 is a valid security access level"), + }), + &[0xAA, 0xBB], + ); } } diff --git a/src/request.rs b/src/request.rs index 5fa2b3a..2429ca9 100644 --- a/src/request.rs +++ b/src/request.rs @@ -184,12 +184,12 @@ impl Request<'_> { pub fn is_positive_response_suppressed(&self) -> bool { match self { Self::CommunicationControl(req) => req.suppress_positive_response(), - Self::ControlDTCSettings(req) => req.suppress_positive_response(), - Self::DiagnosticSessionControl(req) => req.suppress_positive_response(), - Self::EcuReset(req) => req.suppress_positive_response(), - Self::RoutineControl(req) => req.suppress_positive_response(), - Self::SecurityAccess(req) => req.suppress_positive_response(), - Self::TesterPresent(req) => req.suppress_positive_response(), + Self::ControlDTCSettings(req) => req.suppress_positive_response, + Self::DiagnosticSessionControl(req) => req.suppress_positive_response, + Self::EcuReset(req) => req.suppress_positive_response, + Self::RoutineControl(req) => req.suppress_positive_response, + Self::SecurityAccess(req) => req.suppress_positive_response, + Self::TesterPresent(req) => req.suppress_positive_response, _ => false, } } diff --git a/src/services/communication_control.rs b/src/services/communication_control.rs index 5570acc..63c2088 100644 --- a/src/services/communication_control.rs +++ b/src/services/communication_control.rs @@ -34,14 +34,26 @@ pub enum CommunicationControlType { /// shall be enabled for the specified [`CommunicationType`] /// Additionally, enhanced address information shall be included in the request EnableRxAndTxWithEnhancedAddressInfo, - /// These values are reserved by the ISO 14229-1 Specification + /// These values are reserved by the ISO 14229-1 Specification. + /// + /// Construct through [`CommunicationControlType::try_from`] so the raw byte is + /// range-checked and can never collide with the SPRMIB bit. #[cfg_attr(feature = "clap", clap(skip))] + #[non_exhaustive] ISOSAEReserved(u8), - /// Values reserved for use by vehicle manufacturers + /// Values reserved for use by vehicle manufacturers. + /// + /// Construct through [`CommunicationControlType::try_from`] so the raw byte is + /// range-checked and can never collide with the SPRMIB bit. #[cfg_attr(feature = "clap", clap(skip))] + #[non_exhaustive] VehicleManufacturerSpecific(u8), - /// Values reserved for use by system suppliers + /// Values reserved for use by system suppliers. + /// + /// Construct through [`CommunicationControlType::try_from`] so the raw byte is + /// range-checked and can never collide with the SPRMIB bit. #[cfg_attr(feature = "clap", clap(skip))] + #[non_exhaustive] SystemSupplierSpecific(u8), } diff --git a/src/services/control_dtc_settings.rs b/src/services/control_dtc_settings.rs index 234515f..5a4c45d 100644 --- a/src/services/control_dtc_settings.rs +++ b/src/services/control_dtc_settings.rs @@ -43,7 +43,10 @@ impl TryFrom for DtcSettings { #[derive(Clone, Copy, Debug, Eq, PartialEq)] #[non_exhaustive] pub struct ControlDTCSettingsRequest { - setting: SuppressablePositiveResponse, + /// Whether the server should suppress the positive response (SPRMIB). + pub suppress_positive_response: bool, + /// The requested DTC logging setting. + pub setting: DtcSettings, } impl ControlDTCSettingsRequest { @@ -51,21 +54,10 @@ impl ControlDTCSettingsRequest { #[must_use] pub const fn new(suppress_positive_response: bool, setting: DtcSettings) -> Self { Self { - setting: SuppressablePositiveResponse::new(suppress_positive_response, setting), + suppress_positive_response, + setting, } } - - /// Returns the requested DTC logging setting. - #[must_use] - pub fn setting(&self) -> DtcSettings { - self.setting.value() - } - - /// Whether the server should suppress the positive response (SPRMIB). - #[must_use] - pub fn suppress_positive_response(&self) -> bool { - self.setting.suppress_positive_response() - } } impl Encode for ControlDTCSettingsRequest { @@ -74,8 +66,10 @@ impl Encode for ControlDTCSettingsRequest { } fn encode(&self, writer: &mut impl embedded_io::Write) -> Result { + let sub_function = + SuppressablePositiveResponse::new(self.suppress_positive_response, self.setting); writer - .write_all(&[u8::from(self.setting)]) + .write_all(&[u8::from(sub_function)]) .map_err(Error::io)?; Ok(1) } @@ -86,8 +80,14 @@ impl<'a> Decode<'a> for ControlDTCSettingsRequest { if buf.is_empty() { return Err(Error::InsufficientData(1)); } - let setting = SuppressablePositiveResponse::try_from(buf[0])?; - Ok((Self { setting }, &buf[1..])) + let sub_function = SuppressablePositiveResponse::::try_from(buf[0])?; + Ok(( + Self { + suppress_positive_response: sub_function.suppress_positive_response(), + setting: sub_function.value(), + }, + &buf[1..], + )) } } @@ -152,8 +152,8 @@ mod request { assert_eq!(req.encoded_size(), buffer.len()); let (parsed, _) = ::decode(&buffer).unwrap(); - assert_eq!(parsed.setting(), DtcSettings::On); - assert!(parsed.suppress_positive_response()); + assert_eq!(parsed.setting, DtcSettings::On); + assert!(parsed.suppress_positive_response); assert_encode_size_agrees(&req); } diff --git a/src/services/diagnostic_session_control.rs b/src/services/diagnostic_session_control.rs index 73a9039..1e7ac95 100644 --- a/src/services/diagnostic_session_control.rs +++ b/src/services/diagnostic_session_control.rs @@ -25,7 +25,10 @@ use crate::{Decode, Encode, Error, NegativeResponseCode}; #[non_exhaustive] pub enum DiagnosticSessionType { /// This value is reserved by the ISO 14229-1 Specification + /// + /// Construct through [`DiagnosticSessionType::try_from`] so the raw byte is range-checked and can never collide with the SPRMIB bit. #[cfg_attr(feature = "clap", clap(skip))] + #[non_exhaustive] ISOSAEReserved(u8), /// The `DefaultSession` (0x01) enables the standard diagnostic functionality /// - No `TesterPresent` messages are required to remain in this session @@ -42,10 +45,16 @@ pub enum DiagnosticSessionType { /// The `SafetySystemDiagnosticSession` (0x04) enables diagnostics functionality for safety systems SafetySystemDiagnosticSession, /// Value reserved for use by vehicle manufacturers + /// + /// Construct through [`DiagnosticSessionType::try_from`] so the raw byte is range-checked and can never collide with the SPRMIB bit. #[cfg_attr(feature = "clap", clap(skip))] + #[non_exhaustive] VehicleManufacturerSpecificSession(u8), /// Value reserved for use by system suppliers + /// + /// Construct through [`DiagnosticSessionType::try_from`] so the raw byte is range-checked and can never collide with the SPRMIB bit. #[cfg_attr(feature = "clap", clap(skip))] + #[non_exhaustive] SystemSupplierSpecificSession(u8), } @@ -163,7 +172,10 @@ const DIAGNOSTIC_SESSION_CONTROL_NEGATIVE_RESPONSE_CODES: [NegativeResponseCode; #[derive(Clone, Copy, Debug, Eq, PartialEq)] #[non_exhaustive] pub struct DiagnosticSessionControlRequest { - session_type: SuppressablePositiveResponse, + /// Whether a positive response should be suppressed. + pub suppress_positive_response: bool, + /// The requested diagnostic session type. + pub session_type: DiagnosticSessionType, } impl DiagnosticSessionControlRequest { @@ -174,25 +186,11 @@ impl DiagnosticSessionControlRequest { session_type: DiagnosticSessionType, ) -> Self { Self { - session_type: SuppressablePositiveResponse::new( - suppress_positive_response, - session_type, - ), + suppress_positive_response, + session_type, } } - /// Getter for whether a positive response should be suppressed - #[must_use] - pub fn suppress_positive_response(&self) -> bool { - self.session_type.suppress_positive_response() - } - - /// Getter for the requested session type - #[must_use] - pub fn session_type(&self) -> DiagnosticSessionType { - self.session_type.value() - } - /// Get the allowed [`NegativeResponseCode`] variants for this request #[must_use] pub fn allowed_nack_codes() -> &'static [NegativeResponseCode] { @@ -205,8 +203,10 @@ impl Encode for DiagnosticSessionControlRequest { } fn encode(&self, writer: &mut impl embedded_io::Write) -> Result { + let sub_function = + SuppressablePositiveResponse::new(self.suppress_positive_response, self.session_type); writer - .write_all(&[u8::from(self.session_type)]) + .write_all(&[u8::from(sub_function)]) .map_err(Error::io)?; Ok(1) } @@ -217,8 +217,14 @@ impl<'a> Decode<'a> for DiagnosticSessionControlRequest { if buf.is_empty() { return Err(Error::InsufficientData(1)); } - let session_type = SuppressablePositiveResponse::try_from(buf[0])?; - Ok((Self { session_type }, &buf[1..])) + let sub_function = SuppressablePositiveResponse::::try_from(buf[0])?; + Ok(( + Self { + suppress_positive_response: sub_function.suppress_positive_response(), + session_type: sub_function.value(), + }, + &buf[1..], + )) } } @@ -301,11 +307,8 @@ mod request { fn test_diagnostic_session_control_request() { let bytes: [u8; 1] = [0x02]; let (req, _) = ::decode(&bytes).unwrap(); - assert!(!req.suppress_positive_response()); - assert_eq!( - req.session_type(), - DiagnosticSessionType::ProgrammingSession - ); + assert!(!req.suppress_positive_response); + assert_eq!(req.session_type, DiagnosticSessionType::ProgrammingSession); let mut buffer = Vec::new(); Encode::encode(&req, &mut buffer).unwrap(); diff --git a/src/services/ecu_reset.rs b/src/services/ecu_reset.rs index f24246f..9f44c21 100644 --- a/src/services/ecu_reset.rs +++ b/src/services/ecu_reset.rs @@ -15,8 +15,12 @@ use crate::{Decode, Encode, Error, NegativeResponseCode}; #[derive(Clone, Copy, Debug, Eq, PartialEq)] #[non_exhaustive] pub enum ResetType { - /// This value is reserved + /// This value is reserved. + /// + /// Construct reserved values through [`ResetType::try_from`] so the raw byte is + /// range-checked (`0x00..=0x7F`) and can never collide with the SPRMIB bit. #[cfg_attr(feature = "clap", clap(skip))] + #[non_exhaustive] ISOSAEReserved(u8), /// This `SubFunction` identifies a "hard reset" condition which simulates the power-on/start-up sequence /// typically performed after a server has been previously disconnected from its power supply (i.e. battery). @@ -49,11 +53,19 @@ pub enum ResetType { EnableRapidPowerShutDown, /// This `SubFunction` requests the server to disable the previously enabled "rapid power shut down" function. DisableRapidPowerShutDown, - /// Reserved for use by vehicle manufacturers + /// Reserved for use by vehicle manufacturers. + /// + /// Construct through [`ResetType::try_from`] so the raw byte is range-checked + /// (`0x40..=0x5F`) and can never collide with the SPRMIB bit. #[cfg_attr(feature = "clap", clap(skip))] + #[non_exhaustive] VehicleManufacturerSpecific(u8), - /// Reserved for use by system suppliers + /// Reserved for use by system suppliers. + /// + /// Construct through [`ResetType::try_from`] so the raw byte is range-checked + /// (`0x60..=0x7E`) and can never collide with the SPRMIB bit. #[cfg_attr(feature = "clap", clap(skip))] + #[non_exhaustive] SystemSupplierSpecific(u8), } @@ -175,7 +187,10 @@ const ECU_RESET_NEGATIVE_RESPONSE_CODES: [NegativeResponseCode; 4] = [ #[derive(Clone, Copy, Debug, Eq, PartialEq)] #[non_exhaustive] pub struct EcuResetRequest { - reset_type: SuppressablePositiveResponse, + /// Whether the server should suppress a positive response (SPRMIB). + pub suppress_positive_response: bool, + /// The type of reset being requested. + pub reset_type: ResetType, } impl EcuResetRequest { @@ -183,22 +198,11 @@ impl EcuResetRequest { #[must_use] pub const fn new(suppress_positive_response: bool, reset_type: ResetType) -> Self { Self { - reset_type: SuppressablePositiveResponse::new(suppress_positive_response, reset_type), + suppress_positive_response, + reset_type, } } - /// Getter for whether a positive response should be suppressed - #[must_use] - pub fn suppress_positive_response(&self) -> bool { - self.reset_type.suppress_positive_response() - } - - /// Getter for the requested [`ResetType`] - #[must_use] - pub fn reset_type(&self) -> ResetType { - self.reset_type.value() - } - /// Get the allowed [`NegativeResponseCode`] variants for this request #[must_use] pub fn allowed_nack_codes() -> &'static [NegativeResponseCode] { @@ -212,8 +216,11 @@ impl Encode for EcuResetRequest { } fn encode(&self, writer: &mut impl embedded_io::Write) -> Result { + // Fuse the SPRMIB bit into the sub-function byte only at the wire boundary. + let sub_function = + SuppressablePositiveResponse::new(self.suppress_positive_response, self.reset_type); writer - .write_all(&[u8::from(self.reset_type)]) + .write_all(&[u8::from(sub_function)]) .map_err(Error::io)?; Ok(1) } @@ -224,8 +231,14 @@ impl<'a> Decode<'a> for EcuResetRequest { if buf.is_empty() { return Err(Error::InsufficientData(1)); } - let reset_type = SuppressablePositiveResponse::try_from(buf[0])?; - Ok((Self { reset_type }, &buf[1..])) + let sub_function = SuppressablePositiveResponse::::try_from(buf[0])?; + Ok(( + Self { + suppress_positive_response: sub_function.suppress_positive_response(), + reset_type: sub_function.value(), + }, + &buf[1..], + )) } } diff --git a/src/services/mod.rs b/src/services/mod.rs index 2812ef3..d79de5f 100644 --- a/src/services/mod.rs +++ b/src/services/mod.rs @@ -47,7 +47,9 @@ pub use routine_control::{ }; mod security_access; -pub use security_access::{SecurityAccessRequest, SecurityAccessResponse, SecurityAccessType}; +pub use security_access::{ + SecurityAccessLevel, SecurityAccessRequest, SecurityAccessResponse, SecurityAccessType, +}; mod tester_present; pub use tester_present::{TesterPresentRequest, TesterPresentResponse}; diff --git a/src/services/request_transfer_exit.rs b/src/services/request_transfer_exit.rs index ec28285..2b17e5f 100644 --- a/src/services/request_transfer_exit.rs +++ b/src/services/request_transfer_exit.rs @@ -7,7 +7,8 @@ macro_rules! transfer_exit_descriptor { #[derive(Clone, Copy, Debug, Eq, PartialEq)] #[non_exhaustive] pub struct $name<'d> { - parameter_record: &'d [u8], + /// The optional, opaque parameter record (empty slice if absent). + pub parameter_record: &'d [u8], } impl<'d> $name<'d> { /// Create from the optional parameter record (empty slice if absent). @@ -15,11 +16,6 @@ macro_rules! transfer_exit_descriptor { pub const fn new(parameter_record: &'d [u8]) -> Self { Self { parameter_record } } - /// The optional, opaque parameter record. - #[must_use] - pub const fn parameter_record(&self) -> &[u8] { - self.parameter_record - } } impl Encode for $name<'_> { fn encoded_size(&self) -> usize { @@ -66,7 +62,7 @@ mod tests { assert_eq!(&buf[..n], rec); let (d, rest) = ::decode(&buf[..n]).unwrap(); assert!(rest.is_empty()); - assert_eq!(d.parameter_record(), rec); + assert_eq!(d.parameter_record, rec); assert_encode_size_agrees(&req); } } diff --git a/src/services/routine_control.rs b/src/services/routine_control.rs index 22cdd16..6c46b20 100644 --- a/src/services/routine_control.rs +++ b/src/services/routine_control.rs @@ -56,9 +56,14 @@ impl TryFrom for RoutineControlSubFunction { #[derive(Clone, Copy, Debug, Eq, PartialEq)] #[non_exhaustive] pub struct RoutineControlRequest<'d> { - sub_function: SuppressablePositiveResponse, - routine_id: u16, - option_record: &'d [u8], + /// Whether the server should suppress the positive response (SPRMIB). + pub suppress_positive_response: bool, + /// The routine control operation (start, stop, or request results). + pub sub_function: RoutineControlSubFunction, + /// The 16-bit routine identifier. + pub routine_id: u16, + /// Optional routine input parameters (may be empty). + pub option_record: &'d [u8], } impl<'d> RoutineControlRequest<'d> { @@ -71,38 +76,12 @@ impl<'d> RoutineControlRequest<'d> { option_record: &'d [u8], ) -> Self { Self { - sub_function: SuppressablePositiveResponse::new( - suppress_positive_response, - sub_function, - ), + suppress_positive_response, + sub_function, routine_id, option_record, } } - - /// Whether the server should suppress the positive response (SPRMIB). - #[must_use] - pub fn suppress_positive_response(&self) -> bool { - self.sub_function.suppress_positive_response() - } - - /// The routine control operation (start, stop, or request results). - #[must_use] - pub fn sub_function(&self) -> RoutineControlSubFunction { - self.sub_function.value() - } - - /// The 16-bit routine identifier. - #[must_use] - pub const fn routine_id(&self) -> u16 { - self.routine_id - } - - /// Optional routine input parameters (may be empty). - #[must_use] - pub const fn option_record(&self) -> &[u8] { - self.option_record - } } impl Encode for RoutineControlRequest<'_> { @@ -111,8 +90,10 @@ impl Encode for RoutineControlRequest<'_> { } fn encode(&self, writer: &mut impl embedded_io::Write) -> Result { + let sub_function = + SuppressablePositiveResponse::new(self.suppress_positive_response, self.sub_function); writer - .write_all(&[u8::from(self.sub_function)]) + .write_all(&[u8::from(sub_function)]) .map_err(Error::io)?; writer .write_all(&self.routine_id.to_be_bytes()) @@ -127,11 +108,13 @@ impl<'a> Decode<'a> for RoutineControlRequest<'a> { if buf.len() < 3 { return Err(Error::InsufficientData(3)); } - let sub_function = SuppressablePositiveResponse::try_from(buf[0])?; + let sub_function = + SuppressablePositiveResponse::::try_from(buf[0])?; let routine_id = u16::from_be_bytes([buf[1], buf[2]]); Ok(( Self { - sub_function, + suppress_positive_response: sub_function.suppress_positive_response(), + sub_function: sub_function.value(), routine_id, option_record: &buf[3..], }, @@ -147,9 +130,12 @@ impl<'a> Decode<'a> for RoutineControlRequest<'a> { #[derive(Clone, Copy, Debug, Eq, PartialEq)] #[non_exhaustive] pub struct RoutineControlResponse<'d> { - sub_function: RoutineControlSubFunction, - routine_id: u16, - status_record: &'d [u8], + /// The routine control operation echoed from the request (start, stop, or request results). + pub sub_function: RoutineControlSubFunction, + /// The 16-bit routine identifier echoed from the request. + pub routine_id: u16, + /// Optional routine status bytes (may be empty). + pub status_record: &'d [u8], } impl<'d> RoutineControlResponse<'d> { @@ -166,24 +152,6 @@ impl<'d> RoutineControlResponse<'d> { status_record, } } - - /// The routine control operation echoed from the request (start, stop, or request results). - #[must_use] - pub const fn sub_function(&self) -> RoutineControlSubFunction { - self.sub_function - } - - /// The 16-bit routine identifier echoed from the request. - #[must_use] - pub const fn routine_id(&self) -> u16 { - self.routine_id - } - - /// Optional routine status bytes (may be empty). - #[must_use] - pub const fn status_record(&self) -> &[u8] { - self.status_record - } } impl Encode for RoutineControlResponse<'_> { @@ -241,10 +209,10 @@ mod test { assert_eq!(&buf[..n], &[0x81, 0xFF, 0x00, 0xAA]); // 0x81 = StartRoutine | SPRMIB let (d, rest) = ::decode(&buf[..n]).unwrap(); assert!(rest.is_empty()); - assert!(d.suppress_positive_response()); - assert_eq!(d.sub_function(), RoutineControlSubFunction::StartRoutine); - assert_eq!(d.routine_id(), 0xFF00); - assert_eq!(d.option_record(), &[0xAA]); + assert!(d.suppress_positive_response); + assert_eq!(d.sub_function, RoutineControlSubFunction::StartRoutine); + assert_eq!(d.routine_id, 0xFF00); + assert_eq!(d.option_record, &[0xAA]); assert_encode_size_agrees(&req); } @@ -261,8 +229,8 @@ mod test { let n = Encode::encode(&resp, &mut buf.as_mut_slice()).unwrap(); assert_eq!(&buf[..n], &[0x01, 0xFF, 0x00, 0x10]); let (d, _) = ::decode(&buf[..n]).unwrap(); - assert_eq!(d.sub_function(), RoutineControlSubFunction::StartRoutine); - assert_eq!(d.routine_id(), 0xFF00); + assert_eq!(d.sub_function, RoutineControlSubFunction::StartRoutine); + assert_eq!(d.routine_id, 0xFF00); // A response with the SPRMIB bit set (0x81) is malformed and rejected. assert!(::decode(&[0x81, 0xFF, 0x00]).is_err()); assert_encode_size_agrees(&resp); diff --git a/src/services/security_access.rs b/src/services/security_access.rs index f00c49d..11d823d 100644 --- a/src/services/security_access.rs +++ b/src/services/security_access.rs @@ -2,6 +2,37 @@ use crate::shared::SuppressablePositiveResponse; use crate::{Decode, Encode, Error, NegativeResponseCode}; +/// A `SecurityAccess` level byte, guaranteed to fit the 7-bit sub-function field +/// (`0x00..=0x7F`). +/// +/// A request fuses the suppress-positive-response flag into bit 7 of this byte at the wire +/// boundary, so a level with bit 7 already set would be ambiguous. Constraining construction +/// here makes that collision unrepresentable rather than something to catch at encode time. +#[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))] +#[cfg_attr(feature = "utoipa", derive(utoipa::ToSchema))] +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub struct SecurityAccessLevel(u8); + +impl SecurityAccessLevel { + /// Create a level, rejecting any value that would set the SPRMIB bit (`>= 0x80`). + /// + /// # Errors + /// Returns [`Error::InvalidSecurityAccessType`] if `value >= 0x80`. + pub const fn new(value: u8) -> Result { + if value > 0x7F { + Err(Error::InvalidSecurityAccessType(value)) + } else { + Ok(Self(value)) + } + } + + /// The raw level byte (always `0x00..=0x7F`). + #[must_use] + pub const fn get(self) -> u8 { + self.0 + } +} + /// Security Access Type allows for multiple different security challenges within an ECU. /// /// The Security Access Type is used to determine both the sub function, @@ -17,17 +48,25 @@ use crate::{Decode, Encode, Error, NegativeResponseCode}; #[non_exhaustive] pub enum SecurityAccessType { /// This value is reserved for future definition + /// + /// Construct through [`SecurityAccessType::try_from`] so the raw byte is range-checked + /// and can never collide with the SPRMIB bit. + #[non_exhaustive] ISOSAEReserved(u8), /// `RequestSeed` with the level of security defined by the vehicle manufacturer - RequestSeed(u8), + RequestSeed(SecurityAccessLevel), /// `SendKey` with the level of security defined by the vehicle manufacturer - SendKey(u8), + SendKey(SecurityAccessLevel), /// `RequestSeed` with different levels of security defined for end of life /// activation of on-board pyrotechnic devices ISO26021_2Values, /// `SendKey` with different levels of security defined for end of life activation ISO26021_2SendKeyValues, /// This range of values is reserved for system supplier specific use + /// + /// Construct through [`SecurityAccessType::try_from`] so the raw byte is range-checked + /// and can never collide with the SPRMIB bit. + #[non_exhaustive] SystemSupplierSpecific(u8), } @@ -36,8 +75,8 @@ impl From for u8 { fn from(value: SecurityAccessType) -> Self { match value { SecurityAccessType::ISOSAEReserved(val) => val, - SecurityAccessType::RequestSeed(val) => val, - SecurityAccessType::SendKey(val) => val, + SecurityAccessType::RequestSeed(level) => level.get(), + SecurityAccessType::SendKey(level) => level.get(), SecurityAccessType::ISO26021_2Values => 0x5F, SecurityAccessType::ISO26021_2SendKeyValues => 0x60, SecurityAccessType::SystemSupplierSpecific(val) => val, @@ -53,10 +92,11 @@ impl TryFrom for SecurityAccessType { // Security requests alternate, with odd numbers being seed requests, // and even numbers being send key requests 0x01..=0x42 => { + // `value` is 0x01..=0x42 here, so it always fits `SecurityAccessLevel`. if value % 2 == 1 { - Ok(Self::RequestSeed(value)) + Ok(Self::RequestSeed(SecurityAccessLevel(value))) } else { - Ok(Self::SendKey(value)) + Ok(Self::SendKey(SecurityAccessLevel(value))) } } 0x5F => Ok(Self::ISO26021_2Values), @@ -91,13 +131,13 @@ mod security_access_type_tests { for value in &REQUEST_SEED_VALUES { assert_eq!( SecurityAccessType::try_from(*value).unwrap(), - SecurityAccessType::RequestSeed(*value) + SecurityAccessType::RequestSeed(SecurityAccessLevel(*value)) ); } for value in &SEND_KEY_VALUES { assert_eq!( SecurityAccessType::try_from(*value).unwrap(), - SecurityAccessType::SendKey(*value) + SecurityAccessType::SendKey(SecurityAccessLevel(*value)) ); } for i in 0x43..=0x5E { @@ -132,6 +172,20 @@ mod security_access_type_tests { } } + #[test] + fn security_access_level_rejects_sprmib_colliding_values() { + // 0x00..=0x7F are constructible; 0x80..=0xFF (SPRMIB bit set) are rejected. + for value in 0x00..=0x7F { + assert_eq!(SecurityAccessLevel::new(value).unwrap().get(), value); + } + for value in 0x80..=0xFF { + assert!(matches!( + SecurityAccessLevel::new(value), + Err(Error::InvalidSecurityAccessType(v)) if v == value + )); + } + } + #[test] fn security_access_type_round_trip_all_values() { for i in 0..=u8::MAX { @@ -181,8 +235,12 @@ const SECURITY_ACCESS_NEGATIVE_RESPONSE_CODES: [NegativeResponseCode; 8] = [ #[derive(Clone, Copy, Debug, Eq, PartialEq)] #[non_exhaustive] pub struct SecurityAccessRequest<'d> { - access_type: SuppressablePositiveResponse, - request_data: &'d [u8], + /// Whether a positive response should be suppressed. + pub suppress_positive_response: bool, + /// The requested [`SecurityAccessType`]. + pub access_type: SecurityAccessType, + /// The implementation-defined request data (seed data record or key bytes). + pub request_data: &'d [u8], } impl<'d> SecurityAccessRequest<'d> { @@ -194,29 +252,12 @@ impl<'d> SecurityAccessRequest<'d> { request_data: &'d [u8], ) -> Self { Self { - access_type: SuppressablePositiveResponse::new(suppress_positive_response, access_type), + suppress_positive_response, + access_type, request_data, } } - /// Getter for whether a positive response should be suppressed - #[must_use] - pub fn suppress_positive_response(&self) -> bool { - self.access_type.suppress_positive_response() - } - - /// Getter for the requested [`SecurityAccessType`] - #[must_use] - pub fn access_type(&self) -> SecurityAccessType { - self.access_type.value() - } - - /// Getter for the request data - #[must_use] - pub const fn request_data(&self) -> &[u8] { - self.request_data - } - /// Get the allowed [`NegativeResponseCode`] variants for this request #[must_use] pub fn allowed_nack_codes() -> &'static [NegativeResponseCode] { @@ -230,8 +271,10 @@ impl Encode for SecurityAccessRequest<'_> { } fn encode(&self, writer: &mut impl embedded_io::Write) -> Result { + let sub_function = + SuppressablePositiveResponse::new(self.suppress_positive_response, self.access_type); writer - .write_all(&[u8::from(self.access_type)]) + .write_all(&[u8::from(sub_function)]) .map_err(Error::io)?; writer.write_all(self.request_data).map_err(Error::io)?; Ok(self.encoded_size()) @@ -243,10 +286,11 @@ impl<'a> Decode<'a> for SecurityAccessRequest<'a> { if buf.is_empty() { return Err(Error::InsufficientData(1)); } - let access_type = SuppressablePositiveResponse::try_from(buf[0])?; + let sub_function = SuppressablePositiveResponse::::try_from(buf[0])?; Ok(( Self { - access_type, + suppress_positive_response: sub_function.suppress_positive_response(), + access_type: sub_function.value(), request_data: &buf[1..], }, &[], @@ -321,8 +365,11 @@ mod request { ]; let (req, _) = ::decode(&bytes).unwrap(); - assert_eq!(req.access_type(), SecurityAccessType::RequestSeed(0x01)); - assert_eq!(req.request_data(), &[0x00, 0x01, 0x02, 0x03, 0x04]); + assert_eq!( + req.access_type, + SecurityAccessType::RequestSeed(SecurityAccessLevel(0x01)) + ); + assert_eq!(req.request_data, &[0x00, 0x01, 0x02, 0x03, 0x04]); let mut buf = Vec::new(); let written = Encode::encode(&req, &mut buf).unwrap(); @@ -348,7 +395,10 @@ mod response { ]; let (resp, _) = ::decode(&bytes).unwrap(); - assert_eq!(resp.access_type, SecurityAccessType::SendKey(0x02)); + assert_eq!( + resp.access_type, + SecurityAccessType::SendKey(SecurityAccessLevel(0x02)) + ); assert_eq!(resp.security_seed, &[0x00, 0x01, 0x02, 0x03, 0x04]); let mut buf = Vec::new(); diff --git a/src/services/tester_present.rs b/src/services/tester_present.rs index d2eeb71..5049c45 100644 --- a/src/services/tester_present.rs +++ b/src/services/tester_present.rs @@ -56,34 +56,22 @@ impl TryFrom for ZeroSubFunction { #[derive(Clone, Copy, Debug, Eq, PartialEq)] #[non_exhaustive] pub struct TesterPresentRequest { - zero_sub_function: SuppressablePositiveResponse, + /// Whether the server should suppress a positive response (SPRMIB). + /// + /// `TesterPresent` defines only the zero sub-function, so the suppression flag is + /// the request's sole degree of freedom. + pub suppress_positive_response: bool, } impl TesterPresentRequest { /// Create a new `TesterPresentRequest` #[must_use] - pub fn new(suppress_positive_response: bool) -> Self { - Self::with_subfunction(suppress_positive_response, ZeroSubFunction::default()) - } - - fn with_subfunction( - suppress_positive_response: bool, - zero_sub_function: ZeroSubFunction, - ) -> Self { + pub const fn new(suppress_positive_response: bool) -> Self { Self { - zero_sub_function: SuppressablePositiveResponse::new( - suppress_positive_response, - zero_sub_function, - ), + suppress_positive_response, } } - /// Getter for whether a positive response should be suppressed - #[must_use] - pub fn suppress_positive_response(&self) -> bool { - self.zero_sub_function.suppress_positive_response() - } - /// Get the allowed [`NegativeResponseCode`] variants for this request #[must_use] pub fn allowed_nack_codes() -> &'static [NegativeResponseCode] { @@ -97,8 +85,14 @@ impl Encode for TesterPresentRequest { } fn encode(&self, writer: &mut impl embedded_io::Write) -> Result { + // The only defined sub-function is the zero sub-function; fuse the SPRMIB bit + // onto it at the wire boundary. + let sub_function = SuppressablePositiveResponse::new( + self.suppress_positive_response, + ZeroSubFunction::NoSubFunctionSupported, + ); writer - .write_all(&[u8::from(self.zero_sub_function)]) + .write_all(&[u8::from(sub_function)]) .map_err(Error::io)?; Ok(1) } @@ -109,8 +103,16 @@ impl<'a> Decode<'a> for TesterPresentRequest { if buf.is_empty() { return Err(Error::InsufficientData(1)); } - let zero_sub_function = SuppressablePositiveResponse::try_from(buf[0])?; - Ok((Self { zero_sub_function }, &buf[1..])) + // Split out the SPRMIB flag. Once SPRMIB is stripped the low 7 bits are always a + // valid zero sub-function, so this never rejects; the sub-function value itself is + // discarded and normalized to the zero sub-function on re-encode. + let sub_function = SuppressablePositiveResponse::::try_from(buf[0])?; + Ok(( + Self { + suppress_positive_response: sub_function.suppress_positive_response(), + }, + &buf[1..], + )) } } @@ -218,24 +220,17 @@ mod test { assert_eq!(result.unwrap(), expected); } 0x01..=0x7F => { - let result = result.unwrap(); - assert!(!result.suppress_positive_response()); - assert!(matches!( - result.zero_sub_function.value(), - ZeroSubFunction::ISOSAEReserved(_) - )); + // Reserved sub-function bytes decode (SPRMIB clear); the reserved value + // is not retained — re-encoding normalizes to the zero sub-function. + assert!(!result.unwrap().suppress_positive_response); } 0x80 => { let expected = TesterPresentRequest::new(true); assert_eq!(result.unwrap(), expected); } 0x81..=0xFF => { - let result = result.unwrap(); - assert!(result.suppress_positive_response()); - assert!(matches!( - result.zero_sub_function.value(), - ZeroSubFunction::ISOSAEReserved(_) - )); + // SPRMIB set over a reserved value: suppression is retained. + assert!(result.unwrap().suppress_positive_response); } } }