linux-kernel - Re: [RFC 3/6] lib: rspdm: Initial commit of Rust SPDM

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

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

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

Message-ID: <CANiq72=3p50vyA5MsqZD6_Ma53DSLyrqyKYUwH9o-+Bq=REaQQ@mail.gmail.com>
Date: Fri, 15 Nov 2024 18:15:38 +0100
From: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
To: Alistair Francis <alistair@...stair23.me>
Cc: lukas@...ner.de, Jonathan.Cameron@...wei.com, linux-kernel@...r.kernel.org, 
	rust-for-linux@...r.kernel.org, akpm@...ux-foundation.org, 
	bhelgaas@...gle.com, linux-pci@...r.kernel.org, linux-cxl@...r.kernel.org, 
	bjorn3_gh@...tonmail.com, ojeda@...nel.org, tmgross@...ch.edu, 
	boqun.feng@...il.com, benno.lossin@...ton.me, a.hindborg@...nel.org, 
	wilfred.mallawa@....com, alistair23@...il.com, alex.gaynor@...il.com, 
	gary@...yguo.net, aliceryhl@...gle.com
Subject: Re: [RFC 3/6] lib: rspdm: Initial commit of Rust SPDM

Hi Alistair,

It is nice to see more Rust, thanks!

Some quick consistency nits I noticed for future non-RFC versions (I
didn't check the code). Some apply in several cases.

On Fri, Nov 15, 2024 at 6:46 AM Alistair Francis <alistair@...stair23.me> wrote:
>
> +#[allow(dead_code)]

It may be possible to use `expect` here instead of `allow` -- e.g. if
it does not depend on conditional compilation.

Also, later in the series, is it used? (I imagine it is a temporary
`allow`? If so, please delete it when you introduce the first use.
`expect` can help here to not forget to delete it.

> +#[repr(u8)]

It is probably a good idea to mention why it needs this `repr`. I
imagine it is related to `SpdmErrorRsp` being `packed` and so on, but
it wouldn't hurt documenting it.

> +    InvalidRequest = 0x01,

Please feel free to ignore this one (especially if the idea is to
replace the C implementation eventually, or to just showcase how it
would look like if the C one was removed), but one idea here would be
to pick the values from a common C header? i.e. moving that `enum` to
its own header that both use.

> +//! Top level library, including C compatible public functions to be called
> +//! from other subsytems.

Typo.

> +/// spdm_create() - Allocate SPDM session

I think these are copied from the C one, so it is fine for the RFC,
but the subsystem ends up accepting this, then please use the usual
Markdown style of the rest of the Rust code, instead of kernel-doc
style. While we don't render the docs of these just yet, we will start
doing it at some point, and e.g. IDEs may do so too. Even if we
didn't, the comments could be copied into other docs at some point, so
it is always useful to have them formatted properly.

> +    /* Negotiated state */
> +    pub(crate) version: u8,

Please use `//`.

> +                bindings::EINVAL
> +            }
> +        };
> +
> +        to_result(-(ret as i32))

These are errors you create directly, so you can do directly e.g.
`Err(EINVAL)`, i.e. please avoid `bindings::`.

> +        let length = unsafe {

Missing `SAFETY` comment.

If you based this on top of `rust-next` as you noted in the cover
letter, you should be getting a warning under `CLIPPY=1`. There may be
other cleanups under `CLIPPY=1` if you weren't using it so far.

> +            return Ok(length); /* Truncated response is handled by callers */

Please use `//` for comments.

> +// SPDX-License-Identifier: GPL-2.0

New line between SPDX and the crate docs.

> +//! Rust implementation of the DMTF Security Protocol and Data Model (SPDM)
> +//! https://www.dmtf.org/dsp/DSP0274

This should be a link (using <>) or a link for the "DMTF Security
Protocol and Data Model (SPDM)" text.

> +//! Rust sysfs helper functions

This should be the title, at the top. In Rust the first paragraph
(which typically should be short, e.g. a single line) is considered
the "short description"  and used e.g. for lists.

> +//! Copyright (C) 2024 Western Digital

This should be a comment (likely near the SPDX), rather than part of
the documentation -- see e.g. how it is done in `rust/kernel/list.rs`.

> +pub unsafe extern "C" fn rust_authenticated_show(

`unsafe` functions should have a `# Safety` section.

Thanks again!

Cheers,
Miguel