[{"id":3688134,"web_url":"http://patchwork.ozlabs.org/comment/3688134/","msgid":"<20260508034126.4A5B0C2BCB0@smtp.kernel.org>","list_archive_url":null,"date":"2026-05-08T03:41:25","subject":"Re: [PATCH 07/18] lib: rspdm: Initial commit of Rust SPDM","submitter":{"id":93311,"url":"http://patchwork.ozlabs.org/api/people/93311/","name":null,"email":"sashiko-bot@kernel.org"},"content":"Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:\n- [Critical] `spdm_exchange` parses the response buffer before the transport function populates it, violating aliasing rules and causing incorrect behavior.\n- [Critical] Casting untrusted device data to a packed struct containing a Rust enum causes Undefined Behavior.\n- [High] `spdm_destroy` is an empty function, leading to a permanent memory leak of the SPDM state.\n- [High] `spdm_exchange` unsoundly requires a `&'static mut SpdmState` parameter across the FFI boundary.\n- [Medium] `config RSPDM` uses `select RUST` without inheriting its dependencies.\n- [Low] `spdm_exchange` is exported to C but missing a declaration in the header file.\n- [Low] The `LowerHex` implementation for `SpdmErrorCode` injects unexpected newlines into logs.\n--\n\ncommit e8e3509aa659e22c45fe6865916c641e09098dfb\nAuthor: Alistair Francis \n\nlib: rspdm: Initial commit of Rust SPDM\n\nThis is the initial commit of the Rust SPDM library. It is based on and\ncompatible with the C SPDM library in the kernel (lib/spdm).\n\n> diff --git a/include/linux/spdm.h b/include/linux/spdm.h\n> new file mode 100644\n> index 0000000000000..e23e386ed97ab\n> --- /dev/null\n> +++ b/include/linux/spdm.h\n[ ... ]\n> +struct spdm_state *spdm_create(struct device *dev, spdm_transport *transport,\n> +\t\t\t void *transport_priv, u32 transport_sz,\n> +\t\t\t spdm_validate *validate);\n> +\n> +int spdm_authenticate(struct spdm_state *spdm_state);\n> +\n> +void spdm_destroy(struct spdm_state *spdm_state);\n\nIs spdm_exchange() intended to be a part of the public C API? The Rust\nimplementation exports it, but a declaration appears to be missing from this\nheader file.\n\n> +\n> +extern const struct attribute_group spdm_attr_group;\n> +\n> +#endif\n> diff --git a/lib/Kconfig b/lib/Kconfig\n> index 00a9509636c18..5a248709132e2 100644\n> --- a/lib/Kconfig\n> +++ b/lib/Kconfig\n[ ... ]\n> @@ -592,6 +592,23 @@ config LWQ_TEST\n> \thelp\n> Run boot-time test of light-weight queuing.\n> \n> +config RSPDM\n> +\tbool \"Rust SPDM\"\n> +\tselect RUST\n\nDoes this need to use \"depends on RUST\" instead of \"select RUST\"? According\nto the Kconfig subsystem guidelines, selecting a symbol without satisfying\nits dependencies (such as HAVE_RUST) can cause unmet dependency warnings and\npotential build failures.\n\n> +\tselect CRYPTO\n> +\tselect KEYS\n[ ... ]\n> diff --git a/lib/rspdm/consts.rs b/lib/rspdm/consts.rs\n> new file mode 100644\n> index 0000000000000..2feddde678858\n> --- /dev/null\n> +++ b/lib/rspdm/consts.rs\n[ ... ]\n> +pub(crate) const SPDM_REQ: u8 = 0x80;\n> +pub(crate) const SPDM_ERROR: u8 = 0x7f;\n> +\n> +#[expect(dead_code)]\n> +#[derive(Clone, Copy)]\n> +pub(crate) enum SpdmErrorCode {\n> + InvalidRequest = 0x01,\n\nDoes this enum need a #[repr(u8)] attribute? It is embedded in the packed\nSpdmErrorRsp struct and cast from untrusted device data. Without a specified\nrepresentation, its memory layout is compiler-defined, which might lead to\nunexpected behavior when casting raw bytes.\n\n[ ... ]\n> +impl core::fmt::LowerHex for SpdmErrorCode {\n> + /// A debug print format for the SpdmSessionInfo struct\n> + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {\n> + writeln!(f, \"{:#x}\", *self as u8)?;\n\nThis isn't a bug, but using writeln! here appends a newline character. When\nformatted inside macros like pr_err! that already include a newline, this\nresults in two consecutive newlines in the kernel log. Would it be better\nto use write! instead?\n\n> +\n> + Ok(())\n> + }\n> +}\n> diff --git a/lib/rspdm/lib.rs b/lib/rspdm/lib.rs\n> new file mode 100644\n> index 0000000000000..758d43fba5cba\n> --- /dev/null\n> +++ b/lib/rspdm/lib.rs\n[ ... ]\n> +#[no_mangle]\n> +pub unsafe extern \"C\" fn spdm_exchange(\n> + state: &'static mut SpdmState,\n\nCould this signature cause lifetime issues? The state reference is marked\nas &'static mut, which asserts to the compiler that the object lives for\nthe entire duration of the program and has exclusive global access. When\nthe C side frees this state via spdm_destroy(), the 'static guarantee is\nviolated. Should this take a raw pointer or an elided lifetime reference?\n\n> + req: *mut c_void,\n> + req_sz: usize,\n> + rsp: *mut c_void,\n> + rsp_sz: usize,\n> +) -> isize {\n[ ... ]\n> +/// spdm_destroy() - Destroy SPDM session\n> +///\n> +/// @spdm_state: SPDM session state\n> +#[export]\n> +pub unsafe extern \"C\" fn spdm_destroy(_state_ptr: *mut spdm_state) {}\n\nDoes this code leak the SPDM state allocation? spdm_create() allocates the\nmemory on the heap using KBox::new() and transfers ownership to C, but this\nfunction is empty and does not drop the allocation.\n\n> diff --git a/lib/rspdm/state.rs b/lib/rspdm/state.rs\n> new file mode 100644\n> index 0000000000000..18e81f24c7249\n> --- /dev/null\n> +++ b/lib/rspdm/state.rs\n[ ... ]\n> + pub(crate) fn spdm_exchange(\n> + &self,\n> + request_buf: &mut [u8],\n> + response_buf: &mut [u8],\n> + ) -> Result {\n> + let header_size = core::mem::size_of::();\n> + let request: &mut SpdmHeader = Untrusted::new_mut(request_buf).validate_mut()?;\n> + let response: &SpdmHeader = Untrusted::new_ref(response_buf).validate()?;\n\nCan this result in undefined behavior due to aliasing rules? The code\ncreates an immutable reference to the response_buf here, but later passes\na mutable raw pointer to the C transport function below while this reference\nis still alive.\n\nAlso, does this validate uninitialized or stale memory instead of the newly\nreceived response? The transport function has not been called yet.\n\n> +\n> + let transport_function = self.transport.ok_or(EINVAL)?;\n> + // SAFETY: `transport_function` is provided by the new(), we are\n> + // calling the function.\n> + let length = unsafe {\n> + transport_function(\n> + self.transport_priv,\n> + self.dev,\n> + request_buf.as_ptr() as *const c_void,\n> + request_buf.len(),\n> + response_buf.as_mut_ptr() as *mut c_void,\n> + response_buf.len(),\n> + ) as i32\n> + };\n> + to_result(length)?;\n> +\n> + if (length as usize) < header_size {\n> + return Ok(length); // Truncated response is handled by callers\n> + }\n> + if response.code == SPDM_ERROR {\n> + if length as usize >= core::mem::size_of::() {\n> + // SAFETY: The response buffer will be at least as large as\n> + // `SpdmErrorRsp` so we can cast the buffer to `SpdmErrorRsp` which\n> + // is a packed struct.\n> + self.spdm_err(unsafe { &*(response_buf.as_ptr() as *const SpdmErrorRsp) })?;\n\nCan this raw pointer cast cause undefined behavior if the untrusted device\ndata contains an invalid enum variant? SpdmErrorRsp embeds the Rust enum\nSpdmErrorCode, and interpreting unvalidated bytes as a Rust enum that does\nnot match a valid variant is undefined behavior.\n\n> + } else {\n> + return Err(EINVAL);\n> + }\n> + }","headers":{"Return-Path":"\n ","X-Original-To":["incoming@patchwork.ozlabs.org","linux-pci@vger.kernel.org"],"Delivered-To":"patchwork-incoming@legolas.ozlabs.org","Authentication-Results":["legolas.ozlabs.org;\n\tdkim=pass (2048-bit key;\n unprotected) header.d=kernel.org header.i=@kernel.org header.a=rsa-sha256\n header.s=k20201202 header.b=bBzP4/ZI;\n\tdkim-atps=neutral","legolas.ozlabs.org;\n spf=pass (sender SPF authorized) smtp.mailfrom=vger.kernel.org\n (client-ip=172.234.253.10; helo=sea.lore.kernel.org;\n envelope-from=linux-pci+bounces-54175-incoming=patchwork.ozlabs.org@vger.kernel.org;\n receiver=patchwork.ozlabs.org)","smtp.subspace.kernel.org;\n\tdkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org\n header.b=\"bBzP4/ZI\"","smtp.subspace.kernel.org;\n arc=none smtp.client-ip=10.30.226.201"],"Received":["from sea.lore.kernel.org (sea.lore.kernel.org [172.234.253.10])\n\t(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)\n\t key-exchange x25519)\n\t(No client certificate requested)\n\tby legolas.ozlabs.org (Postfix) with ESMTPS id 4gBZfv6VG2z1yCg\n\tfor ; Fri, 08 May 2026 13:41:43 +1000 (AEST)","from smtp.subspace.kernel.org (conduit.subspace.kernel.org\n [100.90.174.1])\n\tby sea.lore.kernel.org (Postfix) with ESMTP id 969ED3011C69\n\tfor ; Fri, 8 May 2026 03:41:27 +0000 (UTC)","from localhost.localdomain (localhost.localdomain [127.0.0.1])\n\tby smtp.subspace.kernel.org (Postfix) with ESMTP id E0F27313537;\n\tFri, 8 May 2026 03:41:26 +0000 (UTC)","from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org\n [10.30.226.201])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby smtp.subspace.kernel.org (Postfix) with ESMTPS id BE42B308F07\n\tfor ; Fri, 8 May 2026 03:41:26 +0000 (UTC)","by smtp.kernel.org (Postfix) with ESMTPSA id 4A5B0C2BCB0;\n\tFri, 8 May 2026 03:41:26 +0000 (UTC)"],"ARC-Seal":"i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116;\n\tt=1778211686; cv=none;\n b=hP6sevRRikr+707qZjwPBNrffrn1mjgpZlsUjx1tz/9iK9d+rPUJnEb0+xOpiUxwTaV8tbuGAaSsfa2uOB3O2k4iMUH7EqrZqQam+pujRRDoVUuadZRZ/UNUh/2Ajtl4DYJUhpWFqIX+iqR6hdnH4ND8tc0wZ6BHLpQO+jNsnEg=","ARC-Message-Signature":"i=1; a=rsa-sha256; d=subspace.kernel.org;\n\ts=arc-20240116; t=1778211686; c=relaxed/simple;\n\tbh=BvDqFMLvyDt4fMplbxM1GWoR5pa0JJfp1yAKvmGgp/k=;\n\th=From:Subject:To:Cc:In-Reply-To:References:Content-Type:Date:\n\t Message-Id;\n b=mHSVKzvJW2+3WxcYtJpJK2qn/x+j5WVHFwm59cP+y2TVfKjDSFo5qYhWTnpWsLjub9VSfKTtk65zcz3ZBJs3qzCL8HCLCS4W1b5zoD7FjFmFOVpIU8uFwypBZBNj5Znr2VaNQxh2Da8QHYEEuScY0gpmptah0pItyJgTnc2+ESo=","ARC-Authentication-Results":"i=1; smtp.subspace.kernel.org;\n dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org\n header.b=bBzP4/ZI; arc=none smtp.client-ip=10.30.226.201","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org;\n\ts=k20201202; t=1778211686;\n\tbh=BvDqFMLvyDt4fMplbxM1GWoR5pa0JJfp1yAKvmGgp/k=;\n\th=From:Subject:Reply-To:To:Cc:In-Reply-To:References:Date:From;\n\tb=bBzP4/ZIEzFvaLMR08EhDJKO/Jj09+sgHOP5JaQ6b/KzexwXToOdBA7W0nH6e+YI6\n\t FhRr53jXRsPKdCWMhm/ZYAoyGec95ss14aEl0YkYaFOBu7ZOlynLXTq9TZPNOAZqcQ\n\t XAUVwtAHHyX351OprtFbr2UL9ARjFU7cysRFnFb9tH0bN3MYo7NqdtZD0RmoGZeC8J\n\t ZDds1FcHMtGi75OeASgP1Hdyfz9Rfzra0hvQDMW49G7vCH7MA8yNNmwppPaKKtqqUf\n\t 9VPvZqs/K7LO5Nzhzg3LAATCgLULY/JEQWlooz1teBW06cvtuOP82v4mf/GmT8NelD\n\t y8ynOgjmPHAqw==","From":"sashiko-bot@kernel.org","Subject":"Re: [PATCH 07/18] lib: rspdm: Initial commit of Rust SPDM","Reply-To":"sashiko@lists.linux.dev","To":"alistair23@gmail.com","Cc":"linux-pci@vger.kernel.org, ojeda@kernel.org","In-Reply-To":"<20260508031710.514574-8-alistair.francis@wdc.com>","References":"<20260508031710.514574-8-alistair.francis@wdc.com>","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","Date":"Fri, 08 May 2026 03:41:25 +0000","Message-Id":"<20260508034126.4A5B0C2BCB0@smtp.kernel.org>","Precedence":"bulk","X-Mailing-List":"linux-pci@vger.kernel.org","List-Id":"","List-Subscribe":"","List-Unsubscribe":""}}]