Re: [PATCH 5/7] drivers: greybus: Add software SVC implementation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] Calling `Module::new` with an empty slice panics the kernel.
- [High] Out-of-bounds bitmap access in `release_operation_id` crashes the kernel.
- [High] Unconditionally calling `msg.sent(0)` followed by returning an error causes a double-free of the operation.
- [High] Initialization error path permanently leaks `GLOBAL_STATE`.
- [High] InterfaceOps are leaked in Svc::module_insert() if send_request() fails.
- [High] Incorrect endianness conversions for Greybus SVC constant values break big-endian platforms.
- [Medium] `module_insert` hardcodes `intf_count` to 1, breaking support for modules with multiple interfaces.
- [Low] Copy-paste errors in `gb_softsvc_exports.c` metadata.
--

--- Patch [5]: [PATCH 5/7] drivers: greybus: Add software SVC implementation ---
commit ae94a156cc024cc7dae613127a7c0c6694200c02
Author: Ayush Singh <ayush@beagleboard.org>

drivers: greybus: Add software SVC implementation

A Greybus network requires an SVC (Supervisory Controller) to bring
interfaces up, assign device IDs and connect CPorts to the AP. This adds
gb-softsvc, a Rust implementation of the SVC that lives in the kernel
and registers itself as a Greybus host device.

> diff --git a/drivers/greybus/gb_softsvc.rs b/drivers/greybus/gb_softsvc.rs
> new file mode 100644
> index 0000000000000..0d125a1a2cc8f
> --- /dev/null
> +++ b/drivers/greybus/gb_softsvc.rs
> @@ -0,0 +1,472 @@
[ ... ]
> +const SVC_VERSION_MAJOR: u8 = 0;
> +const SVC_VERSION_MINOR: u8 = 1;
> +const ENDO_ID: u16 = u16::from_le(0x4755);

[Severity: High]
Are u16::from_le() and u32::from_le() being used correctly here?

Constructor functions like GbSvcHelloRequest::new() expect native-endian
arguments and internally convert them to little-endian. Applying from_le()
followed by the internal to_le() cancels out the byte swap on big-endian
platforms, causing the data to be transmitted in native big-endian format
instead of the required little-endian.

[ ... ]
> +    fn new() -> impl PinInit<Self, Error> {
> +        pin_init!(Self {
> +            intf_map <- new_mutex!(init_array_from_fn(|_| None)),
> +            svc_operation_id <- new_spinlock!(IdPool::with_capacity(u16::MAX.into(), GFP_KERNEL)?)

[Severity: High]
The ID pool is initialized with a capacity of u16::MAX (65535), making valid
indices 0 to 65534.  If an untrusted remote endpoint sends a message with
operation_id = 65535, does this pass the out-of-bounds ID directly to
release_id() in release_operation_id()?

[ ... ]
> +    fn release_operation_id(&self, id: u16) {
> +        let mut ida = self.svc_operation_id.lock();
> +        ida.release_id(id as usize);
> +    }
[ ... ]
> +    fn map_insert(&self, intfs: &[Arc<dyn InterfaceOps>]) -> Result<u8> {
> +        fn inner(
> +            intf_map: &[Option<Arc<dyn InterfaceOps>>; INTF_MAP_LEN],
> +            count: usize,
> +        ) -> Result<u8> {
> +            for (id, window) in intf_map.windows(count).enumerate() {

[Severity: High]
If a caller passes an empty slice of interfaces to Module::new(), won't
count be 0? Calling .windows(0) on a slice unconditionally panics in
Rust, which would crash the kernel.

> +                if window.iter().all(|x| x.is_none()) {
> +                    return Ok(id as u8);
> +                }
> +            }
[ ... ]
> +    fn module_insert(&self, hd: &hd::Device, intfs: &[Arc<dyn InterfaceOps>]) -> Result<u8> {
> +        let intf_id = self.map_insert(intfs)?;
> +        let payload = protocols::GbSvcModuleInsertedRequest::new(intf_id, 1, 0);

[Severity: Medium]
Does hardcoding the intf_count to 1 prevent the Greybus core from
discovering subsequent interfaces when intfs.len() > 1?

> +        self.send_request(hd, protocols::GB_SVC_TYPE_MODULE_INSERTED, payload)?;

[Severity: High]
If self.send_request() fails, the function returns an error, but the
interfaces were already inserted into intf_map via self.map_insert().
Since Module::new() aborts on this error path, Module::drop will never
be called to clean up.  Does this permanently leak the slots in intf_map
and the Arc<dyn InterfaceOps> references?

> +
> +        Ok(intf_id)
> +    }
[ ... ]
> +    fn dme_peer_get(&self, hd: &hd::Device, hdr: &protocols::GbOperationMsgHdr) {
> +        let payload = protocols::GbSvcDmePeerGetResponse::new(0, u32::from_le(0x0126));

[Severity: High]
This seems to have the same endianness cancellation issue as ENDO_ID when
used on big-endian architectures.

> +        self.send_response(hd, hdr, payload);
> +    }
[ ... ]
> +impl hd::HdDriver for GbHdDriver {
> +    fn message_send(data: &Self, dest_cport_id: u16, msg: greybus::Message) -> Result {
> +        let res = data.message_send_inner(dest_cport_id, &msg);
> +        msg.sent(0);

[Severity: High]
If message_send_inner() returns an error, msg.sent(0) will incorrectly
report success to the Greybus core, causing it to drop its reference to the
operation structure. When the error is then returned to the caller, it may also
drop the reference. Could this sequence result in a double-free?

> +
> +        res
> +    }
[ ... ]
> +impl kernel::Module for GreybusSoftSvc {
> +    fn init(_module: &'static ThisModule) -> Result<Self> {
> +        pr_info!("gb_softsvc (init)\n");
> +
> +        // SAFETY: This runs once at module init, before anything else can reach GLOBAL_STATE.
> +        unsafe { GLOBAL_STATE.init() };
> +
> +        let faux = faux::Registration::new(c_str!("gb-softsvc"), None)?;
> +        let svc = Arc::pin_init(Svc::new(), GFP_KERNEL)?;
> +        let data = GbHdDriver(svc.clone());
> +
> +        let dev = faux.as_ref().as_ref();
> +        let hd = hd::Registration::new(dev, GB_OPERATION_SIZE_MAX, CPORT_ID_MAX + 1, Ok(data))?;
> +
> +        let global_state = GlobalState::new(svc.clone(), hd.as_ref().into());
> +        let _ = GLOBAL_STATE.lock().replace(global_state);
> +
> +        svc.send_version(hd.as_ref())?;

[Severity: High]
If svc.send_version() fails, module initialization is aborted and
GreybusSoftSvc::drop() will not be called. Does this leave GLOBAL_STATE
populated, permanently leaking the Arc<Svc> and ARef<hd::Device> references?

> +
> +        Ok(GreybusSoftSvc {
> +            _hd: hd,
> +            _faux: faux,
> +        })
> +    }
> +}
[ ... ]
> +    pub fn submit_message(&self, cport: u16, msg: &[u8]) -> Result<()> {
> +        let intf = self
> +            .gb_hd
> +            .find_connection_by_intf(self.id, cport)

[Severity: Medium]
In addition to hardcoding the interface count to 1 earlier, using self.id
here instead of allowing the caller to specify the interface ID seems to
prevent routing messages for any interfaces beyond the first one.

> +            .ok_or(code::EINVAL)?;
> +        self.gb_hd.data_rcvd(intf.hd_cport_id(), msg);
> +
> +        Ok(())
> +    }
[ ... ]
> diff --git a/drivers/greybus/gb_softsvc_exports.c b/drivers/greybus/gb_softsvc_exports.c
> new file mode 100644
> index 0000000000000..38b99c4de6eec
> --- /dev/null
> +++ b/drivers/greybus/gb_softsvc_exports.c
> @@ -0,0 +1,15 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
> +
> +/*
> + * Exports Rust symbols from the nova_core crate for use by dependent modules.

[Severity: Low]
Are the references to NVIDIA and the nova_core crate here accidental
copy-paste errors from another driver?

> + *
> + * This is a workaround until the build system supports Rust cross-module
> + * dependencies natively.
> + */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260820-gb-uart-transport-v1-0-282da14ab7b7@beagleboard.org?part=5