Re: [PATCH v5 21/23] rust: x509: Add X509 certificate bindings

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] rust: x509: build and link failure when CONFIG_X509_CERTIFICATE_PARSER != y
- [Medium] rust: bindings: build failure in bindings_helper.h when CONFIG_KEYS=n
- [Low] rust: x509: missing inline attributes, import style, and log typo
--

commit bd0a2a1272842d031d7bfad2c6e145a2a0b663dc
Author: Alistair Francis <alistair.francis@wdc.com>

rust: x509: Add X509 certificate bindings

This commit adds initial Rust bindings and a Certificate wrapper around
the kernel X.509 certificate parser APIs for use by the Rust SPDM
implementation.

> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index da8855899c530..66e18819d52b0 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -39,6 +39,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>

[Severity: Medium]
Does including keys/asymmetric-type.h and keys/x509-parser.h
unconditionally break bindgen when CONFIG_KEYS is disabled?

In include/linux/key.h, struct key is only defined under #ifdef CONFIG_KEYS
and is otherwise only forward-declared:

include/linux/key.h:
    struct key;
    struct net;

    #ifdef CONFIG_KEYS

However, include/keys/asymmetric-type.h defines inline functions outside
any #ifdef CONFIG_KEYS guard that dereference const struct key *:

include/keys/asymmetric-type.h:
    static inline
    const struct asymmetric_key_ids *asymmetric_key_ids(const struct key *key)
    {
        return key->payload.data[asym_key_ids];
    }

When building with CONFIG_RUST=y and CONFIG_KEYS=n, clang fails during
bindgen generation with an incomplete type error on struct key. Should
these headers be guarded or should struct key handling in
include/keys/asymmetric-type.h account for !CONFIG_KEYS?

> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index e8b652faa3b49..aa95aafff8c49 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -153,6 +153,8 @@
>  pub mod usb;
>  pub mod validate;
>  pub mod workqueue;
> +#[cfg(CONFIG_CRYPTO)]
> +pub mod x509;

[Severity: Medium]
Should the x509 module be gated on
#[cfg(CONFIG_X509_CERTIFICATE_PARSER = "y")] instead of
#[cfg(CONFIG_CRYPTO)]?

Because rust/kernel.o is always built-in when CONFIG_RUST=y, non-generic
methods in rust/kernel/x509.rs such as Certificate::parse(),
Certificate::verify_signed(), and Certificate::drop() are compiled directly
into vmlinux whenever CONFIG_CRYPTO is enabled.

In Certificate::parse() and Certificate::drop(), the code calls
bindings::x509_get_certificate_length(), bindings::x509_cert_parse(), and
bindings::x509_free_certificate(), which are only built under
CONFIG_X509_CERTIFICATE_PARSER in crypto/asymmetric_keys/Makefile:

    obj-$(CONFIG_X509_CERTIFICATE_PARSER) += x509_key_parser.o
    x509_key_parser-y := \
        x509.asn1.o \
        x509_akid.asn1.o \
        x509_cert_parser.o \
        x509_loader.o \
        x509_public_key.o

Similarly, Certificate::verify_signed() calls
bindings::public_key_verify_signature(), which is built under
CONFIG_ASYMMETRIC_PUBLIC_KEY_SUBTYPE (selected by
CONFIG_X509_CERTIFICATE_PARSER) and falls back to a static inline stub in
include/crypto/public_key.h when disabled, which bindgen does not expose
without a rust_helper_* wrapper.

If CONFIG_CRYPTO is enabled while CONFIG_X509_CERTIFICATE_PARSER is n or m,
won't this fail to compile or fail to link vmlinux with undefined references
from rust/kernel.o?

> diff --git a/rust/kernel/x509.rs b/rust/kernel/x509.rs
> new file mode 100644
> index 0000000000000..1b5bc2a99c91c
> --- /dev/null
> +++ b/rust/kernel/x509.rs
> @@ -0,0 +1,118 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +// Copyright (C) 2026 Western Digital
> +
> +//! Wrappers around the Linux kernel X509 parser APIs.
> +//!
> +//! C headers: [`include/keys/x509-parser.h`](srctree/include/keys/x509-parser.h).
> +
> +use core::ffi::c_void;
> +use core::ptr::{self, NonNull};
> +use kernel::prelude::*;
> +use kernel::{
> +    bindings,
> +    error::{
> +        from_err_ptr,
> +        to_result, //
> +    },
> +    public_key::PublicKey,
> +};
> +
> +/// Owned `struct x509_certificate` wrapper.
> +pub struct Certificate {
> +    cert: NonNull<bindings::x509_certificate>,
> +}
> +
> +impl Certificate {
> +    /// Parses a DER certificate from `data` and returns it with the length
> +    /// of the certificate in bytes.
> +    pub fn parse(data: &[u8]) -> Result<(Self, usize)> {
> +        // SAFETY: `data.as_ptr()` points to at least `data.len()` valid bytes.
> +        let len = unsafe { bindings::x509_get_certificate_length(data.as_ptr(), data.len()) };
> +        let len = usize::try_from(len).map_err(|_| {
> +            pr_err!("Invalid certificate length\n");
> +            EPROTO
> +        })?;
> +
> +        if len > data.len() {
> +            pr_err!("Certificate length is larger then the buffer\n");
> +            return Err(EPROTO);
> +        }
> +
> +        // SAFETY: `data.as_ptr()` points to `data.len()` valid bytes and the
> +        // caller has already checked that the DER certificate length fits in
> +        // that buffer.
> +        let cert = from_err_ptr(unsafe {
> +            bindings::x509_cert_parse(data.as_ptr().cast::<c_void>(), len)
> +        })?;
> +        let cert = NonNull::new(cert).ok_or(EINVAL)?;
> +
> +        Ok((Self { cert }, len))
> +    }
> +
> +    /// Returns the raw pointer for the X.509 certificate.
> +    /// This is what is returned from the C function x509_cert_parse()
> +    pub fn raw(&self) -> *mut bindings::x509_certificate {
> +        self.cert.as_ptr()
> +    }
> +
> +    /// Check that the certificate is not unsupported or blacklisted.
> +    pub fn check_accepted(&self) -> Result {
> +        // SAFETY: `self.cert` was returned by `x509_cert_parse()` and remains
> +        // valid until dropped.
> +        let cert = unsafe { &*self.cert.as_ptr() };
> +        if cert.unsupported_sig || cert.blacklisted {
> +            pr_err!("Certificate was rejected\n");
> +            return Err(EKEYREJECTED);
> +        }
> +
> +        Ok(())
> +    }
> +
> +    /// Verify that `child`'s signature is signed by this cert's public/private key.
> +    pub fn verify_signed(&self, child: &Self) -> Result {
> +        // SAFETY: both certificate pointers were produced by `x509_cert_parse()`
> +        // and remain valid until dropped.
> +        let (prev, child) = unsafe { (&*self.cert.as_ptr(), &*child.cert.as_ptr()) };
> +
> +        let prev_pub = NonNull::new(prev.pub_).ok_or(EINVAL)?;
> +        let child_sig = NonNull::new(child.sig).ok_or(EINVAL)?;
> +
> +        // SAFETY: `prev_pub` and `child_sig` are valid for at least the duration
> +        // of this call.
> +        unsafe {
> +            to_result(bindings::public_key_verify_signature(
> +                prev_pub.as_ptr(),
> +                child_sig.as_ptr(),
> +            ))
> +        }
> +    }
> +
> +    /// Transfers the public key out of the certificate so the certificate can be
> +    /// dropped without freeing the public key.
> +    pub fn take_public_key(&mut self) -> Option<PublicKey> {
> +        // SAFETY: `self.cert` is a valid certificate pointer.
> +        let raw = unsafe { (*self.cert.as_ptr()).pub_ };
> +        let key = NonNull::new(raw)?;
> +
> +        // SAFETY: `self.cert` is a valid certificate pointer. Clearing `pub_`
> +        // transfers ownership of the public key to the returned `PublicKey`.
> +        // The `pub_` field will not be freed by `x509_free_certificate()` in
> +        // Drop
> +        unsafe {
> +            (*self.cert.as_ptr()).pub_ = ptr::null_mut();
> +        }
> +
> +        Some(PublicKey::from_raw(key))
> +    }
> +}

[Severity: Low]
This isn't a bug, but there are a few minor style and coding guideline items
in rust/kernel/x509.rs:

1. At the top of rust/kernel/x509.rs, core::ptr::{self, NonNull} should use
vertical import formatting, and core::ffi::c_void is already provided by
kernel::prelude::*.

2. In Certificate::parse(), the error message has a typo ("larger then"
instead of "larger than").

3. Small abstraction methods in the kernel crate such as Certificate::raw(),
Certificate::check_accepted(), Certificate::verify_signed(), and
Certificate::take_public_key() should be annotated with #[inline].

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027052536.1722081-1-alistair.francis@wdc.com?part=21