Re: [PATCH v3 7/7] greybus: Add Rust UART node driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] greybus: UAF in GbNode::write due to concurrent serdev unbind
- [High] gb_uart_node: hardcoded 256-byte HDLC limit drops Greybus messages
--

commit 3275429a0af2d87e184830cd9fec54db8ff56af7
Author: Ayush Singh <ayush@beagleboard.org>

    greybus: Add Rust UART node driver
    
    Add a driver for Greybus nodes attached over a plain serial port. The
    node is registered with the software SVC (gb-softsvc), which handles the
    SVC protocol on behalf of the AP, so no dedicated coprocessor running
    SVC firmware is needed.

    [0]: https://lore.kernel.org/all/20260622-nova-exports-v5-0-6191773fc977@nvidia.com/

> diff --git a/drivers/greybus/gb_uart_node.rs b/drivers/greybus/gb_uart_node.rs
> new file mode 100644
> index 0000000000000..f719ee35e8f38
> --- /dev/null
> +++ b/drivers/greybus/gb_uart_node.rs
> @@ -0,0 +1,253 @@

[ ... ]
> +struct HdlcRx {
> +    rx_buf: KVec<u8>,
> +    rx_in_esc: bool,
> +    rx_overflow: bool,
> +    sdev: ARef<serdev::Device>,
> +    node: gb_softsvc::Module,
> +}
> +
> +impl HdlcRx {
> +    fn new(sdev: ARef<serdev::Device>, node: gb_softsvc::Module) -> Result<Self> {
> +        Ok(Self {
> +            node,
> +            sdev,
> +            rx_buf: KVec::with_capacity(HDLC_MAX_FRAME_LEN, GFP_KERNEL)?,

[Severity: High]
Does limiting the HDLC buffer to 256 bytes cause Greybus messages to be
dropped?

Looking at HdlcRx::rx(), if the buffer exceeds HDLC_MAX_FRAME_LEN, it clears
the buffer and drops the frame. Greybus payloads (plus headers and CRC) can
easily exceed 256 bytes, especially after HDLC byte escaping which can
increase the size of the payload.

> +            rx_in_esc: false,
> +            rx_overflow: false,
> +        })
> +    }

[ ... ]
> +    fn rx(&mut self, data: &[u8]) -> usize {
> +        for i in data.iter() {
> +            match *i {

[ ... ]
> +                _ => {
> +                    let c = if self.rx_in_esc { *i ^ HDLC_XOR } else { *i };
> +                    self.rx_in_esc = false;
> +
> +                    if self.rx_buf.push_within_capacity(c).is_err() {
> +                        dev_warn!(self.sdev.as_ref(), "buffer overflow. Dropping frame");
> +
> +                        self.rx_overflow = true;
> +                        self.rx_buf.clear();
> +                    }
> +                }
> +            }
> +        }

[ ... ]
> +impl gb_softsvc::InterfaceOps for GbNode {
> +    fn write(&self, data: &[u8], cport: u16, gfp_mask: Flags) -> Result<()> {
> +        // SAFETY: `GbNode` only exists while its serdev driver is bound, so the device is in the
> +        // `Bound` state for the duration of this call.
> +        let bound: &serdev::Device<Bound> =
> +            unsafe { serdev::Device::from_device(self.sdev.as_ref().as_bound()) };

[Severity: High]
Is it possible for GbNode::write() to race with GbUartNode::unbind() and
cause a use-after-free?

Looking at GbUartNode::unbind(), it drops HdlcRx and its gb_softsvc::Module.
However, if another thread is currently holding an Arc<GbNode> (for example,
in GbHdDriver::message_send() via intf_by_id()) and is preempted before
calling intf.write():

Thread 1                                Thread 2
GbHdDriver::message_send()
  intf = intf_by_id(...)
  // Preempted
                                        GbUartNode::unbind()
                                          this.rx.lock().take()
                                          // Device is no longer Bound

  intf.write()

When the preempted thread resumes and calls GbNode::write(), it will execute
the unsafe cast to serdev::Device<Bound>. At this point, the device might no
longer be in the Bound state, leading to undefined behavior or a
use-after-free. 

Does GbUartNode::unbind() need to synchronize and wait for all active
Arc<GbNode> references to be dropped before returning?

> +
> +        let mut buf = KVec::with_capacity(HDLC_MAX_FRAME_LEN, gfp_mask)?;

[Severity: High]
Can this fixed-capacity allocation fail for valid Greybus messages? 

Since HDLC escaping can potentially double the size of the payload, should
this dynamically allocate the TX buffer based on the actual size of the data
to accommodate the worst-case HDLC escaped size?

> +
> +        let mut crc = 0xffff;

[ ... ]
> +    fn unbind<'bound>(_: &'bound serdev::Device<Core<'_>>, this: Pin<&Self::Data<'bound>>) {
> +        // Getting a bound device is not possible after this point. So drop HdlcRx.
> +        // This also drops HdlcRx::node, which should ensure that GbNode::write will not be called
> +        // after this point.
> +        let _ = this.rx.lock().take();
> +    }
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013-gb-uart-transport-v3-0-81d0a8a1acf7@beagleboard.org?part=7