linux-kernel - Re: [RFC 1/2] rust: introduce abstractions for fwctl

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20251103115432.5b593934.zhiw@nvidia.com>
Date: Mon, 3 Nov 2025 11:55:44 +0200
From: Zhi Wang <zhiw@...dia.com>
To: Danilo Krummrich <dakr@...nel.org>
CC: <rust-for-linux@...r.kernel.org>, <bhelgaas@...gle.com>,
	<kwilczynski@...nel.org>, <ojeda@...nel.org>, <alex.gaynor@...il.com>,
	<boqun.feng@...il.com>, <gary@...yguo.net>, <bjorn3_gh@...tonmail.com>,
	<lossin@...nel.org>, <a.hindborg@...nel.org>, <aliceryhl@...gle.com>,
	<tmgross@...ch.edu>, <linux-kernel@...r.kernel.org>, <cjia@...dia.com>,
	<smitra@...dia.com>, <ankita@...dia.com>, <aniketa@...dia.com>,
	<kwankhede@...dia.com>, <targupta@...dia.com>, <zhiwang@...nel.org>,
	<alwilliamson@...dia.com>, <acourbot@...dia.com>, <joelagnelf@...dia.com>,
	<jhubbard@...dia.com>, <jgg@...dia.com>
Subject: Re: [RFC 1/2] rust: introduce abstractions for fwctl

On Sun, 02 Nov 2025 19:33:10 +0100
"Danilo Krummrich" <dakr@...nel.org> wrote:

> On Thu Oct 30, 2025 at 5:03 PM CET, Zhi Wang wrote:
> > +/// Static vtable mapping Rust trait methods to C callbacks.
> > +pub struct FwCtlVTable<T: FwCtlOps>(PhantomData<T>);
> > +
> > +impl<T: FwCtlOps> FwCtlVTable<T> {
> > +    /// Static instance of `fwctl_ops` used by the C core to call
> > into Rust.
> > +    pub const VTABLE: bindings::fwctl_ops = bindings::fwctl_ops {
> > +        device_type: T::DEVICE_TYPE,
> > +        uctx_size: core::mem::size_of::<FwCtlUCtx<T::UCtx>>(),
> 
> The fwctl code uses this size to allocate memory for both, struct
> fwctl_uctx and the driver's private data at the end of the allocation.
> 
> This means that it is not enough to just consider the size of
> T::UCtx, you also have to consider its required alignment, and, if
> required, allocate more memory.
> 

FwCtlUCtx is defined as below:

+#[repr(C)]
+#[pin_data]
+pub struct FwCtlUCtx<T> {
+    /// The core fwctl user context shared with the C implementation.
+    #[pin]
+    pub fwctl_uctx: bindings::fwctl_uctx,
+    /// Driver-specific data associated with this user context.
+    pub uctx: T,
+}

I assume it should be equal to C structure as below and with #[repr(C)],
the handling of layout and the alignment should be as the same as C
structure.

struct FwCtlUCtx {
	struct fwctl_uctx base;
	struct my_driver_data data;
};

uctx_size: core::mem::size_of::<FwCtlUCtx<T::UCtx>>() should be:

sizeof(FWCtlUCtx).

Do we need anything extra for alignment? Or some parts of the flow
doesn't respect the #[repr(C)]?

> > +        open_uctx: Some(Self::open_uctx_callback),
> > +        close_uctx: Some(Self::close_uctx_callback),
> > +        info: Some(Self::info_callback),
> > +        fw_rpc: Some(Self::fw_rpc_callback),
> > +    };
> > +
> > +    /// Called when a new user context is opened by userspace.
> > +    unsafe extern "C" fn open_uctx_callback(uctx: *mut
> > bindings::fwctl_uctx) -> ffi::c_int {
> > +        // SAFETY: `uctx` is guaranteed by the fwctl subsystem to
> > be a valid pointer.
> > +        let ctx = unsafe { FwCtlUCtx::<T::UCtx>::from_raw(uctx) };
> 
> Considering the above, this is incorrect for two reasons.
> 
>   (1) FwCtlUCtx::uctx might not be aligned correctly.
> 
>   (2) FwCtlUCtx::uctx is not initialized, hence creating a reference
> might be undefined behavior.
> 
> I think the correct way to fix (2) is to only provide an abstraction
> of struct fwctl_uctx as argument to T::open_uctx() and let
> T::open_uctx() return an initializer for FwCtlUCtx::uctx, i.e. impl
> PinInit<T::UCtx, Error>.
> 

Nice catch. With this approach, we can force the driver user context to
be explicitly intialized and start to be tracked in the safe world.

> All other callbacks should be correct as they are once the alignment
> is considered.
> 
> > +        match T::open_uctx(ctx) {
> > +            Ok(()) => 0,
> > +            Err(e) => e.to_errno(),
> > +        }
> > +    }