lib: Rust implementation of SPDM
Subsystems (9)
v2 cover lib: Rust implementation of SPDM
From: Alistair Francis <[email protected]>
Security Protocols and Data Models (SPDM) [1] is used for authentication, attestation and key exchange. SPDM is generally used over a range of transports, such as PCIe, MCTP/SMBus/I3C, ATA, SCSI, NVMe or TCP.
From the kernels perspective SPDM is used to authenticate and attest devices. In this threat model a device is considered untrusted until it can be verified by the kernel and userspace using SPDM. As such SPDM data is untrusted data that can be mallicious.
The SPDM specification is also complex, with the 1.2.1 spec being almost 200 pages and the 1.3.0 spec being almost 250 pages long.
As such we have the kernel parsing untrusted responses from a complex specification, which sounds like a possible exploit vector. This is the type of place where Rust excels!
This series implements a SPDM requester in Rust.
This is based on Lukas' C implementation [2], but has been refacted during the first few RFCs. I have included some of the relevent patchesfrom Lukas' C SPDM implementation in this series where they are required.
This is a standalone series and doesn't depend on Lukas' implementation.
The goal of this series is to get the smallest possible SPDM implementation upstream. That will provide building blocks for us to continue working on.
As such we don't yet provide evidence or certificates to userspace, allow userspace to provide a nonce, support PQC or more advanced SPDM features. This is enough to communicate with a device and return "authenticated" to userspace.
Note that RFC v3 did provide evidence and certificates to userspace and allowed a custom nonce. Showing that it's possible. I also have patches that build apon [4] to do this via a TSM driver, again showing it's possible with the current approach.
We just don't support it yet and for TSM I need [4] upstream first.
This series is different to Lukas' original approach and the approach taken in the previous RFCs and instead adds the PCI-CMA support as a TSM driver. This was described by Dan in [3] and [5]. The advantage here is that for PCIe we can leverage the TSM work for a lot of the features and provide userspace a consistient interface between PCI TSM and CMA.
This series also doesn't check the certificate chain against the kernel keyring and will instead leave that to userspace once [4] is merged.
Other transport mode (such as ATA, SCSI, NVMe and MCTP) will therefore need slightly different approaches, as TSM doesn't apply. The library can support this though, it will just need some netlink and sysfs wrappers added as applicable. This way each transport can support SPDM in the way it sees fit.
The entire tree can be seen here:
https://github.com/alistair23/linux/tree/alistair/spdm-rust-tsm
I'm testing this by running the following
```shell cargo run -- --qemu-server response qemu-system-x86_64 \ -nic none \ -object rng-random,filename=/dev/urandom,id=rng0 \ -device virtio-rng-pci,rng=rng0 \ -drive file=deploy/images/qemux86-64/core-image-pcie-qemux86-64.rootfs.ext4,if=virtio,format=raw \ -usb -device usb-tablet -usb -device usb-kbd \ -cpu Skylake-Client \ -machine q35,i8042=off \ -smp 4 -m 2G \ -drive file=blknvme,if=none,id=mynvme,format=raw \ -device nvme,drive=mynvme,serial=deadbeef,spdm_port=2323,spdm_trans=doe \ -snapshot \ -serial mon:stdio -serial null -nographic \ -kernel deploy/images/qemux86-64/bzImage \ -append 'root=/dev/vda rw console=ttyS0 console=ttyS1 oprofile.timer=1 tsc=reliable no_timer_check rcupdate.rcu_expedited=1 swiotlb=0 ' ls /sys/devices/pci0000:00/0000:00:03.0/ ls /sys/devices/pci0000:00/0000:00:03.0/tsm/ cat /sys/devices/pci0000:00/0000:00:03.0/authenticated echo tsm0 > /sys/devices/pci0000:00/0000:00:03.0/tsm/connect cat /sys/devices/pci0000:00/0000:00:03.0/authenticated ```
1: https://www.dmtf.org/standards/spdm
2: https://lore.kernel.org/all/[email protected]/
3: http://lore.kernel.org/[email protected]
4: https://lore.kernel.org/all/[email protected]/
5: https://lore.kernel.org/lkml/[email protected]/
v2:
- Tidy up the PCI/TSM function naming and is_pci_tsm_host()
- A large number of changes (mostly to the Rust code) based on Sashiko reviews
- This includes a few local runs of Sashiko
- This has fixed a few undefined behviour bugs in the Rust code
- Rebase on v4 of Benno's validate patches (patch 1, 2, 3)
v1:
- Add CMA as a TSM driver
- Initial support for SPDM 1.4
- Cleanup a range of comments and concerns from RFC
- Remove kernel keyring checks
RFC v3:
- Use netlink to send information to userspace
- Don't autogenerate Rust helpers
RFC v2:
- Drop support for Rust and C implementations
- Include patches from Lukas to reduce series deps
- Large code cleanups based on more testing
- Support for authentication
Alistair Francis (14):
rust: add bindings for hash.h
rust: error: impl From<FromBytesWithNulError> for Kernel Error
lib: rspdm: Initial commit of Rust SPDM
PCI/TSM: Rename pf0 to host
PCI/TSM: Support connecting to PCIe CMA devices PCI/CMA: Add a PCI TSM CMA driver using SPDM
lib: rspdm: Support SPDM get_version
lib: rspdm: Support SPDM get_capabilities
lib: rspdm: Support SPDM negotiate_algorithms
lib: rspdm: Support SPDM get_digests
lib: rspdm: Support SPDM get_certificate
lib: rspdm: Support SPDM certificate validation
rust: allow extracting the buffer from a CString
lib: rspdm: Support SPDM challenge
Benno Lossin (3):
rust: transmute: add cast_slice[_mut] functions
rust: create basic untrusted data API
rust: validate: add Validate trait
Lukas Wunner (4):
X.509: Make certificate parser public
X.509: Parse Subject Alternative Name in certificates X.509: Move certificate length retrieval into new helper PCI/CMA: Validate Subject Alternative Name in certificates
MAINTAINERS | 13 + crypto/asymmetric_keys/x509_cert_parser.c | 9 + crypto/asymmetric_keys/x509_loader.c | 38 +- crypto/asymmetric_keys/x509_parser.h | 42 +- drivers/crypto/ccp/sev-dev-tio.h | 4 +- drivers/crypto/ccp/sev-dev-tsm.c | 12 +- drivers/pci/Kconfig | 14 + drivers/pci/Makefile | 5 + drivers/pci/cma.asn1 | 41 + drivers/pci/cma.c | 275 +++++ drivers/pci/doe.c | 3 - drivers/pci/tsm.c | 84 +- include/keys/asymmetric-type.h | 2 + include/keys/x509-parser.h | 57 + include/linux/oid_registry.h | 3 + include/linux/pci-doe.h | 4 + include/linux/pci-tsm.h | 29 +- include/linux/spdm.h | 37 + lib/Kconfig | 14 + lib/Makefile | 2 + lib/rspdm/Makefile | 10 + lib/rspdm/consts.rs | 184 ++++ lib/rspdm/lib.rs | 167 +++ lib/rspdm/state.rs | 1202 +++++++++++++++++++++ lib/rspdm/validator.rs | 545 ++++++++++ rust/bindings/bindings_helper.h | 5 + rust/helpers/hash.c | 18 + rust/helpers/helpers.c | 1 + rust/kernel/error.rs | 23 +- rust/kernel/lib.rs | 1 + rust/kernel/str.rs | 13 +- rust/kernel/transmute.rs | 59 + rust/kernel/validate.rs | 216 ++++ 33 files changed, 3008 insertions(+), 124 deletions(-) create mode 100644 drivers/pci/cma.asn1 create mode 100644 drivers/pci/cma.c create mode 100644 include/keys/x509-parser.h create mode 100644 include/linux/spdm.h create mode 100644 lib/rspdm/Makefile create mode 100644 lib/rspdm/consts.rs create mode 100644 lib/rspdm/lib.rs create mode 100644 lib/rspdm/state.rs create mode 100644 lib/rspdm/validator.rs create mode 100644 rust/helpers/hash.c create mode 100644 rust/kernel/validate.rs
Signature · 1 line
2.54.0
PATCH v2 1/21 rust: transmute: add `cast_slice[_mut]` functions
From: Benno Lossin <[email protected]>
Add functions to make casting slices only one unsafe block.
Message-ID: <[email protected]>
--- rust/kernel/transmute.rs | 59 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+)
rust/kernel/transmute.rs +59 -0
@@ -3,6 +3,7 @@//! Traits for transmuting types. use core::mem::size_of; +use core::slice;/// Types for which any bit pattern is valid. ///
@@ -227,3 +228,61 @@ macro_rules! impl_asbytes { {<T: AsBytes>} [T], {<T: AsBytes, const N: usize>} [T; N], } + +/// Casts the type of a slice to another. +/// +/// Also see [`cast_slice_mut`]. +/// +/// # Examples +/// +/// ```rust +/// # use kernel::transmute::cast_slice; +/// #[repr(transparent)] +/// #[derive(Debug)] +/// struct Container<T>(T); +/// +/// let array = [0u32; 42]; +/// let slice = &array; +/// // SAFETY: `Container<u32>` transparently wraps a `u32`. +/// let container_slice = unsafe { cast_slice::<u32, Container<u32>>(slice) }; +/// pr_info!("{container_slice:?}"); +/// ``` +/// +/// # Safety +/// +/// - `T` and `U` must have the same layout. +pub unsafe fn cast_slice <T ,U >(slice: &[T ]) -> &[U ] { +// CAST: by the safety requirements, `T` and `U` have the same layout. +let ptr = slice.as_ptr ().cast ::<U >(); +// SAFETY: `ptr` and `len` come from the same slice reference. +unsafe { slice::from_raw_parts (ptr, slice.len ()) } +} + +/// Casts the type of a slice to another. +/// +/// Also see [`cast_slice`]. +/// +/// # Examples +/// +/// ```rust +/// # use kernel::transmute::cast_slice_mut; +/// #[repr(transparent)] +/// #[derive(Debug)] +/// struct Container<T>(T); +/// +/// let mut array = [0u32; 42]; +/// let slice = &mut array; +/// // SAFETY: `Container<u32>` transparently wraps a `u32`. +/// let container_slice = unsafe { cast_slice_mut::<u32, Container<u32>>(slice) }; +/// pr_info!("{container_slice:?}"); +/// ``` +/// +/// # Safety +/// +/// - `T` and `U` must have the same layout. +pub unsafe fn cast_slice_mut <T ,U >(slice: &mut [T ]) -> &mut [U ] { +// CAST: by the safety requirements, `T` and `U` have the same layout. +let ptr = slice.as_mut_ptr ().cast ::<U >(); +// SAFETY: `ptr` and `len` come from the same slice reference. +unsafe { slice::from_raw_parts_mut (ptr, slice.len ()) } +}
Signature · 1 line
2.54.0
[email protected]
AI reviewRe: [PATCH v2 01/21] rust: transmute: add `cast_slice[_mut]` functions
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] The `# Safety` sections for `cast_slice` and `cast_slice_mut` are incomplete and unsound, omitting…
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The # Safety sections for cast_slice and cast_slice_mut are incomplete and unsound, omitting requirements for validity invariants, interior mutability, and mutable write-back.
- [Low] Missing #[inline] attribute on the cast_slice and cast_slice_mut abstraction functions.
--
commit 81ab9367137984ee8a82555d066400739962f02f
Author: Benno Lossin <[email protected]>
rust: transmute: add cast_slice[_mut] functions
Add functions to make casting slices only one unsafe block.
Message-ID: <[email protected]>
Quoted 3 lines
rust/kernel/transmute.rs +0
-0
[ ... ]
rust/kernel/transmute.rs macro_rules! impl_asbytes { · line 254
Quoted 4 lines
rust/kernel/transmute.rs +4
-0+/// # Safety +/// +/// - `T` and `U` must have the same layout. +pub unsafe fn cast_slice <T ,U >(slice: &[T ]) -> &[U ] {
[Severity: High]
Are the safety requirements here complete? It seems we also need to ensure that any bit pattern valid for T is also valid for U, and that the cast doesn't introduce interior mutability.
[Severity: Low]
This isn't a bug, but should this have an #[inline] attribute since it is a small forwarding function in an abstraction crate?
Quoted 5 lines
rust/kernel/transmute.rs +5
-0+// CAST: by the safety requirements, `T` and `U` have the same layout. +let ptr = slice.as_ptr ().cast ::<U >(); +// SAFETY: `ptr` and `len` come from the same slice reference. +unsafe { slice::from_raw_parts (ptr, slice.len ()) } +}
[ ... ]
rust/kernel/transmute.rs macro_rules! impl_asbytes { · line 283
Quoted 4 lines
rust/kernel/transmute.rs +4
-0+/// # Safety +/// +/// - `T` and `U` must have the same layout. +pub unsafe fn cast_slice_mut <T ,U >(slice: &mut [T ]) -> &mut [U ] {
[Severity: High]
Are the safety preconditions here sufficient to prevent undefined behavior? In addition to layout equivalence, any value of U written to the slice must also be a valid T, and the cast shouldn't introduce interior mutability.
[Severity: Low]
This isn't a bug, but should this also be marked with #[inline] according to the Rust subsystem coding guidelines for abstraction functions?
Quoted 5 lines
rust/kernel/transmute.rs +5
-0+// CAST: by the safety requirements, `T` and `U` have the same layout. +let ptr = slice.as_mut_ptr ().cast ::<U >(); +// SAFETY: `ptr` and `len` come from the same slice reference. +unsafe { slice::from_raw_parts_mut (ptr, slice.len ()) } +}
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
PATCH v2 2/21 rust: create basic untrusted data API
In reply to [email protected]From: Benno Lossin <[email protected]>
When the kernel receives external data (e.g. from userspace), it usually is a very bad idea to directly use the data for logic decision in the kernel. For this reason, such data should be explicitly marked and validated before making decision based on its value.
The Untrusted<T> wrapper type marks a value of type T as untrusted. The particular meaning of "untrusted" highly depends on the type T. For example T = u8 ensures that the value of the byte cannot be retrieved. However, T = [u8] still allows to access the length of the slice. Similarly, T = KVec<U> allows modifications.
Message-ID: <[email protected]>
--- rust/kernel/lib.rs | 1 + rust/kernel/validate.rs | 148 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 149 insertions(+) create mode 100644 rust/kernel/validate.rs
rust/kernel/lib.rs +1 -0
@@ -134,6 +134,7 @@pub mod uaccess;#[cfg( CONFIG_USB = "y" )] pub mod usb; +pub mod validate;pub mod workqueue;pub mod xarray;
rust/kernel/validate.rs new file +148 -0
148 lines not on this page · raw message
Signature · 1 line
2.54.0
[email protected]
AI reviewRe: [PATCH v2 02/21] rust: create basic untrusted data API
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] Invalid pointer cast between `Vec<T, A>` and `Vec<Untrusted<T>, A>` in `Deref` and `DerefMut`…
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Invalid pointer cast between Vec<T, A> and Vec<Untrusted<T>, A> in Deref and DerefMut implementations leads to undefined behavior, memory corruption, and Use-After-Free.
- [Low] Missing #[inline] annotations on small forwarding functions in an abstraction.
- [Low] Incorrect import formatting: use core::ops::{Deref, DerefMut}; does not follow the vertical import style.
--
commit 5d58092058655501597b482a4a94d2d50da29fc6
Author: Benno Lossin <[email protected]>
rust: create basic untrusted data API
This commit introduces the Untrusted<T> wrapper type to explicitly mark and validate untrusted external data. It prevents direct usage of untrusted data in logic decisions while allowing it to be copied back to userspace.
Message-ID: <[email protected]>
rust/kernel/validate.rs new file +0 -0
[ ... ]
Quoted 1 line
+use core::ops::{Deref, DerefMut};
[Severity: Low]
This isn't a bug, but the kernel Rust coding guidelines require vertical import formatting (one import per line). Should this be spread across multiple lines?
Quoted 2 lines
(quoted diff) +2
-0+ +use crate::{
[ ... ]
Quoted 1 line
+impl<T: ?Sized> Untrusted<T> {
[ ... ]
Quoted 1 line
+ pub fn new(value: T) -> Self
[Severity: Low]
This isn't a bug, but the kernel Rust coding guidelines require small or forwarding functions in abstractions to be annotated with #[inline]. Should we add the annotation here?
Review
Quoted 11 lines
(quoted diff) +11
-0+ where + T: Sized, + { + Self(value) + } +} + +impl<T> Deref for Untrusted<[T]> { + type Target = [Untrusted<T>]; + + fn deref(&self) -> &Self::Target {
[Severity: Low]
Similar to new(), should this small forwarding function be annotated with #[inline] to match the guidelines?
Review
Quoted 7 lines
(quoted diff) +7
-0+ // SAFETY: `Untrusted<T>` transparently wraps `T`. + unsafe { cast_slice(&self.0) } + } +} + +impl<T> DerefMut for Untrusted<[T]> { + fn deref_mut(&mut self) -> &mut Self::Target {
[Severity: Low]
Should this small forwarding function also be annotated with #[inline]?
Quoted 4 lines
(quoted diff) +4
-0+ // SAFETY: `Untrusted<T>` transparently wraps `T`. + unsafe { cast_slice_mut(&mut self.0) } + } +}
Review
Quoted 10 lines
(quoted diff) +10
-0+ +impl<T, A: Allocator> Deref for Untrusted<Vec<T, A>> { + type Target = Vec<Untrusted<T>, A>; + + fn deref(&self) -> &Self::Target { + let ptr: *const Untrusted<Vec<T, A>> = self; + // CAST: `Untrusted<T>` transparently wraps `T`. + let ptr: *const Vec<Untrusted<T>, A> = ptr.cast(); + // SAFETY: `ptr` is derived from the reference `self`. + unsafe { &*ptr }
[Severity: High]
Does this pointer cast invoke undefined behavior?
Since Vec has #[repr(Rust)], the compiler provides no guarantee that the field orderings of Vec<T> and Vec<Untrusted<T>> match. If the layout differs between these two monomorphizations, could casting the reference result in memory corruption when accessing the ptr, len, and layout fields?
Review
Quoted 10 lines
(quoted diff) +10
-0+ } +} + +impl<T, A: Allocator> DerefMut for Untrusted<Vec<T, A>> { + fn deref_mut(&mut self) -> &mut Self::Target { + let ptr: *mut Untrusted<Vec<T, A>> = self; + // CAST: `Untrusted<T>` transparently wraps `T`. + let ptr: *mut Vec<Untrusted<T>, A> = ptr.cast(); + // SAFETY: `ptr` is derived from the reference `self`. + unsafe { &mut *ptr }
[Severity: High]
Similar to deref(), does casting a mutable reference between different instantiations of a #[repr(Rust)] type risk memory corruption?
Safe code could call deref_mut to obtain &mut Vec<Untrusted<T>, A>, extract ownership via core::mem::take (since Vec implements Default), and then mutate or drop it. This could process the fields from mismatched offsets and pass garbage pointers to the allocator.
Quoted 2 lines
(quoted diff) +2
-0+ } +}
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
PATCH v2 3/21 rust: validate: add `Validate` trait
In reply to [email protected]From: Benno Lossin <[email protected]>
Introduce the Validate<Input> trait and functions to validate Untrusted<T> using said trait. This allows one to access the inner value of Untrusted<T> via validate{,_ref,_mut} functions which subsequently delegate the validation to user-implemented Validate trait.
The Validate trait is the only entry point for validation code, making it easy to spot where data is being validated.
The reason for restricting the types that can be inputs to Validate::validate is to be able to have the validate... functions on Untrusted. This is also the reason for the suggestions in the Usage in API Design section in the commit that introduced Untrusted<T>.
Message-ID: <[email protected]>
--- rust/kernel/validate.rs | 70 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 69 insertions(+), 1 deletion(-)
rust/kernel/validate.rs +69 -1
@@ -11,6 +11,9 @@//! APIs that write back into userspace usually allow writing untrusted bytes directly, allowing //! direct copying of untrusted user data back into userspace without validation. //! +//! The only way to access untrusted data is to [`Validate::validate`] it. This is facilitated by +//! the [`Validate`] trait. +//! //! # Rationale //! //! When reading data from an untrusted source, it must be validated before it can be used for
@@ -52,7 +55,7 @@/// untrusted, as it would otherwise violate normal Rust rules. For this reason, one can easily /// convert that reference to `&[Untrusted<u8>]`. Another such example is `Untrusted<KVec<T>>`, it /// derefs to `KVec<Untrusted<T>>`. Raw bytes however do not behave in this way, `Untrusted<u8>` is -/// totally opaque. +/// totally opaque and one can only access its value by calling [`Untrusted::validate()`]. /// /// # Usage in API Design ///
@@ -107,6 +110,30 @@ pub fn new(value: T) -> Self {Self (value) } + +/// Validate the underlying untrusted data. +/// +/// See the [`Validate`] trait for more information. +pub fn validate <V :Validate <Self >>(self) ->Result <V ,V ::Err > +where +T :Sized , + { +V ::validate (self.0 ) + } + +/// Validate the underlying untrusted data. +/// +/// See the [`Validate`] trait for more information. +pub fn validate_ref <'a,V :Validate <&'aSelf >>(&'a self) ->Result <V ,V ::Err > { +V ::validate (&self.0 ) + } + +/// Validate the underlying untrusted data. +/// +/// See the [`Validate`] trait for more information. +pub fn validate_mut <'a,V :Validate <&'amut Self >>(&'amut self) ->Result <V ,V ::Err > { +V ::validate (&mut self.0 ) + } }impl <T >Deref for Untrusted <[T ]> {
@@ -146,3 +173,44 @@ fn deref_mut(&mut self) -> &mut Self::Target {unsafe { &mut *ptr } } } + +/// Marks valid input for the [`Validate`] trait. +pub trait ValidateInput : private::Sealed { +/// Type of the inner data. +type Inner : ?Sized ; +} + +impl <T : ?Sized >ValidateInput for Untrusted <T > { +type Inner =T ; +} + +impl <'a,T : ?Sized >ValidateInput for &'aUntrusted <T > { +type Inner = &'aT ; +} + +impl <'a,T : ?Sized >ValidateInput for &'amut Untrusted <T > { +type Inner = &'amut T ; +} + +mod private { +use super ::Untrusted ; + +pub trait Sealed {} + +impl <T : ?Sized >Sealed for Untrusted <T > {} +impl <'a,T : ?Sized >Sealed for &'aUntrusted <T > {} +impl <'a,T : ?Sized >Sealed for &'amut Untrusted <T > {} +} + +/// Validate [`Untrusted`] data. +/// +/// Care must be taken when implementing this trait, as unprotected access to unvalidated data is +/// given to the [`Validate::validate`] function. The implementer must ensure that the data is only +/// used for logic after successful validation. +pub trait Validate <Input :ValidateInput >:Sized { +/// Validation error. +type Err ; + +/// Validate the raw input. +fn validate (raw:Input ::Inner ) ->Result <Self ,Self ::Err >; +}
Signature · 1 line
2.54.0
[email protected]
AI reviewRe: [PATCH v2 03/21] rust: validate: add `Validate` trait
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] The `Validate` trait's design makes it impossible to validate stack-allocated array buffers as a…
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The Validate trait's design makes it impossible to validate stack-allocated array buffers as a single contiguous chunk.
- [Low] Small forwarding functions in a core abstraction are missing the required #[inline] annotation.
--
commit ffc4f142fcfe45cce3f6459e2aef3c631240714d
Author: Benno Lossin <[email protected]>
rust: validate: add Validate trait
This commit introduces the Validate trait and functions to validate Untrusted data. It allows users to access the inner value of untrusted data via validation functions which delegate to a user-implemented trait. It also uses a sealed trait to restrict valid inputs.
Message-ID: <[email protected]>
Quoted 4 lines
rust/kernel/validate.rs +0
-0
[ ... ]
Quoted 4 lines
rust/kernel/validate.rs +0
-0@@ -107,6 +110,30 @@ pub fn new(value: T) -> Self {Self (value) }
rust/kernel/validate.rs pub fn new(value: T) -> Self · line 122
Quoted 10 lines
rust/kernel/validate.rs +10
-0+ +/// Validate the underlying untrusted data. +/// +/// See the [`Validate`] trait for more information. +pub fn validate <V :Validate <Self >>(self) ->Result <V ,V ::Err > +where +T :Sized , + { +V ::validate (self.0 ) + }
[Severity: Low]
This isn't a bug, but should this small forwarding function in a core abstraction be annotated with #[inline] per the Rust subsystem coding guidelines?
rust/kernel/validate.rs pub fn new(value: T) -> Self · line 129
Quoted 7 lines
rust/kernel/validate.rs +7
-0+ +/// Validate the underlying untrusted data. +/// +/// See the [`Validate`] trait for more information. +pub fn validate_ref <'a,V :Validate <&'aSelf >>(&'a self) ->Result <V ,V ::Err > { +V ::validate (&self.0 ) + }
[Severity: Low]
This isn't a bug, but should this small forwarding function be annotated with #[inline] as well?
rust/kernel/validate.rs pub fn new(value: T) -> Self · line 136
Quoted 7 lines
rust/kernel/validate.rs +7
-0+ +/// Validate the underlying untrusted data. +/// +/// See the [`Validate`] trait for more information. +pub fn validate_mut <'a,V :Validate <&'amut Self >>(&'amut self) ->Result <V ,V ::Err > { +V ::validate (&mut self.0 ) + }
[Severity: Low]
This isn't a bug, but should this small forwarding function also be annotated with #[inline]?
[ ... ]
Quoted 7 lines
rust/kernel/validate.rs +7
-0+/// Marks valid input for the [`Validate`] trait. +pub trait ValidateInput : private::Sealed { +/// Type of the inner data. +type Inner : ?Sized ; +} + +impl <T : ?Sized >ValidateInput for Untrusted <T > {
rust/kernel/validate.rs fn deref_mut(&mut self) -> &mut Self::Target { · line 193
Quoted 10 lines
rust/kernel/validate.rs +10
-0+type Inner =T ; +} + +impl <'a,T : ?Sized >ValidateInput for &'aUntrusted <T > { +type Inner = &'aT ; +} + +impl <'a,T : ?Sized >ValidateInput for &'amut Untrusted <T > { +type Inner = &'amut T ; +}
[Severity: Medium]
Does the current ValidateInput implementation prevent validating stack-allocated array buffers as a single contiguous chunk?
If a user populates a stack-allocated buffer like [Untrusted<u8>; N], they can use it as a slice &mut [Untrusted<u8>]. However, since ValidateInput is sealed and only implements Untrusted<T> wrappers (and not slices or arrays of wrappers), they cannot pass this slice to Validate::validate().
Since converting &mut [Untrusted<u8>] back to &mut Untrusted<[u8]> isn't possible outside the module, this seems to force users to either validate byte-by-byte or dynamically allocate.
Could ValidateInput be implemented for slices of Untrusted<T> to support validating stack-allocated buffers?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
PATCH v2 4/21 X.509: Make certificate parser public
In reply to [email protected]From: Lukas Wunner <[email protected]>
The upcoming support for PCI device authentication with CMA-SPDM (PCIe r6.1 sec 6.31) requires validating the Subject Alternative Name in X.509 certificates.
High-level functions for X.509 parsing such as key_create_or_update() throw away the internal, low-level struct x509_certificate after extracting the struct public_key and public_key_signature from it. The Subject Alternative Name is thus inaccessible when using those functions.
Afford CMA-SPDM access to the Subject Alternative Name by making struct x509_certificate public, together with the functions for parsing an X.509 certificate into such a struct and freeing such a struct.
No functional change intended.
--- crypto/asymmetric_keys/x509_parser.h | 42 +-------------------- include/keys/x509-parser.h | 55 ++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 41 deletions(-) create mode 100644 include/keys/x509-parser.h
crypto/asymmetric_keys/x509_parser.h +1 -41
@@ -5,51 +5,11 @@* Written by David Howells ([email protected]) */ -#include <linux/cleanup.h> -#include <linux/time.h> -#include <crypto/public_key.h> -#include <keys/asymmetric-type.h> -#include <crypto/sha2.h> - -struct x509_certificate { -struct x509_certificate *next; -struct x509_certificate *signer;/* Certificate that signed this one */ -struct public_key *pub;/* Public key details */ -struct public_key_signature *sig;/* Signature parameters */ -u8 sha256[SHA256_DIGEST_SIZE ];/* Hash for blacklist purposes */ -char *issuer;/* Name of certificate issuer */ -char *subject;/* Name of certificate subject */ -struct asymmetric_key_id *id;/* Issuer + Serial number */ -struct asymmetric_key_id *skid;/* Subject + subjectKeyId (optional) */ -time64_t valid_from; -time64_t valid_to; -const void *tbs;/* Signed data */ -unsigned tbs_size;/* Size of signed data */ -unsigned raw_sig_size;/* Size of signature */ -const void *raw_sig;/* Signature data */ -const void *raw_serial;/* Raw serial number in ASN.1 */ -unsigned raw_serial_size; -unsigned raw_issuer_size; -const void *raw_issuer;/* Raw issuer name in ASN.1 */ -const void *raw_subject;/* Raw subject name in ASN.1 */ -unsigned raw_subject_size; -unsigned raw_skid_size; -const void *raw_skid;/* Raw subjectKeyId in ASN.1 */ -unsigned index; -bool seen;/* Infinite recursion prevention */ -bool verified; -bool self_signed;/* T if self-signed (check unsupported_sig too) */ -bool unsupported_sig;/* T if signature uses unsupported crypto */ -bool blacklisted; -}; +#include <keys/x509-parser.h> /* * x509_cert_parser.c */ -extern void x509_free_certificate (struct x509_certificate *cert); -DEFINE_FREE(x509_free_certificate, struct x509_certificate *, - if (!IS_ERR(_T)) x509_free_certificate(_T)) -extern struct x509_certificate *x509_cert_parse (const void *data,size_t datalen);extern int x509_decode_time (time64_t *_t,size_t hdrlen,unsigned char tag,const unsigned char *value,size_t vlen);
include/keys/x509-parser.h new file +55 -0
55 lines not on this page · raw message
Signature · 1 line
2.54.0
[email protected]
AI reviewRe: [PATCH v2 04/21] X.509: Make certificate parser public
Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 12 lines
From: Lukas Wunner <[email protected]>
The upcoming support for PCI device authentication with CMA-SPDM (PCIe r6.1 sec 6.31) requires validating the Subject Alternative Name in X.509 certificates.
High-level functions for X.509 parsing such as
key_create_or_update()throw away the internal, low-level structx509_certificateafter extracting the structpublic_keyandpublic_key_signaturefrom it. The Subject Alternative Name is thus inaccessible when using those functions.
Quoted 7 lines
Afford CMA-SPDM access to the Subject Alternative Name by making struct
x509_certificatepublic, together with the functions for parsing an X.509 certificate into such a struct and freeing such a struct.No functional change intended.
Signed-off-by Lukas Wunner <[email protected]>
Sashiko has reviewed this patch and found no issues. It looks great!
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4
PATCH v2 5/21 X.509: Parse Subject Alternative Name in certificates
In reply to [email protected]From: Lukas Wunner <[email protected]>
The upcoming support for PCI device authentication with CMA-SPDM (PCIe r6.1 sec 6.31) requires validating the Subject Alternative Name in X.509 certificates.
Store a pointer to the Subject Alternative Name upon parsing for consumption by CMA-SPDM.
--- crypto/asymmetric_keys/x509_cert_parser.c | 9 +++++++++ include/keys/x509-parser.h | 2 ++ 2 files changed, 11 insertions(+)
crypto/asymmetric_keys/x509_cert_parser.c +9 -0
@@ -596,6 +596,15 @@ int x509_process_extension(void *context, size_t hdrlen,return 0 ; } +if (ctx->last_oid == OID_subjectAltName) { +if (ctx->cert->raw_san) +return -EBADMSG ; + + ctx->cert->raw_san = v; + ctx->cert->raw_san_size = vlen; +return 0 ; + } +if (ctx->last_oid == OID_keyUsage) {/* * Get hold of the keyUsage bit string
include/keys/x509-parser.h +2 -0
@@ -38,6 +38,8 @@ struct x509_certificate {unsigned raw_subject_size;unsigned raw_skid_size;const void *raw_skid;/* Raw subjectKeyId in ASN.1 */ +const void *raw_san;/* Raw subjectAltName in ASN.1 */ +unsigned raw_san_size;unsigned index;bool seen;/* Infinite recursion prevention */ bool verified;
Signature · 1 line
2.54.0
[email protected]
AI reviewRe: [PATCH v2 05/21] X.509: Parse Subject Alternative Name in certificates
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] The X.509 parser completely ignores the critical flag on certificate extensions, allowing certificates with unrecognized critical constraints to be successfully validated. --
commit b12732c2d7a7dc077f0c1d3f0be7c0a415bae376
Author: Lukas Wunner <[email protected]>
X.509: Parse Subject Alternative Name in certificates
This commit adds support for parsing the Subject Alternative Name (SAN) extension in X.509 certificates. It stores a pointer to the SAN for later consumption by CMA-SPDM for PCI device authentication.
Quoted 8 lines
crypto/asymmetric_keys/x509_cert_parser.c +0
-0@@ -596,6 +596,15 @@ int x509_process_extension(void *context, size_t hdrlen,return 0 ; }
crypto/asymmetric_keys/x509_cert_parser.c int x509_process_extension(void *context, size_t hdrlen, · line 608
Quoted 10 lines
crypto/asymmetric_keys/x509_cert_parser.c +9
-0+if (ctx->last_oid == OID_subjectAltName) { +if (ctx->cert->raw_san) +return -EBADMSG ; + + ctx->cert->raw_san = v; + ctx->cert->raw_san_size = vlen; +return 0 ; + } +if (ctx->last_oid == OID_keyUsage) {
[Severity: High]
This is a pre-existing issue, but while looking at how we process X.509 extensions here, does the parser ignore the critical flag on extensions?
Looking at the ASN.1 definition in crypto/asymmetric_keys/x509.asn1:
Extension ::= SEQUENCE {
extnId OBJECT IDENTIFIER ({ x509_note_OID }),
critical BOOLEAN DEFAULT,
extnValue OCTET STRING ({ x509_process_extension })}
Because there is no action attached to the critical BOOLEAN field, its value seems to be discarded during parsing.
At the end of x509_process_extension(), unrecognized extensions are unconditionally ignored:
crypto/asymmetric_keys/x509_cert_parser.c:x509_process_extension() {
...
if (ctx->last_oid == OID_basicConstraints) {
...
return 0;
}
return 0;
}Since X.509 certificates are supplied by potentially untrusted sources (like during SPDM authentication), could an attacker bypass required certificate constraints by presenting unrecognized extensions marked with the critical flag that get silently ignored and treated as a success (0)?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5
PATCH v2 6/21 X.509: Move certificate length retrieval into new helper
In reply to [email protected]From: Lukas Wunner <[email protected]>
The upcoming in-kernel SPDM library (Security Protocol and Data Model,
https://www.dmtf.org/dsp/DSP0274) needs to retrieve the length from ASN.1 DER-encoded X.509 certificates.
Such code already exists in x509_load_certificate_list(), so move it into a new helper for reuse by SPDM.
Export the helper so that SPDM can be tristate. (Some upcoming users of the SPDM libray may be modular, such as SCSI and ATA.)
No functional change intended.
--- crypto/asymmetric_keys/x509_loader.c | 38 +++++++++++++++++++--------- include/keys/asymmetric-type.h | 2 ++ 2 files changed, 28 insertions(+), 12 deletions(-)
crypto/asymmetric_keys/x509_loader.c +26 -12
@@ -4,28 +4,42 @@#include <linux/key.h> #include <keys/asymmetric-type.h> +ssize_t x509_get_certificate_length (const u8 *p,unsigned long buflen) +{ +ssize_t plen; + +/* Each cert begins with an ASN.1 SEQUENCE tag and must be more +* than 256 bytes in size. +*/ +if (buflen <4 ) +return -EINVAL ; + +if (p[0 ] !=0x30 || + p[1 ] !=0x82 ) +return -EINVAL ; + + plen = (p[2 ] <<8 ) | p[3 ]; + plen +=4 ; +if (plen > buflen) +return -EINVAL ; + +return plen; +} +EXPORT_SYMBOL_GPL (x509_get_certificate_length); +int x509_load_certificate_list (const u8 cert_list[],const unsigned long list_size,const struct key *keyring) {key_ref_t key;const u8 *p, *end; -size_t plen; +ssize_t plen; p = cert_list; end = p + list_size;while (p < end) { -/* Each cert begins with an ASN.1 SEQUENCE tag and must be more -* than 256 bytes in size. -*/ -if (end - p <4 ) - goto dodgy_cert; -if (p[0 ] !=0x30 || - p[1 ] !=0x82 ) - goto dodgy_cert; - plen = (p[2 ] <<8 ) | p[3 ]; - plen +=4 ; -if (plen > end - p) + plen =x509_get_certificate_length (p, end - p); +if (plen <0 ) goto dodgy_cert; key = key_create_or_update(make_key_ref(keyring, 1),
include/keys/asymmetric-type.h +2 -0
@@ -84,6 +84,8 @@ extern struct key *find_asymmetric_key(struct key *keyring,const struct asymmetric_key_id *id_2, bool partial); +ssize_t x509_get_certificate_length (const u8 *p,unsigned long buflen); +int x509_load_certificate_list (const u8 cert_list[],const unsigned long list_size,const struct key *keyring);
Signature · 1 line
2.54.0
[email protected]
AI reviewRe: [PATCH v2 06/21] X.509: Move certificate length retrieval into new helper
Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 9 lines
From: Lukas Wunner <[email protected]>
The upcoming in-kernel SPDM library (Security Protocol and Data Model,
https://www.dmtf.org/dsp/DSP0274) needs to retrieve the length from ASN.1 DER-encoded X.509 certificates.Such code already exists in
x509_load_certificate_list(), so move it into a new helper for reuse by SPDM.
Quoted 6 lines
Export the helper so that SPDM can be tristate. (Some upcoming users of the SPDM libray may be modular, such as SCSI and ATA.)
No functional change intended.
Signed-off-by Lukas Wunner <[email protected]>
Sashiko has reviewed this patch and found no issues. It looks great!
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6
PATCH v2 7/21 rust: add bindings for hash.h
In reply to [email protected]From: Alistair Francis <[email protected]>
Make the functions crypto_shash_descsize(), crypto_shash_digestsize() and crypto_free_shash() available to Rust.
--- rust/bindings/bindings_helper.h | 1 + rust/helpers/hash.c | 18 ++++++++++++++++++ rust/helpers/helpers.c | 1 + 3 files changed, 20 insertions(+) create mode 100644 rust/helpers/hash.c
rust/bindings/bindings_helper.h +1 -0
@@ -30,6 +30,7 @@#include <linux/acpi.h> #include <linux/gpu_buddy.h> +#include <crypto/hash.h> #include <drm/drm_device.h> #include <drm/drm_drv.h> #include <drm/drm_file.h>
rust/helpers/hash.c new file +18 -0
@@ -0,0 +1,18 @@ +// SPDX-License-Identifier: GPL-2.0 + +#include <crypto/hash.h> + +__rust_helper unsigned int rust_helper_crypto_shash_descsize(struct crypto_shash *tfm) +{ +return crypto_shash_descsize (tfm); +} + +__rust_helper unsigned int rust_helper_crypto_shash_digestsize(struct crypto_shash *tfm) +{ +return crypto_shash_digestsize (tfm); +} + +__rust_helper void rust_helper_crypto_free_shash(struct crypto_shash *tfm) +{ +crypto_free_shash (tfm); +}
rust/helpers/helpers.c +1 -0
@@ -62,6 +62,7 @@#include "drm.c" #include "drm_gpuvm.c" #include "err.c" +#include "hash.c" #include "irq.c" #include "fs.c" #include "gpu.c"
Signature · 1 line
2.54.0
[email protected]
AI reviewRe: [PATCH v2 07/21] rust: add bindings for hash.h
Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 6 lines
From: Alistair Francis <[email protected]>
Make the functions
crypto_shash_descsize(),crypto_shash_digestsize()andcrypto_free_shash()available to Rust.Signed-off-by Alistair Francis <[email protected]>
Sashiko has reviewed this patch and found no issues. It looks great!
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7
PATCH v2 8/21 rust: error: impl From<FromBytesWithNulError> for Kernel Error
In reply to [email protected]From: Alistair Francis <[email protected]>
Implement From<FromBytesWithNulError> for the Kernel Error type as we will use it in a future patch.
As this is now generally available we can remove it from the test case as well.
--- rust/kernel/error.rs | 18 +++++++++++++++--- rust/kernel/str.rs | 7 ------- 2 files changed, 15 insertions(+), 10 deletions(-)
rust/kernel/error.rs +15 -3
@@ -12,9 +12,14 @@ str::CStr, }; -use core::num::NonZeroI32 ; -use core::num::TryFromIntError ; -use core::str::Utf8Error ; +use core::{ + ffi::FromBytesWithNulError , + num::{ +NonZeroI32 , +TryFromIntError ,// + }, + str::Utf8Error ,// +};/// Contains the C-compatible error codes. #[rustfmt::skip]
@@ -256,6 +261,13 @@ fn from(e: core::convert::Infallible) -> Error { } } +impl From <FromBytesWithNulError >for Error { +#[inline] +fn from (_:FromBytesWithNulError ) ->Error { + code::EINVAL + } +} +/// A [`Result`] with an [`Error`] error type. /// /// To be used as the return type for functions that may fail.
rust/kernel/str.rs +0 -7
@@ -433,13 +433,6 @@ macro_rules! c_str {mod tests {use super ::*; -impl From <core::ffi::FromBytesWithNulError >for Error { -#[inline] -fn from (_: core::ffi::FromBytesWithNulError ) ->Error { -EINVAL - } - } -macro_rules! format { ($($f:tt)*) => ({CString ::try_from_fmt (fmt! ($($f)*))?.to_str ()?
Signature · 1 line
2.54.0
[email protected]
AI reviewRe: [PATCH v2 08/21] rust: error: impl From<FromBytesWithNulError> for Kernel Error
Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 9 lines
From: Alistair Francis <[email protected]>
Implement From<FromBytesWithNulError> for the Kernel Error type as we will use it in a future patch.
As this is now generally available we can remove it from the test case as well.
Signed-off-by Alistair Francis <[email protected]>
Sashiko has reviewed this patch and found no issues. It looks great!
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8
PATCH v2 9/21 lib: rspdm: Initial commit of Rust SPDM
In reply to [email protected]From: Alistair Francis <[email protected]>
This is the initial commit of the Rust SPDM library.
--- MAINTAINERS | 12 ++ include/linux/spdm.h | 37 +++++ lib/Kconfig | 14 ++ lib/Makefile | 2 + lib/rspdm/Makefile | 10 ++ lib/rspdm/consts.rs | 92 +++++++++++++ lib/rspdm/lib.rs | 89 ++++++++++++ lib/rspdm/state.rs | 237 ++++++++++++++++++++++++++++++++ lib/rspdm/validator.rs | 92 +++++++++++++ rust/bindings/bindings_helper.h | 1 + rust/kernel/error.rs | 3 + 11 files changed, 589 insertions(+) create mode 100644 include/linux/spdm.h create mode 100644 lib/rspdm/Makefile create mode 100644 lib/rspdm/consts.rs create mode 100644 lib/rspdm/lib.rs create mode 100644 lib/rspdm/state.rs create mode 100644 lib/rspdm/validator.rs
MAINTAINERS +12 -0
@@ -24398,6 +24398,18 @@ M: Security Officers <[email protected]> S: Supported F: Documentation/process/security-bugs.rst +SECURITY PROTOCOL AND DATA MODEL (SPDM) +M: Jonathan Cameron <[email protected]> +M: Lukas Wunner <[email protected]> +M: Alistair Francis <[email protected]> +L: [email protected] +L: [email protected] +L: [email protected] +S: Maintained +T: git git://git.kernel.org/pub/scm/linux/kernel/git/devsec/spdm.git +F: include/linux/spdm.h +F: lib/rspdm/ + SECURITY SUBSYSTEM M: Paul Moore <[email protected]> M: James Morris <[email protected]>
include/linux/spdm.h new file +37 -0
@@ -0,0 +1,37 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* +* DMTF Security Protocol and Data Model (SPDM) +* https://www.dmtf.org/dsp/DSP0274 +* +* Copyright (C) 2021-22 Huawei +* Jonathan Cameron <[email protected]> +* +* Copyright (C) 2022-24 Intel Corporation +*/ + +#ifndef _SPDM_H_ +#define _SPDM_H_ + +#include <linux/types.h> + +struct key ; +struct device ; +struct spdm_state ; +struct x509_certificate ; + +typedef int (spdm_transport )(void *priv,struct device *dev, +const void *request,size_t request_sz, +void *response,size_t response_sz); + +typedef int (spdm_validate )(struct device *dev,u8 slot, +struct x509_certificate *leaf_cert); + +struct spdm_state *spdm_create (struct device *dev,spdm_transport *transport, +void *transport_priv,u32 transport_sz, +spdm_validate *validate); + +int spdm_authenticate (struct spdm_state *spdm_state); + +void spdm_destroy (struct spdm_state *spdm_state); + +#endif
lib/Kconfig +14 -0
@@ -594,6 +594,20 @@ config LWQ_TEST help Run boot-time test of light-weight queuing. +config RSPDM + bool "Rust SPDM" + depends on RUST + select CRYPTO + select X509_CERTIFICATE_PARSER + help + The Rust implementation of the Security Protocol and Data Model (SPDM) + allows for device authentication, measurement, key exchange and + encrypted sessions. + + Crypto algorithms negotiated with SPDM are limited to those enabled + in .config. Users of SPDM therefore need to also select + any algorithms they deem mandatory. + endmenu config GENERIC_IOREMAP
lib/Makefile +2 -0
@@ -281,6 +281,8 @@ obj-$(CONFIG_PERCPU_TEST) += percpu_test.o obj-$(CONFIG_ASN1) += asn1_decoder.o obj-$(CONFIG_ASN1_ENCODER) += asn1_encoder.o +obj-$(CONFIG_RSPDM) += rspdm/ + obj-$(CONFIG_FONT_SUPPORT) += fonts/ #
lib/rspdm/Makefile new file +10 -0
@@ -0,0 +1,10 @@ +# SPDX-License-Identifier: GPL-2.0 +# +# Rust implementation of the DMTF Security Protocol and Data Model (SPDM) +# https://www.dmtf.org/dsp/DSP0274 +# +# Copyright (C) 2024 Western Digital + +obj-$(CONFIG_RSPDM) += spdm.o + +spdm-y := lib.o
lib/rspdm/consts.rs new file +92 -0
92 lines not on this page · raw message
lib/rspdm/lib.rs new file +89 -0
89 lines not on this page · raw message
lib/rspdm/state.rs new file +237 -0
237 lines not on this page · raw message
lib/rspdm/validator.rs new file +92 -0
92 lines not on this page · raw message
rust/bindings/bindings_helper.h +1 -0
@@ -87,6 +87,7 @@#include <linux/sched.h> #include <linux/security.h> #include <linux/slab.h> +#include <linux/spdm.h> #include <linux/sys_soc.h> #include <linux/task_work.h> #include <linux/tracepoint.h>
rust/kernel/error.rs +3 -0
@@ -91,6 +91,9 @@ macro_rules! declare_err {declare_err! (EIOCBQUEUED ,"iocb queued, will get completion event." );declare_err! (ERECALLCONFLICT ,"Conflict with recalled state." );declare_err! (ENOGRACE ,"NFS file lock reclaim refused." ); +declare_err! (ECONNRESET ,"Connection reset by peer." ); +declare_err! (EINPROGRESS ,"Operation now in progress." ); +declare_err! (EPROTO ,"Protocol error" ); }/// Generic integer kernel error.
Signature · 1 line
2.54.0
[email protected]
AI reviewRe: [PATCH v2 09/21] lib: rspdm: Initial commit of Rust SPDM
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Low] The Kconfig symbol `RSPDM` selects `X509_CERTIFICATE_PARSER` but does not inherit its required…
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The Kconfig symbol RSPDM selects X509_CERTIFICATE_PARSER but does not inherit its required dependencies, causing an unmet direct dependencies warning. --
commit e58dc22b70e82677b92465a9ddd6e228870b413f
Author: Alistair Francis <[email protected]>
lib: rspdm: Initial commit of Rust SPDM
This commit introduces the initial implementation of the Rust SPDM library.
Quoted 4 lines
lib/Kconfig +0
-0
[ ... ]
lib/Kconfig config LWQ_TEST · line 601
Quoted 9 lines
lib/Kconfig +5
-0@@ -594,6 +594,20 @@ config LWQ_TEST help Run boot-time test of light-weight queuing. +config RSPDM + bool "Rust SPDM" + depends on RUST + select CRYPTO + select X509_CERTIFICATE_PARSER
[Severity: Low]
Does this selection need to inherit the dependencies of X509_CERTIFICATE_PARSER?
Selecting a symbol without inheriting its required dependencies can cause an unmet direct dependencies warning. For example, if a user configures the kernel with CONFIG_RSPDM=y on a configuration where CONFIG_ASYMMETRIC_PUBLIC_KEY_SUBTYPE is not enabled, it could lead to build warnings or failures.
Quoted 4 lines
lib/Kconfig +4
-0+ help + The Rust implementation of the Security Protocol and Data Model (SPDM) + allows for device authentication, measurement, key exchange and + encrypted sessions.
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=9
Re: [PATCH v2 09/21] lib: rspdm: Initial commit of Rust SPDM
In reply to [email protected]Hi Alistair,
lib/Kconfig config LWQ_TEST · line 609
Quoted 13 lines · On 6/23/26 05:53, [email protected] wrote:
lib/Kconfig +13
-0+config RSPDM + bool "Rust SPDM" + depends on RUST + select CRYPTO + select X509_CERTIFICATE_PARSER + help + The Rust implementation of the Security Protocol and Data Model (SPDM) + allows for device authentication, measurement, key exchange and + encrypted sessions. + + Crypto algorithms negotiated with SPDM are limited to those enabled + in .config. Users of SPDM therefore need to also select + any algorithms they deem mandatory.
This patch introduces an unmet dependency bug when the user enables RSPDM without enabling ASYMMETRIC_PUBLIC_KEY_SUBTYPE:
WARNING: unmet direct dependencies detected for X509_CERTIFICATE_PARSER
Depends on [n]: CRYPTO [=y] && ASYMMETRIC_KEY_TYPE [=n] &&ASYMMETRIC_PUBLIC_KEY_SUBTYPE [=n]
Selected by [y]:
- RSPDM [=y] && RUST [=y]
- Julian Braha
PATCH v2 10/21 PCI/TSM: Rename pf0 to host
In reply to [email protected]From: Alistair Francis <[email protected]>
Rename pci_tsm_pf0 to pci_tsm_host (and rename variables and function names from pf0 to host) as part of converting pci_tsm_host to be any device that knows how to speak any of CMA, IDE, or TDISP.
This commit just renames the functions and provides no functional change. That will happen in the next commit.
--- drivers/crypto/ccp/sev-dev-tio.h | 4 +- drivers/crypto/ccp/sev-dev-tsm.c | 12 ++--- drivers/pci/tsm.c | 84 ++++++++++++++++---------------- include/linux/pci-tsm.h | 18 ++++--- 4 files changed, 61 insertions(+), 57 deletions(-)
drivers/crypto/ccp/sev-dev-tio.h +2 -2
@@ -53,9 +53,9 @@ struct tsm_dsm_tio {struct pci_ide *ide[TIO_IDE_MAX_TC ]; }; -/* Describes TSM structure for PF0 pointed by pci_dev->tsm */ +/* Describes TSM structure for the link host pointed by pci_dev->tsm */ struct tio_dsm { -struct pci_tsm_pf0 tsm; +struct pci_tsm_host tsm;struct tsm_dsm_tio data;struct sev_device *sev; };
drivers/crypto/ccp/sev-dev-tsm.c +6 -6
@@ -205,7 +205,7 @@ static int stream_alloc(struct pci_dev *pdev, struct pci_ide **ide,return 0 ; } -static struct pci_tsm *tio_pf0_probe (struct pci_dev *pdev,struct sev_device *sev) +static struct pci_tsm *tio_host_probe (struct pci_dev *pdev,struct sev_device *sev) { struct tio_dsm *dsm __free(kfree) = kzalloc_obj(*dsm);int rc;
@@ -213,7 +213,7 @@ static struct pci_tsm *tio_pf0_probe(struct pci_dev *pdev, struct sev_device *seif (!dsm)return NULL ; - rc =pci_tsm_pf0_constructor (pdev, &dsm->tsm, sev->tsmdev); + rc =pci_tsm_host_constructor (pdev, &dsm->tsm, sev->tsmdev);if (rc)return NULL ;
@@ -226,8 +226,8 @@ static struct pci_tsm *dsm_probe(struct tsm_dev *tsmdev, struct pci_dev *pdev) {struct sev_device *sev =tsm_dev_to_sev (tsmdev); -if (is_pci_tsm_pf0 (pdev)) -return tio_pf0_probe (pdev, sev); +if (is_pci_tsm_host (pdev)) +return tio_host_probe (pdev, sev);return NULL ; }
@@ -237,10 +237,10 @@ static void dsm_remove(struct pci_tsm *tsm)pci_dbg (pdev,"TSM disabled\n" ); -if (is_pci_tsm_pf0 (pdev)) { +if (is_pci_tsm_host (pdev)) { struct tio_dsm *dsm = container_of(tsm, struct tio_dsm, tsm.base_tsm); -pci_tsm_pf0_destructor (&dsm->tsm); +pci_tsm_host_destructor (&dsm->tsm);kfree (dsm); } }
drivers/pci/tsm.c +42 -42
250 lines not on this page · raw message
include/linux/pci-tsm.h +11 -7
@@ -40,7 +40,7 @@ struct pci_tsm_ops {* pci_tsm_rwsem held for write to sync with TSM unregistration and * mutual exclusion of @connect and @disconnect. @connect and * @disconnect additionally run under the DSM lock (struct -* pci_tsm_pf0::lock) as well as @probe and @remove of the subfunctions. +* pci_tsm_host::lock) as well as @probe and @remove of the subfunctions. * @bind, @unbind, and @guest_req run under pci_tsm_rwsem held for read * and the DSM lock. */
@@ -115,19 +115,23 @@ struct pci_tsm { };/** -* struct pci_tsm_pf0 - Physical Function 0 TDISP link context +* struct pci_tsm_host - TSM host link context (CMA, IDE, or TDISP) * @base_tsm: generic core "tsm" context * @lock: mutual exclustion for pci_tsm_ops invocation * @doe_mb: PCIe Data Object Exchange mailbox +* +* The host of a TSM link is the device that knows how to speak +* CMA, IDE, or TDISP. For TDISP / IDE that is a Physical Function 0. +* For CMA-only it is a CMA DOE mailbox. */ -struct pci_tsm_pf0 { +struct pci_tsm_host {struct pci_tsm base_tsm;struct mutex lock;struct pci_doe_mb *doe_mb; };/* physical function0 and capable of 'connect' */ -static inline bool is_pci_tsm_pf0 (struct pci_dev *pdev) +static inline bool is_pci_tsm_host (struct pci_dev *pdev) {if (!pdev)return false;
@@ -204,9 +208,9 @@ int pci_tsm_register(struct tsm_dev *tsm_dev);void pci_tsm_unregister (struct tsm_dev *tsm_dev);int pci_tsm_link_constructor (struct pci_dev *pdev,struct pci_tsm *tsm,struct tsm_dev *tsm_dev); -int pci_tsm_pf0_constructor (struct pci_dev *pdev,struct pci_tsm_pf0 *tsm, -struct tsm_dev *tsm_dev); -void pci_tsm_pf0_destructor (struct pci_tsm_pf0 *tsm); +int pci_tsm_host_constructor (struct pci_dev *pdev,struct pci_tsm_host *tsm, +struct tsm_dev *tsm_dev); +void pci_tsm_host_destructor (struct pci_tsm_host *tsm);int pci_tsm_doe_transfer (struct pci_dev *pdev,u8 type,const void *req,size_t req_sz,void *resp,size_t resp_sz);int pci_tsm_bind (struct pci_dev *pdev,struct kvm *kvm,u32 tdi_id);
Signature · 1 line
2.54.0
[email protected]
AI reviewRe: [PATCH v2 10/21] PCI/TSM: Rename pf0 to host
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Use-After-Free and freed mutex unlock in `__pci_tsm_disconnect()` for hosts on PF > 0. - [High]…
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Use-After-Free and freed mutex unlock in __pci_tsm_disconnect() for hosts on PF > 0.
- [High] Disconnecting any TSM host blindly destroys independent TSM contexts of sibling functions in the same PCI slot. --
commit 8bf82a17d0ecf2259067413b6fb0b2c2e2bef4d9
Author: Alistair Francis <[email protected]>
PCI/TSM: Rename pf0 to host
Rename pci_tsm_pf0 to pci_tsm_host (and rename variables and function names from pf0 to host) as part of converting pci_tsm_host to be any device that knows how to speak any of CMA, IDE, or TDISP.
Quoted 3 lines
drivers/pci/tsm.c +0
-0
[ ... ]
drivers/pci/tsm.c static void __pci_tsm_disconnect(struct pci_dev *pdev) · line 446
Quoted 8 lines
drivers/pci/tsm.c +1
-1@@ -441,19 +441,19 @@ static void __pci_tsm_disconnect(struct pci_dev *pdev)/* * disconnect() is uninterruptible as it may be called for device * teardown */ -guard (mutex)(&tsm_pf0->lock); +guard (mutex)(&tsm_host->lock);pci_tsm_walk_fns_reverse (pdev, remove_fn,NULL );
[Severity: High]
Can this lead to a use-after-free and memory corruption when a TSM host is on a Physical Function > 0?
When disconnecting a host on PF > 0 (such as a CMA host on PF 1), pci_tsm_walk_fns_reverse() iterates over the slot. The walker skips the callback only for i == 0. When i > 0, it applies the teardown to the function itself:
drivers/pci/tsm.c:pci_tsm_walk_fns_reverse() {
...
/* on exit, caller will run @cb on function 0 */
if (i > 0)
cb(pf, data);
}This causes remove_fn() to be called on PF 1 itself, which frees the tsm_host object via pci_tsm_host_destructor() and kfree(). When __pci_tsm_disconnect() returns, the guard attempts to unlock the destroyed and freed mutex.
[Severity: High]
Does this unintentionally destroy the TSM contexts of independent sibling functions?
The call to pci_tsm_walk_fns_reverse() unconditionally applies remove_fn() to all functions in the slot. If multiple independent TSM hosts exist within the same PCI slot (for example, PF 0 as a TDISP host and PF 1 as a CMA host), disconnecting one host will cause the walker to execute remove_fn() on the other siblings.
drivers/pci/tsm.c:remove_fn() {
tsm_remove(pdev->tsm);
link_sysfs_disable(pdev);
return 0;
}Since remove_fn() does not verify if the function's TSM context actually belongs to the specific host being disconnected (the data argument is NULL), could disconnecting PF 1 silently wipe out the entirely separate host context of PF 0?
Quoted 2 lines
drivers/pci/tsm.c +0
-0ops->disconnect (pdev); }
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=10
PATCH v2 11/21 PCI/TSM: Support connecting to PCIe CMA devices
In reply to [email protected]From: Alistair Francis <[email protected]>
In the next patch we are going to add a PCIe CMA TSM driver, as such we need to ensure that is_pci_tsm_host() will allow us to connect to CMA capable devices. These devices don't necessarily has DEVCAP_TEE or IDE support.
--- include/linux/pci-tsm.h | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
include/linux/pci-tsm.h +10 -1
@@ -3,6 +3,7 @@#define __PCI_TSM_H#include <linux/mutex.h> #include <linux/pci.h> +#include <linux/pci-doe.h> #include <linux/sockptr.h> struct pci_tsm ;
@@ -130,7 +131,7 @@ struct pci_tsm_host {struct pci_doe_mb *doe_mb; }; -/* physical function0 and capable of 'connect' */ +/* device is a TSM host and capable of 'connect' */ static inline bool is_pci_tsm_host (struct pci_dev *pdev) {if (!pdev)
@@ -142,6 +143,14 @@ static inline bool is_pci_tsm_host(struct pci_dev *pdev)if (pdev->is_virtfn)return false; +/* +* Report capable if CMA is supported, which can be supported on any PCIe +* device. +*/ +if (pci_find_doe_mailbox (pdev,PCI_VENDOR_ID_PCI_SIG , +PCI_DOE_FEATURE_CMA )) +return true; +/* * Allow for a Device Security Manager (DSM) associated with function0 * of an Endpoint to coordinate TDISP requests for other functions
Signature · 1 line
2.54.0
[email protected]
AI reviewRe: [PATCH v2 11/21] PCI/TSM: Support connecting to PCIe CMA devices
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Bypassing the function 0 check for CMA-capable devices breaks the subsystem's architectural…
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Bypassing the function 0 check for CMA-capable devices breaks the subsystem's architectural assumptions and physical function iteration logic, leading to UAF when unlocking a freed mutex on disconnect.
- [High] Calling pci_find_doe_mailbox() inside is_pci_tsm_host() introduces a Use-After-Free vulnerability when iterating over PCI devices, as DOE mailboxes can be concurrently freed by device removal. --
commit 4f85c4326e0a051322285638e0233e1946a251be
Author: Alistair Francis <[email protected]>
PCI/TSM: Support connecting to PCIe CMA devices
In the next patch we are going to add a PCIe CMA TSM driver, as such we need to ensure that is_pci_tsm_host() will allow us to connect to CMA capable devices. These devices don't necessarily has DEVCAP_TEE or IDE support.
Quoted 3 lines
include/linux/pci-tsm.h +0
-0
[ ... ]
include/linux/pci-tsm.h static inline bool is_pci_tsm_host(struct pci_dev *pdev) · line 152
Quoted 11 lines
include/linux/pci-tsm.h +7
-0@@ -142,6 +143,14 @@ static inline bool is_pci_tsm_host(struct pci_dev *pdev)if (pdev->is_virtfn)return false; +/* +* Report capable if CMA is supported, which can be supported on any PCIe +* device. +*/ +if (pci_find_doe_mailbox (pdev,PCI_VENDOR_ID_PCI_SIG , +PCI_DOE_FEATURE_CMA )) +return true;
[Severity: High]
Does returning true for devices where the function is not 0 cause a use after free during device disconnect?
If a CMA-capable device connects where its function number is greater than 0, when the device is disconnected, __pci_tsm_disconnect() invokes pci_tsm_walk_fns_reverse().
The reverse walk hardcodes the assumption that the host is function 0:
drivers/pci/tsm.c:pci_tsm_walk_fns_reverse() {
...
/* on exit, caller will run @cb on function 0 */
if (i > 0)
cb(pf, data);
...
}Because the host is a non-zero function, the walk mistakenly runs remove_fn on the host itself, freeing the memory containing tsm_host.
drivers/pci/tsm.c:__pci_tsm_disconnect() {
guard(mutex)(&tsm_host->lock);
pci_tsm_walk_fns_reverse(pdev, remove_fn, NULL);
ops->disconnect(pdev);
}When __pci_tsm_disconnect() returns, does dropping the guard unlock a mutex in the already freed memory?
[Severity: High]
Is it safe to call pci_find_doe_mailbox() here while iterating over PCI devices without holding the device lock?
The TSM subsystem iterates over all PCI devices using for_each_pci_dev(), repeatedly invoking is_pci_tsm_host(). While this holds a kref on the device, pci_find_doe_mailbox() performs a lockless iteration over the xarray without an RCU read lock:
drivers/pci/doe.c:pci_find_doe_mailbox() {
...
xa_for_each(&pdev->doe_mbs, index, doe_mb)
if (pci_doe_supports_feat(doe_mb, vendor, type))
return doe_mb;
...
}During concurrent device hot-removal, pci_destroy_dev() unconditionally destroys mailboxes directly, without waiting for an RCU grace period:
drivers/pci/doe.c:pci_doe_destroy_mb() {
...
kfree(doe_mb);
...
}Can this lead to a use after free where pci_find_doe_mailbox() dereferences mailboxes that have already been freed by a concurrent removal thread?
Quoted 4 lines
include/linux/pci-tsm.h +1
-0+/* * Allow for a Device Security Manager (DSM) associated with function0 * of an Endpoint to coordinate TDISP requests for other functions
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=11
PATCH v2 12/21 PCI/CMA: Add a PCI TSM CMA driver using SPDM
In reply to [email protected]From: Alistair Francis <[email protected]>
Component Measurement and Authentication (CMA, PCIe r6.2 sec 6.31) allows for measurement and authentication of PCIe devices. It is based on the Security Protocol and Data Model specification (SPDM,
https://www.dmtf.org/dsp/DSP0274).
CMA-SPDM in turn forms the basis for Integrity and Data Encryption (IDE, PCIe r6.2 sec 6.33) because the key material used by IDE is transmitted over a CMA-SPDM session.
As a first step, add support for authentication via a CMA TSM driver.
This was previously discusd here:
http://lore.kernel.org/[email protected]
By utilising a TSM driver we get a lot of the TSM driver probe policies "for free". Currently there is no mechanism to provide evidence to userspace, as the TSM system doesn't support that at the moment. That can be added later when support by TSM.
Credits: Jonathan wrote the original proof-of-concept for a CMA implementation. Lukas reworked that for upstream. Wilfred contributed fixes for issues discovered during testing. Alistair reworked it as a TSM driver.
--- MAINTAINERS | 1 + drivers/pci/Kconfig | 14 ++++ drivers/pci/Makefile | 2 + drivers/pci/cma.c | 154 ++++++++++++++++++++++++++++++++++++++++ drivers/pci/doe.c | 3 - include/linux/pci-doe.h | 4 ++ 6 files changed, 175 insertions(+), 3 deletions(-) create mode 100644 drivers/pci/cma.c
MAINTAINERS +1 -0
@@ -24407,6 +24407,7 @@ L: [email protected] L: [email protected] S: Maintained T: git git://git.kernel.org/pub/scm/linux/kernel/git/devsec/spdm.git +F: drivers/pci/cma.c F: include/linux/spdm.h F: lib/rspdm/
drivers/pci/Kconfig +14 -0
@@ -124,6 +124,20 @@ config PCI_ATS config PCI_IDE bool +config PCI_CMA + bool "Component Measurement and Authentication (CMA-SPDM)" + depends on RSPDM + select CRYPTO_ECDSA + select CRYPTO_RSA + select CRYPTO_SHA256 + select CRYPTO_SHA512 + select PCI_DOE + select PCI_TSM + help + Authenticate devices on enumeration per PCIe r6.2 sec 6.31. + A PCI DOE mailbox is used as transport for DMTF SPDM based + authentication, measurement and secure channel establishment. + config PCI_TSM bool "PCI TSM: Device security protocol support" select PCI_IDE
drivers/pci/Makefile +2 -0
@@ -41,6 +41,8 @@ obj-$(CONFIG_PCI_NPEM) += npem.o obj-$(CONFIG_PCIE_TPH) += tph.o obj-$(CONFIG_CARDBUS) += setup-cardbus.o +obj-$(CONFIG_PCI_CMA) += cma.o + # Endpoint library must be initialized before its users obj-$(CONFIG_PCI_ENDPOINT) += endpoint/
drivers/pci/cma.c new file +154 -0
154 lines not on this page · raw message
drivers/pci/doe.c +0 -3
@@ -31,9 +31,6 @@#define PCI_DOE_FLAG_CANCEL 0#define PCI_DOE_FLAG_DEAD 1 -/* Max data object length is 2^18 dwords */ -#define PCI_DOE_MAX_LENGTH (1 << 18) -/** * struct pci_doe_mb - State for a single DOE mailbox *
include/linux/pci-doe.h +4 -0
@@ -19,6 +19,10 @@ struct pci_doe_mb;#define PCI_DOE_FEATURE_CMA 1#define PCI_DOE_FEATURE_SSESSION 2 +/* Max data object length is 2^18 dwords (including 2 dwords for header) */ +#define PCI_DOE_MAX_LENGTH (1 << 18) +#define PCI_DOE_MAX_PAYLOAD ((PCI_DOE_MAX_LENGTH - 2) * sizeof(u32)) +struct pci_doe_mb *pci_find_doe_mailbox (struct pci_dev *pdev,u16 vendor,u8 type);
Signature · 1 line
2.54.0
[email protected]
AI reviewRe: [PATCH v2 12/21] PCI/CMA: Add a PCI TSM CMA driver using SPDM
Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 13 lines
From: Alistair Francis <[email protected]>
Component Measurement and Authentication (CMA, PCIe r6.2 sec 6.31) allows for measurement and authentication of PCIe devices. It is based on the Security Protocol and Data Model specification (SPDM,
https://www.dmtf.org/dsp/DSP0274).CMA-SPDM in turn forms the basis for Integrity and Data Encryption (IDE, PCIe r6.2 sec 6.33) because the key material used by IDE is transmitted over a CMA-SPDM session.
As a first step, add support for authentication via a CMA TSM driver.
Quoted 8 lines
This was previously discusd here:
http://lore.kernel.org/[email protected][ ... ]Co-developed-by Wilfred Mallawa <[email protected]>Signed-off-by Wilfred Mallawa <[email protected]>Co-developed-by Lukas Wunner <[email protected]>Signed-off-by Lukas Wunner <[email protected]>Signed-off-by Alistair Francis <[email protected]>
Sashiko has reviewed this patch and found no issues. It looks great!
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=12
PATCH v2 13/21 PCI/CMA: Validate Subject Alternative Name in certificates
In reply to [email protected]From: Lukas Wunner <[email protected]>
PCIe r6.1 sec 6.31.3 stipulates requirements for Leaf Certificates presented by devices, in particular the presence of a Subject Alternative Name which encodes the Vendor ID, Device ID, Device Serial Number, etc.
This prevents a mismatch between the device identity in Config Space and the certificate. A device cannot misappropriate a certificate from a different device without also spoofing Config Space. As a corollary, it cannot dupe an arbitrary driver into binding to it. Only drivers which bind to the device identity in the Subject Alternative Name work (PCIe r6.1 sec 6.31 "Implementation Note: Overview of Threat Model").
The Subject Alternative Name is signed, hence constitutes a signed copy of a Config Space portion. It's the same concept as web certificates which contain a set of domain names in the Subject Alternative Name for identity verification.
Parse the Subject Alternative Name using a small ASN.1 module and validate its contents. The theory of operation is explained in a comment at the top of the newly inserted code.
This functionality is introduced in a separate commit on top of basic CMA-SPDM support to split the code into digestible, reviewable chunks.
The CMA OID added here is taken from the official OID Repository (it's not documented in the PCIe Base Spec):
https://oid-rep.orange-labs.fr/get/2.23.147
Side notes:
* PCIe r6.2 removes the spec language on the Subject Alternative Name. It still "requires the leaf certificate to include the information typically used by system software for device driver binding", but no longer specifies how that information is encoded into the certificate.
According to the editor of the PCIe Base Spec and the author of the CMA 1.1 ECN (which caused this change), FPGA cards which mutate their device identity at runtime (due to a firmware update) were thought as unable to satisfy the previous spec language. The Protocol Working Group could not agree on a better solution and therefore dropped the spec language entirely. They acknowledge that the requirement is now under-spec'd. Because products already exist which adhere to the Subject Alternative Name requirement per PCIe r6.1 sec 6.31.3, they recommended to "push through" and use it as the de facto standard.
The FPGA concerns are easily overcome by reauthenticating the device after a firmware update, either via sysfs or pci_cma_reauthenticate() (added by a subsequent commit).
* PCIe r6.1 sec 6.31.3 strongly recommends to verify that "the information provided in the Subject Alternative Name entry is signed by the vendor indicated by the Vendor ID." In other words, the root certificate on pci_cma_keyring which signs the device's certificate chain must have been created for a particular Vendor ID.
Unfortunately the spec neglects to define how the Vendor ID shall be encoded into the root certificate. So the recommendation cannot be implemented at this point and it is thus possible that a vendor signs device certificates of a different vendor.
* Instead of a Subject Alternative Name, Leaf Certificates may include "a Reference Integrity Manifest, e.g., see Trusted Computing Group" or "a pointer to a location where such a Reference Integrity Manifest can be obtained" (PCIe r6.1 sec 6.31.3).
A Reference Integrity Manifest contains "golden" measurements which can be compared to actual measurements retrieved from a device. It serves a different purpose than the Subject Alternative Name, hence it is unclear why the spec says only either of them is necessary. It is also unclear how a Reference Integrity Manifest shall be encoded into a certificate.
Hence ignore the Reference Integrity Manifest requirement.
[ Changed by AF:
- Fixup a few issues caught by Sashiko
]
--- drivers/pci/Makefile | 5 +- drivers/pci/cma.asn1 | 41 ++++++++++++ drivers/pci/cma.c | 123 ++++++++++++++++++++++++++++++++++- include/linux/oid_registry.h | 3 + 4 files changed, 170 insertions(+), 2 deletions(-) create mode 100644 drivers/pci/cma.asn1
drivers/pci/Makefile +4 -1
@@ -41,7 +41,10 @@ obj-$(CONFIG_PCI_NPEM) += npem.o obj-$(CONFIG_PCIE_TPH) += tph.o obj-$(CONFIG_CARDBUS) += setup-cardbus.o -obj-$(CONFIG_PCI_CMA) += cma.o +obj-$(CONFIG_PCI_CMA) += pci-cma.o +pci-cma-y := cma.o cma.asn1.o +$(obj)/cma.o: $(obj)/cma.asn1.h +$(obj)/cma.asn1.o: $(obj)/cma.asn1.c $(obj)/cma.asn1.h # Endpoint library must be initialized before its users obj-$(CONFIG_PCI_ENDPOINT) += endpoint/
drivers/pci/cma.asn1 new file +41 -0
41 lines not on this page · raw message
drivers/pci/cma.c +122 -1
143 lines not on this page · raw message
include/linux/oid_registry.h +3 -0
@@ -150,6 +150,9 @@ enum OID { OID_id_ml_dsa_65,/* 2.16.840.1.101.3.4.3.18 */ OID_id_ml_dsa_87,/* 2.16.840.1.101.3.4.3.19 */ +/* PCI */ +OID_CMA ,/* 2.23.147 */ + OID__NR };
Signature · 1 line
2.54.0
[email protected]
AI reviewRe: [PATCH v2 13/21] PCI/CMA: Validate Subject Alternative Name in certificates
Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 13 lines
From: Lukas Wunner <[email protected]>
PCIe r6.1 sec 6.31.3 stipulates requirements for Leaf Certificates presented by devices, in particular the presence of a Subject Alternative Name which encodes the Vendor ID, Device ID, Device Serial Number, etc.
This prevents a mismatch between the device identity in Config Space and the certificate. A device cannot misappropriate a certificate from a different device without also spoofing Config Space. As a corollary, it cannot dupe an arbitrary driver into binding to it. Only drivers which bind to the device identity in the Subject Alternative Name work (PCIe r6.1 sec 6.31 "Implementation Note: Overview of Threat Model").
Quoted 8 lines
The Subject Alternative Name is signed, hence constitutes a signed copy of a Config Space portion. It's the same concept as web certificates
[ ... ][ Changed by AF:
- Fixup a few issues caught by Sashiko
]Signed-off-by Alistair Francis <[email protected]>
Sashiko has reviewed this patch and found no issues. It looks great!
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=13
PATCH v2 14/21 lib: rspdm: Support SPDM get_version
In reply to [email protected]From: Alistair Francis <[email protected]>
Support the GET_VERSION SPDM command.
--- lib/rspdm/consts.rs | 16 ++++++++-- lib/rspdm/lib.rs | 54 +++++++++++++++++++++++++++------ lib/rspdm/state.rs | 67 ++++++++++++++++++++++++++++++++++++++-- lib/rspdm/validator.rs | 69 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 192 insertions(+), 14 deletions(-)
lib/rspdm/consts.rs +14 -2
@@ -7,16 +7,24 @@//! Rust implementation of the DMTF Security Protocol and Data Model (SPDM) //! <https://www.dmtf.org/dsp/DSP0274> +use crate ::validator::GetVersionRsp ; +use core::mem;use kernel::error::{code::EINVAL ,Error };// SPDM versions supported by this implementation pub (crate )const SPDM_VER_10 :u8 =0x10 ; +#[allow(dead_code)] +pub (crate )const SPDM_VER_11 :u8 =0x11 ; +#[allow(dead_code)] +pub (crate )const SPDM_VER_12 :u8 =0x12 ; +#[allow(dead_code)] +pub (crate )const SPDM_VER_13 :u8 =0x13 ; +pub (crate )const SPDM_VER_14 :u8 =0x14 ;pub (crate )const SPDM_MIN_VER :u8 =SPDM_VER_10 ; +pub (crate )const SPDM_MAX_VER :u8 =SPDM_VER_14 ; -#[allow(dead_code)] pub (crate )const SPDM_REQ :u8 =0x80 ; -#[allow(dead_code)] pub (crate )const SPDM_ERROR :u8 =0x7f ;#[derive( Clone , Copy )]
@@ -90,3 +98,7 @@ fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {write! (f,"{:#x}" , *selfas u8 ) } } + +pub (crate )const SPDM_GET_VERSION :u8 =0x84 ; +pub (crate )const SPDM_GET_VERSION_LEN :usize = + mem::size_of ::<GetVersionRsp >() + (u8::MAX as usize ) * mem::size_of ::<u16 >();
lib/rspdm/lib.rs +44 -10
78 lines not on this page · raw message
lib/rspdm/state.rs +65 -2
105 lines not on this page · raw message
lib/rspdm/validator.rs +69 -0
84 lines not on this page · raw message
Signature · 1 line
2.54.0
[email protected]
AI reviewRe: [PATCH v2 14/21] lib: rspdm: Support SPDM get_version
Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 5 lines
From: Alistair Francis <[email protected]>
Support the
GET_VERSIONSPDM command.Signed-off-by Alistair Francis <[email protected]>
Sashiko has reviewed this patch and found no issues. It looks great!
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=14
PATCH v2 15/21 lib: rspdm: Support SPDM get_capabilities
In reply to [email protected]From: Alistair Francis <[email protected]>
Support the GET_CAPABILITIES SPDM command.
--- lib/rspdm/consts.rs | 19 +++++++- lib/rspdm/lib.rs | 4 ++ lib/rspdm/state.rs | 78 ++++++++++++++++++++++++++++- lib/rspdm/validator.rs | 108 ++++++++++++++++++++++++++++++++++++++++- rust/kernel/error.rs | 1 + 5 files changed, 206 insertions(+), 4 deletions(-)
lib/rspdm/consts.rs +17 -2
@@ -9,13 +9,12 @@use crate ::validator::GetVersionRsp ;use core::mem; +use kernel::bits::bit_u32;use kernel::error::{code::EINVAL ,Error };// SPDM versions supported by this implementation pub (crate )const SPDM_VER_10 :u8 =0x10 ; -#[allow(dead_code)] pub (crate )const SPDM_VER_11 :u8 =0x11 ; -#[allow(dead_code)] pub (crate )const SPDM_VER_12 :u8 =0x12 ;#[allow(dead_code)] pub (crate )const SPDM_VER_13 :u8 =0x13 ;
@@ -102,3 +101,19 @@ fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {pub (crate )const SPDM_GET_VERSION :u8 =0x84 ;pub (crate )const SPDM_GET_VERSION_LEN :usize = mem::size_of ::<GetVersionRsp >() + (u8::MAX as usize ) * mem::size_of ::<u16 >(); + +pub (crate )const SPDM_GET_CAPABILITIES :u8 =0xe1 ; +pub (crate )const SPDM_MIN_DATA_TRANSFER_SIZE :u32 =42 ; + +// SPDM cryptographic timeout of this implementation: +// Assume calculations may take up to 1 sec on a busy machine, which equals +// roughly 1 << 20. That's within the limits mandated for responders by CMA +// (1 << 23 usec, PCIe r6.2 sec 6.31.3) and DOE (1 sec, PCIe r6.2 sec 6.30.2). +// Used in GET_CAPABILITIES exchange. +pub (crate )const SPDM_CTEXPONENT :u8 =20 ; + +pub (crate )const SPDM_CERT_CAP :u32 =bit_u32 (1 ); +pub (crate )const SPDM_CHAL_CAP :u32 =bit_u32 (2 ); + +pub (crate )const SPDM_REQ_CAPS :u32 =SPDM_CERT_CAP |SPDM_CHAL_CAP ; +pub (crate )const SPDM_RSP_MIN_CAPS :u32 =SPDM_CERT_CAP |SPDM_CHAL_CAP ;
lib/rspdm/lib.rs +4 -0
@@ -102,6 +102,10 @@ pub extern "C" fn spdm_authenticate(state_ptr: *mut spdm_state) -> c_int {return e.to_errno ()as c_int ; } +if let Err (e) = state.get_capabilities () { +return e.to_errno ()as c_int ; + } + -(EPROTONOSUPPORT as i32 ) }
lib/rspdm/state.rs +77 -1
109 lines not on this page · raw message
lib/rspdm/validator.rs +107 -1
118 lines not on this page · raw message
rust/kernel/error.rs +1 -0
@@ -94,6 +94,7 @@ macro_rules! declare_err {declare_err! (ECONNRESET ,"Connection reset by peer." );declare_err! (EINPROGRESS ,"Operation now in progress." );declare_err! (EPROTO ,"Protocol error" ); +declare_err! (EPROTONOSUPPORT ,"Protocol not supported" ); }/// Generic integer kernel error.
Signature · 1 line
2.54.0
[email protected]
AI reviewRe: [PATCH v2 15/21] lib: rspdm: Support SPDM get_capabilities
Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 5 lines
From: Alistair Francis <[email protected]>
Support the
GET_CAPABILITIESSPDM command.Signed-off-by Alistair Francis <[email protected]>
Sashiko has reviewed this patch and found no issues. It looks great!
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=15
PATCH v2 16/21 lib: rspdm: Support SPDM negotiate_algorithms
In reply to [email protected]From: Alistair Francis <[email protected]>
Support the NEGOTIATE_ALGORITHMS SPDM command.
--- lib/rspdm/consts.rs | 56 +++++++++- lib/rspdm/lib.rs | 9 +- lib/rspdm/state.rs | 239 ++++++++++++++++++++++++++++++++++++++++- lib/rspdm/validator.rs | 110 ++++++++++++++++++- 4 files changed, 408 insertions(+), 6 deletions(-)
lib/rspdm/consts.rs +55 -1
@@ -9,7 +9,10 @@use crate ::validator::GetVersionRsp ;use core::mem; -use kernel::bits::bit_u32; +use kernel::bits::{ + bit_u32, + bit_u8,// +};use kernel::error::{code::EINVAL ,Error };// SPDM versions supported by this implementation
@@ -114,6 +117,57 @@ fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {pub (crate )const SPDM_CERT_CAP :u32 =bit_u32 (1 );pub (crate )const SPDM_CHAL_CAP :u32 =bit_u32 (2 ); +pub (crate )const SPDM_KEY_EX_CAP :u32 =bit_u32 (9 );pub (crate )const SPDM_REQ_CAPS :u32 =SPDM_CERT_CAP |SPDM_CHAL_CAP ;pub (crate )const SPDM_RSP_MIN_CAPS :u32 =SPDM_CERT_CAP |SPDM_CHAL_CAP ; + +pub (crate )const SPDM_NEGOTIATE_ALGS :u8 =0xe3 ; + +pub (crate )const SPDM_MEAS_SPEC_DMTF :u8 =bit_u8 (0 ); + +pub (crate )const SPDM_ASYM_RSASSA_2048 :u32 =bit_u32 (0 ); +pub (crate )const _SPDM_ASYM_RSAPSS_2048:u32 =bit_u32 (1 ); +pub (crate )const SPDM_ASYM_RSASSA_3072 :u32 =bit_u32 (2 ); +pub (crate )const _SPDM_ASYM_RSAPSS_3072:u32 =bit_u32 (3 ); +pub (crate )const SPDM_ASYM_ECDSA_ECC_NIST_P256 :u32 =bit_u32 (4 ); +pub (crate )const SPDM_ASYM_RSASSA_4096 :u32 =bit_u32 (5 ); +pub (crate )const _SPDM_ASYM_RSAPSS_4096:u32 =bit_u32 (6 ); +pub (crate )const SPDM_ASYM_ECDSA_ECC_NIST_P384 :u32 =bit_u32 (7 ); +pub (crate )const SPDM_ASYM_ECDSA_ECC_NIST_P521 :u32 =bit_u32 (8 ); +pub (crate )const _SPDM_ASYM_SM2_ECC_SM2_P256:u32 =bit_u32 (9 ); +pub (crate )const _SPDM_ASYM_EDDSA_ED25519:u32 =bit_u32 (10 ); +pub (crate )const _SPDM_ASYM_EDDSA_ED448:u32 =bit_u32 (11 ); + +pub (crate )const SPDM_HASH_SHA_256 :u32 =bit_u32 (0 ); +pub (crate )const SPDM_HASH_SHA_384 :u32 =bit_u32 (1 ); +pub (crate )const SPDM_HASH_SHA_512 :u32 =bit_u32 (2 ); + +// If the crypto support isn't enabled don't offer the algorithms +// to the responder +#[cfg( CONFIG_CRYPTO_RSA )] +pub (crate )const SPDM_ASYM_RSA :u32 = +SPDM_ASYM_RSASSA_2048 |SPDM_ASYM_RSASSA_3072 |SPDM_ASYM_RSASSA_4096 ; +#[cfg(not( CONFIG_CRYPTO_RSA ))] +pub (crate )const SPDM_ASYM_RSA :u32 =0 ; + +#[cfg( CONFIG_CRYPTO_ECDSA )] +pub (crate )const SPDM_ASYM_ECDSA :u32 = +SPDM_ASYM_ECDSA_ECC_NIST_P256 |SPDM_ASYM_ECDSA_ECC_NIST_P384 |SPDM_ASYM_ECDSA_ECC_NIST_P521 ; +#[cfg(not( CONFIG_CRYPTO_ECDSA ))] +pub (crate )const SPDM_ASYM_ECDSA :u32 =0 ; + +#[cfg( CONFIG_CRYPTO_SHA256 )] +pub (crate )const SPDM_HASH_SHA2_256 :u32 =SPDM_HASH_SHA_256 ; +#[cfg(not( CONFIG_CRYPTO_SHA256 ))] +pub (crate )const SPDM_HASH_SHA2_256 :u32 =0 ; + +#[cfg( CONFIG_CRYPTO_SHA512 )] +pub (crate )const SPDM_HASH_SHA2_384_512 :u32 =SPDM_HASH_SHA_384 |SPDM_HASH_SHA_512 ; +#[cfg(not( CONFIG_CRYPTO_SHA512 ))] +pub (crate )const SPDM_HASH_SHA2_384_512 :u32 =0 ; + +pub (crate )const SPDM_ASYM_ALGOS :u32 =SPDM_ASYM_RSA |SPDM_ASYM_ECDSA ; +pub (crate )const SPDM_HASH_ALGOS :u32 =SPDM_HASH_SHA2_256 |SPDM_HASH_SHA2_384_512 ; + +pub (crate )const SPDM_OPAQUE_DATA_FMT_GENERAL :u8 =bit_u8 (1 );
lib/rspdm/lib.rs +7 -2
@@ -94,7 +94,7 @@ pub extern "C" fn spdm_authenticate(state_ptr: *mut spdm_state) -> c_int {// exclusive `&mut SpdmState` lives entirely inside the lock guard, so // concurrent FFI callers serialize on the mutex and can never form // aliased `&mut SpdmState` references. -let mutex: &Mutex <SpdmState > =unsafe { &*(state_ptras *const Mutex <SpdmState >) }; +let mutex: &Mutex <SpdmState <'_>> =unsafe { &*(state_ptras *const Mutex <SpdmState <'_>>) };let mut state = mutex.lock ();
@@ -106,6 +106,10 @@ pub extern "C" fn spdm_authenticate(state_ptr: *mut spdm_state) -> c_int {return e.to_errno ()as c_int ; } +if let Err (e) = state.negotiate_algs () { +return e.to_errno ()as c_int ; + } + -(EPROTONOSUPPORT as i32 ) }
@@ -117,11 +121,12 @@ pub extern "C" fn spdm_destroy(state_ptr: *mut spdm_state) {if state_ptr.is_null () {return ; } +// SAFETY: `state_ptr` was returned from `spdm_create()`, which leaked a // `Pin<KBox<Mutex<SpdmState>>>` via `KBox::into_raw`. The caller // guarantees the state is no longer in use. Reconstructing the pinned // box and dropping it runs `Drop` for the `Mutex` and `SpdmState` and // frees the allocation. -let b =unsafe {KBox ::from_raw (state_ptras *mut Mutex <SpdmState >) }; +let b =unsafe {KBox ::from_raw (state_ptras *mut Mutex <SpdmState <'_>>) };drop (unsafe {Pin ::new_unchecked (b) }); }
lib/rspdm/state.rs +237 -2
289 lines not on this page · raw message
lib/rspdm/validator.rs +109 -1
129 lines not on this page · raw message
Signature · 1 line
2.54.0
[email protected]
AI reviewRe: [PATCH v2 16/21] lib: rspdm: Support SPDM negotiate_algorithms
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Incorrect generic type inference in `KVec::from_raw_parts` and lost capacity leads to Rust…
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Incorrect generic type inference in KVec::from_raw_parts and lost capacity leads to Rust Undefined Behavior during descriptor destruction.
- [High] The NEGOTIATE_ALGORITHMS handler rejects valid SPDM 1.0 responses due to a hardcoded 36-byte size validation.
--
commit 886ea37ebdf0a050ca99e3d1c7b0a5330292a00a
Author: Alistair Francis <[email protected]>
lib: rspdm: Support SPDM negotiate_algorithms
Support the NEGOTIATE_ALGORITHMS SPDM command.
Quoted 4 lines
lib/rspdm/state.rs +0
-0
[ ... ]
Quoted 20 lines
lib/rspdm/state.rs +13
-1@@ -72,9 +108,41 @@ pub(crate) struct SpdmState {// Negotiated state pub(crate) version: u8, pub(crate) rsp_caps: u32, + pub(crate) base_asym_alg: u32, + pub(crate) base_hash_alg: u32, + pub(crate) meas_hash_alg: u32, + +/* Signature algorithm */ + base_asym_enc: &'a CStr, + sig_len: usize, + + /* Hash algorithm */ + base_hash_alg_name: &'a CStr, + pub(crate) shash: *mut bindings::crypto_shash, + pub(crate) desc: Option<&'a mut bindings::shash_desc>, + pub(crate) hash_len: usize, } -impl SpdmState {
lib/rspdm/state.rs pub(crate) struct SpdmState { · line 135
Quoted 10 lines
lib/rspdm/state.rs +10
-0+impl Drop for SpdmState <'_> { +fn drop (&mut self) { +if let Some (desc) = self.desc.take () { +// SAFETY: `self.shash` is a valid handle +let desc_len = core::mem::size_of ::<bindings::shash_desc >() + +unsafe { bindings::crypto_shash_descsize (self.shash) }as usize ; + +// SAFETY: `desc` was allocated and converted to a raw pointer with +// into_raw_parts() +let desc_vec =unsafe {KVec ::from_raw_parts (desc, desc_len, desc_len) };
[Severity: High]
Does this code inadvertently cause Undefined Behavior by incorrectly inferring the generic type of KVec?
Since desc is of type &mut bindings::shash_desc, KVec::from_raw_parts infers the new vector as KVec<bindings::shash_desc>. Because desc_len is calculated in bytes, this tells the allocator to free desc_len elements of type shash_desc (which is 16 bytes) instead of desc_len bytes.
Also, it looks like the original allocation capacity was discarded when into_raw_parts() was called in update_response_algs(), and here we are passing desc_len as the capacity, which violates from_raw_parts invariants.
Quoted 9 lines
lib/rspdm/state.rs +9
-0+drop (desc_vec); + } + +unsafe { + bindings::crypto_free_shash (self.shash); + } + } +} +
[ ... ]
Quoted 1 line
+ fn
update_response_algs(&mut self)-> Result<(), Error> {
[ ... ]
lib/rspdm/state.rs pub(crate) fn get_capabilities(&mut self) -> Result<(), Error> { · line 512
Quoted 11 lines
lib/rspdm/state.rs +11
-0+// This is freed in when `SpdmState` is dropped, but this call +// can happen multiple times. +if self.shash != core::ptr::null_mut () { +if let Some (desc) = self.desc.take () { +// SAFETY: `self.shash` is a valid handle +let desc_len = core::mem::size_of ::<bindings::shash_desc >() + +unsafe { bindings::crypto_shash_descsize (self.shash) }as usize ; + +// SAFETY: `desc` was allocated and converted to a raw pointer with +// into_raw_parts() +let desc_vec =unsafe {KVec ::from_raw_parts (desc, desc_len, desc_len) };
[Severity: High]
Will the same Undefined Behavior happen here when replacing an existing hash algorithm context, since it also uses desc and desc_len to reconstruct the KVec?
Quoted 9 lines
lib/rspdm/state.rs +9
-0+drop (desc_vec); + } + +unsafe { + bindings::crypto_free_shash (self.shash); + } + } + + self.shash =
[ ... ]
Quoted 4 lines
lib/rspdm/validator.rs +0
-0
[ ... ]
lib/rspdm/validator.rs fn validate(unvalidated: &mut KVec<u8>) -> Result<Self, Self::Err> { · line 365
Quoted 7 lines
lib/rspdm/validator.rs +7
-0+impl Validate <Untrusted <&[u8 ]>>for &NegotiateAlgsRsp { +type Err =Error ; + +fn validate (unvalidated: &[u8 ]) ->Result <Self ,Self ::Err > { +if unvalidated.len () < mem::size_of ::<NegotiateAlgsRsp >() { +return Err (EINVAL ); + }
[Severity: High]
Could this hardcoded length check reject valid SPDM 1.0 responses?
SpdmState allows negotiating SPDM version 1.0, and the SPDM 1.0.0 specification states that the NEGOTIATE_ALGORITHMS response base header is exactly 32 bytes long.
Because mem::size_of::<NegotiateAlgsRsp>() is 36 bytes (including fields introduced in SPDM 1.1), when communicating with an SPDM 1.0 responder that returns 32 bytes, this will fail with EINVAL and break negotiation.
Quoted 10 lines
lib/rspdm/validator.rs +10
-0+ +let ptr = unvalidated.as_ptr (); +// CAST: `NegotiateAlgsRsp` only contains integers and has `repr(C)`. +let ptr = ptr.cast ::<NegotiateAlgsRsp >(); +// SAFETY: `ptr` came from a reference and the cast above is valid. +let rsp: &NegotiateAlgsRsp =unsafe { &*ptr }; + +Ok (rsp) + } +}
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=16
PATCH v2 17/21 lib: rspdm: Support SPDM get_digests
In reply to [email protected]From: Alistair Francis <[email protected]>
Support the GET_DIGESTS SPDM command.
--- lib/rspdm/consts.rs | 7 ++-- lib/rspdm/lib.rs | 4 +++ lib/rspdm/state.rs | 80 +++++++++++++++++++++++++++++++++++++++++- lib/rspdm/validator.rs | 53 ++++++++++++++++++++++++++++ 4 files changed, 141 insertions(+), 3 deletions(-)
lib/rspdm/consts.rs +5 -2
@@ -19,10 +19,11 @@pub (crate )const SPDM_VER_10 :u8 =0x10 ;pub (crate )const SPDM_VER_11 :u8 =0x11 ;pub (crate )const SPDM_VER_12 :u8 =0x12 ; -#[allow(dead_code)] pub (crate )const SPDM_VER_13 :u8 =0x13 ;pub (crate )const SPDM_VER_14 :u8 =0x14 ; +pub (crate )const SPDM_SLOTS :usize =8 ; +pub (crate )const SPDM_MIN_VER :u8 =SPDM_VER_10 ;pub (crate )const SPDM_MAX_VER :u8 =SPDM_VER_14 ;
@@ -106,7 +107,7 @@ fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { mem::size_of ::<GetVersionRsp >() + (u8::MAX as usize ) * mem::size_of ::<u16 >();pub (crate )const SPDM_GET_CAPABILITIES :u8 =0xe1 ; -pub (crate )const SPDM_MIN_DATA_TRANSFER_SIZE :u32 =42 ; +pub (crate )const SPDM_MIN_DATA_TRANSFER_SIZE :u32 =42 ;// SPDM 1.2.0 margin no 226 // SPDM cryptographic timeout of this implementation: // Assume calculations may take up to 1 sec on a busy machine, which equals
@@ -143,6 +144,8 @@ fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {pub (crate )const SPDM_HASH_SHA_384 :u32 =bit_u32 (1 );pub (crate )const SPDM_HASH_SHA_512 :u32 =bit_u32 (2 ); +pub (crate )const SPDM_GET_DIGESTS :u8 =0x81 ; +// If the crypto support isn't enabled don't offer the algorithms // to the responder #[cfg( CONFIG_CRYPTO_RSA )]
lib/rspdm/lib.rs +4 -0
@@ -110,6 +110,10 @@ pub extern "C" fn spdm_authenticate(state_ptr: *mut spdm_state) -> c_int {return e.to_errno ()as c_int ; } +if let Err (e) = state.get_digests () { +return e.to_errno ()as c_int ; + } + -(EPROTONOSUPPORT as i32 ) }
lib/rspdm/state.rs +79 -1
132 lines not on this page · raw message
lib/rspdm/validator.rs +53 -0
62 lines not on this page · raw message
Signature · 1 line
2.54.0
[email protected]
AI reviewRe: [PATCH v2 17/21] lib: rspdm: Support SPDM get_digests
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues:
- [High] Buffer allocation for GetDigestsRsp does not account for SPDM 1.3 additional fields, causing legitimate responses to exceed the buffer size.
- [High] The returned digest payload is ignored, failing to invalidate stale cached certificates if a device updates its certificate.
Pre-existing issues:
- [High] Rebuilding KVec from a raw pointer infers the wrong generic type parameter, causing the global allocator to deallocate a massively oversized layout. --
commit 464cef22c054f846ef1b297c22ac00c94da2dd17
Author: Alistair Francis <[email protected]>
lib: rspdm: Support SPDM get_digests
Support the GET_DIGESTS SPDM command.
Quoted 10 lines
lib/rspdm/state.rs +2
-0@@ -608,4 +626,64 @@ pub(crate) fn negotiate_algs(&mut self) -> Result<(), Error> {Ok (()) } + +pub (crate )fn get_digests (&mut self) ->Result <(),Error > {
lib/rspdm/state.rs pub(crate) fn negotiate_algs(&mut self) -> Result<(), Error> { · line 640
Quoted 10 lines
lib/rspdm/state.rs +10
-0+let mut request =GetDigestsReq ::default (); + request.version = self.version; + +let req_sz = core::mem::size_of ::<GetDigestsReq >(); +let rsp_sz = core::mem::size_of ::<GetDigestsRsp >() +SPDM_SLOTS * self.hash_len; + +// SAFETY: `request` is repr(C) and packed, so we can convert it to a slice +let request_buf =unsafe {from_raw_parts_mut (&mut requestas *mut _ as *mut u8 , req_sz) }; + +let mut response_vec:KVec <u8 > =KVec ::from_elem (0u8 , rsp_sz,GFP_KERNEL )?;
[Severity: High]
Does this correctly calculate the required buffer size for an SPDM 1.3 responder?
If the responder negotiates SPDM 1.3+, the GET_DIGESTS response appends extra fields (like KeyPairIDs, CertificateInfo, or KeyUsageMask) after the digests. Because rsp_sz strictly allocates only enough for the headers and the hash lengths, the transport may truncate the response or fail the bounds check when it returns a length larger than response_vec.len().
Quoted 8 lines
lib/rspdm/state.rs +8
-0+ +let len = self.spdm_exchange (request_buf, response_vec.as_mut_slice ())?as usize ; + +// The transport must report a length within the buffer we provided. +if len > response_vec.len () { +pr_err! ("Overflowed digests response\n" ); +return Err (EIO ); + }
[ ... ]
lib/rspdm/state.rs pub(crate) fn negotiate_algs(&mut self) -> Result<(), Error> { · line 666
Quoted 6 lines
lib/rspdm/state.rs +6
-0+let mut deprovisioned_slots = self.provisioned_slots & !response.param2; +while (deprovisioned_slots.trailing_zeros ()as usize ) <SPDM_SLOTS { +let slot = deprovisioned_slots.trailing_zeros ()as usize ; + self.certs[slot].clear (); + deprovisioned_slots &= !(1 << slot); + }
[Severity: High]
Does this logic correctly invalidate stale cached certificates if a device updates its certificate in an existing provisioned slot?
While this clears completely deprovisioned slots, it appears to ignore the returned digest payload itself. If the responder rotated its certificate in an existing slot, the digest would change. By not comparing the returned digests against the cached certificates and clearing mismatches, later validation of CHALLENGE_AUTH could fail cryptographically because it verifies against the stale cached certificate.
[ ... ]
Quoted 7 lines
let desc_len = core::mem::size_of::<bindings::shash_desc>()
+ unsafe { bindings::crypto_shash_descsize(self.shash) } as usize;
// SAFETY: `desc` was allocated and converted to a raw pointer with
// into_raw_parts()
let desc_vec = unsafe { KVec::from_raw_parts(desc, desc_len, desc_len) };
drop(desc_vec);[Severity: High]
This isn't a bug introduced by this patch, but can this type inference deallocate a massively oversized layout?
When rebuilding KVec from the raw pointer desc in SpdmState::drop() and update_response_algs(), the generic type T is implicitly inferred as bindings::shash_desc instead of u8.
Since desc_len represents the original allocation length in bytes, the reconstructed KVec<bindings::shash_desc> incorrectly assumes it owns desc_len elements of type bindings::shash_desc. When this vector is dropped, it passes a layout to the global allocator with an extremely large size of (desc_len * size_of::<bindings::shash_desc>()) bytes instead of desc_len bytes.
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=17
PATCH v2 18/21 lib: rspdm: Support SPDM get_certificate
In reply to [email protected]From: Alistair Francis <[email protected]>
Support the GET_CERTIFICATE SPDM command.
The kernel will send a GET_CERTIFICATE request to the the responder and then iterate over all of the certificates returned.
Certificate validation happens in the next commit.
--- lib/rspdm/consts.rs | 2 + lib/rspdm/lib.rs | 15 +++++ lib/rspdm/state.rs | 145 +++++++++++++++++++++++++++++++++++++++++ lib/rspdm/validator.rs | 56 ++++++++++++++++ 4 files changed, 218 insertions(+)
lib/rspdm/consts.rs +2 -0
@@ -146,6 +146,8 @@ fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {pub (crate )const SPDM_GET_DIGESTS :u8 =0x81 ; +pub (crate )const SPDM_GET_CERTIFICATE :u8 =0x82 ; +// If the crypto support isn't enabled don't offer the algorithms // to the responder #[cfg( CONFIG_CRYPTO_RSA )]
lib/rspdm/lib.rs +15 -0
@@ -114,6 +114,21 @@ pub extern "C" fn spdm_authenticate(state_ptr: *mut spdm_state) -> c_int {return e.to_errno ()as c_int ; } +if state.provisioned_slots ==0 { +return -(bindings::EIO as c_int ); + } + +let mut provisioned_slots = state.provisioned_slots; +while (provisioned_slotsas usize ) >0 { +let slot = provisioned_slots.trailing_zeros ()as u8 ; + +if let Err (e) = state.get_certificate (slot) { +return e.to_errno ()as c_int ; + } + + provisioned_slots &= !(1 << slot); + } + -(EPROTONOSUPPORT as i32 ) }
lib/rspdm/state.rs +145 -0
161 lines not on this page · raw message
lib/rspdm/validator.rs +56 -0
65 lines not on this page · raw message
Signature · 1 line
2.54.0
[email protected]
AI reviewRe: [PATCH v2 18/21] lib: rspdm: Support SPDM get_certificate
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
New issues:
- [High] Integer overflow on the offset accumulator allows a malicious SPDM responder to force an infinite memory allocation loop or kernel panic.
- [High] The Validate implementation for &GetCertificateRsp and get_cert_exchange use elided lifetimes, creating a memory safety soundness hole.
- [High] The request.length field is cast to usize for buffer resizing after being converted to little-endian, severely truncating responses on big-endian architectures.
Pre-existing issues:
- [High] Stack-allocated request buffers are passed to a generic transport callback, violating kernel stack DMA rules. --
commit d1fb5590c1a4a5cf4a63bb011d83ed5975824d75
Author: Alistair Francis <[email protected]>
lib: rspdm: Support SPDM get_certificate
This commit adds support for the GET_CERTIFICATE SPDM command. The kernel sends requests to the responder and iterates over the returned certificates, with validation planned for a future patch.
Quoted 3 lines
lib/rspdm/state.rs +0
-0
[ ... ]
Quoted 1 line
lib/rspdm/state.rs +0
-0@@ -686,4 +699,136 @@ pub(crate) fn get_digests(&mut self) -> Result<(), Error> {
[ ... ]
lib/rspdm/state.rs pub(crate) fn get_digests(&mut self) -> Result<(), Error> { · line 707
Quoted 5 lines
lib/rspdm/state.rs +5
-0+fn get_cert_exchange ( + &mut self, + request_buf: &mut [u8 ], + response_vec: &mut KVec <u8 >, + ) ->Result <&GetCertificateRsp ,Error > {
[Severity: High]
Does the elided lifetime here tie the returned reference to self rather than the response_vec buffer?
Standard Rust method lifetime elision binds this detached lifetime to the mutable self reference. This completely disables borrow checking for the underlying response_vec, allowing safe callers to resize, mutate, or drop the heap allocation while retaining a pointer to it, which could trigger a use-after-free.
Quoted 1 line
+ let len =
self.spdm_exchange(request_buf, response_vec.as_mut_slice())? as usize;
[ ... ]
Quoted 4 lines
lib/rspdm/state.rs +4
-0+pub (crate )fn get_certificate (&mut self, slot:u8 ) ->Result <(),Error > { +let mut request =GetCertificateReq ::default (); + request.version = self.version; + request.param1 = slot;
lib/rspdm/state.rs pub(crate) fn get_digests(&mut self) -> Result<(), Error> { · line 747
Quoted 10 lines
lib/rspdm/state.rs +10
-0+ +let req_sz = core::mem::size_of ::<GetCertificateReq >(); +let rsp_sz = (core::mem::size_of ::<GetCertificateRsp >()as u32 + u16::MAX as u32 ) + .min (self.transport_sz)as usize ; + + request.offset =0 ; + request.length = ((rsp_sz - core::mem::size_of ::<GetCertificateRsp >())as u16 ).to_le (); + +// SAFETY: `request` is repr(C) and packed, so we can convert it to a slice +let request_buf =unsafe {from_raw_parts_mut (&mut requestas *mut _ as *mut u8 , req_sz) };
[Severity: High]
This is a pre-existing issue, but does passing stack memory to a generic transport callback violate kernel stack DMA rules?
The GetCertificateReq structure is allocated on the kernel stack and a mutable slice pointing to this memory is passed to spdm_exchange(). Using stack memory for hardware I/O is problematic because stacks may be virtually mapped and lack physical contiguity or proper cacheline alignment. Attempting to map this stack memory for DMA will cause mapping errors or memory corruption.
Quoted 4 lines
lib/rspdm/state.rs +4
-0+ +let mut response_vec:KVec <u8 > =KVec ::from_elem (0u8 , rsp_sz,GFP_KERNEL )?; + +let response = self.get_cert_exchange (request_buf, &mut response_vec)?;
[ ... ]
Quoted 4 lines
lib/rspdm/state.rs +4
-0+let mut offset:u16 = u16::from_le (response.portion_length); +let mut remainder_length = u16::from_le (response.remainder_length)as usize ; + +while remainder_length >0 {
lib/rspdm/state.rs pub(crate) fn get_digests(&mut self) -> Result<(), Error> { · line 781
Quoted 10 lines
lib/rspdm/state.rs +10
-0+ request.offset = offset.to_le (); + request.length = + ((remainder_length.min (rsp_sz - core::mem::size_of ::<GetCertificateRsp >()))as u16 ) + .to_le (); + +let request_buf = +unsafe {from_raw_parts_mut (&mut requestas *mut _ as *mut u8 , req_sz) }; + + response_vec.resize ( + request.length as usize + core::mem::size_of::<GetCertificateRsp>(),
[Severity: High]
Will this resize operation truncate the response buffer on big-endian architectures?
The request.length field is converted to little-endian using .to_le(), but then immediately cast to usize to calculate the new size for resize(). On big-endian architectures, casting this byte-swapped value back to a native usize yields a drastically incorrect size. This will truncate the incoming response to the undersized buffer and reliably fail certificate validation.
Quoted 5 lines
lib/rspdm/state.rs +5
-0+ 0, + GFP_KERNEL, + )?; + +let response = self.get_cert_exchange (request_buf, &mut response_vec)?;
[ ... ]
lib/rspdm/state.rs pub(crate) fn get_digests(&mut self) -> Result<(), Error> { · line 803
Quoted 5 lines
lib/rspdm/state.rs +5
-0+ certs_buf.extend_from_slice ( + &response_vec[8 ..(8 + u16::from_le (response.portion_length)as usize )], +GFP_KERNEL , + )?; + offset += u16::from_le (response.portion_length);
[Severity: High]
Could this offset calculation overflow if a responder provides crafted portion_length and remainder_length values?
The offset variable is explicitly typed as a u16. If an untrusted SPDM responder manipulates the response lengths, offset += portion_length can overflow. On systems with overflow checks enabled, this immediately triggers a panic. Without overflow checks, offset wraps around, allowing the loop:
while remainder_length > 0 {
...
}to run infinitely, continuously appending untrusted data to certs_buf until all kernel memory is exhausted.
Quoted 6 lines
lib/rspdm/state.rs +2
-0+ remainder_length = u16::from_le (response.remainder_length)as usize ; + }lib/rspdm/validator.rs +0
-0
[ ... ]
Quoted 1 line
lib/rspdm/validator.rs +0
-0@@ -426,3 +427,58 @@ fn validate(unvalidated: &[u8]) -> Result<Self, Self::Err> {
[ ... ]
Quoted 1 line
+impl Validate<Untrusted<&[u8]>> for &GetCertificateRsp {
[Severity: High]
Does this Validate implementation fail to link the input slice's lifetime to the output reference's lifetime?
Because explicit lifetimes are missing, the output reference is detached from the input slice. Inside the validate() function, an unsafe block dereferences a pointer into this unconstrained reference, breaking Rust's safety guarantees.
Quoted 16 lines
lib/rspdm/validator.rs +16
-0+type Err =Error ; + +fn validate (unvalidated: &[u8 ]) ->Result <Self ,Self ::Err > { +if unvalidated.len () < mem::size_of ::<GetCertificateRsp >() { +return Err (EINVAL ); + } + +let ptr = unvalidated.as_ptr (); +// CAST: `GetCertificateRsp` only contains integers and has `repr(C)`. +let ptr = ptr.cast ::<GetCertificateRsp >(); +// SAFETY: `ptr` came from a reference and the cast above is valid. +let rsp: &GetCertificateRsp =unsafe { &*ptr }; + +Ok (rsp) + } +}
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=18
PATCH v2 19/21 lib: rspdm: Support SPDM certificate validation
In reply to [email protected]From: Alistair Francis <[email protected]>
Support validating the SPDM certificate chain. This only performs basic sanity checks on the chain before we continue on. This does not ensure that the root CA is trusted, we leave that for userspace to check and enforce. Instead we just make sure that the chain is correct, uses supported signatures and that it isn't blacklisted in the kernel.
We then store the first leaf certificate for use later.
--- lib/rspdm/lib.rs | 12 +++ lib/rspdm/state.rs | 142 +++++++++++++++++++++++++++++++- rust/bindings/bindings_helper.h | 2 + rust/kernel/error.rs | 1 + 4 files changed, 156 insertions(+), 1 deletion(-)
lib/rspdm/lib.rs +12 -0
@@ -129,6 +129,18 @@ pub extern "C" fn spdm_authenticate(state_ptr: *mut spdm_state) -> c_int { provisioned_slots &= !(1 << slot); } +let mut provisioned_slots = state.provisioned_slots; +while (provisioned_slotsas usize ) >0 { +let slot = provisioned_slots.trailing_zeros ()as u8 ; + +if let Err (e) = state.validate_cert_chain (slot) { +pr_err! ("Certificate in slot {slot} failed to verify: {e:?}\n" ); +return e.to_errno ()as c_int ; + } + + provisioned_slots &= !(1 << slot); + } + -(EPROTONOSUPPORT as i32 ) }
lib/rspdm/state.rs +141 -1
168 lines not on this page · raw message
rust/bindings/bindings_helper.h +2 -0
@@ -38,6 +38,8 @@#include <drm/drm_gem_shmem_helper.h> #include <drm/drm_gpuvm.h> #include <drm/drm_ioctl.h> +#include <keys/asymmetric-type.h> +#include <keys/x509-parser.h> #include <kunit/test.h> #include <linux/auxiliary_bus.h> #include <linux/bitmap.h>
rust/kernel/error.rs +1 -0
@@ -95,6 +95,7 @@ macro_rules! declare_err {declare_err! (EINPROGRESS ,"Operation now in progress." );declare_err! (EPROTO ,"Protocol error" );declare_err! (EPROTONOSUPPORT ,"Protocol not supported" ); +declare_err! (EKEYREJECTED ,"Key was rejected by service" ); }/// Generic integer kernel error.
Signature · 1 line
2.54.0
[email protected]
AI reviewRe: [PATCH v2 19/21] lib: rspdm: Support SPDM certificate validation
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Low] `validate_cert_chain` incorrectly accepts empty certificate chains as valid, silently bypassing all…
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] validate_cert_chain incorrectly accepts empty certificate chains as valid, silently bypassing all certificate parsing, signature verification, and the caller-provided validation callback. --
commit 0fd3be1749a0a856066f371b9a3384e6ffb42abc
Author: Alistair Francis <[email protected]>
lib: rspdm: Support SPDM certificate validation
This patch adds basic validation for SPDM certificate chains, checking that the chain is structurally correct, signatures are supported, and certificates aren't blacklisted. It also extracts and stores the first leaf certificate for later use.
Quoted 3 lines
lib/rspdm/state.rs +0
-0
[ ... ]
lib/rspdm/state.rs pub(crate) fn get_certificate(&mut self, slot: u8) -> Result<(), Error> { · line 856
Quoted 10 lines
lib/rspdm/state.rs +10
-0+pub (crate )fn validate_cert_chain (&mut self, slot:u8 ) ->Result <(),Error > { +let cert_chain_buf = &self.certs[slotas usize ]; +let cert_chain_len = cert_chain_buf.len (); +// We skip over the RootHash +let header_len =4 + self.hash_len; + +let mut offset = header_len; +let mut prev_cert:Option <*mut bindings::x509_certificate > =None ; + +while offset < cert_chain_len {
[Severity: Low]
Does this incorrectly accept empty certificate chains as valid?
If a responder provides an empty certificate chain where cert_chain_len is less than or equal to header_len, this while loop is skipped entirely. The function will fall through and implicitly return Ok(()) at the end, silently bypassing all certificate parsing and caller-provided validation callbacks.
While the subsequent CHALLENGE phase safely catches the missing leaf key and fails gracefully, should the structural validation phase explicitly reject an empty chain with an error rather than silently accepting it?
Quoted 2 lines
lib/rspdm/state.rs +2
-0+// SAFETY: `cert_chain_buf[offset..]` is a non-empty slice of +// bytes valid for at least `cert_chain_len` bytes.
[ ... ]
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=19
PATCH v2 20/21 rust: allow extracting the buffer from a CString
In reply to [email protected]From: Alistair Francis <[email protected]>
The kernel CString is a wrapper aroud a KVec. This patch allows retrieving the underlying buffer and consuming the CString. This allows users to create a CString from a string and then retrieve the underlying buffer.
--- rust/kernel/str.rs | 6 ++++++ 1 file changed, 6 insertions(+)
rust/kernel/str.rs +6 -0
@@ -870,6 +870,12 @@ pub fn try_from_fmt(args: fmt::Arguments<'_>) -> Result<Self, Error> {// exist in the buffer. Ok (Self { buf }) } + +/// Return the internal buffer while consuming the original [`CString`] +#[inline] +pub fn into_vec (self) ->KVec <u8 > { + self.buf + } }impl Deref for CString {
Signature · 1 line
2.54.0
[email protected]
AI reviewRe: [PATCH v2 20/21] rust: allow extracting the buffer from a CString
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] The `into_vec` method diverges from Rust's standard library `CString` naming conventions…
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The into_vec method diverges from Rust's standard library CString naming conventions, obscuring the presence of the NUL terminator and creating a dangerous API trap.
- [Low] Typo in the commit message: 'aroud' instead of 'around'.
--
commit a1ef2ecd44b6df9c9b3d012bdcb8952ab17a0f53
Author: Alistair Francis <[email protected]>
rust: allow extracting the buffer from a CString
This patch introduces a method to consume a CString and return its underlying KVec buffer. It allows users to create a CString and then extract the internal byte vector for further use.
Quoted 1 line
The kernel CString is a wrapper aroud a KVec. This patch allows
[Severity: Low]
This isn't a bug, but there is a typo in the commit message: "aroud" should be "around".
rust/kernel/str.rs pub fn try_from_fmt(args: fmt::Arguments<'_>) -> Result<Self, Error> { · line 878
Quoted 13 lines
rust/kernel/str.rs +6
-0@@ -870,6 +870,12 @@ pub fn try_from_fmt(args: fmt::Arguments<'_>) -> Result<Self, Error> {// exist in the buffer. Ok (Self { buf }) } + +/// Return the internal buffer while consuming the original [`CString`] +#[inline] +pub fn into_vec (self) ->KVec <u8 > { + self.buf + }
[Severity: Medium]
Does this naming diverge from standard Rust library conventions and obscure the presence of the NUL terminator?
In the standard library, extracting the underlying buffer from a CString typically strips the NUL byte with a method like into_bytes(), while the method that keeps it is explicitly named into_bytes_with_nul().
Calling this into_vec() might mislead callers into assuming the NUL byte is already stripped, which could cause developers to unintentionally include a NUL byte in buffers or calculate incorrect offsets.
Could this be renamed to into_bytes_with_nul() to match standard conventions and clarify the buffer's contents?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=20
PATCH v2 21/21 lib: rspdm: Support SPDM challenge
In reply to [email protected]From: Alistair Francis <[email protected]>
Support the CHALLENGE SPDM command.
--- lib/rspdm/consts.rs | 6 + lib/rspdm/lib.rs | 10 +- lib/rspdm/state.rs | 232 +++++++++++++++++++++++++++++++- lib/rspdm/validator.rs | 61 +++++++++ rust/bindings/bindings_helper.h | 1 + 5 files changed, 305 insertions(+), 5 deletions(-)
lib/rspdm/consts.rs +6 -0
@@ -148,6 +148,8 @@ fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {pub (crate )const SPDM_GET_CERTIFICATE :u8 =0x82 ; +pub (crate )const SPDM_CHALLENGE :u8 =0x83 ; +// If the crypto support isn't enabled don't offer the algorithms // to the responder #[cfg( CONFIG_CRYPTO_RSA )]
@@ -176,3 +178,7 @@ fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {pub (crate )const SPDM_HASH_ALGOS :u32 =SPDM_HASH_SHA2_256 |SPDM_HASH_SHA2_384_512 ;pub (crate )const SPDM_OPAQUE_DATA_FMT_GENERAL :u8 =bit_u8 (1 ); + +pub (crate )const SPDM_PREFIX_SZ :usize =64 ; +pub (crate )const SPDM_COMBINED_PREFIX_SZ :usize =100 ; +pub (crate )const SPDM_MAX_OPAQUE_DATA :usize =1024 ;
lib/rspdm/lib.rs +7 -3
@@ -11,8 +11,7 @@//! from other subsytems. use crate ::bindings::{ - spdm_state, -EPROTONOSUPPORT ,// + spdm_state,// };use core::ffi::{ c_int,
@@ -141,7 +140,12 @@ pub extern "C" fn spdm_authenticate(state_ptr: *mut spdm_state) -> c_int { provisioned_slots &= !(1 << slot); } - -(EPROTONOSUPPORT as i32 ) +let provisioned_slots = state.provisioned_slots.trailing_zeros (); +if let Err (e) = state.challenge (provisioned_slotsas u8 ) { +return e.to_errno ()as c_int ; + } + +0 }/// spdm_destroy() - Destroy SPDM session
lib/rspdm/state.rs +230 -2
347 lines not on this page · raw message
lib/rspdm/validator.rs +61 -0
70 lines not on this page · raw message
rust/bindings/bindings_helper.h +1 -0
@@ -31,6 +31,7 @@#include <linux/acpi.h> #include <linux/gpu_buddy.h> #include <crypto/hash.h> +#include <crypto/public_key.h> #include <drm/drm_device.h> #include <drm/drm_drv.h> #include <drm/drm_file.h>
Signature · 1 line
2.54.0
[email protected]
AI reviewRe: [PATCH v2 21/21] lib: rspdm: Support SPDM challenge
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] A truncated SPDM capability response causes a kernel panic due to an out-of-bounds slice access.…
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] A truncated SPDM capability response causes a kernel panic due to an out-of-bounds slice access.
- [High] Incorrect zero-padding placement in the SPDM 1.2+ combined prefix causes signature verification failure.
- [High] Incorrect memory layout passed to allocator when dropping KVec constructed via from_raw_parts. --
commit 60ad79ffc915550973efe94b73f27dbe90a466d5
Author: Alistair Francis <[email protected]>
lib: rspdm: Support SPDM challenge
Support the CHALLENGE SPDM command.
Quoted 4 lines
lib/rspdm/state.rs +0
-0
[ ... ]
lib/rspdm/state.rs pub(crate) fn get_capabilities(&mut self) -> Result<(), Error> { · line 508
Quoted 13 lines
lib/rspdm/state.rs +3
-1@@ -463,12 +498,15 @@ pub(crate) fn get_capabilities(&mut self) -> Result<(), Error> {let rc = self.spdm_exchange (request_buf, response_vec.as_mut_slice ())?as usize ;// The transport must report a length within the buffer we provided. -if rc > response_vec.len () { +if rc > response_vec.len () || rc > rsp_sz {pr_err! ("Overflowed capabilities response\n" );return Err (EIO ); } response_vec.truncate (rc); + self.transcript + .extend_from_slice (&response_vec[..rsp_sz],GFP_KERNEL )?;
[Severity: High]
If spdm_exchange() returns a smaller rc than rsp_sz, response_vec will be truncated to rc elements. Will the slice &response_vec[..rsp_sz] then panic due to being out of bounds?
[ ... ]
Quoted 1 line
lib/rspdm/state.rs +0
-0@@ -971,4 +1028,175 @@ pub(crate) fn validate_cert_chain(&mut self, slot: u8) -> Result<(), Error> {
[ ... ]
lib/rspdm/state.rs pub(crate) fn validate_cert_chain(&mut self, slot: u8) -> Result<(), Error> { · line 1082
Quoted 9 lines
lib/rspdm/state.rs +9
-0+let major = self.version >>4 ; +let minor = self.version &0xF ; + +let output =CString ::try_from_fmt (fmt! ("dmtf-spdm-v{major:x}.{minor:x}.*dmtf-spdm-v{major:x}.{minor:x}.*dmtf-spdm-v{major:x}.{minor:x}.*dmtf-spdm-v{major:x}.{minor:x}.*" ))?; +let mut buf = output.into_vec (); +let zero_pad_len =SPDM_COMBINED_PREFIX_SZ -SPDM_PREFIX_SZ -SPDM_CONTEXT .len () -1 ; + + buf.extend_with (zero_pad_len,0 ,GFP_KERNEL )?; + buf.extend_from_slice (SPDM_CONTEXT .as_bytes (),GFP_KERNEL )?;
[Severity: High]
Does this code append the zero padding before the context string?
According to the SPDM 1.2+ specification (DSP0274), the combined prefix format should be VersionPrefix || 0x00 || ContextString || ZeroPadding. Could placing the zero padding before SPDM_CONTEXT cause the transcript hash to mismatch the signature provided by the device?
[Severity: High]
Additionally, there appears to be a memory layout mismatch regression when dropping the shash_desc context in SpdmState::drop() and update_response_algs():
lib/rspdm/state.rs:SpdmState::drop() {
let desc_vec = unsafe { KVec::from_raw_parts(desc, desc_len, desc_len) };
drop(desc_vec);
}Is the generic parameter T incorrectly inferred as bindings::shash_desc instead of u8? Since desc_len is a byte count, would this pass the wrong layout to the allocator during drop, assuming desc_len elements of type shash_desc instead of desc_len bytes?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=21