linux-kernel - Re: [PATCH v2 2/2] firmware_loader: fix soundness issue in `request

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] [day] [month] [year] [list]

Message-ID: <12a986aa-3fed-4686-b5bc-99cbe446cca6@gmail.com>
Date: Tue, 9 Jul 2024 00:16:30 +0200
From: Christian Schrefl <chrisi.schrefl@...il.com>
To: Danilo Krummrich <dakr@...hat.com>, mcgrof@...nel.org,
 russ.weight@...ux.dev, gregkh@...uxfoundation.org
Cc: linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org,
 Gary Guo <gary@...yguo.net>
Subject: Re: [PATCH v2 2/2] firmware_loader: fix soundness issue in
 `request_internal`

Greetings

On 08.07.24 10:07 PM, Danilo Krummrich wrote:
> `request_internal` must be called with one of the following function
> pointers: request_firmware(), firmware_request_nowarn(),
> firmware_request_platform() or request_firmware_direct().
> 
> The previous `FwFunc` alias did not guarantee this, which is unsound.
> 
> In order to fix this up, implement `FwFunc` as new type with a
> corresponding type invariant.
> 
> Reported-by: Gary Guo <gary@...yguo.net>
> Closes: https://lore.kernel.org/lkml/20240620143611.7995e0bb@eugeo/
> Signed-off-by: Danilo Krummrich <dakr@...hat.com>
> ---
> v2:
>   - provide method for each wrapped `FwFunc` (Christian)> ---
>  rust/kernel/firmware.rs | 27 ++++++++++++++++++++-------
>  1 file changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs
> index 106a928a535e..2ba03af9f036 100644
> --- a/rust/kernel/firmware.rs
> +++ b/rust/kernel/firmware.rs
> @@ -7,10 +7,23 @@
>  use crate::{bindings, device::Device, error::Error, error::Result, str::CStr};
>  use core::ptr::NonNull;
>  
> -// One of the following: `bindings::request_firmware`, `bindings::firmware_request_nowarn`,
> -// `firmware_request_platform`, `bindings::request_firmware_direct`
> -type FwFunc =
> -    unsafe extern "C" fn(*mut *const bindings::firmware, *const i8, *mut bindings::device) -> i32;
> +/// # Invariants
> +///
> +/// One of the following: `bindings::request_firmware`, `bindings::firmware_request_nowarn`,
> +/// `bindings::firmware_request_platform`, `bindings::request_firmware_direct`.
> +struct FwFunc(
> +    unsafe extern "C" fn(*mut *const bindings::firmware, *const i8, *mut bindings::device) -> i32,
> +);
> +
> +impl FwFunc {
> +    fn request() -> Self {
> +        Self(bindings::request_firmware)
> +    }
> +
> +    fn request_nowarn() -> Self {
> +        Self(bindings::firmware_request_nowarn)
> +    }

I'm not sure if we should have a comment here that describes how the invariant is fulfilled.

In this case its not too bad since the Invariants are described just a few lines above, so:

Reviewed-by: Christian Schrefl <chrisi.schrefl@...il.com>


Cheers,
Christian