Pldm improvement

[CourtneyDrant]

This pull request was done to improve on a previous design of the pldm-service. It has one process which is the Firmware Device. Firmware Device is the OpenProt Pldm terminus for FW update. It will loop to receive messages from the UA and when ready, will request data from the UA. The loop ensures that between each request the terminus will check for UA requests to receive status or cancel an update.

[chrysh]

A bit AI-y, but the problems it raises seem legit.

Two regression tests for services/pldm/tests/base_host.rs (they reuse the existing MockFdOps and common.rs wiring, so they drop straight in at the end of the file — no new imports needed).

1. Responder must filter by UA EID. Right now run_terminus acts on any inbound command regardless of source EID, so any endpoint on the bus can inject SetTid/CancelUpdate/etc. mid-update. This test currently fails (the FD applies a SetTid from EID 99); it passes once the responder path checks meta.remote_eid against the remote_eid passed to run_terminus.

/// Security regression: the FD must only act on commands from the Update
/// Agent EID it was told to serve (`run_terminus`'s `remote_eid`), not from
/// any endpoint that happens to be on the bus.
#[test]
fn responder_ignores_commands_from_unexpected_eid() {
    const ATTACKER_EID: u8 = 99;

    let fd_ops = MockFdOps {
        component_accepted: Cell::new(false),
        download_bytes_received: Cell::new(0),
        verified: Cell::new(false),
        applied: Cell::new(false),
        activated: Cell::new(false),
    };

    let ua_to_fd_packets = RefCell::new(Vec::new());
    let ua_server: RefCell<Server<_, 16>> = RefCell::new(Server::new(
        Eid(UA_EID),
        0,
        BufferSender { packets: &ua_to_fd_packets },
    ));

    let attacker_to_fd_packets = RefCell::new(Vec::new());
    let attacker_server: RefCell<Server<_, 16>> = RefCell::new(Server::new(
        Eid(ATTACKER_EID),
        0,
        BufferSender { packets: &attacker_to_fd_packets },
    ));

    let fd_to_ua_packets = RefCell::new(Vec::new());
    let fd_server: RefCell<Server<_, 16>> = RefCell::new(Server::new(
        Eid(FD_EID),
        0,
        BufferSender { packets: &fd_to_ua_packets },
    ));

    // Responder pump delivers BOTH queues, so whichever is pending is seen.
    let responder_client = DirectClientWithPump::new(&fd_server, || {
        transfer(&ua_to_fd_packets, &mut fd_server.borrow_mut());
        ua_to_fd_packets.borrow_mut().clear();
        transfer(&attacker_to_fd_packets, &mut fd_server.borrow_mut());
        attacker_to_fd_packets.borrow_mut().clear();
    });
    let responder_transport = MctpPldmTransport::new(responder_client);
    let requester_transport =
        MctpPldmTransport::new(DirectClientWithPump::new(&fd_server, || {}));

    let mut fd = FirmwareDevice::init(
        &fd_ops,
        &pldm_interface::config::PLDM_PROTOCOL_CAPABILITIES,
        responder_transport,
        requester_transport,
    );
    let mut fd_buf = [0u8; 1024];

    let mut run_fd_once =
        || match fd.run_terminus(UA_EID, &mut fd_buf, TIMEOUT_MILLIS, TIMEOUT_MILLIS) {
            Ok(()) => {}
            Err(PldmServiceError::Mctp(e)) if e.is_timeout() => {}
            Err(e) => panic!("firmware device failed: {e:?}"),
        };

    let mut buf = [0u8; 1024];
    // Attacker (EID 99) sends SetTid(0x99).
    let set_tid = SetTidRequest::new(0, PldmMsgType::Request, 0x99);
    let req_len = set_tid.encode(&mut buf).expect("encode attacker SetTid");
    let attacker_handle = attacker_server
        .borrow_mut()
        .req(FD_EID)
        .expect("attacker allocate request handle to FD");
    attacker_server
        .borrow_mut()
        .send(Some(attacker_handle), 0x01, None, None, false, &buf[..req_len])
        .expect("attacker send SetTid");
    run_fd_once();
    fd_to_ua_packets.borrow_mut().clear();

    // Legitimate UA (EID 8) queries the TID.
    let get_tid = GetTidRequest::new(1, PldmMsgType::Request);
    let req_len = get_tid.encode(&mut buf).expect("encode UA GetTid");
    let ua_handle = ua_server
        .borrow_mut()
        .req(FD_EID)
        .expect("UA allocate request handle to FD");
    ua_server
        .borrow_mut()
        .send(Some(ua_handle), 0x01, None, None, false, &buf[..req_len])
        .expect("UA send GetTid");
    run_fd_once();

    transfer(&fd_to_ua_packets, &mut ua_server.borrow_mut());
    fd_to_ua_packets.borrow_mut().clear();

    let mut resp = [0u8; 1024];
    let meta = ua_server
        .borrow_mut()
        .try_recv(ua_handle, &mut resp)
        .expect("GetTid response should be available");
    assert!(meta.payload_size >= 5, "GetTid response too short");
    assert_ne!(
        resp[4], 0x99,
        "FD acted on a SetTid from an unexpected EID ({ATTACKER_EID}); \
         the responder path must filter by the UA EID passed to run_terminus"
    );
}

2. Pin the run_terminus idle-exit contract. The doc says it "returns only on error", but an idle poll with a non-zero timeout exits with Mctp(TimedOut) — and both host tests rely on that as the "done" signal. This test makes that explicit so a future change that swallows idle timeouts (turning the loop into a hang) fails loudly here instead of hanging the suite. Passes as-is.

/// Contract regression: an idle `run_terminus` with a non-zero timeout must
/// exit with `Mctp(TimedOut)` rather than block or loop forever. Both host
/// tests use this as their "queue drained / done" signal.
#[test]
fn run_terminus_exits_with_timeout_when_idle() {
    let fd_ops = MockFdOps {
        component_accepted: Cell::new(false),
        download_bytes_received: Cell::new(0),
        verified: Cell::new(false),
        applied: Cell::new(false),
        activated: Cell::new(false),
    };

    let fd_to_ua_packets = RefCell::new(Vec::new());
    let fd_server: RefCell<Server<_, 16>> = RefCell::new(Server::new(
        Eid(FD_EID),
        0,
        BufferSender { packets: &fd_to_ua_packets },
    ));

    let responder_transport =
        MctpPldmTransport::new(DirectClientWithPump::new(&fd_server, || {}));
    let requester_transport =
        MctpPldmTransport::new(DirectClientWithPump::new(&fd_server, || {}));

    let mut fd = FirmwareDevice::init(
        &fd_ops,
        &pldm_interface::config::PLDM_PROTOCOL_CAPABILITIES,
        responder_transport,
        requester_transport,
    );
    let mut fd_buf = [0u8; 1024];

    match fd.run_terminus(UA_EID, &mut fd_buf, 5, 5) {
        Err(PldmServiceError::Mctp(e)) if e.is_timeout() => {}
        other => panic!(
            "idle run_terminus should exit with Mctp(TimedOut); got {other:?}"
        ),
    }
}

[chrysh]

Two more regression guards for the EID fix. These aren't new findings — they lock in that the filter covers paths the current unexpected_eid_host.rs (SetTid) doesn't: the firmware-update state machine (#1) and not leaking a response at all (#2). I checked both fail if the source_eid != remote_eid guard is removed (state advances to 1; a response leaks), so they're not vacuous.

I also collapsed the per-test MCTP wiring into one with_bus scope helper, so each test reads as intent rather than plumbing — a third case costs ~5 lines. Drop these into unexpected_eid_host.rs (reuse its MockFdOps), or as a new tests/eid_compact.rs with a rust_test target like the others (boilerplate collapsed below).

/// Stand up UA (EID 8), a rogue endpoint (EID 99) and the FD (EID 42) over
/// in-memory MCTP, then run `body`. `body` gets a `send(src_eid, pldm_bytes)`
/// closure that delivers one command, runs the terminus once, and returns the
/// response the *sender* receives — `None` if the FD stayed silent.
fn with_bus(body: impl FnOnce(&MockFdOps, &mut dyn FnMut(u8, &[u8]) -> Option<Vec<u8>>)) {
    let fd_ops = MockFdOps::default();
    let (ua_q, atk_q, fd_q) = (
        RefCell::new(Vec::new()),
        RefCell::new(Vec::new()),
        RefCell::new(Vec::new()),
    );
    let ua = RefCell::new(Server::<_, 16>::new(Eid(UA_EID), 0, BufferSender { packets: &ua_q }));
    let atk =
        RefCell::new(Server::<_, 16>::new(Eid(ATTACKER_EID), 0, BufferSender { packets: &atk_q }));
    let fd_srv = RefCell::new(Server::<_, 16>::new(Eid(FD_EID), 0, BufferSender { packets: &fd_q }));

    // One responder pump drains both senders' queues into the FD before recv.
    let responder = DirectClientWithPump::new(&fd_srv, || {
        transfer(&ua_q, &mut fd_srv.borrow_mut());
        ua_q.borrow_mut().clear();
        transfer(&atk_q, &mut fd_srv.borrow_mut());
        atk_q.borrow_mut().clear();
    });
    let requester = DirectClientWithPump::new(&fd_srv, || {});
    let mut fd = FirmwareDevice::init(
        &fd_ops,
        &pldm_interface::config::PLDM_PROTOCOL_CAPABILITIES,
        MctpPldmTransport::new(responder),
        MctpPldmTransport::new(requester),
    );
    let mut fd_buf = [0u8; 1024];

    let mut send = |src_eid: u8, payload: &[u8]| -> Option<Vec<u8>> {
        let sender = if src_eid == UA_EID { &ua } else { &atk };
        let h = sender.borrow_mut().req(FD_EID).expect("req handle");
        sender
            .borrow_mut()
            .send(Some(h), 0x01, None, None, false, payload)
            .expect("send");
        let _ = fd.run_terminus(UA_EID, &mut fd_buf, TIMEOUT_MILLIS, TIMEOUT_MILLIS);
        transfer(&fd_q, &mut sender.borrow_mut());
        fd_q.borrow_mut().clear();
        let mut resp = [0u8; 1024];
        sender
            .borrow_mut()
            .try_recv(h, &mut resp)
            .map(|m| resp[..m.payload_size].to_vec())
    };

    body(&fd_ops, &mut send);
}

/// #1 — a rogue endpoint must not drive the firmware-update state machine.
#[test]
fn rogue_eid_cannot_drive_update() {
    with_bus(|fd_ops, send| {
        send(ATTACKER_EID, &request_update()); // dropped, no response expected
        let status = send(UA_EID, &get_status()).expect("UA gets a status reply");
        let st = GetStatusResponse::decode(&status).expect("decode status");
        assert_eq!(
            st.current_state,
            FirmwareDeviceState::Idle as u8,
            "rogue RequestUpdate advanced the FD state machine"
        );
        assert!(!fd_ops.component_accepted.get());
    });
}

/// #2 (bonus) — the FD must not disclose state to a rogue endpoint (no response).
#[test]
fn rogue_eid_gets_no_response() {
    with_bus(|_fd_ops, send| {
        assert!(
            send(ATTACKER_EID, &get_status()).is_none(),
            "FD replied to a GetStatus from an unexpected EID"
        );
    });
}

Encode helpers + minimal MockFdOps + imports (boilerplate)

use core::cell::{Cell, RefCell};

use mctp::Eid;
use openprot_mctp_server::Server;
use openprot_pldm_service::firmware_device::FirmwareDevice;
use openprot_pldm_service::MctpPldmTransport;
use pldm_common::codec::PldmCodec;
use pldm_common::message::firmware_update::apply_complete::ApplyResult;
use pldm_common::message::firmware_update::get_fw_params::FirmwareParameters;
use pldm_common::message::firmware_update::get_status::{
    GetStatusRequest, GetStatusResponse, ProgressPercent,
};
use pldm_common::message::firmware_update::request_update::RequestUpdateRequest;
use pldm_common::message::firmware_update::transfer_complete::TransferResult;
use pldm_common::message::firmware_update::verify_complete::VerifyResult;
use pldm_common::protocol::base::PldmMsgType;
use pldm_common::protocol::firmware_update::{
    ComponentResponseCode, Descriptor, FirmwareDeviceState, PldmFirmwareString, VersionStringType,
    PLDM_FWUP_IMAGE_SET_VER_STR_MAX_LEN,
};
use pldm_common::util::fw_component::FirmwareComponent;
use pldm_interface::firmware_device::fd_ops::{ComponentOperation, FdOps, FdOpsError};

mod common;
use common::{transfer, BufferSender, DirectClientWithPump, FD_EID, TIMEOUT_MILLIS, UA_EID};

const ATTACKER_EID: u8 = 99;

fn request_update() -> Vec<u8> {
    let ver = PldmFirmwareString {
        str_type: VersionStringType::Ascii as u8,
        str_len: 4,
        str_data: {
            let mut d = [0u8; PLDM_FWUP_IMAGE_SET_VER_STR_MAX_LEN];
            d[..4].copy_from_slice(b"v1.0");
            d
        },
    };
    let mut b = [0u8; 128];
    let n = RequestUpdateRequest::new(0, PldmMsgType::Request, 1024, 1, 1, 0, &ver)
        .encode(&mut b)
        .unwrap();
    b[..n].to_vec()
}

fn get_status() -> Vec<u8> {
    let mut b = [0u8; 64];
    let n = GetStatusRequest::new(0, PldmMsgType::Request).encode(&mut b).unwrap();
    b[..n].to_vec()
}

// Only `component_accepted` is observed here; the rest are trait stubs.
#[derive(Default)]
struct MockFdOps {
    component_accepted: Cell<bool>,
}

impl FdOps for MockFdOps {
    fn get_device_identifiers(&self, _d: &mut [Descriptor]) -> Result<usize, FdOpsError> { Ok(0) }
    fn get_firmware_parms(&self, p: &mut FirmwareParameters) -> Result<(), FdOpsError> {
        *p = FirmwareParameters::default();
        Ok(())
    }
    fn get_xfer_size(&self, s: usize) -> Result<usize, FdOpsError> { Ok(s.min(512)) }
    fn handle_component(
        &self,
        _c: &FirmwareComponent,
        _p: &FirmwareParameters,
        _o: ComponentOperation,
    ) -> Result<ComponentResponseCode, FdOpsError> {
        self.component_accepted.set(true);
        Ok(ComponentResponseCode::CompCanBeUpdated)
    }
    fn query_download_offset_and_length(
        &self,
        _c: &FirmwareComponent,
    ) -> Result<(usize, usize), FdOpsError> { Ok((0, 1024)) }
    fn download_fw_data(
        &self,
        _o: usize,
        _d: &[u8],
        _c: &FirmwareComponent,
    ) -> Result<TransferResult, FdOpsError> { Ok(TransferResult::TransferSuccess) }
    fn is_download_complete(&self, _c: &FirmwareComponent) -> bool { true }
    fn query_download_progress(
        &self,
        _c: &FirmwareComponent,
        _p: &mut ProgressPercent,
    ) -> Result<(), FdOpsError> { Ok(()) }
    fn verify(
        &self,
        _c: &FirmwareComponent,
        _p: &mut ProgressPercent,
    ) -> Result<VerifyResult, FdOpsError> { Ok(VerifyResult::VerifySuccess) }
    fn apply(
        &self,
        _c: &FirmwareComponent,
        _p: &mut ProgressPercent,
    ) -> Result<ApplyResult, FdOpsError> { Ok(ApplyResult::ApplySuccess) }
    fn activate(&self, _s: u8, _e: &mut u16) -> Result<u8, FdOpsError> { Ok(0) }
    fn cancel_update_component(&self, _c: &FirmwareComponent) -> Result<(), FdOpsError> { Ok(()) }
}

[CourtneyDrant]

Thanks,
The handler used by respond_once in run_terminus leaves before reaching handle_responder_msg when it doesn't recognize the eid as being from our designated EID. How do we currently impact the FD state machine without executing the handle_responder_msg? Is that possible?

[chrysh]

Thanks, The handler used by respond_once in run_terminus leaves before reaching handle_responder_msg when it doesn't recognize the eid as being from our designated EID. How do we currently impact the FD state machine without executing the handle_responder_msg? Is that possible?

@CourtneyDrant Not currently, it is meant a regression guard for future code changes.

[CourtneyDrant]

Thanks, The handler used by respond_once in run_terminus leaves before reaching handle_responder_msg when it doesn't recognize the eid as being from our designated EID. How do we currently impact the FD state machine without executing the handle_responder_msg? Is that possible?

@CourtneyDrant Not currently, it is meant a regression guard for future code changes.
Okay, so your concern is having a regression test that verifies attacker EIDs can't influence the FD state machine. So essentially, we need an attacker Type 5 regression test.

[chrysh]

Thanks — most of the last round is addressed and the interface reads much cleaner now.

[CourtneyDrant]

Can we move any new regression requests to issues and finish this PR?

[chrysh]

Yes, let's finish this PR. The RunTerminusResult idle-timeout mapping (timeout -> Completed so callers can drop the is_timeout() special case) can move to a follow-up issue — feel free to open it, or I will.

One small thing to fix here: README.md line 97 still references FD_IPC_MAX_MSG; the constant is now FD_MAX_MSG.

Approving already so this isn't blocked on me.

[chrysh]

Filed the timeout-mapping follow-up as #429.

[leongross]

Resolve the merge conflict and then we can get this merged.

[wmaroneAMD]

The merge conflicts are in MODULE.bazel.lock - can we deploy the Bazel conflict autoresolver?

https://bazel.build/external/lockfile#automatic-resolution

[leongross]

The merge conflicts are in MODULE.bazel.lock - can we deploy the Bazel conflict autoresolver?

bazel.build/external/lockfile#automatic-resolution

That looks very promising, good catch!