Add bounds checking for apob_read (#2484)

This PR adds bounds checking when obtaining space for the requested APOB read buffer. Previously, this was not explicitly checked, and could lead to a panic (and restart) of the `host-sp-comms` task.

This PR also fixes `ringbuf` logging for `host-sp-comms`, which could sometimes report a success to the ring buffer when in fact the action errored out.

Author: jamesmunns

app/cosmo/base.toml

@@ -258,7 +258,7 @@ features = ["stm32h753", "usart6", "baud_rate_3M", "hardware_flow_control", "vla
 uses = ["usart6", "dbgmcu"]
 interrupts = {"usart6.irq" = "usart-irq"}
 priority = 9
-max-sizes = {flash = 70000, ram = 65536}
+max-sizes = {flash = 74000, ram = 65536}
 stacksize = 5400
 start = true
 task-slots = ["sys", { cpu_seq = "cosmo_seq" }, "hf", "control_plane_agent", "net", "packrat", "i2c_driver", { spi_driver = "spi2_driver" }, "sprot", "auxflash"]

app/gimlet/base.toml

@@ -243,7 +243,7 @@ features = ["stm32h753", "uart7", "baud_rate_3M", "hardware_flow_control", "vlan
 uses = ["uart7", "dbgmcu"]
 interrupts = {"uart7.irq" = "usart-irq"}
 priority = 8
-max-sizes = {flash = 70000, ram = 65536}
+max-sizes = {flash = 72000, ram = 65536}
 stacksize = 5376
 start = true
 task-slots = ["sys", { cpu_seq = "gimlet_seq" }, "hf", "control_plane_agent", "net", "packrat", "i2c_driver", { spi_driver = "spi2_driver" }, "sprot"]

task/host-sp-comms/src/main.rs

@@ -1269,6 +1269,10 @@ impl ServerImpl {
     fn apob_read(&mut self, sequence: u64, offset: u64, size: u64) {
         use drv_hf_api::ApobReadError;
         use host_sp_messages::ApobReadResult;
+
+        // NOTE: This ONLY checks we are not out of bounds for `usize`,
+        // we will check if the requested size can be honored later in
+        // the `fill` closure.
         let Ok(size) = usize::try_from(size) else {
             self.tx_buf.encode_response(
                 sequence,
@@ -1285,33 +1289,37 @@ impl ServerImpl {
             );
             return;
         };
+
+        // closure for performing the actual apob read
+        let fill = |buf: &mut [u8]| {
+            // Did the user pass in a reasonable size?
+            let Some(rbuf) = buf.get_mut(..size) else {
+                return Err(SpToHost::ApobRead(ApobReadResult::InvalidSize));
+            };
+
+            // If successful: return the number of bytes read
+            let err = match self.hf.apob_read(offset, rbuf) {
+                Ok(n) => return Ok(n),
+                Err(e) => e,
+            };
+
+            // Something went wrong, ringbuf the error, and convert
+            // the error types
+            ringbuf_entry!(Trace::ApobReadError { offset, err });
+            let read_err = match err {
+                ApobReadError::NotImplemented => ApobReadResult::NotImplemented,
+                ApobReadError::InvalidState => ApobReadResult::InvalidState,
+                ApobReadError::NoValidApob => ApobReadResult::NoValidApob,
+                ApobReadError::InvalidOffset => ApobReadResult::InvalidOffset,
+                ApobReadError::InvalidSize => ApobReadResult::InvalidSize,
+                ApobReadError::ReadFailed => ApobReadResult::ReadFailed,
+            };
+            Err(SpToHost::ApobRead(read_err))
+        };
         self.tx_buf.try_encode_response(
             sequence,
             &SpToHost::ApobRead(ApobReadResult::Ok),
-            |buf| match self.hf.apob_read(offset, &mut buf[..size]) {
-                Ok(n) => Ok(n),
-                Err(err) => {
-                    ringbuf_entry!(Trace::ApobReadError { offset, err });
-                    Err(SpToHost::ApobRead(match err {
-                        ApobReadError::NotImplemented => {
-                            ApobReadResult::NotImplemented
-                        }
-                        ApobReadError::InvalidState => {
-                            ApobReadResult::InvalidState
-                        }
-                        ApobReadError::NoValidApob => {
-                            ApobReadResult::NoValidApob
-                        }
-                        ApobReadError::InvalidOffset => {
-                            ApobReadResult::InvalidOffset
-                        }
-                        ApobReadError::InvalidSize => {
-                            ApobReadResult::InvalidSize
-                        }
-                        ApobReadError::ReadFailed => ApobReadResult::ReadFailed,
-                    }))
-                }
-            },
+            fill,
         );
     }
 

task/host-sp-comms/src/tx_buf.rs

@@ -271,17 +271,35 @@ impl TxBuf {
             sequence: sequence | SEQ_REPLY,
         };
 
-        ringbuf_entry!(Trace::Response {
-            now: sys_get_timer().now,
-            sequence: header.sequence,
-            message: *response
-        });
+        // The caller's closure *may* change what the actual response is! The
+        // passed in `response` is only valid if `try_serialize` returns `Ok`,
+        // otherwise the new error type is returned. This closure intercepts the
+        // caller's closure, and logs *after* that closure has run, so we can
+        // accurately log what the response was.
+        let outer_fill_data = |buf: &mut [u8]| {
+            let res = fill_data(buf);
+            let msg = match &res {
+                Ok(_n) => *response,
+                Err(e) => *e,
+            };
+            ringbuf_entry!(Trace::Response {
+                now: sys_get_timer().now,
+                sequence: header.sequence,
+                message: msg
+            });
+            res
+        };
 
         // Serializing can only fail if we pass unexpected types as `response`,
         // but we're using `SpToHost` for both the response and error, so it
         // cannot fail.
-        host_sp_messages::try_serialize(self.msg, &header, response, fill_data)
-            .unwrap_lite()
+        host_sp_messages::try_serialize(
+            self.msg,
+            &header,
+            response,
+            outer_fill_data,
+        )
+        .unwrap_lite()
     }
 
     // Encodes `self.msg[..msg_len]` with corncobs.